From c65bf50b44a38bc55224d8967e0df7af60ea4f1b Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 16 May 2022 18:21:15 +0100 Subject: [PATCH 1/2] Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278 --- src/blockencodings.cpp | 4 ++-- src/blockencodings.h | 2 +- src/net_processing.cpp | 6 +++--- src/test/blockencodings_tests.cpp | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 2a7bf9397c8..f96353510fc 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -16,7 +16,7 @@ #include -CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block, bool fUseWTXID) : +CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block) : nonce(GetRand()), shorttxids(block.vtx.size() - 1), prefilledtxn(1), header(block) { FillShortTxIDSelector(); @@ -24,7 +24,7 @@ CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block, bool f prefilledtxn[0] = {0, block.vtx[0]}; for (size_t i = 1; i < block.vtx.size(); i++) { const CTransaction& tx = *block.vtx[i]; - shorttxids[i - 1] = GetShortID(fUseWTXID ? tx.GetWitnessHash() : tx.GetHash()); + shorttxids[i - 1] = GetShortID(tx.GetWitnessHash()); } } diff --git a/src/blockencodings.h b/src/blockencodings.h index 326db1b4a7c..67c4e571566 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -104,7 +104,7 @@ public: // Dummy for deserialization CBlockHeaderAndShortTxIDs() {} - CBlockHeaderAndShortTxIDs(const CBlock& block, bool fUseWTXID); + CBlockHeaderAndShortTxIDs(const CBlock& block); uint64_t GetShortID(const uint256& txhash) const; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 349d7fd8fb7..c75fdb12522 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1632,7 +1632,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo */ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { - std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); + auto pcmpctblock = std::make_shared(*pblock); const CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main); @@ -1978,7 +1978,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); } else { - CBlockHeaderAndShortTxIDs cmpctblock(*pblock, true); + CBlockHeaderAndShortTxIDs cmpctblock{*pblock}; m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); } } else { @@ -4771,7 +4771,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) CBlock block; bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams); assert(ret); - CBlockHeaderAndShortTxIDs cmpctblock(block, true); + CBlockHeaderAndShortTxIDs cmpctblock{block}; m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); } state.pindexBestHeaderSent = pBestIndex; diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 9dfbd7ba7c2..875241094dc 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) // Do a simple ShortTxIDs RT { - CBlockHeaderAndShortTxIDs shortIDs(block, true); + CBlockHeaderAndShortTxIDs shortIDs{block}; CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); stream << shortIDs; @@ -122,7 +122,7 @@ public: stream >> *this; } explicit TestHeaderAndShortIDs(const CBlock& block) : - TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs(block, true)) {} + TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs{block}) {} uint64_t GetShortID(const uint256& txhash) const { CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); @@ -279,7 +279,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest) // Test simple header round-trip with only coinbase { - CBlockHeaderAndShortTxIDs shortIDs(block, false); + CBlockHeaderAndShortTxIDs shortIDs{block}; CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); stream << shortIDs; From bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 16 May 2022 18:50:59 +0100 Subject: [PATCH 2/2] [test] Remove segwit argument from build_block_on_tip() The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in https://github.com/bitcoin/bitcoin/pull/20799#discussion_r867782119 --- test/functional/p2p_compactblocks.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 8eec8dbc0d4..b9ac3c32c5b 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -146,10 +146,8 @@ class CompactBlocksTest(BitcoinTestFramework): ]] self.utxos = [] - def build_block_on_tip(self, node, segwit=False): + def build_block_on_tip(self, node): block = create_block(tmpl=node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)) - if segwit: - add_witness_commitment(block) block.solve() return block @@ -381,7 +379,7 @@ class CompactBlocksTest(BitcoinTestFramework): # Try announcing a block with an inv or header, expect a compactblock # request for announce in ["inv", "header"]: - block = self.build_block_on_tip(node, segwit=True) + block = self.build_block_on_tip(node) if announce == "inv": test_node.send_message(msg_inv([CInv(MSG_BLOCK, block.sha256)]))