Merge #1565: Guard against trimmed chain_start in TryLowWorkHeadersSync

e151cad test: update and re-enable trim headers functional test (Byron Hambly)
3824d7d add guard to TryLowWorkHeadersSync to prevent assert_untrimmed (Tom Trevethan)

Pull request description:

  When -trim_headers is enabled, TryLowWorkHeadersSync can receive a chain_start_header pointing to a trimmed block index entry. The `HeadersSyncState` constructor immediately calls `GetBlockHeader()` on it via the m_last_header_received initialiser, which hits `assert_untrimmed()` and aborts the node.

  Conditional untrim_to guard added inside `TryLowWorkHeadersSync`, before `HeadersSyncState`.

  For nodes running -trim_headers, in practice the crash path is not reachable on Elements because `already_validated_work` is always true on a federated chain (nChainWork == nMinimumChainWork == 0), so `TryLowWorkHeadersSync` is not entered. But this ensures consistency with guarded serving paths.

  `TryLowWorkHeadersSync` and `HeadersSyncState` were introduced in Bitcoin v24 (bitcoin/bitcoin#25717) and so this is not applicable to Elements v23.

ACKs for top commit:
  delta1:
    ACK e151cad; tested locally

Tree-SHA512: 824344079b2136f7e2005968eafa3ad4385ed13ee3b3f3f79b3f878ed7e7dd88c68c85c1d6dd6e989f8484561984e6350fa81a6a19c96550787d0e615b76e87c

Author: tomt1664

src/net_processing.cpp

@@ -2634,6 +2634,15 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
         // otherwise they don't have more headers after this so no point in
         // trying to sync their too-little-work chain.
         if (headers.size() == m_opts.max_headers_result) {
+            // chain_start_header may refer to a block deep in the chain that
+            // has been trimmed from memory by -trim_headers. The HeadersSyncState
+            // constructor calls GetBlockHeader() on it via m_last_header_received,
+            // which asserts untrimmed. Reload from disk first if needed.
+            CBlockIndex tmpBlockIndexFull;
+            const CBlockIndex* chain_start_untrimmed = chain_start_header->trimmed()
+                ? chain_start_header->untrim_to(&tmpBlockIndexFull)
+                : chain_start_header;
+
             // Note: we could advance to the last header in this set that is
             // known to us, rather than starting at the first header (which we
             // may already have); however this is unlikely to matter much since
@@ -2645,7 +2654,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
             // advancing to the first unknown header would be a small effect.
             LOCK(peer.m_headers_sync_mutex);
             peer.m_headers_sync.reset(new HeadersSyncState(peer.m_id, m_chainparams.GetConsensus(),
-                chain_start_header, minimum_chain_work));
+                chain_start_untrimmed, minimum_chain_work));
 
             // Now a HeadersSyncState object for tracking this synchronization
             // is created, process the headers using it as normal. Failures are

src/wallet/wallet.cpp

@@ -1654,6 +1654,13 @@ isminetype CWallet::IsMine(const CScript& script) const
         return spkm->IsMine(script);
     }
 
+    // ELEMENTS: OP_TRUE outputs aren't derived from any descriptor, so they never
+    // land in m_cached_spks above. Custom chains (e.g. elementsregtest) rely on
+    // -anyonecanspendaremine to treat them as wallet funds regardless of wallet type.
+    if (Params().anyonecanspend_aremine && script == CScript() << OP_TRUE) {
+        return ISMINE_SPENDABLE;
+    }
+
     return ISMINE_NO;
 }
 

test/functional/feature_trim_headers.py

@@ -5,25 +5,20 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 from test_framework import (
-    address,
     key,
 )
 from test_framework.messages import (
     CBlock,
     from_hex,
+    ser_uint256,
 )
 from test_framework.script import (
     OP_NOP,
     OP_RETURN,
-    CScript
+    CScript,
+    SIGHASH_ALL,
 )
 
-# Generate wallet import format from private key.
-def wif(pk):
-    # Base58Check version for regtest WIF keys is 0xef = 239
-    pk_compressed = pk + bytes([0x1])
-    return address.byte_to_base58(pk_compressed, 239)
-
 # The signblockscript is a Bitcoin Script k-of-n multisig script.
 def make_signblockscript(num_nodes, required_signers, keys):
     assert num_nodes >= required_signers
@@ -39,21 +34,17 @@ def make_signblockscript(num_nodes, required_signers, keys):
 class TrimHeadersTest(BitcoinTestFramework):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
-        self.skip_if_no_bdb()
 
     def add_options(self, parser):
         self.add_wallet_options(parser)
 
     # Dynamically generate N keys to be used for block signing.
     def init_keys(self, num_keys):
         self.keys = []
-        self.wifs = []
         for i in range(num_keys):
             k = key.ECKey()
             k.generate()
-            w = wif(k.get_bytes())
             self.keys.append(k)
-            self.wifs.append(w)
 
     def set_test_params(self):
         self.num_nodes = 3
@@ -92,6 +83,21 @@ def check_height(self, expected_height, all=False, verbose=True):
         else:
             assert_equal(self.nodes[0].getblockcount(), expected_height)
 
+    # The signblock RPC only works on legacy (BDB) wallets.
+    # Sign the block header hash directly with the key instead,
+    # then use combineblocksigs (a non-wallet RPC).
+    def sign_block(self, key, block):
+        block.rehash()
+        msg = ser_uint256(block.sha256)
+        sig = key.sign_ecdsa(msg)
+        if not block.m_dynafed_params.is_null():
+            sig += bytes([SIGHASH_ALL])
+
+        return [{
+            "pubkey": key.get_pubkey().get_bytes().hex(),
+            "sig": sig.hex(),
+        }]
+
     def mine_block(self, make_transactions):
         # alternate mining between the signing nodes
         mineridx = self.nodes[0].getblockcount() % self.required_signers  # assuming in sync
@@ -141,7 +147,7 @@ def mine_block(self, make_transactions):
         sigs = []
         for i in range(self.num_keys):
             result = miner.combineblocksigs(block, sigs, self.witnessScript)
-            sigs = sigs + self.nodes[i].signblock(block, self.witnessScript)
+            sigs = sigs + self.sign_block(self.keys[i], block_struct)
             assert_equal(result["complete"], i >= self.required_signers)
             # submitting should have no effect pre-threshhold
             if i < self.required_signers:
@@ -171,7 +177,7 @@ def mine_large_blocks(self, n):
             block.solve()
             h = block.serialize().hex()
 
-            sigs = node.signblock(h, self.witnessScript)
+            sigs = self.sign_block(self.keys[0], block)
 
             result = node.combineblocksigs(h, sigs, self.witnessScript)
             assert_equal(result["complete"], True)
@@ -180,9 +186,6 @@ def mine_large_blocks(self, n):
 
 
     def run_test(self):
-        for i in range(self.num_keys):
-            self.nodes[i].importprivkey(self.wifs[i])
-
         expected_height = 0
         self.check_height(expected_height, all=True)
 

test/functional/test_runner.py

@@ -87,14 +87,14 @@
 #    'feature_dbcrash.py', ELEMENTS: long running test and uses excessive disk space on GHA
 #    'feature_fee_estimation.py', ELEMENTS: this is broken on v23
     'feature_index_prune.py',
-    'feature_trim_headers.py',
     'wallet_pruning.py --legacy-wallet',
 ]
 
 BASE_SCRIPTS = [
     # Scripts that are run by default.
     # vv First elements tests vv
     'example_elements_code_tutorial.py',
+    'feature_trim_headers.py',
     'feature_fedpeg.py --legacy-wallet',
     'feature_fedpeg.py --pre_transition --legacy-wallet',
     'feature_fedpeg.py --post_transition --legacy-wallet',
@@ -365,7 +365,7 @@
     #'wallet_upgradewallet.py --legacy-wallet',
     'wallet_crosschain.py',
     'mining_basic.py',
-    # ELEMENTS: PoW test set-up disabled. 
+    # ELEMENTS: PoW test set-up disabled.
     # 'mining_mainnet.py',
     'feature_signet.py',
     'p2p_mutated_blocks.py',