Merge #6240: backport: partial bitcoin#27920, #28799, #29007 - to imp…

…rove CI experience 2eadcf2 partial Merge bitcoin/bitcoin#29007: test: create deterministic addrman in the functional tests (stratospher) f95ca4e fix: unify with bitcoin: removed requirement of txindex=0 (Konstantin Akimov) bf46e7a partial Merge bitcoin/bitcoin#28799: wallet: cache descriptor ID to avoid repeated descriptor string creation (Andrew Chow) 05e5966 partial Merge bitcoin/bitcoin#27920: wallet: bugfix, always use apostrophe for spkm descriptor ID (furszy) Pull request description: ## Issue being fixed or feature implemented Lately our CI for tsan is flapping many functional tests and take long times. This PR has several important changes backported from the latest bitcoin's version to improve CI experience ## What was done? This PR has several backports that improved CI experience drastically. **Firstly, it aims to fix flapping test p2p_node_network_limited.py** For example: https://gitlab.com/dashpay/dash/-/jobs/7692635307 <details> <summary>p2p_node_network_limited.py | ✖ Failed | 28 s</summary> ``` test 2024-08-29T02:50:53.929000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_framework.py", line 158, in main self.run_test() File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/p2p_node_network_limited.py", line 79, in run_test self.nodes[0].disconnect_p2ps() File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_node.py", line 611, in disconnect_p2ps wait_until_helper(check_peers, timeout=5) File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/util.py", line 262, in wait_until_helper raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) AssertionError: Predicate '''' def check_peers(): for p in self.getpeerinfo(): for p2p in self.p2ps: if p['subver'] == p2p.strSubVer: return False return True ''' not true after 5.0 seconds ``` </details> **Secondly, it improves performance of Descriptor wallets significantly for case of `tsan` CI**. It is tiny improvement for Release build and local runs, but some fucnctional tests run as fast as twice: https://gitlab.com/dashpay/dash/-/jobs/7694458953 https://gitlab.com/dashpay/dash/-/jobs/7665132625 ``` wallet_create_tx.py --descriptors | ✓ Passed | 236 s <-- new version wallet_create_tx.py --legacy-wallet | ✓ Passed | 108 s wallet_basic.py --descriptors | ✓ Passed | 135 s <---- new version wallet_basic.py --legacy-wallet | ✓ Passed | 97 s wallet_create_tx.py --descriptors | ✓ Passed | 456 s <-- old version wallet_create_tx.py --legacy-wallet | ✓ Passed | 98 s wallet_basic.py --descriptors | ✓ Passed | 189 s <--- old version wallet_basic.py --legacy-wallet | ✓ Passed | 131 s ``` See performance investigation here: dashpay/dash#6226 ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 2eadcf2 UdjinM6: utACK 2eadcf2 Tree-SHA512: 127fbaa65c160aa95e2145a6b40d3811f7c42e36fbee9ce98a9ac021abd9cbe6edc7791870b331a54855ba891e3804885db7936ef212647b693f50f79a60d232
dashpay · Aug 30, 2024 · cddbc2a · cddbc2a
2 parents e3a5afd + 2eadcf2
commit cddbc2a
Show file tree

Hide file tree

Showing 7 changed files with 22 additions and 25 deletions.
diff --git a/src/init.cpp b/src/init.cpp
@@ -717,7 +717,6 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-watchquorums=<n>", strprintf("Watch and validate quorum communication (default: %u)", llmq::DEFAULT_WATCH_QUORUMS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_BOOL | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-disablegovernance", strprintf("Disable governance validation (0-1, default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

diff --git a/src/net.cpp b/src/net.cpp
@@ -257,10 +257,6 @@ bool IsPeerAddrLocalGood(CNode *pnode)
 std::optional<CService> GetLocalAddrForPeer(CNode& node)
 {
     CService addrLocal{GetLocalAddress(node.addr)};
-    if (gArgs.GetBoolArg("-addrmantest", false)) {
-        // use IPv4 loopback during addrmantest
-        addrLocal = CService(LookupNumeric("127.0.0.1", GetListenPort()));
-    }
     // If discovery is enabled, sometimes give our peer the address it
     // tells us that it sees us as in case it has a better idea of our
     // address than we do.
@@ -280,7 +276,7 @@ std::optional<CService> GetLocalAddrForPeer(CNode& node)
             addrLocal.SetIP(node.GetAddrLocal());
         }
     }
-    if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
+    if (addrLocal.IsRoutable())
     {
         LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), node.GetId());
         return addrLocal;

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
@@ -1162,6 +1162,14 @@ std::unique_ptr<Descriptor> InferDescriptor(const CScript& script, const Signing
     return InferScript(script, ParseScriptContext::TOP, provider);
 }
 
+uint256 DescriptorID(const Descriptor& desc)
+{
+    std::string desc_str = desc.ToString();
+    uint256 id;
+    CSHA256().Write((unsigned char*)desc_str.data(), desc_str.size()).Finalize(id.begin());
+    return id;
+}
+
 void DescriptorCache::CacheParentExtPubKey(uint32_t key_exp_pos, const CExtPubKey& xpub)
 {
     m_parent_xpubs[key_exp_pos] = xpub;

diff --git a/src/script/descriptor.h b/src/script/descriptor.h
@@ -182,4 +182,9 @@ std::string GetDescriptorChecksum(const std::string& descriptor);
  */
 std::unique_ptr<Descriptor> InferDescriptor(const CScript& script, const SigningProvider& provider);
 
+/** Unique identifier that may not change over time, unless explicitly marked as not backwards compatible.
+*   This is not part of BIP 380, not guaranteed to be interoperable and should not be exposed to the user.
+*/
+uint256 DescriptorID(const Descriptor& desc);
+
 #endif // BITCOIN_SCRIPT_DESCRIPTOR_H
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
@@ -2309,10 +2309,7 @@ std::unique_ptr<CKeyMetadata> DescriptorScriptPubKeyMan::GetMetadata(const CTxDe
 uint256 DescriptorScriptPubKeyMan::GetID() const
 {
     LOCK(cs_desc_man);
-    std::string desc_str = m_wallet_descriptor.descriptor->ToString();
-    uint256 id;
-    CSHA256().Write((unsigned char*)desc_str.data(), desc_str.size()).Finalize(id.begin());
-    return id;
+    return m_wallet_descriptor.id;
 }
 
 void DescriptorScriptPubKeyMan::SetInternal(bool internal)
@@ -2371,7 +2368,7 @@ bool DescriptorScriptPubKeyMan::AddCryptedKey(const CKeyID& key_id, const CPubKe
 bool DescriptorScriptPubKeyMan::HasWalletDescriptor(const WalletDescriptor& desc) const
 {
     LOCK(cs_desc_man);
-    return m_wallet_descriptor.descriptor != nullptr && desc.descriptor != nullptr && m_wallet_descriptor.descriptor->ToString() == desc.descriptor->ToString();
+    return !m_wallet_descriptor.id.IsNull() && !desc.id.IsNull() && m_wallet_descriptor.id == desc.id;
 }
 
 void DescriptorScriptPubKeyMan::WriteDescriptor()

diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h
@@ -67,6 +67,7 @@ class WalletDescriptor
 {
 public:
     std::shared_ptr<Descriptor> descriptor;
+    uint256 id; // Descriptor ID (calculated once at descriptor initialization/deserialization)
     uint64_t creation_time = 0;
     int32_t range_start = 0; // First item in range; start of range, inclusive, i.e. [range_start, range_end). This never changes.
     int32_t range_end = 0; // Item after the last; end of range, exclusive, i.e. [range_start, range_end). This will increment with each TopUp()
@@ -80,7 +81,8 @@ class WalletDescriptor
         descriptor = Parse(str, keys, error, true);
         if (!descriptor) {
             throw std::ios_base::failure("Invalid descriptor: " + error);
-         }
+        }
+        id = DescriptorID(*descriptor);
     }
 
     SERIALIZE_METHODS(WalletDescriptor, obj)
@@ -92,7 +94,7 @@ class WalletDescriptor
      }
 
     WalletDescriptor() {}
-    WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index) : descriptor(descriptor), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index) {}
+    WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index) : descriptor(descriptor), id(DescriptorID(*descriptor)), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index) { }
 };
 
 #endif // BITCOIN_WALLET_WALLETUTIL_H
diff --git a/test/functional/p2p_node_network_limited.py b/test/functional/p2p_node_network_limited.py
@@ -8,7 +8,7 @@
 and that it responds to getdata requests for blocks correctly:
     - send a block within 288 + 2 of the tip
     - disconnect peers who request blocks older than that."""
-from test_framework.messages import CInv, MSG_BLOCK, msg_getdata, NODE_BLOOM, NODE_NETWORK_LIMITED, NODE_HEADERS_COMPRESSED, msg_verack
+from test_framework.messages import CInv, MSG_BLOCK, msg_getdata, NODE_BLOOM, NODE_NETWORK_LIMITED, NODE_HEADERS_COMPRESSED
 from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
@@ -32,7 +32,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
-        self.extra_args = [['-prune=550', '-txindex=0', '-addrmantest'], [], []]
+        self.extra_args = [['-prune=550'], [], []]
 
     def disconnect_all(self):
         self.disconnect_nodes(0, 1)
@@ -66,16 +66,6 @@ def run_test(self):
         self.log.info("Requesting block at height 2 (tip-289) must fail (ignored).")
         node.send_getdata_for_block(blocks[0])  # first block outside of the 288+2 limit
         node.wait_for_disconnect(5)
-
-        self.log.info("Check local address relay, do a fresh connection.")
-        self.nodes[0].disconnect_p2ps()
-        node1 = self.nodes[0].add_p2p_connection(P2PIgnoreInv())
-        node1.send_message(msg_verack())
-
-        node1.wait_for_addr()
-        #must relay address with NODE_NETWORK_LIMITED
-        assert_equal(node1.firstAddrnServices, expected_services)
-
         self.nodes[0].disconnect_p2ps()
 
         # connect unsynced node 2 with pruned NODE_NETWORK_LIMITED peer