net processing: Only support version 2 compact blocks

jnewbery commented at 2:54 pm on December 29, 2020: member

Compact blocks are used for efficient relay of blocks, either through High Bandwidth or Low Bandwidth mode. See BIP 152 for full details.

For compact block relay to work, the receiver must have a mempool containing transactions which are likely to be included in the block. The receiver uses these transactions to reconstruct the block from the short transaction ids included in the cmpctblock message. Compact blocks are therefore only useful for relaying blocks at or near the tip of the block chain. For older blocks, the recipient won’t have the transactions in their mempool and so would need to request them using a getblocktxn message. In such cases, just requesting the full block is more efficient.

BIP 152 supports two versions: version 1 (without witnesses) and version 2 (with witnesses). Version 2 is required to reconstruct segwit blocks. Segwit was activated in August 2017, and providing non-witness blocks to peers is no longer useful. Since the witnesses are not included, the peer would not be able to fully validate all the consensus rules on the provided block.

Therefore, stop supporting version 1 compact blocks. Ignore sendcmpct messages with version=1, and don’t advertise support by sending sendcmpct with version=1. Only send sendcmpct to peers with NODE_WITNESS. Respond to all requests for compact blocks or blocktxns with witness-serialized blocks and transactions.

fanquake added the label P2P on Dec 29, 2020

MarcoFalke commented at 3:16 pm on December 29, 2020: member

Segwit was activated in August 2017, and providing non-witness blocks to peers is no longer useful

I think there may be some edge-case scenarios where a peer skips the download of witnesses because they are never read (due to an assumevalid setting), but that has nothing to do with compact blocks. So:

Concept ACK on removing code that doesn’t serve any use case in practice.

sipa commented at 6:35 pm on December 29, 2020: member

Concept ACK on removing non-witness cmpctblock support. IIRC its only purpose is serving Bitcoin Core v0.13.0 nodes (0.12.x didn’t have compact blocks; 0.13.1 added segwit activation parameters).

The first commit here seems to mix removal of functionality with some refactoring. Is it possible to separate those more cleanly into separate commits?

jnewbery commented at 7:06 pm on December 29, 2020: member

The first commit here seems to mix removal of functionality with some refactoring. Is it possible to separate those more cleanly into separate commits?

Yes, I’ll see what I can do to split it up a bit more.

DrahtBot commented at 8:17 pm on December 29, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer by jnewbery)
#24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
#24062 (refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex by theStack)
#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift commented at 8:39 pm on December 29, 2020: contributor

Concept ACK

in src/net_processing.cpp:546 in 58083b6dd0 outdated

559-                    return true;
560-                });
561-                lNodesAnnouncingHeaderAndIDs.pop_front();
562-            }
563-            connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
564+            connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT,  /* high_bandwidth= */ true, /* version= */ uint64_t{2}));

ariard commented at 0:11 am on December 30, 2020:

Maybe a static const int CMPCTBLOCK_VERSION? It sounds non-ambiguous, after this PR we only support one version.

jnewbery commented at 2:26 pm on December 30, 2020:

Good idea! I’ll add.

jnewbery commented at 4:14 pm on January 2, 2021:

Done

ariard commented at 0:17 am on December 30, 2020: member

Concept ACK, thanks for cleaning a bit compact blocks code.

jonatack commented at 11:01 am on January 2, 2021: member

Concept ACK, looks like a good simplification. Worth reviewing the diffs with -w in some parts.

jnewbery force-pushed on Jan 2, 2021

jnewbery commented at 4:15 pm on January 2, 2021: member

Rebased and broken up the first commit.

I’ve also dropped the last two commits (Clean up PeerManager::BlockChecked() and Clean up MaybeSetPeerAsAnnouncingHeaderAndIDs()). There’s already enough happening in this PR.

in src/net_processing.cpp:531 in 5ca83df9ac outdated

527@@ -535,7 +528,7 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
528 {
529     AssertLockHeld(cs_main);
530     CNodeState* nodestate = State(nodeid);
531-    if (!nodestate || !nodestate->fSupportsDesiredCmpctVersion) {
532+    if (!nodestate || !nodestate->fProvidesHeadersAndIDs) {

mzumsande commented at 0:08 am on January 6, 2021:

nit: typo in 5ca83df9ac1f9f1454ebe7fd93c3b79012df104c (should be fProvidesHeaderAndIDs) here and in two other spots. It is fixed in the next commit but this intermediate one won’t compile.

jnewbery commented at 4:49 pm on January 6, 2021:

:flushed: Thanks!

jnewbery commented at 6:45 pm on January 6, 2021:

fixed

jnewbery force-pushed on Jan 6, 2021

jnewbery renamed this:
~~net processing: Only support compact blocks with witnesses~~
net processing: Only support version 2 compact blocks
on Jan 7, 2021

felipsoarez commented at 1:00 pm on January 9, 2021: none

utACK

fjahr commented at 2:40 pm on January 23, 2021: member

Concept ACK

in test/functional/p2p_compactblocks.py:193 in befb06801e outdated

137@@ -141,21 +138,16 @@ def make_utxos(self):
138     #   made with compact blocks.
139     # - If sendcmpct is then sent with boolean 1, then new block announcements
140     #   are made with compact blocks.
141-    # If old_node is passed in, request compact blocks with version=preferred-1

jonatack commented at 3:01 pm on January 25, 2021:

652c6f8 in line 136 just above, preferred_version no longer exists in this test after this commit; perhaps update that line

jnewbery commented at 9:09 am on February 3, 2021:

Good catch. Fixed!

in src/net_processing.cpp:155 in befb06801e outdated

145@@ -146,6 +146,8 @@ static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
146 static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
147 /** the maximum percentage of addresses from our addrman to return in response to a getaddr message. */
148 static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23;
149+/** The compactblocks version we support. See BIP 152. */
150+static constexpr uint64_t CMPCTBLOCKS_VERSION = 2;

jonatack commented at 3:15 pm on January 25, 2021:

d0f352eb uint8_t (range 0-255) would suffice for now?

0BIP152
1====sendcmpct====
2# The sendcmpct message is defined as a message containing a 1-byte integer followed by a 8-byte integer where pchCommand == "sendcmpct".
3# The first integer SHALL be interpreted as a boolean (and MUST have a value of either 1 or 0)
4# The second integer SHALL be interpreted as a little-endian version number. Nodes sending a sendcmpct message MUST currently set this value to 1.

 0@@ -147,7 +147,7 @@ static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
 1 /** the maximum percentage of addresses from our addrman to return in response to a getaddr message. */
 2 static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23;
 3 /** The compactblocks version we support. See BIP 152. */
 4-static constexpr uint64_t CMPCTBLOCKS_VERSION = 2;
 5+static constexpr uint8_t CMPCTBLOCKS_VERSION = 2;
 6 
 7 struct COrphanTx {
 8     // When modifying, adapt the copy of this definition in tests/DoS_tests.
 9@@ -2494,7 +2494,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
10 
11     if (msg_type == NetMsgType::SENDCMPCT) {
12         bool sendcmpct_hb{false};
13-        uint64_t sendcmpct_version{0};
14+        uint8_t sendcmpct_version{0};
15         vRecv >> sendcmpct_hb >> sendcmpct_version;

jnewbery commented at 9:13 am on February 3, 2021:

The version field needs to be 8 bytes (uint64_t). If we change this const to a uint8_t, then the message serialized here:

0            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /* high_bandwidth= */ false, /* version= */ CMPCTBLOCKS_VERSION));

would have a payload size of 2 (1 byte for high_bandwidth, and 1 byte for version). That would fail to be parsed by other clients.

sipa commented at 9:16 am on February 3, 2021:

That’s a bit brittle in any case. Perhaps put uint64_t{CMPCTBLOCKS_VERSION} there instead?

jonatack commented at 9:24 am on February 3, 2021:

“followed by a 8-byte integer” -> I misread, thanks

jnewbery commented at 9:36 am on February 3, 2021:

or perhaps:

0            static_assert(sizeof(CMPCTBLOCKS_VERSION) == sizeof(uint64_t), "sendcmpct version field must be 8 bytes");
1            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /* high_bandwidth= */ false, /* version= */ CMPCTBLOCKS_VERSION));

(why check at runtime what you can test at compile time?)

MarcoFalke commented at 9:50 am on February 3, 2021:

uint64_t{CMPCTBLOCKS_VERSION} is a compile time check ;)

jnewbery commented at 9:52 am on February 3, 2021:

I’ve added the static_assert.

jnewbery commented at 10:11 am on February 3, 2021:

uint64_t{CMPCTBLOCKS_VERSION} is a compile time check ;)

uint64_t{CMPCTBLOCKS_VERSION} initializes a new uint64_t from CMPCTBLOCKS_VERSION at runtime, no? But I guess the compiler will optimize that away?

In any case, I’ve removed the static assert and replaced with uint64_t{CMPCTBLOCKS_VERSION}.

in src/net_processing.cpp:2714 in befb06801e outdated

2472@@ -2496,18 +2473,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2473             // nodes)
2474             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDHEADERS));
2475         }
2476-        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION) {
2477-            // Tell our peer we are willing to provide version 1 or 2 cmpctblocks
2478+        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION &&
2479+            WITH_LOCK(cs_main, {return State(pfrom.GetId())->fHaveWitness;})) {

jonatack commented at 3:27 pm on January 25, 2021:

d0f352eb can we avoid locking the mutex (and order dependance on fHaveWitness being set) with

0- if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION &&
1-     WITH_LOCK(cs_main, {return State(pfrom.GetId())->fHaveWitness;})) {
2+ if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION && (pfrom.nServices & NODE_WITNESS)) {

or

0- if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION &&
1-     WITH_LOCK(cs_main, {return State(pfrom.GetId())->fHaveWitness;})) {
2+ if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION && (pfrom.GetLocalServices() & NODE_WITNESS)) {

?

ariard commented at 9:30 pm on January 27, 2021:

Maybe we can rename SHORT_IDS_BLOCK_VERSION to SENDCMPCT_VERSION ? A block of short-txn-ids is better known as a compact block or do we have ambiguity here ?

jnewbery commented at 9:26 am on February 3, 2021:

can we avoid locking the mutex

Very nice observation, although I think this is ok for now. We take cs_main when processing almost all messages in net_processing (although I agree with you that it’d be slightly nicer if we didn’t have to). In the new code, we’re only sending sendcmpct if the peer has told us that they support witness serialization. That’s indicated by fHaveWitness or pfrom.nServices (pfrom.GetLocalServices() is something different - it’s the services we offered to the peer).

fHaveWitness will move into Peer very soon as part of #19398, at which point it’ll no longer be guarded by cs_main.

(and order dependance on fHaveWitness being set)

fHaveWitness can only be set once in version handling, and this code in verack handling can only be hit after that.

jnewbery commented at 9:44 am on February 3, 2021:

Maybe we can rename SHORT_IDS_BLOCK_VERSION to SENDCMPCT_VERSION

I do have a branch somewhere that does that, but it’s out of scope for this PR.

jnewbery commented at 9:51 am on February 3, 2021:

I do have a branch somewhere that does that, but it’s out of scope for this PR.

Here you go: https://github.com/jnewbery/bitcoin/commit/82372335cc3cf8fa238678629dda6a2f465e6005. Feel free to PR that commit if you think it’s worth it.

theuni commented at 6:15 pm on November 3, 2021:

Agree with @jonatack about taking from (pfrom.nServices & NODE_WITNESS) rather than State(pfrom.GetId())->fHaveWitness;}) just because it’s easier to not have to worry about where the latter comes from.

Understood that it’s moot as it’s changing soon anyway, so not a big deal.

in src/net_processing.cpp:2484 in befb06801e outdated

2486-            uint64_t nCMPCTBLOCKVersion = 2;
2487-            if (pfrom.GetLocalServices() & NODE_WITNESS)
2488-                m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
2489-            nCMPCTBLOCKVersion = 1;
2490-            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
2491+            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /* high_bandwidth= */ false, /* version= */ CMPCTBLOCKS_VERSION));

jonatack commented at 3:36 pm on January 25, 2021:

d0f352e if you go with /* high_bandwidth= */ here, maybe update the remaining 2 lines that use fAnnounceUsingCMPCTBLOCK

 0@@ -718,14 +718,14 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
 1             // As per BIP152, we only get 3 of our peers to announce
 2             // blocks using compact encodings.
 3             connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman](CNode* pnodeStop){
 4-                connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, CMPCTBLOCKS_VERSION));
 5+                connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /* high_bandwidth= */false, CMPCTBLOCKS_VERSION));
 6                 // save BIP152 bandwidth state: we select peer to be low-bandwidth
 7                 pnodeStop->m_bip152_highbandwidth_to = false;
 8                 return true;
 9             });
10             lNodesAnnouncingHeaderAndIDs.pop_front();
11         }
12-        connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, CMPCTBLOCKS_VERSION));
13+        connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /* high_bandwidth= */true, CMPCTBLOCKS_VERSION));
14         // save BIP152 bandwidth state: we select peer to be high-bandwidth

jnewbery commented at 9:48 am on February 3, 2021:

Good suggestion. Done.

jonatack commented at 5:33 pm on January 25, 2021: member

Reviewed first three commits up to 55c9231dc then started getting bogged down in the granular ones going over some of the same parts repeatedly and with the renamings; skim-reviewed them for now and will need to return. All of the commits build cleanly rebased to master with p2p_compactblocks.py passing on each.

in test/functional/p2p_compactblocks.py:197 in 652c6f8388 outdated

151         test_node.wait_until(received_sendcmpct, timeout=30)
152         with p2p_lock:
153-            # Check that the first version received is the preferred one
154-            assert_equal(test_node.last_sendcmpct[0].version, preferred_version)
155+            # Check that the first version received is version 2
156+            assert_equal(test_node.last_sendcmpct[0].version, 2)

ariard commented at 5:52 pm on January 27, 2021:

Constify test value same as net_processing, CMPCTBLOCKS_VERSION ?

jnewbery commented at 9:54 am on February 3, 2021:

I’m not sure if that adds to the readability here. Leaving for now.

in src/net_processing.cpp:2847 in d0f352eb21 outdated

2497@@ -2496,18 +2498,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2498             // nodes)
2499             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDHEADERS));
2500         }
2501-        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION) {
2502-            // Tell our peer we are willing to provide version 1 or 2 cmpctblocks
2503+        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION &&
2504+            WITH_LOCK(cs_main, {return State(pfrom.GetId())->fHaveWitness;})) {
2505+            // Tell our peer we are willing to provide version 2 cmpctblocks.
2506             // However, we do not request new block announcements using

ariard commented at 9:40 pm on January 27, 2021:

AFAIU this comment tries to hint the double opt-in for CB to be bidirectional, maybe it can be clearer. “However, we request new block announcements using CMPCTBLOCK only after receiving a SENDCMPCT from peer.”

jnewbery commented at 9:58 am on February 3, 2021:

This comment is saying that we don’t request hb mode from the peer. We only add the peer to one of our three hb peers once it provides us with a block.

In any case, this comment is unchanged by this PR, so I’m going to leave it.

in src/net_processing.cpp:2526 in 55c9231dc3 outdated

2540-                else
2541-                    State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 1);
2542-            }
2543+
2544+        // Only support compact block relay with witnesses
2545+        if (nCMPCTBLOCKVersion != CMPCTBLOCKS_VERSION) return;

ariard commented at 9:56 pm on January 27, 2021:

I think we should keep to verify NODE_WITNESS existence among peer services, that’s a different network namespace than SENDCMPCT, they might diverge.

I believe that’s an oversight compared to “Specification for version 2” but at least should avoid to promote a buggy peer as a CB one ?

Edit: reestablished at 06ab570, you should modify 55c9231 message to warn reviewers

jnewbery commented at 10:01 am on February 3, 2021:

(pfrom.GetLocalServices() & NODE_WITNESS) is always true (unless running on regtest with -segwitheight=-1). Perhaps this will be clearer after #21009 is merged.

in src/net_processing.cpp:325 in 7b9c9eef61 outdated

320@@ -321,18 +321,12 @@ struct CNodeState {
321     bool fPreferHeaderAndIDs;
322     /**
323       * Whether this peer will send us cmpctblocks if we request them.
324-      * This is not used to gate request logic, as we really only care about fSupportsDesiredCmpctVersion,
325-      * but is used as a flag to "lock in" the version of compact blocks (fWantsCmpctWitness) we send.
326       */
327     bool fProvidesHeaderAndIDs;

ariard commented at 10:53 pm on January 27, 2021:

Current usage of fProvidersHeaderAndIDs is confusing to me.

We latched it to true at SENCMPCT reception, following this comment, it does mean this peer has signaled its availability to serve CMPCTBLOCKS.

But per-BIP152 , “Upon receipt of a “sendcmpct” message with the first and second integers set to 1, the node SHOULD announce new blocks by sending a cmpctblock message.”, so a SENDCMPCT reception should only modify our behavior and doesn’t constitute a commitment of the peer to send us CMPCTBLOCKS ?

jnewbery commented at 10:04 am on February 3, 2021:

Yes, these parameters are currently confusing (hence this PR to clean them up!)

fProvidersHeaderAndIDs is currently used to make sure that the compact blocks version doesn’t change after negotiation. If a peer sends sendcmpct(version=2), then any future sendcmpct messages where version!=2 have no effect. The peer can change hb mode by sending sendcmpct(version=2, hb_mode={true|false}).

This is going away with this PR because the software will only recognize version 2 sendcmpct messages from now on.

in src/net_processing.cpp:2877 in 019db79d7c outdated

2533+        CNodeState* nodestate = State(pfrom.GetId());
2534+        nodestate->fProvidesHeaderAndIDs = true;
2535+        nodestate->fPreferHeaderAndIDs = sendcmpct_hb;
2536+        // save whether peer selects us as BIP152 high-bandwidth peer
2537+        // (receiving sendcmpct(1) signals high-bandwidth, sendcmpct(0) low-bandwidth)
2538+        pfrom.m_bip152_highbandwidth_from = sendcmpct_hb;

ariard commented at 10:58 pm on January 27, 2021:

I think this variable should be better named m_bip152_bandwidth_mode, right now when it sets to false it means low-bandwidth, which is counter-intuitive given variable name and comment (src/net.h, L533).

jnewbery commented at 10:05 am on February 3, 2021:

This is unchanged by this PR. Eventually both of these fields should be moved to the Peer structure.

in src/net_processing.cpp:701 in c30fcbb959 outdated

322-    /**
323-      * Whether this peer will send us cmpctblocks if we request them.
324-      */
325-    bool fProvidesHeaderAndIDs;
326+    /** Whether this peer wants cmpctblocks (when possible) for block announcements. */
327+    bool m_sendcmpct_hb{false};

ariard commented at 11:02 pm on January 27, 2021:

IMO m_prefercmcpt better, otherwise name collusion is too close.

jnewbery commented at 10:07 am on February 3, 2021:

m_prefercmpct is not as precise. The peer might ‘prefer’ to fetch blocks using getdata(CMPCT), which is not what this means. m_sendcmpct_hb unambiguously means “send blocks to this peer using BIP 152 high bandwidth mode”.

theuni commented at 6:17 pm on November 15, 2021:

Could you add the “high bandwidth” part of that to the comment so the _hb makes more sense?

ariard commented at 11:05 pm on January 27, 2021: member

I think you should also modify doc/bips.md to mention we don’t support BIP 152 version 1 anymore.

jnewbery force-pushed on Feb 3, 2021

jnewbery commented at 10:14 am on February 3, 2021: member

Thanks for the review @jonatack @ariard @sipa @MarcoFalke . I think I’ve addresses all review comments now.

This PR is probably easier to review if rebased after #21009 is merged. That PR always sets NODE_WITNESS in local services, and so removes all the pfrom->GetLocalServices() & NODE_WITNESS calls in net_processing.

dhruv commented at 8:49 pm on February 5, 2021: member

#21009 was split into #21009 and #21090. #21090 now sets NODE_WITNESS in local services, and so removes all the pfrom->GetLocalServices() & NODE_WITNESS calls in net_processing.

jnewbery marked this as a draft on Feb 15, 2021

jnewbery commented at 12:01 pm on February 15, 2021: member

Marking this as draft. It’s a smaller and cleaner change once #21090 is merged, so let’s wait for that to land first.

jnewbery force-pushed on Apr 27, 2021

jnewbery commented at 11:05 am on April 27, 2021: member

I’ve rebased this on top of #21090. I’ll still leave it as draft since we need that PR to be merged first, but this is ready for review if people are interested.

dhruv commented at 6:18 pm on April 29, 2021: member

Reviewers interested in this PR, might also be interested in #21090 which is now ready for review.

jnewbery force-pushed on Jul 22, 2021

jnewbery commented at 5:18 pm on July 22, 2021: member

#21090 has been merged, so I’ve rebased this on master. This is now ready for review.

jnewbery marked this as ready for review on Jul 22, 2021

fanquake requested review from amitiuttarwar on Aug 17, 2021

naumenkogs commented at 8:54 am on September 28, 2021: member

ACK ef698b6eb3a684f52cbd2414ffc07893bbf69bbd

The PR seems ready to get rid of pre-segwit compact blocks, which makes sense now that segwit is used.

DrahtBot added the label Needs rebase on Oct 1, 2021

jnewbery force-pushed on Oct 7, 2021

jnewbery commented at 1:12 pm on October 7, 2021: member

rebased on master

jnewbery force-pushed on Oct 7, 2021

jnewbery commented at 2:18 pm on October 7, 2021: member

fixed silent merge conflict in p2p_compactblocks_blocksonly.py

jnewbery removed the label Needs rebase on Oct 7, 2021

in test/functional/p2p_compactblocks.py:268 in 10ad415037 outdated

260         # Now turn off announcements
261-        test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version))
262+        test_node.send_and_ping(msg_sendcmpct(announce=False, version=2))
263         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
264 
265-        if old_node is not None:

theuni commented at 6:04 pm on November 3, 2021:

If I’m reading correctly, I think we want to keep this test but reverse the expected outcome? That way we test that v1 doesn’t work anymore as opposed to assuming it doesn’t.

Disregard if I’m missing something else that exercises this.

jnewbery commented at 2:44 pm on November 4, 2021:

No, that’s a good point. I’ve added the test in commit [net processing] Only accept SENDCMPCT with version = 2

in src/net_processing.cpp:2734 in 8511a0841c outdated

2729@@ -2728,17 +2730,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2730             // nodes)
2731             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDHEADERS));
2732         }
2733-        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION) {
2734-            // Tell our peer we are willing to provide version 1 or 2 cmpctblocks
2735+        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION &&
2736+            WITH_LOCK(cs_main, {return State(pfrom.GetId())->fHaveWitness;})) {

theuni commented at 6:18 pm on November 3, 2021:

Also, is this a behavioral change? It’s not clear to me why the fHaveWitness check was added here. Edit: Rather, why wasn’t it there before? Edit2: Ok, so witness data is required now, but this still looks like a new enforcement for v2.

jnewbery commented at 2:44 pm on November 4, 2021:

I’ve removed the new check since it’s not necessary.

in src/net_processing.cpp:2766 in aa78fbbab2 outdated

2760@@ -2761,17 +2761,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2761         // fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
2762         if (!State(pfrom.GetId())->fProvidesHeaderAndIDs) {
2763             State(pfrom.GetId())->fProvidesHeaderAndIDs = true;
2764-            State(pfrom.GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
2765+            State(pfrom.GetId())->fWantsCmpctWitness = true;
2766         }
2767-        if (State(pfrom.GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) { // ignore later version announces
2768+        if (State(pfrom.GetId())->fWantsCmpctWitness) { // ignore later version announces

theuni commented at 6:27 pm on November 3, 2021:

Comment no longer makes sense here.

jnewbery commented at 2:44 pm on November 4, 2021:

removed!

in src/net_processing.cpp:2761 in d2e90fae0f outdated

2756@@ -2758,6 +2757,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2757         if (nCMPCTBLOCKVersion != CMPCTBLOCKS_VERSION) return;
2758 
2759         LOCK(cs_main);
2760+        // Only support compact block relay with witness peers
2761+        if (!State(pfrom.GetId())->fHaveWitness) return;

theuni commented at 6:44 pm on November 3, 2021:

According to BIP144, NODE_WITNESS means

A node will signal that it can provide witnesses using the following service bit

So I’m not sure this is strictly necessary. As a general theme though, I’m not sure bundling this logic with the value of NODE_WITNESS is necessary/correct.

jnewbery commented at 2:44 pm on November 4, 2021:

Agree. I’ve now removed it.

theuni commented at 6:47 pm on November 3, 2021: member

Concept ACK, code looks good, but I’m unsure about behavior.

I’m a little hazy on now NODE_WITNESS factors into these changes other than “one kinda seems to imply the other”.

jnewbery commented at 2:44 pm on November 4, 2021: member

Thanks for the review @theuni. I believe I’ve addressed all of your review comments.

I’m a little hazy on now NODE_WITNESS factors into these changes other than “one kinda seems to imply the other”.

You’re right. I’ve removed those changes.

Also rebased on master.

jnewbery force-pushed on Nov 4, 2021

theuni commented at 2:58 pm on November 4, 2021: member

I’m a little hazy on now NODE_WITNESS factors into these changes other than “one kinda seems to imply the other”.

You’re right. I’ve removed those changes.

Thanks. To be clear though, these are seemingly reasonable changes worth evaluating separately. Just not necessary here for dropping v1 support.

in src/net_processing.cpp:2889 in 255c528a0f outdated

2751@@ -2752,22 +2752,24 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2752         bool fAnnounceUsingCMPCTBLOCK = false;
2753         uint64_t nCMPCTBLOCKVersion = 0;
2754         vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
2755-        if (nCMPCTBLOCKVersion == 1 || nCMPCTBLOCKVersion == 2) {

theuni commented at 4:48 pm on November 4, 2021:

Was there a reason for switching this to an early return, or is that now an artifact of old changes and rebases?

Just for readability, it’s more clear imo wrapped in a if for the version check. I missed the early return when re-reviewing and was confused about why this change is safe.

jnewbery commented at 4:58 pm on November 4, 2021:

I think this is just personal taste. I much prefer early returns where they’re possible, especially compared with some of the very deeply nested if statements that we have in the code.

theuni commented at 5:04 pm on November 15, 2021:

This is nitty and not related to the code change, but I disagree with this. Deeply nested code, ugly as it may be, demonstrates the complexity of the logic in-place. Early returns don’t simplify the logic, they just add sneaky trap-doors that aren’t visible in diffs (In this case I misread the intent of the code because github didn’t show enough context)

in src/net_processing.cpp:2765 in fe4a59efa7 outdated

2759@@ -2760,17 +2760,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2760         // fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
2761         if (!State(pfrom.GetId())->fProvidesHeaderAndIDs) {
2762             State(pfrom.GetId())->fProvidesHeaderAndIDs = true;
2763-            State(pfrom.GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
2764+            State(pfrom.GetId())->fWantsCmpctWitness = true;
2765         }
2766-        if (State(pfrom.GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) { // ignore later version announces
2767+        if (State(pfrom.GetId())->fWantsCmpctWitness) {

theuni commented at 5:02 pm on November 4, 2021:

We could’ve previously fallen into this if fWantsCmpctWitness == false && nCMPCTBLOCKVersion != 2, but we won’t now. I’m not sure what the consequence of that is.

jnewbery commented at 5:23 pm on November 8, 2021:

This if block is handling subsequent sendcmpct messages from the peer. The idea is that we’ll ignore any subsequent sendcmpct message where the version doesn’t match the version in the first received sendcmpct message, i.e. if we receive:

0sendcmpct false 2
1sendcmpct true 2

Then the first sendcmpct announces support for compact blocks and the second sendcmpct enables high-bandwidth mode. However, if we receive:

0sendcmpct false 2
1sendcmpct true 1

the second sendcmpct is ignored and we don’t enable high-bandwidth mode.

This PR is removing all support for sendcmcpt <true|false> 1, so this specialized logic is no longer required.

theuni commented at 5:57 pm on November 15, 2021:

Yes, I see now. Thanks.

in src/net_processing.cpp:2757 in 255c528a0f outdated

2768-            if (!State(pfrom.GetId())->fSupportsDesiredCmpctVersion) {
2769-                State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
2770-            }
2771+
2772+        // Only support compact block relay with witnesses
2773+        if (nCMPCTBLOCKVersion != CMPCTBLOCKS_VERSION) return;

theuni commented at 5:21 pm on November 4, 2021:

I believe this is a behavioral change in a pathological case:

0SENDCMPCT true 2
1SENDCMPCT false 1

Before this would have turned off announcements, but now it looks like it won’t. Again, I’m unsure of the consequence.

jnewbery commented at 5:25 pm on November 8, 2021:

Before this would have turned off announcements, but now it looks like it won’t.

I don’t think this is true for the reason in #20799 (review). The second sendcmpct would not turn off announcements, since the if statement here:

0            if (State(pfrom.GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) { // ignore later version announces

would not be true, so the logic to turn off hb announcements here:

0                State(pfrom.GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;

would not be hit.

theuni commented at 5:41 pm on November 15, 2021:

I agree with this now, thanks for explaining!

jnewbery commented at 5:25 pm on November 8, 2021: member

Thanks for the re-review Cory. I’ve responded to your review comments.

in src/net_processing.cpp:1632 in 1e814a9e50 outdated

1513@@ -1515,18 +1514,18 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
1514         return;
1515     nHighestFastAnnounce = pindex->nHeight;
1516 
1517-    bool fWitnessEnabled = DeploymentActiveAt(*pindex, m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT);
1518+    if (!DeploymentActiveAt(*pindex, m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) return;

theuni commented at 6:43 pm on November 15, 2021:

Could you help me get confident that none of the side-effects below are still necessary? Namely:

most_recent_* = bar
ForEachNode(ProcessBlockAvailability())

Some justification in the commit message would be helpful.

jnewbery commented at 4:56 pm on March 25, 2022:

I’ve updated the commit message for the [net processing] Only relay blocks by cmpctblock and cache for fast relay if segwit is enabled. Is the explanation in there sufficient?

in src/net_processing.cpp:1539 in 1e814a9e50 outdated

1534@@ -1536,8 +1535,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
1535         CNodeState &state = *State(pnode->GetId());
1536         // If the peer has, or we announced to them the previous block already,
1537         // but we don't think they have this one, go ahead and announce it
1538-        if (state.m_sendcmpct_hb && (!fWitnessEnabled || state.m_sendcmpct) &&

theuni commented at 6:55 pm on November 15, 2021:

Could you explain what (!fWitnessEnabled || state.m_sendcmpct) represented before? I’m having trouble understanding how it used to work before even unpacking whether the change is safe :)

jnewbery commented at 5:04 pm on March 25, 2022:

On master, this line is:

0        if (state.fPreferHeaderAndIDs && (!fWitnessEnabled || state.fWantsCmpctWitness) &&

i.e. if:

the peer requested that we relay hb compact blocks and
- (the peer wants segwit-encoded (v2) compact blocks and this is a segwit block) or
- (the peer wants non-segwit-encoded (v1) compact blocks and this block is from before segwit activation)

In early commits, we remove support for v1 compact blocks, so fWantsCmpctWitness is always true if fPreferHeaderAndIDs is true.

I’ve moved the changes around in the commits a bit, so hopefully this is a bit clearer now!

theuni commented at 7:06 pm on November 15, 2021: member

Thanks for clarifying my other questions.

Is the commit “[net processing] Only relay blocks by cmpctblock and cache for fast relay if segwit is enabled” still supposed to be included here? Maybe it was intended to be dropped with the NODE_WITNESS changes?

I skimmed through it a few times and don’t understand why it’s ok, but I’m happy to take another pass at it if I’ve just missed something obvious.

DrahtBot added the label Needs rebase on Mar 21, 2022

jnewbery force-pushed on Mar 25, 2022

jnewbery commented at 5:05 pm on March 25, 2022: member

Rebased and addressed @theuni’s review comments.

DrahtBot removed the label Needs rebase on Mar 25, 2022

fanquake added this to the milestone 24.0 on Apr 7, 2022

DrahtBot added the label Needs rebase on Apr 30, 2022

jnewbery commented at 1:49 pm on April 30, 2022: member

Rebased

jnewbery force-pushed on Apr 30, 2022

DrahtBot removed the label Needs rebase on Apr 30, 2022

dergoegge commented at 2:42 pm on May 5, 2022: member

Code review ACK 4c6d983708c94af10676048d1c1379be225b8f33

I think we could also remove the bool fUseWTXID param from the CBlockHeaderAndShortTxIDs constructor as it is now always set to true. What do you think?

jnewbery commented at 6:06 pm on May 5, 2022: member

Thanks for the review @dergoegge!

I think we could also remove the bool fUseWTXID param from the CBlockHeaderAndShortTxIDs constructor as it is now always set to true. What do you think?

I agree. I left it out of this PR since there are already lots of commits. I agree it should be done in a follow-up.

in test/functional/p2p_compactblocks.py:384 in c83fbd3a71 outdated

379         node = self.nodes[0]
380         # Try announcing a block with an inv or header, expect a compactblock
381         # request
382         for announce in ["inv", "header"]:
383-            block = self.build_block_on_tip(node, segwit=segwit)
384+            block = self.build_block_on_tip(node, segwit=True)

MarcoFalke commented at 8:52 am on May 9, 2022:

Unrelated in c83fbd3a71f549a9faf1e3cd4d1c9bc99e844d36: The segwit flag is pretty nonsense. All generated blocks only have the coinbase transaction, which doesn’t have witness, so the commitment is optional, so it seems confusing to either include it or omit it without reason. If this is relevant at all, it would be better to actually test with non-coinbase transactions in the future.

jnewbery commented at 7:49 pm on May 15, 2022:

That sounds like it could be done in a follow-up. This PR is already doing a lot.

I’m happy to review if you make this change.

in src/net_processing.cpp:2895 in 3223906fb7 outdated

2891@@ -2892,17 +2892,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2892         // fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
2893         if (!State(pfrom.GetId())->fProvidesHeaderAndIDs) {
2894             State(pfrom.GetId())->fProvidesHeaderAndIDs = true;
2895-            State(pfrom.GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
2896+            State(pfrom.GetId())->fWantsCmpctWitness = true;

MarcoFalke commented at 9:03 am on May 9, 2022:

nit: I don’t see why 3223906fb7a50d089308c49c9f63e88982c515c2 is split up. In the line before you are ensuring that the version is only 2, but you leave this line untouched, which makes it confusing to review the previous commit.

Might be best to squash.

jnewbery commented at 7:51 pm on May 15, 2022:

I tried to split this PR into small commits after this comment: #20799 (comment). I’m not sure if there’s a way to structure these changes that is going to satisfy everyone, so I’d prefer to leave this as is.

in src/net_processing.cpp:373 in 7440110628 outdated

374-      */
375-    bool fProvidesHeaderAndIDs{false};
376+    /** Whether this peer wants invs or cmpctblocks (when possible) for block announcements. */
377+    bool m_requested_hb_cmpctblocks{false};
378+    /** Whether this peer will send us cmpctblocks if we request them. */
379+    bool m_provides_cmpctblocks{false};

MarcoFalke commented at 9:22 am on May 9, 2022:

744011062864d2d6dd03a5c8c381807d41063ac5: Any reason this isn’t done as a scripted diff? The only other change seems to be the comment change, which I believe will be touched again anyway when this is moved to Peer.

jnewbery commented at 7:50 pm on May 15, 2022:

Yes, I did this as a regular commit because I also wanted to update the comments to be formatted in the same way as the surrounding comments.

I agree that this could also be done as a scripted diff, and the comments changed in a different commit. I don’t think it matters which way this is done.

in src/net_processing.cpp:678 in 1aa6a56423 outdated

677@@ -678,7 +678,6 @@ class PeerManagerImpl final : public PeerManager
678     std::shared_ptr<const CBlock> m_most_recent_block GUARDED_BY(m_most_recent_block_mutex);

MarcoFalke commented at 9:24 am on May 9, 2022:

1aa6a5642327ee028d18b34e2c9b2be74c81454c commit message: s/is only to/is only used to/

?

jnewbery commented at 8:19 pm on May 15, 2022:

Updated commit text: s/that function is only to update/that function only updates/

in src/net_processing.cpp:3228 in 1aa6a56423 outdated

3224@@ -3231,9 +3225,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3225         // expensive disk reads, because it will require the peer to
3226         // actually receive all the data read from disk over the network.
3227         LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block > %i deep\n", pfrom.GetId(), MAX_BLOCKTXN_DEPTH);
3228-        CInv inv;
3229-        WITH_LOCK(cs_main, inv.type = State(pfrom.GetId())->m_provides_cmpctblocks ? MSG_WITNESS_BLOCK : MSG_BLOCK);
3230-        inv.hash = req.blockhash;
3231+        CInv inv(MSG_WITNESS_BLOCK, req.blockhash);

MarcoFalke commented at 9:39 am on May 9, 2022:

nit in 1aa6a5642327ee028d18b34e2c9b2be74c81454c:

CInv inv{…};

jnewbery commented at 8:23 pm on May 15, 2022:

Done

MarcoFalke approved

MarcoFalke commented at 9:40 am on May 9, 2022: member

ACK 4c6d983708c94af10676048d1c1379be225b8f33 🐉

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 4c6d983708c94af10676048d1c1379be225b8f33 🐉
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiu9Qv/eRGOPOxkDIxtTbbq7u/B5eVKZvLidzH3yNyz2rSFgVpLJyI8DcCdrxrL
 8b6sORYAtL428YdGe8YaVVMp+RHC/grTiP8jHPzL+dyFTZ9UNNL32bDMaFWyGY8Kd
 9/ppUcMPlGuSOmU4JPeTBuE/i8Sp0ii0u9kM4P1rYtGr2XKjJqhF7EOluFBuS3Pjf
10WCYq6zDgLu6zcTLgdUJd9TEtEuOvQYa77LXJwZ3I7EJxUiDTLAflmgnoff9R/xiU
119+TTGV8bE1iTkRwwR86J4t2hpOlMe7e7tddCWzo/Jo6bwXdSMRpwlxrOjJ+3XrOo
12l+iqrSewv9h2lajD+sdD8zj4R8ougXe61/Fgv6UbVA2olKM7VCFSCQdKOEBBNqtn
13gMNJVoS6qm9bngN1kBJnDqmi8DFC//R29mWyvRHGDvy5dTT/CSqwkRK2mh+Cl2Pg
14Q4EFJ0jVw5o07XumKei+iKSh0YgKgs6gzpfVI07aveP2i4E6zI7MZqbkkjvRDP1u
15Ip3XHQrm
16=/d5g
17-----END PGP SIGNATURE-----

DrahtBot added the label Needs rebase on May 13, 2022

[net] Stop testing version 1 compact blocks.

Support for version 1 is removed in the following commits.

cba909eaf9

[net processing] Only advertise support for version 2 compact blocks

Subsequent commits will remove support.

16730b64bb

[net processing] Only accept `sendcmpct` with version=2

Subsequent commits will remove support for other versions of compact blocks.

Add a test that a received `sendcmpct` message with version = 1 is
ignored.

42882fc8fc

[net processing] Simplify `sendcmpct` processing

nCMPCTBLOCKVersion must always be 2 when processing.

25edb2b7bd

[net processing] Remove fSupportsDesiredCmpctVersion

It is now completely redundant with fProvidesHeadersAndIDs.

a45d53cab5

[net processing] Remove fWantsCmpctWitness

It is now completely redundant with fProvidesHeadersAndIDs.

b486f72176

[net processing] fPreferHeaderAndIDs implies fProvidesHeaderAndIDs

Remove all if(fProvidesHeaderAndIDs) conditionals inside
if(fPreferHeaderAndIDs) conditionals.

30c3a01874

[net processing] Tidy up `sendcmpct` processing

- use better local variable names
- drop unnecessary if statements

d0e9774174

[net processing] Rename CNodeState compact block members

fPreferHeaderAndIDs -> m_requested_hb_cmpctblocks
fProvidesHeaderAndIDs -> m_provides_cmpctblocks

3b6bfbce38

[net processing] Only relay blocks by cmpctblock and cache for fast relay if segwit is enabled

This introduces an early exit in PeerManagerImpl::NewPoWValidBlock() if
segwit has not been activated for the block. This means that we won't cache the
block/compact block for fast relay and won't relay the cmpctblock
immediately to peers that have requested hb compact blocks. This is fine
because any block where segwit is not yet activated is buried deep in
the chain, and so compact block relay will not be effective.

It's ok not to cache the block/compact block for fast relay for the same
reason - the block must be very deeply buried in the block chain.

ProcessBlockAvailability() also won't get called for all nodes. This is
also fine, since that function only updates hashLastUnknownBlock
and pindexBestKnownBlock, and is called early in every SendMessages()
call.

bb985a7b6a

[net processing] Remove redundant nodestate->m_sendcmpct check in MaybeSetPeerAsAnnouncingHeaderAndIDs() a50e34c367

jnewbery force-pushed on May 15, 2022

jnewbery deleted a comment on May 15, 2022

jnewbery commented at 8:24 pm on May 15, 2022: member

Thanks for the reviews @dergoegge and @MarcoFalke. I’ve addressed all review comments and rebased on latest master.

DrahtBot removed the label Needs rebase on May 15, 2022

MarcoFalke approved

MarcoFalke commented at 8:43 am on May 16, 2022: member

re-ACK a50e34c367608fcec9697893981bfa294a7c08d after rebase 👓

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK a50e34c367608fcec9697893981bfa294a7c08d after rebase 👓
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgSIwwAx5jdanYqhTgoX2H5SG6pQ9aoBujOCn7R5sg8loLagrlzE9YOMkNxqTHJ
 8HY3V4lI3HFaa6TGqgowJNY2MbxNGQ3zqfepGsYe18NINnbnNDt6ZHG54uFQ65foV
 9hv4GRKRAZ4NGZ2mq28LVwLrVJTyQPIepurdlGLRmzSpRPcLnO81maL+48vFgle7T
10H6o1sabgMWGffyfk6sUF+DxgYlk7JiKV8qVYcW3THAZhPuU1UBULB/fCZHnNjNEI
11in5vTyIjEaraWyCpawc2OED0MjXghU1bwi8VU630hNE9DdnpBYMhq2TXKC52KM/n
12SHIokmIIPk4mpif8S6RBZZGwUS5B28PgAoRfteEk5ce0P+CeewWnMppZFl7lMkSk
13aHxl6e7UU7jqjtvotOE5TAubGMG0vn4oWnPx0k3PTtMMFRuSRj5gVvyH5D0y0BbL
1490+0z3sS709purkQ+5q+zx11l1sMewFHTxcuVOyjMZ6aM2jd3SFi2MDfB/kPeyqd
15mNXfG6pu
16=E4BX
17-----END PGP SIGNATURE-----

fanquake requested review from dergoegge on May 16, 2022

dergoegge commented at 10:51 am on May 16, 2022: member

ACK a50e34c367608fcec9697893981bfa294a7c08d1 - Only changes since my last review were a rebase and some nits.

Reviewed using git range-diff 5d53cf38784df9ad9ed10306bf3fba3002fd9244..4c6d983708c94af10676048d1c1379be225b8f33 b74a6dde8cf50703a8814d402333580e4cdfea59..a50e34c367608fcec9697893981bfa294a7c08d1

dergoegge approved

fanquake merged this on May 16, 2022

fanquake closed this on May 16, 2022

jnewbery deleted the branch on May 16, 2022

jnewbery referenced this in commit 82eb676325 on May 16, 2022

jnewbery referenced this in commit 3ab9f69e2a on May 16, 2022

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

Review suggestions implemented in #25147

jnewbery referenced this in commit c65bf50b44 on May 17, 2022

jnewbery referenced this in commit 97e4a5d7dc on May 17, 2022

jnewbery referenced this in commit 8184dcbdd1 on May 17, 2022

jnewbery referenced this in commit bf6526f4a0 on May 18, 2022

fanquake referenced this in commit fdb82a30be on May 19, 2022

naumenkogs referenced this in commit 58f9a11a1c on May 23, 2022

naumenkogs referenced this in commit 32bfd8dc37 on May 23, 2022

sidhujag referenced this in commit 0f6f2e49d1 on May 28, 2022

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

janus referenced this in commit 094e009176 on Aug 4, 2022

janus referenced this in commit fb4c773fd8 on Aug 4, 2022

MarcoFalke locked this on Feb 8, 2023

net processing: Only support version 2 compact blocks #20799

Conflicts