From 58b95338ebe1d94e5e69efe8b6976820ab1b283c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 5 Mar 2021 08:15:57 +0100 Subject: [PATCH 01/12] Merge #21345: test: bring p2p_leak.py up to date a061a299708d39ad63f85085ae07c457308823cf test: bring p2p_leak.py up to date. (Martin Zumsande) Pull request description: After the introduction of wtxidrelay and sendaddrv2 messages during version handshake, extend p2p_leak.py test to reflect this. Also, some minor fixes and doc improvements. I also added a test that peers not completing the version handshake will be disconnected for timeout, as suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19723#issuecomment-699540294. ACKs for top commit: brunoerg: Tested ACK a061a299708d39ad63f85085ae07c457308823cf theStack: Tested ACK a061a299708d39ad63f85085ae07c457308823cf Tree-SHA512: 26c601491fa8710fc972d1b8f15da6b387a95b42bbfb629ec4c668769ad3824b6dd6a33d97363bca2171e403d8d1ce08abf3e5c9cab34f98d53e0109b1c7a0e5 --- test/functional/p2p_leak.py | 46 +++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index 2db2c31f1f..d966537631 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -4,8 +4,8 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test message sending before handshake completion. -A node should never send anything other than VERSION/VERACK until it's -received a VERACK. +Before receiving a VERACK, a node should not send anything but VERSION/VERACK +and feature negotiation messages ( SENDADDRV2). This test connects to a node and sends it a few messages, trying to entice it into sending us something it shouldn't. @@ -36,10 +36,11 @@ class LazyPeer(P2PInterface): super().__init__() self.unexpected_msg = False self.ever_connected = False + self.got_sendaddrv2 = False def bad_message(self, message): self.unexpected_msg = True - self.log.info("should not have received message: %s" % message.msgtype) + print("should not have received message: %s" % message.msgtype) def on_open(self): self.ever_connected = True @@ -64,6 +65,7 @@ class LazyPeer(P2PInterface): def on_cmpctblock(self, message): self.bad_message(message) def on_getblocktxn(self, message): self.bad_message(message) def on_blocktxn(self, message): self.bad_message(message) + def on_sendaddrv2(self, message): self.got_sendaddrv2 = True # Peer that sends a version but not a verack. @@ -96,15 +98,25 @@ class P2PLeakTest(BitcoinTestFramework): self.num_nodes = 1 self.disable_mocktime = True + def create_old_version(self, nversion): + old_version_msg = msg_version() + old_version_msg.nVersion = nversion + old_version_msg.strSubVer = P2P_SUBVERSION + old_version_msg.nServices = P2P_SERVICES + old_version_msg.relay = P2P_VERSION_RELAY + return old_version_msg + + def run_test(self): - # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node. + self.log.info('Check that the node doesn\'t send unexpected messages before handshake completion') + # Peer that never sends a version, nor any other messages. It shouldn't receive anything from the node. no_version_idle_peer = self.nodes[0].add_p2p_connection(LazyPeer(), send_version=False, wait_for_verack=False) # Peer that sends a version but not a verack. no_verack_idle_peer = self.nodes[0].add_p2p_connection(NoVerackIdlePeer(), wait_for_verack=False) - # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the - # verack, since we never sent one + # Wait until the peer gets the verack in response to the version. Though, don't wait for the node to receive the + # verack, since the peer never sent one no_verack_idle_peer.wait_for_verack() no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected) @@ -113,14 +125,19 @@ class P2PLeakTest(BitcoinTestFramework): # Mine a block and make sure that it's not sent to the connected peers self.generate(self.nodes[0], nblocks=1) - #Give the node enough time to possibly leak out a message + # Give the node enough time to possibly leak out a message time.sleep(5) - self.nodes[0].disconnect_p2ps() + # Make sure only expected messages came in + assert not no_version_idle_peer.unexpected_msg + assert not no_version_idle_peer.got_sendaddrv2 - # Make sure no unexpected messages came in - assert no_version_idle_peer.unexpected_msg == False - assert no_verack_idle_peer.unexpected_msg == False + assert not no_verack_idle_peer.unexpected_msg + assert no_verack_idle_peer.got_sendaddrv2 + + # Expect peers to be disconnected due to timeout + assert not no_version_idle_peer.is_connected + assert not no_verack_idle_peer.is_connected self.log.info('Check that the version message does not leak the local address of the node') p2p_version_store = self.nodes[0].add_p2p_connection(P2PVersionStore()) @@ -135,13 +152,8 @@ class P2PLeakTest(BitcoinTestFramework): self.log.info('Check that old peers are disconnected') p2p_old_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False) - old_version_msg = msg_version() - old_version_msg.nVersion = 31799 - old_version_msg.strSubVer = P2P_SUBVERSION - old_version_msg.nServices = P2P_SERVICES - old_version_msg.relay = P2P_VERSION_RELAY with self.nodes[0].assert_debug_log(['peer=3 using obsolete version 31799; disconnecting']): - p2p_old_peer.send_message(old_version_msg) + p2p_old_peer.send_message(self.create_old_version(31799)) p2p_old_peer.wait_for_disconnect() From 0698be3680c3be6b0cc28072b42fd5a7883c311d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 5 Jun 2021 08:38:19 +0200 Subject: [PATCH 02/12] Merge bitcoin/bitcoin#22153: test: Fix p2p_leak.py intermittent failure ca3a77068b8c9c6107d078ea083f4ab7c0010548 test: Fix p2p_leak.py intermittent failure by lowering timeout (Martin Zumsande) Pull request description: Fixes #22085 Root cause: There was just 1 second between the wait (5 seconds) and the `-peertimeout=4`. Since `ShouldRunInactivityChecks` in `net.cpp` measures the timeout in seconds, its result can only change once per second, even though it is called more often. So in situations when the connection is established early in a given second like [here](https://bitcoinbuilds.org/index.php?ansilog=d7b3e075-683a-45cc-94d4-9645fc17e0b6.log#l3117) (2021-05-27T12:28:04.**001**913Z ), the 1 second leeway was not be sufficient, leading to the intermittent failures. Fix this by lowering the timeout by one second. ACKs for top commit: MarcoFalke: re-ACK ca3a77068b8c9c6107d078ea083f4ab7c0010548 Tree-SHA512: e7e22356d276c65a5b4f0a1b7ee5a9ad07d27691220746c7d02af3fad22cab1d53fd0ef59a938167ec80e4571c96649132d6922ad10667fc91baa47892f27a3e --- test/functional/p2p_leak.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index d966537631..9824586c42 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -30,6 +30,8 @@ from test_framework.util import ( assert_greater_than_or_equal, ) +PEER_TIMEOUT = 3 + class LazyPeer(P2PInterface): def __init__(self): @@ -97,6 +99,8 @@ class P2PLeakTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.disable_mocktime = True + self.extra_args = [[f"-peertimeout={PEER_TIMEOUT}"]] + def create_old_version(self, nversion): old_version_msg = msg_version() @@ -126,7 +130,7 @@ class P2PLeakTest(BitcoinTestFramework): self.generate(self.nodes[0], nblocks=1) # Give the node enough time to possibly leak out a message - time.sleep(5) + time.sleep(PEER_TIMEOUT + 2) # Make sure only expected messages came in assert not no_version_idle_peer.unexpected_msg From 97f0d91d3edbc03e78ce73c98b0a062585a073ed Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 1 Jul 2021 08:31:55 +0200 Subject: [PATCH 03/12] Merge bitcoin/bitcoin#22376: ci: Do not clone `bitcoin-core/qa-assets` git repository if not necessary 30450a1bd5d278e285f50a7e4cfc755545960e92 Do not clone qa-assets git repository if not necessary (Kiminuo) Pull request description: This PR attempts to remove an unnecessary step when CI runs. The main motivation for the change is that I locally use `MAKEJOBS="-j15" FILE_ENV="./ci/test/00_setup_env_android.sh" ./ci/test_run_all.sh` to find out if a patch of mine works or not. Cloning `bitcoin-core/qa-assets` is slow on my machine (which is by no means slow). ACKs for top commit: MarcoFalke: cr ACK 30450a1bd5d278e285f50a7e4cfc755545960e92 Tree-SHA512: 5763b53da9554b06039c39f8fc729de1b106cce2a242de8f97528d001bfa01d4f48d2a128f458a3cdee3da36312354c6714839b947f313c089c2c5cb30233a39 --- ci/test/04_install.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ci/test/04_install.sh b/ci/test/04_install.sh index 967e437fc0..da2f5714b4 100755 --- a/ci/test/04_install.sh +++ b/ci/test/04_install.sh @@ -79,12 +79,13 @@ fi DOCKER_EXEC echo "Free disk space:" DOCKER_EXEC df -h -if [ ! -d ${DIR_QA_ASSETS} ]; then - if [ "$RUN_FUZZ_TESTS" = "true" ]; then - DOCKER_EXEC git clone https://github.com/bitcoin-core/qa-assets ${DIR_QA_ASSETS} - fi +if [ "$RUN_FUZZ_TESTS" = "true" ] || [ "$RUN_UNIT_TESTS" = "true" ] || [ "$RUN_UNIT_TESTS_SEQUENTIAL" = "true" ]; then + if [ ! -d ${DIR_QA_ASSETS} ]; then + DOCKER_EXEC git clone --depth=1 https://github.com/bitcoin-core/qa-assets ${DIR_QA_ASSETS} + fi + + export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/ fi -export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/ DOCKER_EXEC mkdir -p "${BASE_SCRATCH_DIR}/sanitizer-output/" From 397fe9c0a54d8b32fdce061e2be5d5f908d7d964 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sun, 18 Jul 2021 18:39:07 +1200 Subject: [PATCH 04/12] Merge bitcoin/bitcoin#22461: wallet: Change ScriptPubKeyMan::Upgrade default to True 5012a7912ee9fa35bc417cb073eebffd85f36c6c Test that descriptor wallet upgrade does nothing (Andrew Chow) 48bd7d3b7737656052d2c745ed40c7f6670842cf Change ScriptPubKeyMan::Upgrade to default to return true (Andrew Chow) Pull request description: When adding a new ScriptPubKeyMan, it's likely that there will be nothing for `Upgrade` to do. If it is called (via `upgradewallet`), then it should do nothing, successfully. This PR changes the default `ScriptPubKeyMan::Upgrade` function so that it returns a success instead of failure when doing nothing. Fixes #22460 ACKs for top commit: jonatack: ACK 5012a7912ee9fa35bc417cb073eebffd85f36c6c meshcollider: utACK 5012a7912ee9fa35bc417cb073eebffd85f36c6c Tree-SHA512: 578c6521e997f7bb5cc44be2cfe9e0a760b6bd4aa301026a6b8b3282e8757473e4cb9f68b2e79dacdc2b42dddae718450072e0a38817df205dfea177a74d7f3d --- test/functional/wallet_upgradewallet.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index a662fbd387..cc2b4168c2 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -72,10 +72,11 @@ class UpgradeWalletTest(BitcoinTestFramework): def test_upgradewallet(self, wallet, previous_version, requested_version=None, expected_version=None): unchanged = expected_version == previous_version new_version = previous_version if unchanged else expected_version if expected_version else requested_version - assert_equal(wallet.getwalletinfo()["walletversion"], previous_version) + old_wallet_info = wallet.getwalletinfo() + assert_equal(old_wallet_info["walletversion"], previous_version) assert_equal(wallet.upgradewallet(requested_version), { - "wallet_name": "", + "wallet_name": old_wallet_info["walletname"], "previous_version": previous_version, "current_version": new_version, "result": "Already at latest version. Wallet version unchanged." if unchanged else "Wallet upgraded successfully from version {} to version {}.".format(previous_version, new_version), @@ -232,6 +233,11 @@ class UpgradeWalletTest(BitcoinTestFramework): assert_equal(120200, wallet.getwalletinfo()['walletversion']) + if self.is_sqlite_compiled(): + self.log.info("Checking that descriptor wallets do nothing, successfully") + self.nodes[0].createwallet(wallet_name="desc_upgrade", descriptors=True) + desc_wallet = self.nodes[0].get_wallet_rpc("desc_upgrade") + self.test_upgradewallet(desc_wallet, previous_version=120200, expected_version=120200) if __name__ == '__main__': UpgradeWalletTest().main() From cba01aa8f9ef4014f660e6aeeb8063cad9765295 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 1 Jul 2021 10:08:33 +0800 Subject: [PATCH 05/12] Merge bitcoin/bitcoin#20191: wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag 181181019c5baa3e2d5b675d1843a45aa028781c refactor: remove m_internal from DescriptorSPKman (S3RK) Pull request description: Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface. Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size). ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/20191/commits/181181019c5baa3e2d5b675d1843a45aa028781c achow101: reACK 181181019c5baa3e2d5b675d1843a45aa028781c Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff --- src/wallet/scriptpubkeyman.cpp | 20 +++----------------- src/wallet/scriptpubkeyman.h | 20 ++++---------------- src/wallet/wallet.cpp | 14 +++++++++----- 3 files changed, 16 insertions(+), 38 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 52d07ff7fa..2a0a19a5da 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1779,12 +1779,10 @@ bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const return !m_hd_chain.IsNull(); } -void LegacyScriptPubKeyMan::SetInternal(bool internal) {} - bool DescriptorScriptPubKeyMan::GetNewDestination(CTxDestination& dest, bilingual_str& error) { // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later - if (!CanGetAddresses(m_internal)) { + if (!CanGetAddresses()) { error = _("No addresses available"); return false; } @@ -2042,7 +2040,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } } -bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key) +bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, bool internal) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -2060,7 +2058,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ std::string desc_prefix = strprintf("pkh(%s/44'/%d'", xpub, Params().ExtCoinType()); std::string desc_suffix = "/*)"; - std::string internal_path = m_internal ? "/1" : "/0"; + std::string internal_path = internal ? "/1" : "/0"; std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix; // Make the descriptor @@ -2115,13 +2113,6 @@ int64_t DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const return 0; } -size_t DescriptorScriptPubKeyMan::KeypoolCountExternalKeys() const -{ - if (m_internal) { - return 0; - } - return GetKeyPoolSize(); -} unsigned int DescriptorScriptPubKeyMan::GetKeyPoolSize() const { @@ -2332,11 +2323,6 @@ uint256 DescriptorScriptPubKeyMan::GetID() const return m_wallet_descriptor.id; } -void DescriptorScriptPubKeyMan::SetInternal(bool internal) -{ - this->m_internal = internal; -} - void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) { LOCK(cs_desc_man); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index e3f1916272..7b248d7736 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -195,7 +195,6 @@ public: virtual int64_t GetOldestKeyPoolTime() const { return GetTime(); } - virtual size_t KeypoolCountExternalKeys() const { return 0; } virtual unsigned int GetKeyPoolSize() const { return 0; } virtual int64_t GetTimeFirstKey() const { return 0; } @@ -219,8 +218,6 @@ public: virtual uint256 GetID() const { return uint256(); } - virtual void SetInternal(bool internal) {} - /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ template void WalletLogPrintf(std::string fmt, Params... parameters) const { @@ -345,8 +342,7 @@ public: void RewriteDB() override; int64_t GetOldestKeyPoolTime() const override; - size_t KeypoolCountExternalKeys() const override; - + size_t KeypoolCountExternalKeys() const; unsigned int GetKeyPoolSize() const override; int64_t GetTimeFirstKey() const override; @@ -366,8 +362,6 @@ public: uint256 GetID() const override; - void SetInternal(bool internal) override; - // Map from Key ID to key metadata. std::map mapKeyMetadata GUARDED_BY(cs_KeyStore); @@ -520,8 +514,6 @@ private: PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man); int32_t m_max_cached_index = -1; - bool m_internal = false; - KeyMap m_map_keys GUARDED_BY(cs_desc_man); CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man); @@ -544,9 +536,8 @@ public: : ScriptPubKeyMan(storage), m_wallet_descriptor(descriptor) {} - DescriptorScriptPubKeyMan(WalletStorage& storage, bool internal) - : ScriptPubKeyMan(storage), - m_internal(internal) + DescriptorScriptPubKeyMan(WalletStorage& storage) + : ScriptPubKeyMan(storage) {} mutable RecursiveMutex cs_desc_man; @@ -571,12 +562,11 @@ public: bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey - bool SetupDescriptorGeneration(const CExtKey& master_key); + bool SetupDescriptorGeneration(const CExtKey& master_key, bool internal); bool HavePrivateKeys() const override; int64_t GetOldestKeyPoolTime() const override; - size_t KeypoolCountExternalKeys() const override; unsigned int GetKeyPoolSize() const override; int64_t GetTimeFirstKey() const override; @@ -596,8 +586,6 @@ public: uint256 GetID() const override; - void SetInternal(bool internal) override; - void SetCache(const DescriptorCache& cache); bool AddKey(const CKeyID& key_id, const CKey& key); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a5db987bbb..aa70eef716 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4145,9 +4145,14 @@ size_t CWallet::KeypoolCountExternalKeys() const { AssertLockHeld(cs_wallet); + auto legacy_spk_man = GetLegacyScriptPubKeyMan(); + if (legacy_spk_man) { + return legacy_spk_man->KeypoolCountExternalKeys(); + } + unsigned int count = 0; - for (auto spk_man : GetActiveScriptPubKeyMans()) { - count += spk_man->KeypoolCountExternalKeys(); + if (m_external_spk_managers) { + count += m_external_spk_managers->GetKeyPoolSize(); } return count; @@ -5847,7 +5852,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() for (bool internal : {false, true}) { { // OUTPUT_TYPE is only one: LEGACY - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, internal)); + auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this)); if (IsCrypted()) { if (IsLocked()) { throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); @@ -5856,7 +5861,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); } } - spk_manager->SetupDescriptorGeneration(master_key); + spk_manager->SetupDescriptorGeneration(master_key, internal); uint256 id = spk_manager->GetID(); m_spk_managers[id] = std::move(spk_manager); AddActiveScriptPubKeyMan(id, internal); @@ -5883,7 +5888,6 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, bool internal) auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers; auto spk_man = m_spk_managers.at(id).get(); - spk_man->SetInternal(internal); spk_mans = spk_man; if (spk_mans_other == spk_man) { From 42d4f9a9b932c0045aa52a2e6b5c7eee718d5817 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 28 Dec 2020 22:40:33 +0100 Subject: [PATCH 06/12] partial Merge #20755: [rpc] Remove deprecated fields from getpeerinfo BACKPORT NOTE: the field `banscore` is used by functional test p2p_quorum_data.py and can't be removed now 454a4088a87eac5878070b26d13d5574da891877 [doc] Add release notes for removed getpeerinfo fields. (Amiti Uttarwar) b1a936d4ae7dd9030b0720ef05579a90ce2894f1 [rpc] Remove deprecated "whitelisted" field from getpeerinfo (Amiti Uttarwar) 094c3beaa47c909070607e94f2544ed1472ddb17 [rpc] Remove deprecated "banscore" field from getpeerinfo (Amiti Uttarwar) 537053336fbc1b633e7c99286c3e3492eaca1e9d [rpc] Remove deprecated "addnode" field from getpeerinfo (Amiti Uttarwar) Pull request description: This PR removes support for 3 fields on the `getpeerinfo` RPC that were deprecated in v0.21- `addnode`, `banscore` & `whitelisted`. ACKs for top commit: sipa: utACK 454a4088a87eac5878070b26d13d5574da891877 jnewbery: ACK 454a4088a87eac5878070b26d13d5574da891877. Tree-SHA512: ccc0e90c0763eeb8529cf0c46162dbaca3f7773981b3b52d9925166ea7421aed086795d56b320e16c9340f68862388785f52a9b78314865070917b33180d7cd6 --- doc/release-notes-20755.md | 7 +++ src/net.cpp | 4 -- src/net.h | 4 -- src/rpc/net.cpp | 12 ----- test/functional/p2p_blocksonly.py | 2 +- test/functional/p2p_permissions.py | 50 ++++--------------- test/functional/rpc_external_queue.py | 0 .../functional/rpc_getpeerinfo_deprecation.py | 11 ---- 8 files changed, 19 insertions(+), 71 deletions(-) create mode 100644 doc/release-notes-20755.md mode change 100644 => 100755 test/functional/rpc_external_queue.py diff --git a/doc/release-notes-20755.md b/doc/release-notes-20755.md new file mode 100644 index 0000000000..f0b4ca0cd3 --- /dev/null +++ b/doc/release-notes-20755.md @@ -0,0 +1,7 @@ +Updated RPCs +------------ +- `getpeerinfo` no longer returns the following fields: `addnode`, + and `whitelisted`, which were previously deprecated in v21. Instead of + `addnode`, the `connection_type` field returns manual. Instead of + `whitelisted`, the `permissions` field indicates if the peer has special + privileges. (#20755) diff --git a/src/net.cpp b/src/net.cpp index 7bd90d0973..34a3ec0ba9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -736,7 +736,6 @@ void CNode::CopyStats(CNodeStats& stats) X(cleanSubVer); } stats.fInbound = IsInboundConn(); - stats.m_manual_connection = IsManualConn(); X(m_bip152_highbandwidth_to); X(m_bip152_highbandwidth_from); { @@ -752,7 +751,6 @@ void CNode::CopyStats(CNodeStats& stats) stats.m_transport_type = info.transport_type; if (info.session_id) stats.m_session_id = HexStr(*info.session_id); } - X(m_legacyWhitelisted); X(m_permission_flags); X(m_last_ping_time); @@ -2010,8 +2008,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, .use_v2transport = use_v2transport, }); pnode->AddRef(); - // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) - pnode->m_legacyWhitelisted = legacyWhitelisted; m_msgproc->InitializeNode(*pnode, nodeServices); { diff --git a/src/net.h b/src/net.h index effa460e51..41f446c705 100644 --- a/src/net.h +++ b/src/net.h @@ -241,7 +241,6 @@ public: int nVersion; std::string cleanSubVer; bool fInbound; - bool m_manual_connection; bool m_bip152_highbandwidth_to; bool m_bip152_highbandwidth_from; int m_starting_height; @@ -250,7 +249,6 @@ public: uint64_t nRecvBytes; mapMsgTypeSize mapRecvBytesPerMsgType; NetPermissionFlags m_permission_flags; - bool m_legacyWhitelisted; std::chrono::microseconds m_last_ping_time; std::chrono::microseconds m_min_ping_time; // Our address, as reported by the peer @@ -789,8 +787,6 @@ public: bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permission_flags, permission); } - // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level - bool m_legacyWhitelisted{false}; /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */ std::atomic_bool fSuccessfullyConnected{false}; // Setting fDisconnect to true will cause the node to be disconnected the diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index cb69c3112a..8d3dc39846 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -137,8 +137,6 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"}, {RPCResult::Type::BOOL, "bip152_hb_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"}, {RPCResult::Type::BOOL, "bip152_hb_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"}, - {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection\n" - "(DEPRECATED, returned only if the config option -deprecatedrpc=getpeerinfo_addnode is passed)"}, {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"}, {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"}, {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"}, @@ -151,8 +149,6 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"}, {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"}, - {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions\n" - "(DEPRECATED, returned only if config option -deprecatedrpc=whitelisted is passed)"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"}, @@ -241,10 +237,6 @@ static RPCHelpMan getpeerinfo() obj.pushKV("inbound", stats.fInbound); obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to); obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from); - if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) { - // addnode is deprecated in v21 for removal in v22 - obj.pushKV("addnode", stats.m_manual_connection); - } obj.pushKV("masternode", stats.m_masternode_connection); if (fStateStats) { if (IsDeprecatedRPCEnabled("banscore")) { @@ -264,10 +256,6 @@ static RPCHelpMan getpeerinfo() obj.pushKV("addr_processed", statestats.m_addr_processed); obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); } - if (IsDeprecatedRPCEnabled("whitelisted")) { - // whitelisted is deprecated in v0.21 for removal in v0.22 - obj.pushKV("whitelisted", stats.m_legacyWhitelisted); - } UniValue permissions(UniValue::VARR); for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) { permissions.push_back(permission); diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index 95030b2428..828079c5b9 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -51,7 +51,7 @@ class P2PBlocksOnly(BitcoinTestFramework): assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) self.log.info("Restarting node 0 with relay permission and blocksonly") - self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted']) + self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"]) assert_equal(self.nodes[0].getrawmempool(), []) first_peer = self.nodes[0].add_p2p_connection(P2PInterface()) second_peer = self.nodes[0].add_p2p_connection(P2PInterface()) diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 2fc0c8e4cf..e7e82eccb5 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -36,35 +36,24 @@ class P2PPermissionsTests(BitcoinTestFramework): # default permissions (no specific permissions) ["-whitelist=127.0.0.1"], # Make sure the default values in the command line documentation match the ones here - ["relay", "noban", "mempool", "download"], - True) - - self.checkpermission( - # check without deprecatedrpc=whitelisted - ["-whitelist=127.0.0.1"], - # Make sure the default values in the command line documentation match the ones here - ["relay", "noban", "mempool", "download"], - None) + ["relay", "noban", "mempool", "download"]) self.checkpermission( # no permission (even with forcerelay) ["-whitelist=@127.0.0.1", "-whitelistforcerelay=1"], - [], - False) + []) self.checkpermission( # relay permission removed (no specific permissions) ["-whitelist=127.0.0.1", "-whitelistrelay=0"], - ["noban", "mempool", "download"], - True) + ["noban", "mempool", "download"]) self.checkpermission( # forcerelay and relay permission added # Legacy parameter interaction which set whitelistrelay to true # if whitelistforcerelay is true ["-whitelist=127.0.0.1", "-whitelistforcerelay"], - ["forcerelay", "relay", "noban", "mempool", "download"], - True) + ["forcerelay", "relay", "noban", "mempool", "download"]) # Let's make sure permissions are merged correctly # For this, we need to use whitebind instead of bind @@ -74,39 +63,28 @@ class P2PPermissionsTests(BitcoinTestFramework): self.checkpermission( ["-whitelist=noban@127.0.0.1"], # Check parameter interaction forcerelay should activate relay - ["noban", "bloomfilter", "forcerelay", "relay", "download"], - False) + ["noban", "bloomfilter", "forcerelay", "relay", "download"]) self.replaceinconfig(1, "whitebind=bloomfilter,forcerelay@" + ip_port, "bind=127.0.0.1") self.checkpermission( # legacy whitelistrelay should be ignored ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"], - ["noban", "mempool", "download"], - False) - - self.checkpermission( - # check without deprecatedrpc=whitelisted - ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"], - ["noban", "mempool", "download"], - None) + ["noban", "mempool", "download"]) self.checkpermission( # legacy whitelistforcerelay should be ignored ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"], - ["noban", "mempool", "download"], - False) + ["noban", "mempool", "download"]) self.checkpermission( # missing mempool permission to be considered legacy whitelisted ["-whitelist=noban@127.0.0.1"], - ["noban", "download"], - False) + ["noban", "download"]) self.checkpermission( # all permission added ["-whitelist=all@127.0.0.1"], - ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"], - False) + ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"]) self.stop_node(1) self.nodes[1].assert_start_raises_init_error(["-whitelist=oopsie@127.0.0.1"], "Invalid P2P permission", match=ErrorMatch.PARTIAL_REGEX) @@ -170,19 +148,13 @@ class P2PPermissionsTests(BitcoinTestFramework): reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid) ) - def checkpermission(self, args, expectedPermissions, whitelisted): - if whitelisted is not None: - args = [*args, '-deprecatedrpc=whitelisted'] + def checkpermission(self, args, expectedPermissions): self.restart_node(1, args) self.connect_nodes(0, 1) peerinfo = self.nodes[1].getpeerinfo()[0] - if whitelisted is None: - assert 'whitelisted' not in peerinfo - else: - assert_equal(peerinfo['whitelisted'], whitelisted) assert_equal(len(expectedPermissions), len(peerinfo['permissions'])) for p in expectedPermissions: - if not p in peerinfo['permissions']: + if p not in peerinfo['permissions']: raise AssertionError("Expected permissions %r is not granted." % p) def replaceinconfig(self, nodeid, old, new): diff --git a/test/functional/rpc_external_queue.py b/test/functional/rpc_external_queue.py old mode 100644 new mode 100755 diff --git a/test/functional/rpc_getpeerinfo_deprecation.py b/test/functional/rpc_getpeerinfo_deprecation.py index 340a66e12f..bde2058464 100755 --- a/test/functional/rpc_getpeerinfo_deprecation.py +++ b/test/functional/rpc_getpeerinfo_deprecation.py @@ -14,7 +14,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework): def run_test(self): self.test_banscore_deprecation() - self.test_addnode_deprecation() def test_banscore_deprecation(self): self.log.info("Test getpeerinfo by default no longer returns a banscore field") @@ -23,16 +22,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework): self.log.info("Test getpeerinfo returns banscore with -deprecatedrpc=banscore") assert "banscore" in self.nodes[1].getpeerinfo()[0].keys() - def test_addnode_deprecation(self): - self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"]) - self.connect_nodes(0, 1) - - self.log.info("Test getpeerinfo by default no longer returns an addnode field") - assert "addnode" not in self.nodes[0].getpeerinfo()[0].keys() - - self.log.info("Test getpeerinfo returns addnode with -deprecatedrpc=addnode") - assert "addnode" in self.nodes[1].getpeerinfo()[0].keys() - if __name__ == "__main__": GetpeerinfoDeprecationTest().main() From 16052f10aed7070d280a556c79c66a136429a0ea Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 29 Dec 2020 08:55:58 +0100 Subject: [PATCH 07/12] Merge #20791: p2p: remove unused legacyWhitelisted in AcceptConnection() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8f9ca31782372fb60377ef319fefd727bb8c5d75 p2p: remove unused legacyWhitelisted variable (Jon Atack) Pull request description: Noticed while compiling master: ``` net.cpp: In member function ‘void CConnman::AcceptConnection(const CConnman::ListenSocket&)’: net.cpp:1041:10: warning: variable ‘legacyWhitelisted’ set but not used [-Wunused-but-set-variable] 1041 | bool legacyWhitelisted = false; | ^~~~~~~~~~~~~~~~~ ``` ACKs for top commit: glozow: utACK https://github.com/bitcoin/bitcoin/pull/20791/commits/8f9ca31782372fb60377ef319fefd727bb8c5d75 MarcoFalke: review ACK 8f9ca31782372fb60377ef319fefd727bb8c5d75 Tree-SHA512: e3fb94d3d34b364b063a115ef9ed06cfd58729a790ba6ed27c7c32430fabf3ca0aa61bc6d33a1236bff802302779c8db28af351208f06c09ad12ce4873c244a6 --- src/net.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 34a3ec0ba9..c884ed264c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1893,14 +1893,12 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, int nMaxInbound = nMaxConnections - m_max_outbound; AddWhitelistPermissionFlags(permission_flags, addr); - bool legacyWhitelisted = false; if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) { NetPermissions::ClearFlag(permission_flags, NetPermissionFlags::Implicit); if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::ForceRelay); if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Relay); NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool); NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan); - legacyWhitelisted = true; } { From 6ed62b323c51d20359b53ce5ac3af76db756c5f0 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 25 Apr 2021 10:08:52 +0200 Subject: [PATCH 08/12] Merge bitcoin/bitcoin#21563: net: Restrict period when cs_vNodes mutex is locked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov) 229ac1892d807a1eea5a7c24ae0fe27dc913b1bd net: Combine two loops into one, and update comments (Hennadii Stepanov) a3d090d1103cd6c25daf07afdf4e65febca6d3f7 net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov) Pull request description: This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`. This change makes the explicit locking of recursive mutexes in the explicit order redundant. ACKs for top commit: jnewbery: utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f vasild: ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f ajtowns: utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound MarcoFalke: review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢 Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73 --- src/init.cpp | 15 +-------------- src/net.cpp | 20 ++++++++------------ src/test/fuzz/process_message.cpp | 1 - src/test/fuzz/process_messages.cpp | 1 - 4 files changed, 9 insertions(+), 28 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index e92742be1d..cbae172cb2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -254,20 +254,7 @@ void PrepareShutdown(NodeContext& node) // Because these depend on each-other, we make sure that neither can be // using the other before destroying them. if (node.peerman) UnregisterValidationInterface(node.peerman.get()); - // Follow the lock order requirements: - // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraFullOutboundCount - // which locks cs_vNodes. - // * ProcessMessage locks cs_main and g_cs_orphans before indirectly calling ForEachNode which - // locks cs_vNodes. - // * CConnman::Stop calls DeleteNode, which calls FinalizeNode, which locks cs_main and calls - // EraseOrphansFor, which locks g_cs_orphans. - // - // Thus the implicit locking order requirement is: (1) cs_main, (2) g_cs_orphans, (3) cs_vNodes. - if (node.connman) { - node.connman->StopThreads(); - LOCK2(::cs_main, ::g_cs_orphans); - node.connman->StopNodes(); - } + if (node.connman) node.connman->Stop(); StopTorControl(); diff --git a/src/net.cpp b/src/net.cpp index c884ed264c..67dee39d44 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4301,13 +4301,15 @@ void CConnman::StopNodes() } } - { - LOCK(m_nodes_mutex); - - // Close sockets - for (CNode *pnode : m_nodes) - pnode->CloseSocketDisconnect(this); + // Delete peer connections. + std::vector nodes; + WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes)); + for (CNode *pnode : nodes) { + pnode->CloseSocketDisconnect(this); + DeleteNode(pnode); } + + // Close listening sockets. for (ListenSocket& hListenSocket : vhListenSocket) { if (hListenSocket.sock) { if (m_edge_trig_events && !m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get())) { @@ -4316,12 +4318,6 @@ void CConnman::StopNodes() } } - // clean up some globals (to help leak detection) - std::vector nodes; - WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes)); - for (CNode* pnode : nodes) { - DeleteNode(pnode); - } for (CNode* pnode : m_nodes_disconnected) { DeleteNode(pnode); } diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 7c042b5cb0..a5a59e3154 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -103,7 +103,6 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE } g_setup->m_node.peerman->SendMessages(&p2p_node); SyncWithValidationInterfaceQueue(); - LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement g_setup->m_node.connman->StopNodes(); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 938f189db1..5eb6e14925 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -76,6 +76,5 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages) g_setup->m_node.peerman->SendMessages(&random_node); } SyncWithValidationInterfaceQueue(); - LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement g_setup->m_node.connman->StopNodes(); } From c52a582a3fbb00b61c80aea44fc45dd93cfa2ac8 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 8 Oct 2024 17:44:00 +0700 Subject: [PATCH 09/12] refactor: re-order conditions over flags for m_edge_trig_events - follow-up for bitcoin#21563 --- src/net.cpp | 12 ++++++------ src/util/edge.h | 1 + 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 67dee39d44..8f88057e78 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -663,8 +663,8 @@ void CNode::CloseSocketDisconnect(CConnman* connman) connman->mapSendableNodes.erase(GetId()); } - if (connman->m_edge_trig_events && !connman->m_edge_trig_events->UnregisterEvents(m_sock->Get())) { - LogPrint(BCLog::NET, "EdgeTriggeredEvents::UnregisterEvents() failed\n"); + if (connman->m_edge_trig_events) { + connman->m_edge_trig_events->UnregisterEvents(m_sock->Get()); } LogPrint(BCLog::NET, "disconnecting peer=%d\n", id); @@ -4310,10 +4310,10 @@ void CConnman::StopNodes() } // Close listening sockets. - for (ListenSocket& hListenSocket : vhListenSocket) { - if (hListenSocket.sock) { - if (m_edge_trig_events && !m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get())) { - LogPrintf("EdgeTriggeredEvents::RemoveSocket() failed\n"); + if (m_edge_trig_events) { + for (ListenSocket& hListenSocket : vhListenSocket) { + if (hListenSocket.sock) { + m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get()); } } } diff --git a/src/util/edge.h b/src/util/edge.h index d1de2f7ba6..1137ce624a 100644 --- a/src/util/edge.h +++ b/src/util/edge.h @@ -17,6 +17,7 @@ enum class SocketEventsMode : int8_t; * A manager for abstracting logic surrounding edge-triggered socket events * modes like kqueue and epoll. */ +// TODO: simplify this class to 2-3 flags; kick out everything else to Sock/~Sock and inherited classes class EdgeTriggeredEvents { public: From 7aeb0adeb9c63ac70fc82796eeff0855dd8c6c0b Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 5 Jul 2021 23:43:26 +0300 Subject: [PATCH 10/12] Merge bitcoin-core/gui#365: Draw "eye" sign at the beginning of watch-only addresses cd46c11577a05f3dc9eac94f27a6985f6ba0509e qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov) 9ea1da6fc91e17bdaa722001b97aadf576f07f65 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov) Pull request description: This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels. No longer need to extend `TransactionOverviewWidget` widget width to make "eye" signs shown: ![Screenshot from 2021-06-15 00-21-05](https://user-images.githubusercontent.com/32963518/121961807-9123b600-cd70-11eb-8cdd-8b2b0d1bf44f.png) Fixes https://github.com/bitcoin-core/gui/issues/373 ACKs for top commit: jarolrod: ACK cd46c11577a05f3dc9eac94f27a6985f6ba0509e Tree-SHA512: 0602b5bb65d53c5b18e86260750006bba03adbae181917b5a2b7f89b17290bd1f57b4f80adaba32f42cc6fb468598a888b12c0b6b09005d2f2c07bd4d1ad334a --- src/qt/overviewpage.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 30b238d4db..ca079d9e89 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -95,17 +95,21 @@ public: // Address/Label colorForeground = qvariant_cast(indexAddress.data(Qt::ForegroundRole)); QString address = indexAddress.data(Qt::DisplayRole).toString(); - painter->setPen(colorForeground); - painter->drawText(rectBottomHalf, Qt::AlignLeft | Qt::AlignVCenter, address, &rectBounding); - int address_rect_min_width = rectBounding.width(); + // Optional Watchonly indicator + QRect addressRect{rectBottomHalf}; if (index.data(TransactionTableModel::WatchonlyRole).toBool()) { QIcon iconWatchonly = qvariant_cast(index.data(TransactionTableModel::WatchonlyDecorationRole)); - QRect rectWatchonly(rectBounding.right() + 5, rectBottomHalf.top(), 16, halfheight); - iconWatchonly.paint(painter, rectWatchonly); + QRect watchonlyRect(rectBottomHalf.left(), rectBottomHalf.top(), rectBottomHalf.height(), halfheight); + iconWatchonly.paint(painter, watchonlyRect); + addressRect.setLeft(addressRect.left() + watchonlyRect.width() + 5); } + painter->setPen(colorForeground); + painter->drawText(addressRect, Qt::AlignLeft | Qt::AlignVCenter, address, &rectBounding); + int address_rect_min_width = rectBounding.width(); + const int minimum_width = std::max(address_rect_min_width, amount_bounding_rect.width() /*+ date_bounding_rect.width() */); const auto search = m_minimum_width.find(index.row()); if (search == m_minimum_width.end() || search->second != minimum_width) { From f358f2bcdd51a3f581e036876e6f4a7eeb03cc61 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 6 Jul 2021 00:00:33 +0300 Subject: [PATCH 11/12] Merge bitcoin-core/gui#375: Emit dataChanged signal to dynamically re-sort Peers table 986bf78d7e8fd9b69841ecb0decaff840efe9cff qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov) Pull request description: [By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy` > dynamically re-sorts ... data whenever the original model changes. That is not the case on master (8cdf91735f2bdc55577d84a9915f5920ce23b00a) as in ecbd91153875c8cdd5b92b840afc116f65e457fb (#164) no signals are emitted to notify about model changes. This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal. Fixes #367. An alternative to #374. ACKs for top commit: jarolrod: ACK 986bf78d7e8fd9b69841ecb0decaff840efe9cff Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4 --- src/qt/peertablemodel.cpp | 4 +++- src/qt/peertablemodel.h | 3 --- src/qt/rpcconsole.cpp | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp index 182ff17b55..ed43c513d4 100644 --- a/src/qt/peertablemodel.cpp +++ b/src/qt/peertablemodel.cpp @@ -182,5 +182,7 @@ void PeerTableModel::refresh() m_peers_data.swap(new_peers_data); } - Q_EMIT changed(); + const auto top_left = index(0, 0); + const auto bottom_right = index(rowCount() - 1, columnCount() - 1); + Q_EMIT dataChanged(top_left, bottom_right); } diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h index cf101149fd..03fc2b45da 100644 --- a/src/qt/peertablemodel.h +++ b/src/qt/peertablemodel.h @@ -74,9 +74,6 @@ public: public Q_SLOTS: void refresh(); -Q_SIGNALS: - void changed(); - private: //! Internal peer data structure. QList m_peers_data{}; diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index b2be393418..5c623b6ac6 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -39,6 +39,7 @@ #endif #include +#include #include #include #include @@ -743,7 +744,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_ // peer table signal handling - update peer details when selecting new node connect(ui->peerWidget->selectionModel(), &QItemSelectionModel::selectionChanged, this, &RPCConsole::updateDetailWidget); - connect(model->getPeerTableModel(), &PeerTableModel::changed, this, &RPCConsole::updateDetailWidget); + connect(model->getPeerTableModel(), &QAbstractItemModel::dataChanged, [this] { updateDetailWidget(); }); // set up ban table ui->banlistWidget->setModel(model->getBanTableModel()); From 24c01934a2851a9946f9b09c1695824e12a30726 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 2 Sep 2024 16:03:02 +0700 Subject: [PATCH 12/12] fix: small fixup for bitcoin#14918 and bitcoin#21679 --- src/rpc/mining.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 0444fc5a91..2a24bf1eb7 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -571,7 +571,7 @@ static RPCHelpMan getblocktemplate() {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported feature, 'longpoll', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"}, }, }, - {"rules", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "A list of strings", + {"rules", RPCArg::Type::ARR, RPCArg::Optional::NO, "A list of strings", { {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported softfork deployment"}, },