Follow-ups to 19107 #20364

troygiorshev · 2020-11-10T21:46:00Z

This PR contains a few follow-ups to #19107.

Most notable is a change to GetMessage, where it no longer returns an optional. Ultimately this was done in a heavy-handed way that didn't improve readability and would probably cause more problems than it would save.

DrahtBot · 2020-11-11T02:57:42Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Move SocketSendData lock annotation to header #20864 (net: Move SocketSendData lock annotation to header 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.

troygiorshev · 2020-11-11T16:23:57Z

CI fails with "Timed out!" when running the fuzz tests, at exactly 2h of running time. Looks to be unrelated to this PR.

maflcko · 2020-11-11T16:32:24Z

You can fix that by rebasing on current master

ryanofsky

Code review ACK ec4fd8a. Thanks for followups #19107 (comment) and #19107 (comment)!

src/net.cpp

troygiorshev · 2020-11-17T05:40:42Z

git range-diff master ec4fd8a a10135e

Trivial rebase to master, to fix the CI error

GetMesage no longer returns an optional. Additionally, we now access mapRecvBytesPerMsgCmd with `.at` not `.find`. This is shorter and more clear. 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. For this part, I think this is a definite improvement, but I'm not sure it's the best solution. Open to suggestions.

troygiorshev · 2020-11-17T05:53:34Z

git range-diff master a10135e 78fb468

Initialized out parameter reject_message
Renamed COMMAND to MSGTYPE in error logs

ajtowns

PR title/description should summarise what was actually being done in this PR -- I think it's "[refactor] net.cpp: various minor cleanups" or similar?

I think you could split out ".find() to .at()" and "drop redundant out_err_raw_size" as separate commits; they seem pretty independent. I'm not sure having "this function might throw, but we're sure it never will" is better than "this function might deref end() but we're sure it never will" is better. Maybe an Assert() would be better, or maybe using &map[NET_MESSAGE_COMMAND_OTHER] (which creates it if missing) would be better.

I'm not seeing what the benefit of dropping the optional is though, it just changes:

    Optional<CNetMessage> x = GetMessage(time);
    if (!x) { ...; return; }
    do_stuff(*x);

to

    bool reject_message;
    CNetMessage x = GetMessage(time, reject_message);
    if (reject_message) { ...; return; }
    do_stuff(x);

which doesn't seem like an improvement to me?

ajtowns · 2020-11-19T08:39:57Z

src/net.cpp

@@ -2980,7 +2980,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
        LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
    }

-    m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION));
+    m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));


GetId() is an inline method that returns id already; this change seems pretty pointless?

We should remove GetId() entirely. id is a const member so should just be made public.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-get

We certainly shouldn't be using trivial getters inside CNode methods. They can access member variables directly.

ajtowns · 2020-11-19T08:49:58Z

src/net.cpp

                continue;
            }

            //store received bytes per message command
            //to prevent a memory DOS, only allow valid commands
-            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command);
+            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command);
            if (i == mapRecvBytesPerMsgCmd.end())
                i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);


Should change this .find() to .at() as well?

std::map::at returns a reference to the value, not an iterator on the map, which is what is required here for the i->second call below. Alternatively:

diff --git a/src/net.cpp b/src/net.cpp index 9c2a79b809..e6a3d69810 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -670,11 +670,11 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete //store received bytes per message command //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command); - if (i == mapRecvBytesPerMsgCmd.end()) - i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); - assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += msg.m_raw_message_size; + if (auto i = mapRecvBytesPerMsgCmd.find(msg.m_command); i != mapRecvBytesPerMsgCmd.end()) { + i->second += msg.m_raw_message_size; + } else { + mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size; + }

ryanofsky · 2020-11-19T12:09:13Z

I'm not seeing what the benefit of dropping the optional is though, it just changes:

The benefit of dropping optional is that the calling code can reliably use msg.m_raw_message_size, instead of sometimes using m_raw_message_size and sometimes having to look in out_err_raw_size depending on the code path. It is easy to imagine a future change changing the logic a little and causing the wrong size to be read from the wrong location. Getting rid of optional also would have reduced the size of #19107 diff, which was the original reason for the suggestion, but that ship already sailed.

I'm not sure having "this function might throw, but we're sure it never will" is better than "this function might deref end() but we're sure it never will" is better.

I won't die on this hill but I do think an error is better than undefined behavior

ryanofsky

Code review ACK 78fb468. Only changes since last review are resetting output parameter and tweaking log messages

DrahtBot · 2021-01-07T15:06:15Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jnewbery · 2021-02-04T11:32:19Z

Code review ACK 78fb468

Needs rebase

jnewbery · 2021-02-04T11:33:49Z

Suggest you remove "For this part, I think this is a definite improvement, but I'm not sure it's the best solution. Open to suggestions." from the first commit log. The commit messages should be used to explain the code changes, and github should be used for review/meta-commentary.

fanquake · 2021-08-18T12:09:12Z

Closing this as up for grabs.

…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

DrahtBot added the P2P label Nov 10, 2020

This was referenced Nov 11, 2020

net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection #19771

Merged

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

Allow whitelisting outgoing connections #17167

Closed

ryanofsky approved these changes Nov 11, 2020

View reviewed changes

src/net.cpp Show resolved Hide resolved

maflcko reviewed Nov 12, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

troygiorshev force-pushed the 2020-11-19107-follow-ups branch from ec4fd8a to a10135e Compare November 17, 2020 05:40

troygiorshev added 3 commits November 17, 2020 00:50

Replace GetID() with id in TransportDeserializer constructor

b9f6623

Make error log messages more descriptive

78fb468

troygiorshev force-pushed the 2020-11-19107-follow-ups branch from a10135e to 78fb468 Compare November 17, 2020 05:52

ajtowns reviewed Nov 19, 2020

View reviewed changes

ryanofsky approved these changes Nov 19, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 17, 2020

Add I2P support using I2P SAM #20685

Merged

DrahtBot mentioned this pull request Jan 6, 2021

net: Move SocketSendData lock annotation to header #20864

Merged

DrahtBot added the Needs rebase label Jan 7, 2021

fanquake added the Up for grabs label Aug 18, 2021

fanquake closed this Aug 18, 2021

jnewbery mentioned this pull request Aug 18, 2021

[net] Don't return an optional from TransportDeserializer::GetMessage() #22735

Merged

fanquake removed the Up for grabs label Aug 18, 2021

fanquake removed the Needs rebase label Aug 18, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Follow-ups to 19107 #20364

Follow-ups to 19107 #20364

Uh oh!

Conversation

troygiorshev commented Nov 10, 2020

Uh oh!

DrahtBot commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

troygiorshev commented Nov 11, 2020

Uh oh!

maflcko commented Nov 11, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

troygiorshev commented Nov 17, 2020

Uh oh!

troygiorshev commented Nov 17, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 19, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 7, 2021

Uh oh!

jnewbery commented Feb 4, 2021

Uh oh!

jnewbery commented Feb 4, 2021

Uh oh!

fanquake commented Aug 18, 2021

Uh oh!

Uh oh!

DrahtBot commented Nov 11, 2020 •

edited

Loading