p2p: Move all header verification into the network layer, extend logging #19107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
troygiorshev
force-pushed
the
p2p-refactor-header
branch
2 times, most recently
from
821f4af
to
800e56c
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
troygiorshev
force-pushed
the
p2p-refactor-header
branch
from
800e56c
to
d5db6ad
Compare
troygiorshev
force-pushed
the
p2p-refactor-header
branch
from
d5db6ad
to
c8fb757
Compare
This was referenced May 29, 2020
|
Concept ACK |
|
There was a problem hiding this comment.
Concept ACK. Verified unit and functional tests are passing. Thanks for adding the doxy comment on ReceiveMsgBytes, that cleared some of my earlier doubts.
I found the commits to be a little difficult to parse as the same function is being changed in different commits. This can be good for demonstrating the stepwise changes, but in this case, it resulted in reviewing some lines which later got removed in later commits. So maybe a little consolidation of commits can help future review.
Overall strong concept ACK as this refactor has been discussed in review club and generally accepted as a good idea except the connection dropping behavior.
Below are few comments on the code which shows some probable scope of improvement.
jnewbery
reviewed
Jun 1, 2020
There was a problem hiding this comment.
Great stuff so far @troygiorshev !
I try to format my commit logs as:
[component]: short summary (50 chars or less if possible)
Reason for the change (why not what or how) (wrapped at 72 or 80 columns)
See https://chris.beams.io/posts/git-commit/ for more tips on writing great commit logs.
troygiorshev
force-pushed
the
p2p-refactor-header
branch
from
598e6d3
to
d8cbdc4
Compare
|
Rebased, fixed failing fuzz tests. Removed some of the unneeded test changes, they're now in #19177. Overall the commits are cleaned up. They are all atomic now, with full commit messages. I've also implemented many of the suggestions, thanks again to everyone for the review so far! |
troygiorshev
force-pushed
the
p2p-refactor-header
branch
from
367bb95
to
ead6393
Compare
|
|
|
Thanks for the review @jonatack and @ryanofsky! I'm going to leave the suggestions for a follow-up PR. As it stands, this PR has three ACKs from people experienced in this part of the codebase. I'd like to reduce the diff that they have to go through.
|
This was referenced Sep 19, 2020
This is intended to only be used for logging. This will allow log messages in the following commits to keep recording the peer's ID, even when logging is moved into V1TransportDeserializer.
This removes the m_valid_checksum member from CNetMessage. Instead, GetMessage() returns an Optional. Additionally, GetMessage() has been given an out parameter to be used to hold error information. For now it is specifically a uint32_t used to hold the raw size of the corrupt message. The checksum check is now done in GetMessage.
This commit removes the single-parameter contructor of CMessageHeader and replaces it with a default constructor. The single parameter contructor isn't used anywhere except for tests. There is no reason to initialize a CMessageHeader with a particular messagestart. This messagestart should always be replaced when deserializing an actual message header so that we can run checks on it. The default constructor initializes it to zero, just like the command and checksum. This also removes a parameter of a V1TransportDeserializer constructor, as it was only used for this purpose.
This adds a CChainParams& member to V1TransportDeserializer member, and use it in place of many Params() calls. In addition to reducing the number of calls to a global, this removes a parameter from GetMessage (and will later allow us to remove one from CMessageHeader::IsValid())
This moves header size and netmagic checking out of net_processing and into net. This check now runs in ReadHeader, so that net can exit early out of receiving bytes from the peer. IsValid is now slimmed down, so it no longer needs a MessageStartChars& parameter. Additionally this removes the rest of the m_valid_* members from CNetMessage.
troygiorshev
force-pushed
the
p2p-refactor-header
branch
from
ead6393
to
deb5271
Compare
|
Trivial rebase |
|
Code review ACK deb5271. Should be ready for merge with a reACK from @dongcarl and @ryanofsky |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 29, 2020
…rk layer, extend logging deb5271 Remove header checks out of net_processing (Troy Giorshev) 52d4ae4 Give V1TransportDeserializer CChainParams& member (Troy Giorshev) 5bceef6 Change CMessageHeader Constructor (Troy Giorshev) 1ca20c1 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev) 890b1d7 Move checksum check from net_processing to net (Troy Giorshev) 2716647 Give V1TransportDeserializer an m_node_id member (Troy Giorshev) Pull request description: Inspired by bitcoin#15206 and bitcoin#15197, this PR moves all message header verification from the message processing layer and into the network/transport layer. In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor. For more context, see https://bitcoincore.reviews/15206.html#l-81. This PR improves the separation between the p2p layers, helping improvements like [BIP324](bitcoin#18242) and bitcoin#18989. ACKs for top commit: ryanofsky: Code review ACK deb5271 just rebase due to conflict on adjacent line jnewbery: Code review ACK deb5271. Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
Closed
jnewbery
pushed a commit
to jnewbery/bitcoin
that referenced
this pull request
Aug 18, 2021
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This throws an error if COMMAND_OTHER doesn't exist, which should never happen. `find()` instead just accessed the last element, which could make debugging more difficult. Resolves review comments from PR19107: - bitcoin#19107 (comment) - bitcoin#19107 (comment)
jnewbery
pushed a commit
to jnewbery/bitcoin
that referenced
this pull request
Aug 19, 2021
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This throws an error if COMMAND_OTHER doesn't exist, which should never happen. `find()` instead just accessed the last element, which could make debugging more difficult. Resolves review comments from PR19107: - bitcoin#19107 (comment) - bitcoin#19107 (comment)
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Nov 2, 2021
…sportDeserializer::GetMessage() f3e451b [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev) 8c96008 [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev) Pull request description: Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This throws an error if COMMAND_OTHER doesn't exist, which should never happen. `find()` instead just accessed the last element, which could make debugging more difficult. Resolves review comments from PR19107: - bitcoin/bitcoin#19107 (comment) - bitcoin/bitcoin#19107 (comment) ACKs for top commit: theStack: Code-review ACK f3e451b ryanofsky: Code review ACK f3e451b. Changes since last review in bitcoin/bitcoin#20364 (review) were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit. Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.
In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.
For more context, see https://bitcoincore.reviews/15206.html#l-81.
This PR improves the separation between the p2p layers, helping improvements like BIP324 and #18989.