net: Reduce `TransportDeserializer` interface to 2 methods #18644

pull dongcarl wants to merge 4 commits into bitcoin:master from dongcarl:2020-04-transport-readmessages changing 2 files +67 −32

dongcarl commented at 1:44 AM on April 15, 2020: member

Mostly inspired by #16202 (comment), I believe this will also allow some simplifications in #18242.

If you're concerned about the net added line count, don't be: this PR also adds some documentation to net.{h,cpp}.

I'm happy for this to go in after #18242 and I'll rebase.
net: Add ReceiveMsgBytes documentation. 77eed48315

net: TransportDeserializer: Add and use ReadMessages.

This combines some unnecessary public separation of logic between the
member functions of TransportDeserializers and will allow us to greatly
simplify the TransportDeserializer interface in a subsequent commit.

This will also allow us to eliminate some member variables when
implementing BIP324.

558227414b

net: Make ReadMessages conform to developer-notes.md. 5b76445c94

net: Simplify TransportDeserializer interface.

Now that we have TransportDeserializer::ReadMessages, we don't need
any other functions in the TransportDeserializer interface.

The SetVersion function is left alone because I assume that BIP324 will
use it somehow.

52e9088042

dongcarl added the label P2P on Apr 15, 2020
fanquake requested review from jonasschnelli on Apr 15, 2020

in src/net.h:651 in 52e9088042

 652 | +     *
 653 | +     * @param[in] message_start The expected message start string.
 654 | +     * @param[in] time The approximate time at which these messages were
 655 | +     *                 received in microseconds since UNIX epoch.
 656 | +     *
 657 | +     * @returns A tuple where the first element indicates whether or not an

MarcoFalke commented at 1:52 AM on April 15, 2020:

Don't we use Optional<> for this?

dongcarl commented at 2:02 AM on April 15, 2020:

See the @note

DrahtBot commented at 3:00 AM on April 15, 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:
- #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
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.

in src/net.cpp:589 in 52e9088042

 602 | -            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command);
 603 | -            if (i == mapRecvBytesPerMsgCmd.end())
 604 | -                i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
 605 | -            assert(i != mapRecvBytesPerMsgCmd.end());
 606 | -            i->second += msg.m_raw_message_size;
 607 | +    std::vector<CNetMessage> msgs = std::get<1>(tup);

sipa commented at 6:51 AM on April 15, 2020:

This will copy the vector. Two options:

Add an std::move() around the std::get here
Change the declaration to std::vector<CNetMessage>& msgs (or auto& msgs).

in src/net.h:662 in 52e9088042

 663 | +     *       already been decoded. These CNetMessage%s should not be ignored and
 664 | +     *       probably should be processed.
 665 | +     *
 666 | +     * @see CNode::ReceiveMsgBytes(const char *, unsigned int, bool&)
 667 | +     */
 668 | +    virtual std::tuple<bool, std::vector<CNetMessage>> ReadMessages(const char *receive_buffer, unsigned int num_bytes, const CMessageHeader::MessageStartChars& message_start, int64_t time) = 0;

sipa commented at 6:52 AM on April 15, 2020:

Any reason to use an std::tuple with just two members here (instead of an std::pair)?

in src/net.h:721 in 52e9088042

 718 | +        std::vector<CNetMessage> out_msgs;
 719 | +        while (num_bytes > 0) {
 720 | +            // absorb network data
 721 | +            int handled = Read(receive_buffer, num_bytes);
 722 | +            if (handled < 0) {
 723 | +                return std::make_tuple(false, out_msgs);

sipa commented at 6:53 AM on April 15, 2020:

Add an std::move around out_msgs here to avoid a copy.

in src/net.h:731 in 52e9088042

 728 | +
 729 | +            if (Complete()) {
 730 | +                out_msgs.push_back(GetMessage(message_start, time));
 731 | +            }
 732 | +        }
 733 | +        return std::make_tuple(true, out_msgs);

sipa commented at 6:53 AM on April 15, 2020:

Same here.

jonasschnelli commented at 8:06 AM on April 15, 2020: contributor
Thanks @dongcarl.

I'm NACKish on this. Once we have the handshake implemented, the CNode instances should be capable to individually read and process messages.

I'd suggest to keep the per-message-process-logic in the CNode instance to allow switching the deserializer between individual messages.

Let me make an example:
- Peer A sends BIP324 handshake to peer B (very first message, 32bytes)
- Peer A sends encrypted version message after BIP324 in the same buffer
- Peer B processes the buffer, needs to switch to encrypted traffic (switch the deserializer) between the two messages in the same buffer.
Have a look at the following draft commit BIP324 handshake, (not available in a PR): https://github.com/jonasschnelli/bitcoin/commit/0a50bdb16d66b3bc24b761d435fbe02a141ff8e3

Maybe there are better ways how to do this,... open to suggestions.
jonasschnelli removed review request from jonasschnelli on Apr 15, 2020
dongcarl commented at 4:43 PM on April 15, 2020: member
@jonasschnelli
Let me make an example:

Peer A sends BIP324 handshake to peer B (very first message, 32bytes)

Peer A sends encrypted version message after BIP324 in the same buffer

Peer B processes the buffer, needs to switch to encrypted traffic (switch the deserializer) between the two messages in the same buffer.

Have a look at the following draft commit BIP324 handshake, (not available in a PR): jonasschnelli@0a50bdb
That's a good point! I think I have a way to do this cleanly, but I'm going to shut my mouth until I have the code for it...

Is the right way to rebase your https://github.com/bitcoin/bitcoin/compare/master...jonasschnelli:2019/08/net_v2 branch to: rebase #14049 over #18242 then cherry-picking the last 3 commits from https://github.com/bitcoin/bitcoin/compare/master...jonasschnelli:2019/08/net_v2?

Happy to send you the rebased branches so you can use them in your PRs!
jonasschnelli commented at 6:09 PM on April 15, 2020: contributor

That's a good point! I think I have a way to do this cleanly, but I'm going to shut my mouth until I have the code for it...

Great! Let's see with what you come up.

Is the right way to rebase your master...jonasschnelli:2019/08/net_v2 branch to: rebase #14049 over #18242 then cherry-picking the last 3 commits from master...jonasschnelli:2019/08/net_v2?

The jonasschnelli:2019/08/net_v2 branch is experimental and in an extrem draft state. The purpose of it was to validate the BIP and look for conceptual flaws during an (draft) implementation. You can try to rebase the handshake on top of 18242 (yeah, you'll need #14049/enabling ECDH as well).
dongcarl commented at 6:12 PM on May 26, 2020: member

Closing, will reopen or open a new PR once I have something!
dongcarl closed this on May 26, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#16202 p2p: Refactor network message deserialization #18242 Add BIP324 encrypted p2p transport de-/serializer (only used in tests)

net: Reduce `TransportDeserializer` interface to 2 methods #18644

Conflicts