From a51e91783aac0beefcb604be159eb1cb96a39051 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 23 Apr 2024 18:08:35 -0400 Subject: [PATCH 01/40] validation: add RecalculateBestHeader() function It recalculates m_best_header by looping over the entire block index. Even though this is not very performant, it will only be used in rare situations that cannot be triggered by others without a cost: As part of to invalidateblock / reconsiderblock rpcs, or when a block with an accepted header with valid PoW turns out to be invalid later during full validation. --- src/validation.cpp | 11 +++++++++++ src/validation.h | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 8e4ea8eda2f62..7430ec60d1133 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6396,6 +6396,17 @@ std::optional ChainstateManager::GetSnapshotBaseHeight() const return base ? std::make_optional(base->nHeight) : std::nullopt; } +void ChainstateManager::RecalculateBestHeader() +{ + AssertLockHeld(cs_main); + m_best_header = ActiveChain().Tip(); + for (auto& entry : m_blockman.m_block_index) { + if (!(entry.second.nStatus & BLOCK_FAILED_MASK) && m_best_header->nChainWork < entry.second.nChainWork) { + m_best_header = &entry.second; + } + } +} + bool ChainstateManager::ValidatedSnapshotCleanup() { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index 059ae52bdf4ca..61a9586d7445a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1321,6 +1321,11 @@ class ChainstateManager //! nullopt. std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! If, due to invalidation / reconsideration of blocks, the previous + //! best header is no longer valid / guaranteed to be the most-work + //! header in our block-index not known to be invalid, recalculate it. + void RecalculateBestHeader() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + CCheckQueue& GetCheckQueue() { return m_script_check_queue; } ~ChainstateManager(); From 9275e9689a426964f5eaee65e356754a0548d926 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 14 Aug 2024 16:47:49 -0400 Subject: [PATCH 02/40] rpc: call RecalculateBestHeader as part of reconsiderblock Co-authored-by: Fabian Jahr --- src/rpc/blockchain.cpp | 1 + test/functional/rpc_invalidateblock.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7ffd03e71af5a..4a7af39b15144 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1648,6 +1648,7 @@ void ReconsiderBlock(ChainstateManager& chainman, uint256 block_hash) { } chainman.ActiveChainstate().ResetBlockFailureFlags(pblockindex); + chainman.RecalculateBestHeader(); } BlockValidationState state; diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index db79d55259007..c7c7d578bbba2 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -83,6 +83,8 @@ def run_test(self): self.nodes[1].reconsiderblock(blocks[-4]) # Should be back at the tip by now assert_equal(self.nodes[1].getbestblockhash(), blocks[-1]) + # Should report consistent blockchain info + assert_equal(self.nodes[1].getblockchaininfo()["headers"], self.nodes[1].getblockchaininfo()["blocks"]) self.log.info("Verify that invalidating an unknown block throws an error") assert_raises_rpc_error(-5, "Block not found", self.nodes[1].invalidateblock, "00" * 32) From 783cb7337f72a3c7b2e74efd677a8ff0c375fe10 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 14 Aug 2024 17:28:55 -0400 Subject: [PATCH 03/40] validation: call RecalculateBestHeader in InvalidChainFound This means that it is being called in two situations: 1.) As part of the invalidateblock rpc 2.) When we receive a block for which we have a valid header in our block index, but the block turns out to be invalid --- src/validation.cpp | 2 +- test/functional/rpc_invalidateblock.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7430ec60d1133..1ccf5656fc631 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2046,7 +2046,7 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew) m_chainman.m_best_invalid = pindexNew; } if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { - m_chainman.m_best_header = m_chain.Tip(); + m_chainman.RecalculateBestHeader(); } LogPrintf("%s: invalid block=%s height=%d log2_work=%f date=%s\n", __func__, diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index c7c7d578bbba2..f83bc901a2aac 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -41,6 +41,8 @@ def run_test(self): self.nodes[0].invalidateblock(badhash) assert_equal(self.nodes[0].getblockcount(), 4) assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) + # Should report consistent blockchain info + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) self.log.info("Make sure we won't reorg to a lower work chain:") self.connect_nodes(1, 2) From f5149ddb9b7de3559943d7fda0f440e59413dfb5 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 23 Apr 2024 17:36:39 -0400 Subject: [PATCH 04/40] validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD Without doing so, header-only chains building on a chain that will be marked as invalid would still be eligible for m_best_header. This improves both getblockchaininfo and getchaintips behavior. While this adds an iteration over the entire block index, it can only be triggered by the user (invalidateblock) or by others at a cost (the header needs to be accepted in the first place, so it needs valid PoW). Co-authored-by: TheCharlatan --- src/validation.cpp | 12 ++++++++++++ src/validation.h | 3 +++ test/functional/rpc_invalidateblock.py | 25 ++++++++++++++++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1ccf5656fc631..dcaa8dba26602 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2045,6 +2045,7 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew) if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) { m_chainman.m_best_invalid = pindexNew; } + SetBlockFailureFlags(pindexNew); if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { m_chainman.RecalculateBestHeader(); } @@ -3785,6 +3786,17 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde return true; } +void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block) +{ + AssertLockHeld(cs_main); + + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (block_index.GetAncestor(invalid_block->nHeight) == invalid_block && !(block_index.nStatus & BLOCK_FAILED_MASK)) { + block_index.nStatus |= BLOCK_FAILED_CHILD; + } + } +} + void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index 61a9586d7445a..29ff6bd94cd4e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -735,6 +735,9 @@ class Chainstate EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) LOCKS_EXCLUDED(::cs_main); + /** Set invalidity status to all descendants of a block */ + void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Remove invalidity status from a block and its descendants. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index f83bc901a2aac..e2eb033f09c75 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -6,6 +6,10 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR +from test_framework.blocktools import ( + create_block, + create_coinbase, +) from test_framework.util import ( assert_equal, assert_raises_rpc_error, @@ -35,15 +39,34 @@ def run_test(self): self.connect_nodes(0, 1) self.sync_blocks(self.nodes[0:2]) assert_equal(self.nodes[0].getblockcount(), 6) - badhash = self.nodes[1].getblockhash(2) + + # Add a header to the tip of node 0 without submitting the block. This shouldn't + # affect results since this chain will be invalidated next. + tip = self.nodes[0].getbestblockhash() + block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time'] + 1 + block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=4) + block.solve() + self.nodes[0].submitheader(block.serialize().hex()) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1) self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain") + badhash = self.nodes[1].getblockhash(2) self.nodes[0].invalidateblock(badhash) assert_equal(self.nodes[0].getblockcount(), 4) assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) # Should report consistent blockchain info assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) + self.log.info("Reconsider block 6 on node 0 again and verify that the best header is set correctly") + self.nodes[0].reconsiderblock(tip) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1) + + self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain again") + self.nodes[0].invalidateblock(badhash) + assert_equal(self.nodes[0].getblockcount(), 4) + assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) + self.log.info("Make sure we won't reorg to a lower work chain:") self.connect_nodes(1, 2) self.log.info("Sync node 2 to node 1 so both have 6 blocks") From ccd98ea4c88fc1aa959e41e0686d8dff00a44209 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 29 May 2024 16:48:16 -0400 Subject: [PATCH 05/40] test: cleanup rpc_getchaintips.py Remove whitespace that doesn't conform with pep8 and turn some comments into log messages. --- test/functional/rpc_getchaintips.py | 39 +++++++++++++++-------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py index 1bd4413ce1ec2..e6d8544d22c75 100755 --- a/test/functional/rpc_getchaintips.py +++ b/test/functional/rpc_getchaintips.py @@ -18,44 +18,45 @@ def set_test_params(self): self.num_nodes = 4 def run_test(self): + self.log.info("Test getchaintips behavior with two chains of different length") tips = self.nodes[0].getchaintips() assert_equal(len(tips), 1) assert_equal(tips[0]['branchlen'], 0) assert_equal(tips[0]['height'], 200) assert_equal(tips[0]['status'], 'active') - # Split the network and build two chains of different lengths. + self.log.info("Split the network and build two chains of different lengths.") self.split_network() self.generate(self.nodes[0], 10, sync_fun=lambda: self.sync_all(self.nodes[:2])) self.generate(self.nodes[2], 20, sync_fun=lambda: self.sync_all(self.nodes[2:])) - tips = self.nodes[1].getchaintips () - assert_equal (len (tips), 1) + tips = self.nodes[1].getchaintips() + assert_equal(len(tips), 1) shortTip = tips[0] - assert_equal (shortTip['branchlen'], 0) - assert_equal (shortTip['height'], 210) - assert_equal (tips[0]['status'], 'active') + assert_equal(shortTip['branchlen'], 0) + assert_equal(shortTip['height'], 210) + assert_equal(tips[0]['status'], 'active') - tips = self.nodes[3].getchaintips () - assert_equal (len (tips), 1) + tips = self.nodes[3].getchaintips() + assert_equal(len(tips), 1) longTip = tips[0] - assert_equal (longTip['branchlen'], 0) - assert_equal (longTip['height'], 220) - assert_equal (tips[0]['status'], 'active') + assert_equal(longTip['branchlen'], 0) + assert_equal(longTip['height'], 220) + assert_equal(tips[0]['status'], 'active') - # Join the network halves and check that we now have two tips + self.log.info("Join the network halves and check that we now have two tips") # (at least at the nodes that previously had the short chain). - self.join_network () + self.join_network() - tips = self.nodes[0].getchaintips () - assert_equal (len (tips), 2) - assert_equal (tips[0], longTip) + tips = self.nodes[0].getchaintips() + assert_equal(len(tips), 2) + assert_equal(tips[0], longTip) - assert_equal (tips[1]['branchlen'], 10) - assert_equal (tips[1]['status'], 'valid-fork') + assert_equal(tips[1]['branchlen'], 10) + assert_equal(tips[1]['status'], 'valid-fork') tips[1]['branchlen'] = 0 tips[1]['status'] = 'active' - assert_equal (tips[1], shortTip) + assert_equal(tips[1], shortTip) if __name__ == '__main__': GetChainTipsTest(__file__).main() From 0bd53d913c1c2ffd2d0779f01bc51c81537b6992 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 16 Aug 2024 11:38:55 -0400 Subject: [PATCH 06/40] test: add test for getchaintips behavior with invalid chains This test would fail to mark the chain tip as "invalid" instead of "headers-only" without the previous commit marking the headers as BLOCK_FAILED_CHILD. --- test/functional/rpc_getchaintips.py | 32 +++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py index e6d8544d22c75..226e995307cef 100755 --- a/test/functional/rpc_getchaintips.py +++ b/test/functional/rpc_getchaintips.py @@ -10,6 +10,10 @@ - verify that getchaintips now returns two chain tips. """ +from test_framework.blocktools import ( + create_block, + create_coinbase, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal @@ -58,5 +62,33 @@ def run_test(self): tips[1]['status'] = 'active' assert_equal(tips[1], shortTip) + self.log.info("Test getchaintips behavior with invalid blocks") + self.disconnect_nodes(0, 1) + n0 = self.nodes[0] + tip = int(n0.getbestblockhash(), 16) + start_height = self.nodes[0].getblockcount() + # Create invalid block (too high coinbase) + block_time = n0.getblock(n0.getbestblockhash())['time'] + 1 + invalid_block = create_block(tip, create_coinbase(start_height+1, nValue=100), block_time) + invalid_block.solve() + + block_time += 1 + block2 = create_block(invalid_block.sha256, create_coinbase(2), block_time, version=4) + block2.solve() + + self.log.info("Submit headers-only chain") + n0.submitheader(invalid_block.serialize().hex()) + n0.submitheader(block2.serialize().hex()) + tips = n0.getchaintips() + assert_equal(len(tips), 3) + assert_equal(tips[0]['status'], 'headers-only') + + self.log.info("Submit invalid block that invalidates the headers-only chain") + n0.submitblock(invalid_block.serialize().hex()) + tips = n0.getchaintips() + assert_equal(len(tips), 3) + assert_equal(tips[0]['status'], 'invalid') + + if __name__ == '__main__': GetChainTipsTest(__file__).main() From 192dac1d3370edd579db235d69c034726f37c8da Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 2 Sep 2024 14:07:31 +0200 Subject: [PATCH 07/40] [refactor] Cleanup BlockAssembler mempool usage The `addPackageTxs` method of the `BlockAssembler` currently has access to two mempool variables, as an argument and as a member. Clean this up and clarify that they both are the same mempool instance by removing the argument and instead only using the member variable in the method. Co-Authored-By: Anthony Towns Co-authored-by: stickies-v --- src/node/miner.cpp | 8 ++++---- src/node/miner.h | 7 +++++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 97f6ac346a5fc..afa410da1001a 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -142,8 +142,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc int nPackagesSelected = 0; int nDescendantsUpdated = 0; if (m_mempool) { - LOCK(m_mempool->cs); - addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated); + addPackageTxs(nPackagesSelected, nDescendantsUpdated); } const auto time_1{SteadyClock::now()}; @@ -292,9 +291,10 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve // Each time through the loop, we compare the best transaction in // mapModifiedTxs with the next transaction in the mempool to decide what // transaction package to work on next. -void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) +void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) { - AssertLockHeld(mempool.cs); + const auto& mempool{*Assert(m_mempool)}; + LOCK(mempool.cs); // mapModifiedTx will store sorted packages after they are modified // because some of their txs are already in the block diff --git a/src/node/miner.h b/src/node/miner.h index 1b8294376692e..25ce110b34886 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -187,8 +187,11 @@ class BlockAssembler // Methods for how to add transactions to a block. /** Add transactions based on feerate including unconfirmed ancestors * Increments nPackagesSelected / nDescendantsUpdated with corresponding - * statistics from the package selection (for logging statistics). */ - void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); + * statistics from the package selection (for logging statistics). + * + * @pre BlockAssembler::m_mempool must not be nullptr + */ + void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(!m_mempool->cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ From 33af14b62e47d8ef145575882b3efe1a7d7aa9a8 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 5 Oct 2024 13:00:23 -0400 Subject: [PATCH 08/40] test: reduce assert_debug_log reliance p2p_orphan_handling now uses tx_in_orphanage to more directly check for inclusion/exclusion in the orphanage. --- test/functional/p2p_orphan_handling.py | 27 +++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 9ec23e9d6eb25..db7f079d6d815 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -210,9 +210,9 @@ def test_orphan_rejected_parents_exceptions(self): # Relay the child. It should not be accepted because it has missing inputs. # Its parent should not be requested because its hash (txid == wtxid) has been added to the rejection filter. - with node.assert_debug_log(['not keeping orphan with rejected parents {}'.format(child_nonsegwit["txid"])]): - self.relay_transaction(peer2, child_nonsegwit["tx"]) + self.relay_transaction(peer2, child_nonsegwit["tx"]) assert child_nonsegwit["txid"] not in node.getrawmempool() + assert not tx_in_orphanage(node, child_nonsegwit["tx"]) # No parents are requested. self.nodes[0].bumpmocktime(GETDATA_TX_INTERVAL) @@ -402,15 +402,15 @@ def test_orphan_inherit_rejection(self): # Relay the child. It should be rejected for having missing parents, and this rejection is # cached by txid and wtxid. - with node.assert_debug_log(['not keeping orphan with rejected parents {}'.format(child["txid"])]): - self.relay_transaction(peer1, child["tx"]) + self.relay_transaction(peer1, child["tx"]) assert_equal(0, len(node.getrawmempool())) + assert not tx_in_orphanage(node, child["tx"]) peer1.assert_never_requested(parent_low_fee_nonsegwit["txid"]) # Grandchild should also not be kept in orphanage because its parent has been rejected. - with node.assert_debug_log(['not keeping orphan with rejected parents {}'.format(grandchild["txid"])]): - self.relay_transaction(peer2, grandchild["tx"]) + self.relay_transaction(peer2, grandchild["tx"]) assert_equal(0, len(node.getrawmempool())) + assert not tx_in_orphanage(node, grandchild["tx"]) peer2.assert_never_requested(child["txid"]) peer2.assert_never_requested(child["tx"].getwtxid()) @@ -446,8 +446,9 @@ def test_same_txid_orphan(self): # 3. Honest peer relays the real child, which is also missing parents and should be placed # in the orphanage. - with node.assert_debug_log(["missingorspent", "stored orphan tx"]): + with node.assert_debug_log(["missingorspent"]): honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + assert tx_in_orphanage(node, tx_child["tx"]) # Time out the previous request for the parent (node will not request the same transaction # from multiple nodes at the same time) @@ -496,16 +497,16 @@ def test_same_txid_orphan_of_orphan(self): # 3. Honest peer relays the grandchild, which is missing a parent. The parent by txid already # exists in orphanage, but should be re-requested because the node shouldn't assume that the # witness data is the same. In this case, a same-txid-different-witness transaction exists! - with node.assert_debug_log(["stored orphan tx"]): - honest_peer.send_and_ping(msg_tx(tx_grandchild["tx"])) + honest_peer.send_and_ping(msg_tx(tx_grandchild["tx"])) + assert tx_in_orphanage(node, tx_grandchild["tx"]) middle_txid_int = int(tx_middle["txid"], 16) node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) honest_peer.wait_for_getdata([middle_txid_int]) # 4. Honest peer relays the real child, which is also missing parents and should be placed # in the orphanage. - with node.assert_debug_log(["stored orphan tx"]): - honest_peer.send_and_ping(msg_tx(tx_middle["tx"])) + honest_peer.send_and_ping(msg_tx(tx_middle["tx"])) + assert tx_in_orphanage(node, tx_middle["tx"]) assert_equal(len(node.getrawmempool()), 0) # 5. Honest peer sends tx_grandparent @@ -550,8 +551,8 @@ def test_orphan_txid_inv(self): # 4. The child is requested. Honest peer sends it. node.bumpmocktime(TXREQUEST_TIME_SKIP) honest_peer.wait_for_getdata([child_txid_int]) - with node.assert_debug_log(["stored orphan tx"]): - honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + assert tx_in_orphanage(node, tx_child["tx"]) # 5. After first parent request times out, the node sends another one for the missing parent # of the real orphan child. From 9de9c858d5aa412e1c6086e564fbe85984a4f2d5 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 5 Oct 2024 13:01:04 -0400 Subject: [PATCH 09/40] test: enhance p2p_orphan_handling Increases test robustness by adding checks for orphanage size and presence of orphans in the orphanage --- test/functional/p2p_orphan_handling.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index db7f079d6d815..bcb4dd7f8db31 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -231,6 +231,7 @@ def test_orphan_rejected_parents_exceptions(self): # Relay the child. It should not be accepted because it has missing inputs. self.relay_transaction(peer2, child_low_fee["tx"]) assert child_low_fee["txid"] not in node.getrawmempool() + assert tx_in_orphanage(node, child_low_fee["tx"]) # The parent should be requested because even though the txid commits to the fee, it doesn't # commit to the feerate. Delayed because it's by txid and this is not a preferred relay peer. @@ -250,6 +251,7 @@ def test_orphan_rejected_parents_exceptions(self): # Relay the child. It should not be accepted because it has missing inputs. self.relay_transaction(peer2, child_invalid_witness["tx"]) assert child_invalid_witness["txid"] not in node.getrawmempool() + assert tx_in_orphanage(node, child_invalid_witness["tx"]) # The parent should be requested since the unstripped wtxid would differ. Delayed because # it's by txid and this is not a preferred relay peer. @@ -298,6 +300,7 @@ def test_orphan_multiple_parents(self): self.relay_transaction(peer, orphan["tx"]) self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) peer.sync_with_ping() + assert tx_in_orphanage(node, orphan["tx"]) assert_equal(len(peer.last_message["getdata"].inv), 2) peer.wait_for_parent_requests([int(txid_conf_old, 16), int(missing_tx["txid"], 16)]) @@ -347,6 +350,7 @@ def test_orphans_overlapping_parents(self): # Relay orphan child_A self.relay_transaction(peer_orphans, child_A["tx"]) self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + assert tx_in_orphanage(node, child_A["tx"]) # There are 3 missing parents. missing_parent_A and missing_parent_AB should be requested. # But inflight_parent_AB should not, because there is already an in-flight request for it. peer_orphans.wait_for_parent_requests([int(missing_parent_A["txid"], 16), int(missing_parent_AB["txid"], 16)]) @@ -355,6 +359,7 @@ def test_orphans_overlapping_parents(self): # Relay orphan child_B self.relay_transaction(peer_orphans, child_B["tx"]) self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + assert tx_in_orphanage(node, child_B["tx"]) # Only missing_parent_B should be requested. Not inflight_parent_AB or missing_parent_AB # because they are already being requested from peer_txrequest and peer_orphans respectively. peer_orphans.wait_for_parent_requests([int(missing_parent_B["txid"], 16)]) @@ -374,12 +379,14 @@ def test_orphan_of_orphan(self): # The node should put missing_parent_orphan into the orphanage and request missing_grandparent self.relay_transaction(peer, missing_parent_orphan["tx"]) self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + assert tx_in_orphanage(node, missing_parent_orphan["tx"]) peer.wait_for_parent_requests([int(missing_grandparent["txid"], 16)]) # The node should put the orphan into the orphanage and request missing_parent, skipping # missing_parent_orphan because it already has it in the orphanage. self.relay_transaction(peer, orphan["tx"]) self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + assert tx_in_orphanage(node, orphan["tx"]) peer.wait_for_parent_requests([int(missing_parent["txid"], 16)]) @cleanup @@ -399,6 +406,7 @@ def test_orphan_inherit_rejection(self): # Relay the parent. It should be rejected because it pays 0 fees. self.relay_transaction(peer1, parent_low_fee_nonsegwit["tx"]) + assert parent_low_fee_nonsegwit["txid"] not in node.getrawmempool() # Relay the child. It should be rejected for having missing parents, and this rejection is # cached by txid and wtxid. @@ -438,6 +446,7 @@ def test_same_txid_orphan(self): # 1. Fake orphan is received first. It is missing an input. bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + assert tx_in_orphanage(node, tx_orphan_bad_wit) # 2. Node requests the missing parent by txid. parent_txid_int = int(tx_parent["txid"], 16) @@ -488,6 +497,7 @@ def test_same_txid_orphan_of_orphan(self): # 1. Fake orphan is received first. It is missing an input. bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + assert tx_in_orphanage(node, tx_orphan_bad_wit) # 2. Node requests missing tx_grandparent by txid. grandparent_txid_int = int(tx_grandparent["txid"], 16) @@ -519,6 +529,7 @@ def test_same_txid_orphan_of_orphan(self): assert tx_middle["txid"] in node_mempool assert tx_grandchild["txid"] in node_mempool assert_equal(node.getmempoolentry(tx_middle["txid"])["wtxid"], tx_middle["wtxid"]) + assert_equal(len(node.getorphantxs()), 0) @cleanup def test_orphan_txid_inv(self): @@ -537,6 +548,7 @@ def test_orphan_txid_inv(self): # 1. Fake orphan is received first. It is missing an input. bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + assert tx_in_orphanage(node, tx_orphan_bad_wit) # 2. Node requests the missing parent by txid. parent_txid_int = int(tx_parent["txid"], 16) @@ -569,6 +581,7 @@ def test_orphan_txid_inv(self): assert tx_parent["txid"] in node_mempool assert tx_child["txid"] in node_mempool assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + assert_equal(len(node.getorphantxs()), 0) @cleanup def test_max_orphan_amount(self): From 184f34f2d0fa2e56ad594966b2b99ff4cf840d95 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 28 Oct 2024 19:11:16 -0400 Subject: [PATCH 10/40] util: Support dynamic width & precision in ConstevalFormatString This is needed in the next commit to add compile-time checking to strprintf calls, because bitcoin-cli.cpp uses dynamic width in many format strings. This change is easiest to review ignoring whitespace. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> Co-authored-by: l0rinc --- src/test/util_string_tests.cpp | 44 ++++++++++++++++--- src/util/string.h | 78 ++++++++++++++++++++-------------- 2 files changed, 82 insertions(+), 40 deletions(-) diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp index 1574fe2358279..9f5b702acd1e3 100644 --- a/src/test/util_string_tests.cpp +++ b/src/test/util_string_tests.cpp @@ -20,7 +20,7 @@ inline void PassFmt(util::ConstevalFormatString fmt) decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt); } template -inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error) +inline void FailFmtWithError(const char* wrong_fmt, std::string_view error) { BOOST_CHECK_EXCEPTION(util::ConstevalFormatString::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error)); } @@ -44,6 +44,8 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec) PassFmt<1>("%+2s"); PassFmt<1>("%.6i"); PassFmt<1>("%5.2f"); + PassFmt<1>("%5.f"); + PassFmt<1>("%.f"); PassFmt<1>("%#x"); PassFmt<1>("%1$5i"); PassFmt<1>("%1$-5i"); @@ -54,15 +56,27 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec) PassFmt<1>("%_"); PassFmt<1>("%\n"); - // The `*` specifier behavior is unsupported and can lead to runtime - // errors when used in a ConstevalFormatString. Please refer to the - // note in the ConstevalFormatString docs. - PassFmt<1>("%*c"); - PassFmt<2>("%2$*3$d"); - PassFmt<1>("%.*f"); + PassFmt<2>("%*c"); + PassFmt<2>("%+*c"); + PassFmt<2>("%.*f"); + PassFmt<3>("%*.*f"); + PassFmt<3>("%2$*3$d"); + PassFmt<3>("%2$*3$.9d"); + PassFmt<3>("%2$.*3$d"); + PassFmt<3>("%2$9.*3$d"); + PassFmt<3>("%2$+9.*3$d"); + PassFmt<4>("%3$*2$.*4$f"); + + // Make sure multiple flag characters "- 0+" are accepted + PassFmt<3>("'%- 0+*.*f'"); + PassFmt<3>("'%1$- 0+*3$.*2$f'"); auto err_mix{"Format specifiers must be all positional or all non-positional!"}; FailFmtWithError<1>("%s%1$s", err_mix); + FailFmtWithError<2>("%2$*d", err_mix); + FailFmtWithError<2>("%*2$d", err_mix); + FailFmtWithError<2>("%.*3$d", err_mix); + FailFmtWithError<2>("%2$.*d", err_mix); auto err_num{"Format specifier count must match the argument count!"}; FailFmtWithError<1>("", err_num); @@ -70,16 +84,32 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec) FailFmtWithError<2>("%s", err_num); FailFmtWithError<0>("%1$s", err_num); FailFmtWithError<2>("%1$s", err_num); + FailFmtWithError<1>("%*c", err_num); auto err_0_pos{"Positional format specifier must have position of at least 1"}; FailFmtWithError<1>("%$s", err_0_pos); FailFmtWithError<1>("%$", err_0_pos); FailFmtWithError<0>("%0$", err_0_pos); FailFmtWithError<0>("%0$s", err_0_pos); + FailFmtWithError<2>("%2$*$d", err_0_pos); + FailFmtWithError<2>("%2$*0$d", err_0_pos); + FailFmtWithError<3>("%3$*2$.*$f", err_0_pos); + FailFmtWithError<3>("%3$*2$.*0$f", err_0_pos); auto err_term{"Format specifier incorrectly terminated by end of string"}; FailFmtWithError<1>("%", err_term); + FailFmtWithError<1>("%9", err_term); + FailFmtWithError<1>("%9.", err_term); + FailFmtWithError<1>("%9.9", err_term); + FailFmtWithError<1>("%*", err_term); + FailFmtWithError<1>("%+*", err_term); + FailFmtWithError<1>("%.*", err_term); + FailFmtWithError<1>("%9.*", err_term); FailFmtWithError<1>("%1$", err_term); + FailFmtWithError<1>("%1$9", err_term); + FailFmtWithError<2>("%1$*2$", err_term); + FailFmtWithError<2>("%1$.*2$", err_term); + FailFmtWithError<2>("%1$9.*2$", err_term); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/string.h b/src/util/string.h index c5183d6c80191..c059f989250dc 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -25,53 +25,65 @@ namespace util { * strings, to reduce the likelihood of tinyformat throwing exceptions at * run-time. Validation is partial to try and prevent the most common errors * while avoiding re-implementing the entire parsing logic. - * - * @note Counting of `*` dynamic width and precision fields (such as `%*c`, - * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as - * they are not used in the codebase. Usage of these fields is not counted and - * can lead to run-time exceptions. Code wanting to use the `*` specifier can - * side-step this struct and call tinyformat directly. */ template struct ConstevalFormatString { const char* const fmt; consteval ConstevalFormatString(const char* str) : fmt{str} { Detail_CheckNumFormatSpecifiers(fmt); } - constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str) + constexpr static void Detail_CheckNumFormatSpecifiers(const char* str) { unsigned count_normal{0}; // Number of "normal" specifiers, like %s unsigned count_pos{0}; // Max number in positional specifier, like %8$s - for (auto it{str.begin()}; it < str.end();) { - if (*it != '%') { - ++it; - continue; - } + for (auto it{str}; *it != '\0'; ++it) { + if (*it != '%' || *++it == '%') continue; // Skip escaped %% - if (++it >= str.end()) throw "Format specifier incorrectly terminated by end of string"; - if (*it == '%') { - // Percent escape: %% - ++it; - continue; - } + auto add_arg = [&] { + unsigned maybe_num{0}; + while ('0' <= *it && *it <= '9') { + maybe_num *= 10; + maybe_num += *it - '0'; + ++it; + } - unsigned maybe_num{0}; - while ('0' <= *it && *it <= '9') { - maybe_num *= 10; - maybe_num += *it - '0'; - ++it; + if (*it == '$') { + ++it; + // Positional specifier, like %8$s + if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; + count_pos = std::max(count_pos, maybe_num); + } else { + // Non-positional specifier, like %s + ++count_normal; + } }; - if (*it == '$') { - // Positional specifier, like %8$s - if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; - count_pos = std::max(count_pos, maybe_num); - if (++it >= str.end()) throw "Format specifier incorrectly terminated by end of string"; - } else { - // Non-positional specifier, like %s - ++count_normal; + // Increase argument count and consume positional specifier, if present. + add_arg(); + + // Consume flags. + while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it; + + auto parse_size = [&] { + if (*it == '*') { + ++it; + add_arg(); + } else { + while ('0' <= *it && *it <= '9') ++it; + } + }; + + // Consume dynamic or static width value. + parse_size(); + + // Consume dynamic or static precision value. + if (*it == '.') { ++it; + parse_size(); } - // The remainder "[flags][width][.precision][length]type" of the - // specifier is not checked. Parsing continues with the next '%'. + + if (*it == '\0') throw "Format specifier incorrectly terminated by end of string"; + + // Length and type in "[flags][width][.precision][length]type" + // is not checked. Parsing continues with the next '%'. } if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!"; unsigned count{count_normal | count_pos}; From fe39acf88ff552bfc4a502c99774375b91824bb1 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 28 Oct 2024 19:13:46 -0400 Subject: [PATCH 11/40] tinyformat: Add compile-time checking for literal format strings Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> --- src/tinyformat.h | 30 +++++++++++++++++------------- src/util/string.h | 9 --------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/tinyformat.h b/src/tinyformat.h index f536306375e59..0d0e9149ccd55 100644 --- a/src/tinyformat.h +++ b/src/tinyformat.h @@ -145,6 +145,7 @@ namespace tfm = tinyformat; #include #include #include // Added for Bitcoin Core +#include // Added for Bitcoin Core #ifndef TINYFORMAT_ASSERT # include @@ -178,6 +179,18 @@ namespace tfm = tinyformat; namespace tinyformat { +// Added for Bitcoin Core. Wrapper for checking format strings at compile time. +// Unlike ConstevalFormatString this supports std::string for runtime string +// formatting without compile time checks. +template +struct FormatStringCheck { + consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString{str}.fmt} {} + FormatStringCheck(const std::string& str) : fmt{str.c_str()} {} + FormatStringCheck(util::ConstevalFormatString str) : fmt{str.fmt} {} + operator const char*() { return fmt; } + const char* fmt; +}; + // Added for Bitcoin Core class format_error: public std::runtime_error { @@ -1056,7 +1069,7 @@ inline void vformat(std::ostream& out, const char* fmt, FormatListRef list) /// Format list of arguments to the stream according to given format string. template -void format(std::ostream& out, const char* fmt, const Args&... args) +void format(std::ostream& out, FormatStringCheck fmt, const Args&... args) { vformat(out, fmt, makeFormatList(args...)); } @@ -1064,7 +1077,7 @@ void format(std::ostream& out, const char* fmt, const Args&... args) /// Format list of arguments according to the given format string and return /// the result as a string. template -std::string format(const char* fmt, const Args&... args) +std::string format(FormatStringCheck fmt, const Args&... args) { std::ostringstream oss; format(oss, fmt, args...); @@ -1073,13 +1086,13 @@ std::string format(const char* fmt, const Args&... args) /// Format list of arguments to std::cout, according to the given format string template -void printf(const char* fmt, const Args&... args) +void printf(FormatStringCheck fmt, const Args&... args) { format(std::cout, fmt, args...); } template -void printfln(const char* fmt, const Args&... args) +void printfln(FormatStringCheck fmt, const Args&... args) { format(std::cout, fmt, args...); std::cout << '\n'; @@ -1145,15 +1158,6 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_FORMAT_FUNCS) #endif -// Added for Bitcoin Core -template -std::string format(const std::string &fmt, const Args&... args) -{ - std::ostringstream oss; - format(oss, fmt.c_str(), args...); - return oss.str(); -} - } // namespace tinyformat // Added for Bitcoin Core: diff --git a/src/util/string.h b/src/util/string.h index c059f989250dc..c9e33e6592638 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -6,7 +6,6 @@ #define BITCOIN_UTIL_STRING_H #include -#include #include #include @@ -247,12 +246,4 @@ template } } // namespace util -namespace tinyformat { -template -std::string format(util::ConstevalFormatString fmt, const Args&... args) -{ - return format(fmt.fmt, args...); -} -} // namespace tinyformat - #endif // BITCOIN_UTIL_STRING_H From 42066f45ff5d48e78a317eda63c035809bd657c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 27 Jun 2024 15:53:08 +0200 Subject: [PATCH 12/40] Refactor SipHash_32b benchmark to improve accuracy and avoid optimization issues - Modify `SipHash_32b` benchmark to use `FastRandomContext` for generating initial values. - Cycle through and modify each byte of the `uint256` value to ensure no part of it can be optimized away. The lack of "recursion" (where the method call overwrites the used inputs partially) and the systematic modification of each input byte makes the benchmark usage more reliable and thorough. --- src/bench/crypto_hash.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/bench/crypto_hash.cpp b/src/bench/crypto_hash.cpp index 65da942ad7c3b..a0799aea7adf2 100644 --- a/src/bench/crypto_hash.cpp +++ b/src/bench/crypto_hash.cpp @@ -192,10 +192,16 @@ static void SHA512(benchmark::Bench& bench) static void SipHash_32b(benchmark::Bench& bench) { - uint256 x; - uint64_t k1 = 0; + FastRandomContext rng{/*fDeterministic=*/true}; + auto k0{rng.rand64()}, k1{rng.rand64()}; + auto val{rng.rand256()}; + auto i{0U}; bench.run([&] { - *((uint64_t*)x.begin()) = SipHashUint256(0, ++k1, x); + ankerl::nanobench::doNotOptimizeAway(SipHashUint256(k0, k1, val)); + ++k0; + ++k1; + ++i; + val.data()[i % uint256::size()] ^= i & 0xFF; }); } From ac286e0d1bd4c8dbe6d21cbe8c0a131ad8ba1854 Mon Sep 17 00:00:00 2001 From: secp512k2 <187356958+secp512k2@users.noreply.github.com> Date: Tue, 5 Nov 2024 12:13:02 -0800 Subject: [PATCH 13/40] doc: Fix grammatical errors in multisig-tutorial.md This pull request fixes grammatical errors in the 'multisig-tutorial.md' document. Corrections: 1. Incorrect Phrase "As can been seen": - Before: There are discussions about eliminating this redundancy, as can been seen in the issue #17190 (https://github.com/bitcoin/bitcoin/issues/17190). - After: There are discussions about eliminating this redundancy, as can be seen in the issue #17190 (https://github.com/bitcoin/bitcoin/issues/17190). 2. Clarity Improvement in a Sentence: - Before: Note that at least two descriptors are usually used, one for internal derivation paths and external ones. - After: Note that at least two descriptors are usually used, one for internal derivation paths and one for external ones. Explanation: - Corrected "been" to "be" to fix the grammatical error. - Added "one for" before "external ones" to improve clarity and parallel structure in the sentence. These minor corrections enhance the readability and professionalism of the documentation. Thank you for considering this pull request. --- doc/multisig-tutorial.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/multisig-tutorial.md b/doc/multisig-tutorial.md index 1f21acc2be9d6..28b5a70b2c138 100644 --- a/doc/multisig-tutorial.md +++ b/doc/multisig-tutorial.md @@ -82,7 +82,7 @@ multisig_desc="[$multisig_ext_desc, $multisig_int_desc]" `external_desc` and `internal_desc` specify the output type (`wsh`, in this case) and the xpubs involved. They also use BIP 67 (`sortedmulti`), so the wallet can be recreated without worrying about the order of xpubs. Conceptually, descriptors describe a list of scriptPubKey (along with information for spending from it) [[source](https://github.com/bitcoin/bitcoin/issues/21199#issuecomment-780772418)]. -Note that at least two descriptors are usually used, one for internal derivation paths and external ones. There are discussions about eliminating this redundancy, as can been seen in the issue [#17190](https://github.com/bitcoin/bitcoin/issues/17190). +Note that at least two descriptors are usually used, one for internal derivation paths and one for external ones. There are discussions about eliminating this redundancy, as can be seen in the issue [#17190](https://github.com/bitcoin/bitcoin/issues/17190). After creating the descriptors, it is necessary to add the checksum, which is required by the `importdescriptors` RPC. From b619bdc3303217f4415342fe60e586e18fa48308 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 6 Nov 2024 15:40:34 +0000 Subject: [PATCH 14/40] cmake: Revamp `FindLibevent` module This change generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former. --- cmake/module/FindLibevent.cmake | 56 ++++++++++++++++++--------------- doc/build-unix.md | 2 +- src/CMakeLists.txt | 8 +++-- src/test/CMakeLists.txt | 2 +- src/test/fuzz/CMakeLists.txt | 2 +- 5 files changed, 38 insertions(+), 32 deletions(-) diff --git a/cmake/module/FindLibevent.cmake b/cmake/module/FindLibevent.cmake index 901a4f3bd41ec..280e38adf914a 100644 --- a/cmake/module/FindLibevent.cmake +++ b/cmake/module/FindLibevent.cmake @@ -38,43 +38,47 @@ function(check_evhttp_connection_get_peer target) set(HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR ${HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR} PARENT_SCOPE) endfunction() +set(_libevent_components core extra) +if(NOT WIN32) + list(APPEND _libevent_components pthreads) +endif() + +find_package(Libevent ${Libevent_FIND_VERSION} QUIET + NO_MODULE +) include(FindPackageHandleStandardArgs) -if(VCPKG_TARGET_TRIPLET) - find_package(Libevent ${Libevent_FIND_VERSION} NO_MODULE QUIET - COMPONENTS extra +if(Libevent_FOUND) + find_package(Libevent ${Libevent_FIND_VERSION} QUIET + REQUIRED COMPONENTS ${_libevent_components} + NO_MODULE ) find_package_handle_standard_args(Libevent REQUIRED_VARS Libevent_DIR VERSION_VAR Libevent_VERSION ) check_evhttp_connection_get_peer(libevent::extra) - add_library(libevent::libevent ALIAS libevent::extra) - mark_as_advanced(Libevent_DIR) - mark_as_advanced(_event_h) - mark_as_advanced(_event_lib) else() find_package(PkgConfig REQUIRED) - pkg_check_modules(libevent QUIET - IMPORTED_TARGET - libevent>=${Libevent_FIND_VERSION} - ) - set(_libevent_required_vars libevent_LIBRARY_DIRS libevent_FOUND) - if(NOT WIN32) - pkg_check_modules(libevent_pthreads QUIET - IMPORTED_TARGET - libevent_pthreads>=${Libevent_FIND_VERSION} + foreach(component IN LISTS _libevent_components) + pkg_check_modules(libevent_${component} + REQUIRED QUIET + IMPORTED_TARGET GLOBAL + libevent_${component}>=${Libevent_FIND_VERSION} ) - list(APPEND _libevent_required_vars libevent_pthreads_FOUND) - endif() + if(TARGET PkgConfig::libevent_${component} AND NOT TARGET libevent::${component}) + add_library(libevent::${component} ALIAS PkgConfig::libevent_${component}) + endif() + endforeach() find_package_handle_standard_args(Libevent - REQUIRED_VARS ${_libevent_required_vars} - VERSION_VAR libevent_VERSION + REQUIRED_VARS libevent_core_LIBRARY_DIRS + VERSION_VAR libevent_core_VERSION ) - unset(_libevent_required_vars) - check_evhttp_connection_get_peer(PkgConfig::libevent) - add_library(libevent::libevent ALIAS PkgConfig::libevent) - if(NOT WIN32) - add_library(libevent::pthreads ALIAS PkgConfig::libevent_pthreads) - endif() + check_evhttp_connection_get_peer(PkgConfig::libevent_extra) endif() + +unset(_libevent_components) + +mark_as_advanced(Libevent_DIR) +mark_as_advanced(_event_h) +mark_as_advanced(_event_lib) diff --git a/doc/build-unix.md b/doc/build-unix.md index a5ad4df11de80..4f04b4fd9f10a 100644 --- a/doc/build-unix.md +++ b/doc/build-unix.md @@ -182,7 +182,7 @@ Setup and Build Example: Arch Linux ----------------------------------- This example lists the steps necessary to setup and build a command line only distribution of the latest changes on Arch Linux: - pacman --sync --needed cmake boost gcc git libevent make pkgconf python sqlite + pacman --sync --needed cmake boost gcc git libevent make python sqlite git clone https://github.com/bitcoin/bitcoin.git cd bitcoin/ cmake -B build diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 0a651644166df..488bd3fc74bc5 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -290,13 +290,14 @@ target_link_libraries(bitcoin_node core_interface bitcoin_common bitcoin_util + $ leveldb minisketch univalue Boost::headers - $ + libevent::core + libevent::extra $ - $ $ ) @@ -366,7 +367,8 @@ if(BUILD_CLI) bitcoin_cli bitcoin_common bitcoin_util - $ + libevent::core + libevent::extra ) list(APPEND installable_targets bitcoin-cli) endif() diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index c376c1905a201..38562be63390a 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -153,7 +153,7 @@ target_link_libraries(test_bitcoin minisketch secp256k1 Boost::headers - $ + libevent::extra ) if(ENABLE_WALLET) diff --git a/src/test/fuzz/CMakeLists.txt b/src/test/fuzz/CMakeLists.txt index 2d5f93b4f12dd..f9330286dc73f 100644 --- a/src/test/fuzz/CMakeLists.txt +++ b/src/test/fuzz/CMakeLists.txt @@ -140,7 +140,7 @@ target_link_libraries(fuzz univalue secp256k1 Boost::headers - $ + libevent::extra ) if(ENABLE_WALLET) From ffda355b5a2113fa0f7db8015f7b08bf1351e245 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 30 Oct 2024 07:33:51 +0000 Subject: [PATCH 15/40] cmake, refactor: Move `HAVE_EVHTTP_...` to `libevent` interface --- cmake/bitcoin-build-config.h.in | 3 --- cmake/module/FindLibevent.cmake | 4 +++- src/httpserver.cpp | 2 -- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cmake/bitcoin-build-config.h.in b/cmake/bitcoin-build-config.h.in index dce9261da2a35..56e0519fac1f9 100644 --- a/cmake/bitcoin-build-config.h.in +++ b/cmake/bitcoin-build-config.h.in @@ -71,9 +71,6 @@ */ #cmakedefine01 HAVE_DECL_SETSID -/* Define this symbol if evhttp_connection_get_peer expects const char** */ -#cmakedefine HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR 1 - /* Define to 1 if fdatasync is available. */ #cmakedefine HAVE_FDATASYNC 1 diff --git a/cmake/module/FindLibevent.cmake b/cmake/module/FindLibevent.cmake index 280e38adf914a..c006b43d60408 100644 --- a/cmake/module/FindLibevent.cmake +++ b/cmake/module/FindLibevent.cmake @@ -35,7 +35,9 @@ function(check_evhttp_connection_get_peer target) " HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR ) cmake_pop_check_state() - set(HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR ${HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR} PARENT_SCOPE) + target_compile_definitions(${target} INTERFACE + $<$:HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR> + ) endfunction() set(_libevent_components core extra) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index b8772ed852f04..e37bc21dea64c 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -2,8 +2,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include // IWYU pragma: keep - #include #include From 5a96767e3f531ba9e8a676eec47727421f9f589f Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 30 Oct 2024 07:34:00 +0000 Subject: [PATCH 16/40] depends, libevent: Do not install *.pc files and remove patches for them --- depends/packages/libevent.mk | 9 +++---- depends/patches/libevent/cmake_fixups.patch | 22 ---------------- depends/patches/libevent/fix_mingw_link.patch | 25 ------------------- 3 files changed, 4 insertions(+), 52 deletions(-) delete mode 100644 depends/patches/libevent/fix_mingw_link.patch diff --git a/depends/packages/libevent.mk b/depends/packages/libevent.mk index 4c05e8a0a7425..91bc75c1d30d3 100644 --- a/depends/packages/libevent.mk +++ b/depends/packages/libevent.mk @@ -4,7 +4,6 @@ $(package)_download_path=https://github.com/libevent/libevent/releases/download/ $(package)_file_name=$(package)-$($(package)_version).tar.gz $(package)_sha256_hash=92e6de1be9ec176428fd2367677e61ceffc2ee1cb119035037a27d346b0403bb $(package)_patches=cmake_fixups.patch -$(package)_patches+=fix_mingw_link.patch $(package)_build_subdir=build # When building for Windows, we set _WIN32_WINNT to target the same Windows @@ -23,8 +22,7 @@ define $(package)_set_vars endef define $(package)_preprocess_cmds - patch -p1 < $($(package)_patch_dir)/cmake_fixups.patch && \ - patch -p1 < $($(package)_patch_dir)/fix_mingw_link.patch + patch -p1 < $($(package)_patch_dir)/cmake_fixups.patch endef define $(package)_config_cmds @@ -40,7 +38,8 @@ define $(package)_stage_cmds endef define $(package)_postprocess_cmds - rm -rf bin && \ + rm -rf bin lib/pkgconfig && \ rm include/ev*.h && \ - rm include/event2/*_compat.h + rm include/event2/*_compat.h && \ + rm lib/libevent.a endef diff --git a/depends/patches/libevent/cmake_fixups.patch b/depends/patches/libevent/cmake_fixups.patch index d80c1a94898c5..a8812afd1e1ba 100644 --- a/depends/patches/libevent/cmake_fixups.patch +++ b/depends/patches/libevent/cmake_fixups.patch @@ -1,8 +1,5 @@ cmake: set minimum version to 3.5 -Fix generated pkg-config files, see -https://github.com/libevent/libevent/pull/1165. - --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,7 +19,7 @@ @@ -14,22 +11,3 @@ https://github.com/libevent/libevent/pull/1165. if (POLICY CMP0054) cmake_policy(SET CMP0054 NEW) -diff --git a/cmake/AddEventLibrary.cmake b/cmake/AddEventLibrary.cmake -index 04f5837e..d8ea42c4 100644 ---- a/cmake/AddEventLibrary.cmake -+++ b/cmake/AddEventLibrary.cmake -@@ -20,12 +20,12 @@ macro(generate_pkgconfig LIB_NAME) - - set(LIBS "") - foreach (LIB ${LIB_PLATFORM}) -- set(LIBS "${LIBS} -L${LIB}") -+ set(LIBS "${LIBS} -l${LIB}") - endforeach() - - set(OPENSSL_LIBS "") - foreach(LIB ${OPENSSL_LIBRARIES}) -- set(OPENSSL_LIBS "${OPENSSL_LIBS} -L${LIB}") -+ set(OPENSSL_LIBS "${OPENSSL_LIBS} -l${LIB}") - endforeach() - - configure_file("lib${LIB_NAME}.pc.in" "lib${LIB_NAME}.pc" @ONLY) diff --git a/depends/patches/libevent/fix_mingw_link.patch b/depends/patches/libevent/fix_mingw_link.patch deleted file mode 100644 index 41cbd463c9122..0000000000000 --- a/depends/patches/libevent/fix_mingw_link.patch +++ /dev/null @@ -1,25 +0,0 @@ -commit d108099913c5fdbe518f3f4d711f248f8522bd10 -Author: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> -Date: Mon Apr 22 06:39:35 2024 +0100 - - build: Add `Iphlpapi` to `Libs.private` in `*.pc` files on Windows - - It has been required since https://github.com/libevent/libevent/pull/923 - at least for the `if_nametoindex` call. - - See https://github.com/libevent/libevent/pull/1622. - - -diff --git a/configure.ac b/configure.ac -index d00e063a..cd1fce37 100644 ---- a/CMakeLists.txt -+++ b/CMakeLists.txt -@@ -906,7 +906,7 @@ if(WIN32) - list(APPEND HDR_PRIVATE WIN32-Code/getopt.h) - - set(EVENT__DNS_USE_FTIME_FOR_ID 1) -- set(LIB_PLATFORM ws2_32 shell32 advapi32) -+ set(LIB_PLATFORM ws2_32 shell32 advapi32 iphlpapi) - add_definitions( - -D_CRT_SECURE_NO_WARNINGS - -D_CRT_NONSTDC_NO_DEPRECATE) From ec375de39ff8e0d44fc026618a234e37019e46c1 Mon Sep 17 00:00:00 2001 From: secp512k2 <187356958+secp512k2@users.noreply.github.com> Date: Wed, 6 Nov 2024 10:36:30 -0800 Subject: [PATCH 17/40] doc: Add missing 'blank=true' option in offline-signing-tutorial.md Issue: The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option. Correction: Added `blank=true` to the command to match the options described in the text. Explanation: The `blank=true` option is necessary to create a blank wallet. Including this option ensures the command matches the options specified in the text. --- doc/offline-signing-tutorial.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/offline-signing-tutorial.md b/doc/offline-signing-tutorial.md index e03a0003a10a5..8ec4355bc780a 100644 --- a/doc/offline-signing-tutorial.md +++ b/doc/offline-signing-tutorial.md @@ -60,7 +60,8 @@ The `watch_only_wallet` wallet will be used to track and validate incoming trans ```sh [online]$ ./build/src/bitcoin-cli -signet -named createwallet \ wallet_name="watch_only_wallet" \ - disable_private_keys=true + disable_private_keys=true \ + blank=true { "name": "watch_only_wallet" From 99d9a093cf6d53b24d4a48f5845e0e0299f47800 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 7 Nov 2024 14:20:59 +0100 Subject: [PATCH 18/40] test: clarify log messages when handling SOCKS5 proxy connections Co-authored-by: Martin Zumsande --- test/functional/test_framework/socks5.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/socks5.py b/test/functional/test_framework/socks5.py index 3387c8a1fef11..28be5a0f55e08 100644 --- a/test/functional/test_framework/socks5.py +++ b/test/functional/test_framework/socks5.py @@ -183,9 +183,9 @@ def handle(self): with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to: forward_sockets(self.conn, conn_to) else: - logger.debug(f"Closing connection to {requested_to}: the destinations factory returned None") + logger.debug(f"Can't serve the connection to {requested_to}: the destinations factory returned None") else: - logger.debug(f"Closing connection to {requested_to}: no destinations factory") + logger.debug(f"Can't serve the connection to {requested_to}: no destinations factory") # Fall through to disconnect except Exception as e: @@ -194,6 +194,8 @@ def handle(self): finally: if not self.serv.keep_alive: self.conn.close() + else: + logger.debug(f"Keeping client connection alive") class Socks5Server(): def __init__(self, conf): From 83fab3212c91d91fc5502f940c901a07772ff747 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 10 Jun 2024 16:52:46 -0400 Subject: [PATCH 19/40] test: Add combinerawtransaction test to rpc_createmultisig The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed. --- test/functional/rpc_createmultisig.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index d95820bbf876f..0692a1203dfd5 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -194,13 +194,19 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) - rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) - assert rawtx3['complete'] - - tx = node0.sendrawtransaction(rawtx3["hex"], 0) + assert_equal(rawtx2["complete"], False) + rawtx3 = node2.signrawtransactionwithkey(rawtx, [priv_keys[-1]], prevtxs) + assert_equal(rawtx3["complete"], False) + assert_raises_rpc_error(-22, "TX decode failed", node2.combinerawtransaction, [rawtx2['hex'], rawtx3['hex'] + "00"]) + assert_raises_rpc_error(-22, "Missing transactions", node2.combinerawtransaction, []) + combined_rawtx = node2.combinerawtransaction([rawtx2["hex"], rawtx3["hex"]]) + + tx = node0.sendrawtransaction(combined_rawtx, 0) blk = self.generate(node0, 1)[0] assert tx in node0.getblock(blk)["tx"] + assert_raises_rpc_error(-25, "Input not found or already spent", node2.combinerawtransaction, [rawtx2['hex'], rawtx3['hex']]) + # When the wallet is enabled, assert node2 sees the incoming amount if self.is_wallet_compiled(): assert_equal(node2.getbalances()['mine']['trusted'], node2_balance + outval) From e80e4c6ff91e27d7d40f099a2d7942c29085234c Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 11 Nov 2024 10:24:30 +0100 Subject: [PATCH 20/40] validation: Remove RECENT_CONSENSUS_CHANGE validation result The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear: https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133 https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747 Since they are part of the validation interface and need to exposed by the kernel library keeping them around may also be confusing to future users of the library. --- src/bitcoin-chainstate.cpp | 3 --- src/consensus/validation.h | 16 ---------------- src/net_processing.cpp | 2 -- src/test/fuzz/partially_downloaded_block.cpp | 1 - src/test/fuzz/txdownloadman.cpp | 1 - src/test/txdownload_tests.cpp | 1 - src/validation.cpp | 10 ++-------- 7 files changed, 2 insertions(+), 32 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 9cbafa233dde3..4bb463acf534c 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -253,9 +253,6 @@ int main(int argc, char* argv[]) case BlockValidationResult::BLOCK_CONSENSUS: std::cerr << "invalid by consensus rules (excluding any below reasons)" << std::endl; break; - case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: - std::cerr << "Invalid by a change to consensus rules more recent than SegWit." << std::endl; - break; case BlockValidationResult::BLOCK_CACHED_INVALID: std::cerr << "this block was cached as being invalid and we didn't store the reason why" << std::endl; break; diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 1556c7888f95a..dffa9f2dd6ede 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -23,14 +23,6 @@ static constexpr size_t MINIMUM_WITNESS_COMMITMENT{38}; enum class TxValidationResult { TX_RESULT_UNSET = 0, //!< initial value. Tx has not yet been rejected TX_CONSENSUS, //!< invalid by consensus rules - /** - * Invalid by a change to consensus rules more recent than SegWit. - * Currently unused as there are no such consensus rule changes, and any download - * sources realistically need to support SegWit in order to provide useful data, - * so differentiating between always-invalid and invalid-by-pre-SegWit-soft-fork - * is uninteresting. - */ - TX_RECENT_CONSENSUS_CHANGE, TX_INPUTS_NOT_STANDARD, //!< inputs (covered by txid) failed policy rules TX_NOT_STANDARD, //!< otherwise didn't meet our local policy rules TX_MISSING_INPUTS, //!< transaction was missing some of its inputs @@ -65,14 +57,6 @@ enum class TxValidationResult { enum class BlockValidationResult { BLOCK_RESULT_UNSET = 0, //!< initial value. Block has not yet been rejected BLOCK_CONSENSUS, //!< invalid by consensus rules (excluding any below reasons) - /** - * Invalid by a change to consensus rules more recent than SegWit. - * Currently unused as there are no such consensus rule changes, and any download - * sources realistically need to support SegWit in order to provide useful data, - * so differentiating between always-invalid and invalid-by-pre-SegWit-soft-fork - * is uninteresting. - */ - BLOCK_RECENT_CONSENSUS_CHANGE, BLOCK_CACHED_INVALID, //!< this block was cached as being invalid and we didn't store the reason why BLOCK_INVALID_HEADER, //!< invalid proof of work or time too old BLOCK_MUTATED, //!< the block's data didn't match the data committed to by the PoW diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 170352f7299a5..482e0f675c193 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1790,7 +1790,6 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati case BlockValidationResult::BLOCK_MISSING_PREV: if (peer) Misbehaving(*peer, message); return; - case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: case BlockValidationResult::BLOCK_TIME_FUTURE: break; } @@ -1810,7 +1809,6 @@ void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat if (peer) Misbehaving(*peer, ""); return; // Conflicting (but not necessarily invalid) data or different policy: - case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE: case TxValidationResult::TX_INPUTS_NOT_STANDARD: case TxValidationResult::TX_NOT_STANDARD: case TxValidationResult::TX_MISSING_INPUTS: diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 77952cab9e5a5..913d056ff6d24 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -114,7 +114,6 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) fuzzed_data_provider.PickValueInArray( {BlockValidationResult::BLOCK_RESULT_UNSET, BlockValidationResult::BLOCK_CONSENSUS, - BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE, BlockValidationResult::BLOCK_CACHED_INVALID, BlockValidationResult::BLOCK_INVALID_HEADER, BlockValidationResult::BLOCK_MUTATED, diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index a313caa1117e7..8a4d7d55c26cd 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -32,7 +32,6 @@ COutPoint COINS[NUM_COINS]; static TxValidationResult TESTED_TX_RESULTS[] = { // Skip TX_RESULT_UNSET TxValidationResult::TX_CONSENSUS, - TxValidationResult::TX_RECENT_CONSENSUS_CHANGE, TxValidationResult::TX_INPUTS_NOT_STANDARD, TxValidationResult::TX_NOT_STANDARD, TxValidationResult::TX_MISSING_INPUTS, diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index e401fbf59c435..41f6bd83c5ff8 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -58,7 +58,6 @@ struct Behaviors { // Txid and Wtxid are assumed to be different here. For a nonsegwit transaction, use the wtxid results. static std::map expected_behaviors{ {TxValidationResult::TX_CONSENSUS, {/*txid_rejects*/0,/*wtxid_rejects*/1,/*txid_recon*/0,/*wtxid_recon*/0,/*keep*/1,/*txid_inv*/0,/*wtxid_inv*/1}}, - {TxValidationResult::TX_RECENT_CONSENSUS_CHANGE, { 0, 1, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_INPUTS_NOT_STANDARD, { 1, 1, 0, 0, 1, 1, 1}}, {TxValidationResult::TX_NOT_STANDARD, { 0, 1, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_MISSING_INPUTS, { 0, 0, 0, 0, 1, 0, 1}}, diff --git a/src/validation.cpp b/src/validation.cpp index f9288fcc0d7c0..c6de748f57f78 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2201,15 +2201,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // string by reporting the error from the second check. error = check2.GetScriptError(); } + // MANDATORY flag failures correspond to - // TxValidationResult::TX_CONSENSUS. Because CONSENSUS - // failures are the most serious case of validation - // failures, we may need to consider using - // RECENT_CONSENSUS_CHANGE for any script failure that - // could be due to non-upgraded nodes which we may want to - // support, to avoid splitting the network (but this - // depends on the details of how net_processing handles - // such errors). + // TxValidationResult::TX_CONSENSUS. return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(error))); } } From faf21625652fd0d4bbf9b86fd9ebedb5857505ea Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 11 Nov 2024 11:10:32 +0100 Subject: [PATCH 21/40] refactor: Drop deprecated space in operator""_mst --- src/script/miniscript.cpp | 15 +++++++++------ src/script/miniscript.h | 23 ++++++++++++++--------- src/test/fuzz/miniscript.cpp | 2 +- src/test/miniscript_tests.cpp | 2 +- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index 455bd56283684..4b8d3673f95f8 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -1,14 +1,17 @@ -// Copyright (c) 2019-2022 The Bitcoin Core developers +// Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include +#include #include -#include