Net processing: follow ups to #20799 (removing support for v1 compact blocks) #25147

jnewbery commented at 5:54 PM on May 16, 2022: member

This implements two of the suggestions from code reviews of PR 20799:

Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
Remove segwit argument from build_block_on_tip()

jnewbery commented at 5:54 PM on May 16, 2022: member

cc @dergoegge @MarcoFalke

DrahtBot added the label P2P on May 16, 2022

in src/net_processing.cpp:1981 in 3ab9f69e2a outdated

1977 | @@ -1978,7 +1978,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
1978 |                  if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) {
1979 |                      m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block));
1980 |                  } else {
1981 | -                    CBlockHeaderAndShortTxIDs cmpctblock(*pblock, true);
1982 | +                    CBlockHeaderAndShortTxIDs cmpctblock(*pblock);

MarcoFalke commented at 6:31 AM on May 17, 2022:

                    CBlockHeaderAndShortTxIDs cmpctblock{*pblock};

nit: {}

jnewbery commented at 9:41 AM on May 17, 2022:

Done (here and all other lines I touched)

in src/net_processing.cpp:1635 in 3ab9f69e2a outdated

1631 | @@ -1632,7 +1632,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr<const CBlock> &blo
1632 |   */
1633 |  void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock)
1634 |  {
1635 | -    std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs> (*pblock, true);
1636 | +    std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs> (*pblock);

MarcoFalke commented at 6:31 AM on May 17, 2022:

    std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);

nit: space

MarcoFalke commented at 6:36 AM on May 17, 2022:

    auto pcmpctblock{std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock)};

nit: Or maybe even use auto :sweat_smile:

MarcoFalke commented at 7:55 AM on May 17, 2022:

nit: Or maybe even inline into lazy_ser to avoid constructing it when there is no need to. (Might minimally speed up IBD?)

jnewbery commented at 9:41 AM on May 17, 2022:

Removed space

jnewbery commented at 9:42 AM on May 17, 2022:

I've changed to auto.

Or maybe even inline into lazy_ser to avoid constructing it when there is no need to. (Might minimally speed up IBD?)

I'm not sure how this would work. pcmpctblock is required further down the function outside the lazy_ser lambda. I have moved the instantiation below the early exits, which I think addresses your point about speeding up IBD.

MarcoFalke commented at 9:48 AM on May 17, 2022:

Yeah, I missed that assignment. Seems fine to just move it down for now.

MarcoFalke approved

MarcoFalke commented at 6:32 AM on May 17, 2022: member

lgtm

MarcoFalke added the label Refactoring on May 17, 2022

naumenkogs commented at 7:38 AM on May 17, 2022: member

Concept ACK. I will give a full ACK once you resolve Marko's suggestions, which I endorse too.

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

c65bf50b44

jnewbery referenced this in commit a2e357e3dc on May 17, 2022

jnewbery force-pushed on May 17, 2022

jnewbery commented at 9:44 AM on May 17, 2022: member

Thanks for the review @MarcoFalke and @naumenkogs. I've addressed all your comments.

in src/net_processing.cpp:1645 in 97e4a5d7dc outdated

1639 | @@ -1643,10 +1640,11 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
1640 |  
1641 |      if (!DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_SEGWIT)) return;
1642 |  
1643 | -    uint256 hashBlock(pblock->GetHash());
1644 | +    auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);
1645 |      const std::shared_future<CSerializedNetMsg> lazy_ser{
1646 | -        std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
1647 | +        std::async(std::launch::deferred, [&] { return CNetMsgMaker(PROTOCOL_VERSION).Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};

MarcoFalke commented at 9:46 AM on May 17, 2022:

        std::async(std::launch::deferred, [&] { return CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};

nit

jnewbery commented at 9:56 AM on May 17, 2022:

Done

in src/net_processing.cpp:1647 in 97e4a5d7dc outdated

1644 | +    auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);
1645 |      const std::shared_future<CSerializedNetMsg> lazy_ser{
1646 | -        std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
1647 | +        std::async(std::launch::deferred, [&] { return CNetMsgMaker(PROTOCOL_VERSION).Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
1648 |  
1649 | +    uint256 hashBlock{pblock->GetHash()};

MarcoFalke commented at 9:47 AM on May 17, 2022:

I think it is fine to leave untouched lines as-is.

jnewbery commented at 9:57 AM on May 17, 2022:

I moved this variable assignment down to where it's first used, so it's not untouched.

MarcoFalke approved

MarcoFalke commented at 9:48 AM on May 17, 2022: member

still lgtm

jnewbery referenced this in commit 40ae310795 on May 17, 2022

jnewbery force-pushed on May 17, 2022

naumenkogs commented at 8:03 AM on May 18, 2022: member

ACK 8184dcbdd18d8b732dc285a05884ecda115062e1

fanquake requested review from dergoegge on May 18, 2022

[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

bf6526f4a0

in src/net_processing.cpp:1643 in 8184dcbdd1 outdated

1639 | @@ -1643,10 +1640,11 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
1640 |  
1641 |      if (!DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_SEGWIT)) return;
1642 |  
1643 | -    uint256 hashBlock(pblock->GetHash());
1644 | +    auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);

dergoegge commented at 10:09 AM on May 18, 2022:

It looks like we don't call NewPoWValidBlock while we are in IBD (see here), so 40ae3107956e2425c4a3cf0cb3a4a2d6d952894c won't really affect IBD speed. Consider dropping that commit or changing the commit message if there is another motivation for the change.

jnewbery commented at 12:48 PM on May 18, 2022:

Good catch! I've dropped the commit (this function could be tidied up a bit in terms of commenting/variable instantiation, but that doesn't seem strictly related to this PR).

jnewbery force-pushed on May 18, 2022

dergoegge commented at 1:47 PM on May 18, 2022: member

Code review ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6

fanquake requested review from naumenkogs on May 18, 2022

naumenkogs commented at 6:14 AM on May 19, 2022: member

ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6

fanquake merged this on May 19, 2022

fanquake closed this on May 19, 2022

jnewbery deleted the branch on May 19, 2022

sidhujag referenced this in commit 44c0a043a9 on May 28, 2022

DrahtBot locked this on May 19, 2023