Building on #19590 and the recent wtxid and GenTxid changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with -w to ignore spacing.
Co-authored by John Newbery.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
443 bool IsMsgWitnessTx() const { return type == MSG_WITNESS_TX; }
444+ bool IsMsgFilteredWitnessBlk() const { return type == MSG_FILTERED_WITNESS_BLOCK; }
445
446 // Combined-message helper methods
447- bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
448+ bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
AlreadyHave() is to split that function up into AlreadyHaveTx() and AlreadyHaveBlock() since those are two totally separate concepts. @jonatack would you be interested in taking the commits from here: https://github.com/jnewbery/bitcoin/commits/2020-07-split-already-have? (see also #19569 (review))
445
446 // Combined-message helper methods
447- bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
448+ bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
449+ bool IsMsgBlkOrMsgWitnessBlk() const { return type == MSG_BLOCK || type == MSG_WITNESS_BLOCK; }
450+ bool IsGenBlkMsg() const { return type == MSG_BLOCK || type == MSG_FILTERED_BLOCK || type == MSG_CMPCT_BLOCK || type == MSG_WITNESS_BLOCK; }
MSG_FILTERED_WITNESS_BLOCK?
MSG_FILTERED_WITNESS_BLOCK, so we’d need to be sure including it doesn’t change behavior (or was an oversight).
MSG_FILTERED_WITNESS_BLOCK isn’t actually used and is ‘reserved for future use’: https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#relay. I recommend removing it from here, removing IsMsgFilteredWitnessBlk() (since it’s not used anywhere) and adding a comment to MSG_FILTERED_WITNESS_BLOCK saying it’s unused (I think we could even comment out the value since it’s unused in the code).
I think a better approach than the changes within AlreadyHave() is to split that function up into AlreadyHaveTx() and AlreadyHaveBlock() since those are two totally separate concepts.
Great idea. The block and transaction paths have drifted apart so much that it no longer makes sense to have a common function there.
399@@ -400,9 +400,11 @@ enum GetDataMsg : uint32_t {
400 /** inv message data */
401 class CInv
402 {
403+private:
CInv::type from the outside so it was easy to make it private.
CInv to Inv to make it a struct would be too much noise. Done, dropped the last commit “p2p: refactor CInv::type from public to private class member”.
411@@ -412,14 +412,22 @@ class CInv
412 std::string ToString() const;
413
414 // Single-message helper methods
415- bool IsMsgTx() const { return type == MSG_TX; }
416- bool IsMsgWtx() const { return type == MSG_WTX; }
417+ bool IsUndefined() const { return type == UNDEFINED; }
418+ bool IsMsgTx() const { return type == MSG_TX; }
419+ bool IsMsgBlk() const { return type == MSG_BLOCK; }
[continuing conversation from another PR]
nit: s/Blk/Block/
0$ git grep -i blk src |wc -l
1 108
2$ git grep -i block src |wc -l
3 11014
tx, blk)
(b) block would make the methods names longer
(c) blk is an established abbreviation: in the codebase, block data files called blk*.dat, etc.
(d) the shorter blk seems more consistent with the abbreviated naming style in use in recent PRs like #19569
424 bool IsMsgWitnessTx() const { return type == MSG_WITNESS_TX; }
425+ bool IsMsgFilteredWitnessBlk() const { return type == MSG_FILTERED_WITNESS_BLOCK; }
426
427 // Combined-message helper methods
428- bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
429+ bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
Did you consider making these switch statements without defaults, so that the compiler warns if a new type is added to the enum but not here?
I think that requires turning enum GetDataMsg into an enum class, and changing the enum type from int to GetDataMsg would break the remaining five GetDataMsg::MSG_BLOCK/MSG_WTX | GetFetchFlags(pfrom) operations.
One inline comment. A couple more suggestions:
1760@@ -1771,7 +1761,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
1761 // expensive to process.
1762 if (it != pfrom.vRecvGetData.end() && !pfrom.fPauseSend) {
1763 const CInv &inv = *it++;
1764- if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) {
1765+ if (inv.IsGenBlkMsg()) {
Could this result in unintended behavioral change?
Previously if inv.type == MSG_FILTERED_WITNESS_BLOCK we would not execute the code inside this if block. With this patch we would execute it.
Related to #19610 (review)
In commits p2p: remove unused nFetchFlags from NetMsgType::(TX|INV) processing - why would it be unused? It looks used to me. Maybe I am missing something obscure? If that is the case, then maybe the commit messages warrant some more elaborate explanations?
The change
0- } else {
1+ } else if (inv.IsGenTxMsg()) {
does not belong to commit [net processing] Remove mempool argument from AlreadyHaveBlock()
In commits p2p: remove unused nFetchFlags from NetMsgType::(TX|INV) processing - why would it be unused? It looks used to me. Maybe I am missing something obscure? If that is the case, then maybe the commit messages warrant some more elaborate explanations?
Because GetFetchFlags() can only add the MSG_WITNESS_FLAG, which is added to the CInv.type field. That CInv only passed to AlreadyHave() or ToGenTxid() and neither of those functions do anything different depending on whether the CInv type is MSG_TX MSG_WITNESS_TX`. I agree that the git commit log could be expanded to explain this.
The change […] does not belong to commit [net processing] Remove mempool argument from AlreadyHaveBlock()
It’s an important change. See #19569 (review) for context. Perhaps it should be split into a separate commit or have a commit log explaining why it’s needed.
utACK 1723d0025b744f12b853935aead9acd9f8f32b42
The last commit is tagged as authored by and co-authored by me. I don’t think we need both tags!
2680@@ -2697,14 +2681,18 @@ void ProcessMessage(
2681 // then fetch the blocks we need to catch up.
2682 best_block = &inv.hash;
2683 }
2684- } else {
2685+ } else if (inv.IsGenTxMsg()) {
inv.IsMsgBlk() section where it’s used.
proposed a commit to move the code immediately after, which sends and logs a GETHEADERS message, into the inv.IsMsgBlk() section where it’s used.
This is a (bad) behaviour change. It means if a peer sends us an inv message with multiple MSG_BLOCK items, we’ll send a getheaders for each of those items instead of just the last one. Each getheaders contains a locator, so this could result in a lot of bandwidth usage.
Oops, I misread the nesting level and the tests didn’t mind. Thank you! Dropped the commit, and fixed up the bracket as follows in 3b79249dbd45b43
0- for (CInv &inv : vInv)
1- {
2+ for (CInv& inv : vInv) {
utACK 2a2ee28a2ac2d2cc11ad8a44b036bb4218c13d9a
One suggestion for improving logging inline.
Rebased.
git range-diff 7f609f6 25176e6 ac0617d
utACK ac0617d3718ac3e330a70b30ef1edd8a46faaf9a
Verified the range-diff locally
417@@ -418,12 +418,24 @@ class CInv
418 std::string ToString() const;
419
420 // Single-message helper methods
421- bool IsMsgTx() const { return type == MSG_TX; }
422- bool IsMsgWtx() const { return type == MSG_WTX; }
423+ bool IsUndefined() const { return type == UNDEFINED; }
IsUndefined() and IsMsgWitnessTx() are unused.
ACK ac0617d37
Looks good, compiled locally and ran tests.
CInv::type change from int to uint32_t change should be added back (not necessarily the private qualifier). While fuzzing it brings up the following complaint https://github.com/bitcoin/bitcoin/issues/19678
ACK 533a9a0d2
I can’t make sense of the CI failure (and tell whether it is related to this PR).
The nFetchFlags code can be removed here because GetFetchFlags() can only add
the MSG_WITNESS_FLAG, which is added to the CInv::type field. That CInv is only
passed to AlreadyHave() or ToGenTxid(), and neither of those functions do
anything different depending on whether the CInv type is MSG_TX or
MSG_WITNESS_TX.
Co-authored by: John Newbery <john@johnnewbery.com>
and otherwise log that an unknown INV type was received.
In INV processing, when handling transaction type inv messages,
ToGenTxid() expects that we constructed the CInv ourselves or
that we verified that it is for a transaction type CInv.
Therefore, change this `else` branch into an `else if (inv.GenMsgTx())`
to make this safer and log any INVs that fall through.
git diff 533a9a0 125468a
I think the CInv::type change from int to uint32_t change should be added back (not necessarily the private qualifier). While fuzzing it brings up the following complaint #19678
Why? This seems unrelated to this PR (the UBSan warning happens before this PR). Changing the type of CInv.type (which is serialized/deserialized for network messages) is a very different change from the internal refactors in this PR. Can you save it for a different PR?