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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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; }
Could re-use the helper methods from above here in the combined helpers
Thanks Fabian. I mulled on doing this, but I think I'd rather keep the method definitions independent.
Code review ACK 82c92976792f9c9c11b38cb0092f18728c97e64e
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. @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; }
Should this contain MSG_FILTERED_WITNESS_BLOCK?
Well-spotted; added it in the rebase for now. The one place this helper is currently used (https://github.com/bitcoin/bitcoin/pull/19610/files#diff-eff7adeaec73a769788bb78858815c91L1763) did not include MSG_FILTERED_WITNESS_BLOCK, so we'd need to be sure including it doesn't change behavior (or was an oversight).
Oh, it looks like 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).
Concept ACK. Needs rebase.
Rebased with four commits pulled in from @jnewbery's https://github.com/bitcoin/bitcoin/compare/master...jnewbery:2020-07-split-already-have branch as well as the changes in #19611.
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:
Personally I don't really know if it makes sense for "plain-data" network message types like CInv to have private fields. I see it more as a struct with helper methods than really an encapsulated object with functionality.
Oh ok, in that case we may want to make CInv a struct to avoid confusion. As a class, it looked like encapsulation was desirable, and only one place in the code was still accessing CInv::type from the outside so it was easy to make it private.
Er, renaming 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/
$ git grep -i blk src |wc -l
108
$ git grep -i block src |wc -l
11014
I prefer to pass on this for these reasons:
(a) to maintain a consistent naming style in CInv (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?
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)
That message type is unused (see #19610 (review)) and has since been commented out by #19721 (merged).
Removing it in the rebase.
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
- } else {
+ } 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.
Still awaiting rebase
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()) {
Perhaps we should add an else branch after this that logs that an unknown INV type was received. Previously we'd log it as AlreadyHave.
Done in the same commit and updated the commit message, and 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.
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
- for (CInv &inv : vInv)
- {
+ for (CInv& inv : vInv) {
utACK 2a2ee28a2ac2d2cc11ad8a44b036bb4218c13d9a
One suggestion for improving logging inline.
utACK 25176e62d0e60a3bca92aa758f935ee782531f75
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; }
nit: the methods IsUndefined() and IsMsgWitnessTx() are unused.
Good point; probably worth removing them for now.
Done
ACK ac0617d37
Looks good, compiled locally and ran tests.
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 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.
Removed the unused helper methods from the first commit per git diff 533a9a0 125468a
ACK 125468af9
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?
Code review ACK fb56d37612dea6666e7da73d671311a697570dae Agree it's better to split out the P2P change.
utACK fb56d37612dea6666e7da73d671311a697570dae
ACK fb56d3761