Separate Contextual checks and handling & switch on enum in net_processing.cpp #10145

JeremyRubin · 2017-04-03T22:13:28Z

edit 0: Updated to reflect updates mentioned in #10145 (comment)

This PR separates ProcessMessage into two functions, ContextualProcessMessage and ~~ProcessMessage~~ Process*Message. This helps with readability, verifiability, and maintainability of the code.

ContextualProcessMessage generates a list of context dependent "whitelists", all of which must pass for the incoming message before a call to ProcessMessage may be made. If the whitelists fail, the code that follows should be identical to the previous behavior. The choice of whitelists over blacklists is because it is better to explicitly enable the behaviors desired, rather than to try to block the potential bad features (e.g., adding something unsafe and new won't be permitted in unstudied contexts). This design should be extensible for adding new features (~~up to 64 netmsgs total~~ unlimited network messages) as well as new contexts (easy to add new whitelists). There should be very little overhead to check these whitelists as it is all ~~bitwise~~ bool array lookups.

~~ProcessMessage now uses an enum to switch to~~ Dispatch is now done using a std::map lookup to get the appropriate handler, and is semi "stateless" (the map is const). This makes it easier to verify the code and make dispatch more modular.

I haven't benchmarked that the conversion from string->~~enum~~_std::pair<handler_t, whitelist_index>_ has any performance implication, negative or positive. In theory this code could be faster given fewer branch mispredictions due to the ~~switch~~_function pointer call_. Another PR could improve the lookup algorithm ~~(trivially, inlining getAllNetMessageTypes might help the compiler a lot)~~, but unless it is exotic it should be compatible with this design by replacing the map with the desired scheme. ~~I didn't think there was something obviously faster than the linear lookup, because n is small.~~ A std::map lookup should be fairly fast, but perhaps a custom map could be faster.

The correctness of this code is dependent on NetMsgTypeEnum::tag and allNetMessageTypes having the same index order. It would be nice to verify this property at compile time, which should be possible with some recursive constexpr static_assert magic. The default return of ProcessNewMessage is now also false, because the last return is unreachable.

See #9608 and theuni@f1e4e28 for related work/alternatives.

gmaxwell · 2017-04-03T22:23:32Z

I don't get the bitfield stuff here. Why? It adds a lot of code with indirect effects, and means that we cannot use a perfect hash to set the enum value (e.g. stuck with a map of strings at best, though this code doesn't do that).

theuni · 2017-04-03T22:39:25Z

@gmaxwell I agree. I asked @JeremyRubin to check my logic in theuni@f1e4e28, and after reviewing, he wanted to take a stab at a more direct mapping for the initial filter.

This is an interesting approach, but I think this is much less clear than f1e4e28, and it tangles the rules up with the enum values.

JeremyRubin · 2017-04-03T22:41:12Z

It's actually trivial to make this work very well with a perfect hash and this implementation doesn't add any network dependency on the order. If you have a perfect hash available I could demonstrate :) The bitwise stuff is the set of current policies. It's much more explicit about which operations are allowed in which contexts compared to a list of conditionals. There might be a better way to aggregate those rules together, but those are the rules. Blacklists would be shorter (I used one for the bloom stuff, now that I think of it), but it's generally easier to audit what is permitted rather than what is not.

gmaxwell · 2017-04-03T23:16:55Z

If you have a perfect hash available

gperf works in a pinch

cat protocol.cpp | grep '^const char *' | cut -d'"' -f2 | sort | gperf -lCcE

. It's much more explicit about which operations are allowed in which contexts compared to a list of conditionals.

/Generally/ moving function preconditions far away from their code results in defects. When its something that applies basically universally (like the check for the version handshake finishing), then it can make sense... but I think having if (importing) return; at the top of a message handling function is a lot more maintainable than what is effectively an additional state machine.

JeremyRubin · 2017-04-04T02:12:05Z

cat protocol.cpp | grep '^const char *' | cut -d'"' -f2 | sort | gperf -lCcE

As promised, perfect hashing patch in https://github.com/JeremyRubin/bitcoin/tree/perfect_hashing (a little bit less clean than it could be, because I didn't want to modify protocol.h, but you get the idea).

It's like 8 lines of code. At initialize you just fill up a translation table to map the perfect hashes. Note that I slightly tweaked the API of the gperf hash to return the hash key and max+1 on fail to make this design easier.

What's nice about the translation table is it could be modified to store the function pointers to the handlers directly as well, skipping the jump table/switch.

/Generally/ moving function preconditions far away from their code results in defects. When its something that applies basically universally (like the check for the version handshake finishing), then it can make sense... but I think having if (importing) return; at the top of a message handling function is a lot more maintainable than what is effectively an additional state machine.

I'm mixed on this one. I agree that having preconditions closer to the code can be good; but it's also good to have a non-exposed function which only takes sanitized inputs, and do the sanitizing elsewhere. I agree with the state machine comment, but that was the existing state of the code: there is currently an implicit state machine on what order messages were allowed to come in. This PR makes it more explicit; if you want to make it even more explicit (e.g., ProtocolStateMachine class I think that would be great :)). This at least makes it really easy to do whatever you want with the actual dispatch as it is state independent.

I don't think it's more maintainable to have repeated preconditions throughout the code, because then it is easier to forget to check a precondition in a handler and you repeat precondition checking code, leading to more opportunity for error. It's a trade off.

It is possible to re-check these preconditions if critical, which would maybe be "the best/worst" of both worlds.

laanwj · 2017-04-04T06:27:52Z

A serious drawback of perfect hashing is that it makes it hard to add message types, especially in PRs or third-party patches. It reduces flexibility.

Unless it can be clearly shown from performance results that matching a small string in e.g. a sorted table or run-time constructed hash is really a performance sink, and given the small number of small messages I would be really surprised (12 bytes isn't even two 64-bit words!), I'd prefer if adding a message type was just adding a line.

ProtocolStateMachine

If you go this way, I don't think there should be one protocol state machine. Different concerns (e.g. initial negotiation, handling pings, handling transactions, handling blocks, handling filters) could be separate state machines that handle groups of messages (this was @sipa's idea). Creating a separate handler class for every message type would be typical OOP overkill, but for separate concerns I think it'd make sense and would help untangle the current labyrinth.

sipa · 2017-04-04T07:07:29Z

I think using a perfect hash is unnecessary overkill here.

JeremyRubin · 2017-04-04T21:09:50Z

Ultimately if we want to check preconditions, it can't hurt to check some general preconditions in ContextualProcessMessage which apply to either the ProcessMessage state machine (e.g., that only one version may be received) or affect more than one handler. Individual handlers that rely on global preconditions should also re-check these global preconditions to harden them against a failure in ContextualProcessMessage.

I pushed up a better version.

More Modular:

there is no external dependency on the value of message types in the enum
Each handler is a separate function (this duplicates/replaces some of @jtimon's work)

Cleaner:

The whitelists are now a std::array<bool, ...> rather than (previously) a uint64_t, so no masking just indexing
Identifiers are renamed to be a bit more clear
whitelists are now separated into global and connection specific
diff is easier to read

More Extensible:

The whitelists are now a std::array<bool, ...> rather than (previously) a uint64_t, so no refactoring needed to have more than 64 messages. Adding new whitelists is also easier/cleaner.
The use of the function pointers/whitelist arrays makes future work easier, including having per-node handler tables/whitelists (e.g., if a client says it is SPV disable certain operations).

old version exists at https://github.com/jeremyrubin/bitcoin/tree/netprocessing_enum_backup for reference.

jtimon · 2017-04-05T16:44:17Z

I only looked fast at the changes and read the rest of the comments, but this does way more than #9608 , even if #9608 would rebase on top of theuni@f1e4e28 (which at a glance look like something good to do before #9608, but I haven't reviewed it deeply because I'm not that familiar with the network code).

#9608 is very easy to review and trivial to rebase (or re-write) and personally I think it makes the network code easier to read for people like me (perhaps combined with theuni@f1e4e28 even more, I haven't tried combining them). In contrast, it seems like this PR would make harder for me to read at a glance (or maybe not, or maybe it is still worth it, I honestly don't know). Although I've written #9608 (which as said is trivial to write IMO, just a little bit painful [could have been lesss painful if I still used eclipse http://help.eclipse.org/neon/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2FgettingStarted%2Fqs-ExtractMethod.htm ]), I don't feel very capacitated to review this PR or even theuni@f1e4e28 which looks much simpler.

JeremyRubin · 2017-09-05T23:34:27Z

rebased to 1b389b0

ryanofsky

Light utACK 1b389b0. Light because I didn't spend as much time as I would like verifying changes in this PR don't change current behavior. Will spend more time when there are concept acks for the overall approach.

I think this is a nice improvement. The logic and explanations in ContextProcessMessage make it easier to understand why various checks are being done. The centralized whitelists should make it easier to add new message types with correct checking in the future because you can see at a glance how all the existing message types behave. The separate message handling functions make code more navigable.

ryanofsky · 2017-09-06T17:24:02Z

src/net_processing.cpp

-            return false;
-        }
-    }
+static bool ContextualProcessMessage(CNode* pfrom, const std::string&


Maybe leave this as ProcessMessage instead of renaming to ContextualProcessMessage. "Contextual" doesn't seem that elucidating, and the PR could be a little smaller without this change.

ryanofsky · 2017-09-06T17:28:06Z

src/net_processing.cpp

+        strCommand, CDataStream& vRecv, int64_t nTimeReceived, const
+        CChainParams& chainparams, CConnman& connman, const std::atomic<bool>&
+        interruptMsgProc);
+namespace ProcessMessageHandler {


Style guide probably changed since this PR was written, but currently says to use a snake_case name for a namespace, put the brace on the next line, and not indent the contents. Could maybe name this "handlers" instead of "handler" too since it is a collection of handlers.

ryanofsky · 2017-09-06T17:47:11Z

src/net_processing.cpp

    }

-    else if (strCommand == NetMsgType::VERSION)
+static bool ProcessVersionMessage(CNode* pfrom, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman& connman, const std::atomic<bool>& interruptMsgProc)


You might be able to cut down on the size of the PR by making these handlers classes instead of functions. E.g. you wouldn't need to repeat these pfrom, vRecv, nTimeReceived declarations because they could be members initialized by an inherited constructor. strCommand variables could be static members, so PushMessage calls wouldn't need to change. And you could replace handler_registry table with initialization from a flat list of handler classes.

I could imagine other reviewers liking functions more than classes though, and the differences are pretty superficial, so should just consider this a tentative suggestion.

ryanofsky · 2017-09-06T17:57:43Z

src/net_processing.cpp

+        // Unknown Index
+        MSG_TYPE_UNKNOWN,
+        // Size
+        INDEX_COUNT,


Maybe add a comment here that this must be last.

ryanofsky · 2017-09-06T18:05:10Z

src/net_processing.cpp

    }



+    //! Message Processing Whitelists (default init 0)
+    //! Not const for initialization
+    static std::array<bool, INDEX_COUNT> KNOWN_MESSAGES {};


No good reason not to make these constant, I think. You can assign them values at initialization by returning from a function or lambda.

ryanofsky · 2017-09-06T18:36:46Z

src/net_processing.cpp

+    static std::array<bool, INDEX_COUNT> NO_REQUIRE_BLOOM {};
+
+    bool init_whitelists() {
+        for (auto x : { VERSION, VERACK, ADDR, INV, GETDATA, MERKLEBLOCK,


Could you initialize this from the registry table instead of listing all the messages again?

ryanofsky · 2017-09-06T18:39:26Z

src/net_processing.cpp

    }



+    //! Message Processing Whitelists (default init 0)
+    //! Not const for initialization
+    static std::array<bool, INDEX_COUNT> KNOWN_MESSAGES {};


Would be good to have comments describing what each whitelist does, and maybe the types of messages make sense to have in the whitelist. For example, it's not immediately obvious which messages should go in KNOWN_MESSAGES (all the message types, or only the messages types with handlers).

Alternately could add an overall comment here pointing to the ContextualProcessMessage function for specifics on what the whitelists do.

theuni · 2017-09-22T22:25:05Z

I've thought about this a good bit lately, sorry for the delayed response.

After rewriting this about a dozen different ways now, I'm now convinced that the approach in #9608 is the way to go.

A dispatcher with registered functions and a state checker (like you've done here, and like I attempted as well) seems like the obviously correct approach, but once implemented, it's far less straightforward than the simplistic #9608.

We can avoid some duplication and keep some of the whitelist functionality here with a few helper functions like CanProcessWhileImporting(command).

A dumb if/then/else parser seems icky, but I think it's the least likely to cause us issues in the future :(

JeremyRubin · 2017-09-23T00:16:17Z

A dumb if/then/else parser seems icky, but I think it's the least likely to cause us issues in the future :(

Can you clarify the kinds of future issues you're worried about? I think something like #9608 is more likely to cause issues. I think verifying correctness of changes to the processing logic is more complicated under #9608. I'd love to understand what other future considerations you're making though.

A dispatcher with registered functions and a state checker (like you've done here, and like I attempted as well) seems like the obviously correct approach, but once implemented, it's far less straightforward than the simplistic #9608.

I think that there may be a similarity bias. #9608 is more similar to the existing code, which makes it more straightforward. But my guess would be that #10145 is much easier to understand for anyone new to the project/seeing the code for the first time.

Personally, I think this design has the following benefits over the 'simpler' design:

Easier to 'prove correct' design/write exhaustive tests for
Extensible to per connection handler tables
Extensible to middlewares (e.g., a statistics middleware or maybe an encryption one)
Easier to add new message types & less chance of hitting an unforseen edge case
Lower latency dispatch

theuni · 2017-09-25T20:26:18Z

Can you clarify the kinds of future issues you're worried about? I think something like #9608 is more likely to cause issues. I think verifying correctness of changes to the processing logic is more complicated under #9608. I'd love to understand what other future considerations you're making though.

Because of all of the pesky interactions with our own state, we can only go so far in laying out the rules up-front.

For example, someone could be forgiven for assuming (based on the whitelist here) that INVs are processed while importing/reindexing, though in reality, there's only a tiny wallet interaction.

Similarly, IsInitialBlockDownload() isn't handled by the dispatcher, leaving it up to individual messages to decide. And that's as it should be, because each message will have their own criteria for that.

So if the rules can't be completely enumerated, we're only spreading them out and adding more places to check (or miss). The current behavior (and that of #9608) is to only check for proper handshake behavior outside of individual message parsing. That basically boils down to:

if (!node->nVersion && strCommand != NetMsgType::VERSION) return;
else if (!node->fSuccessfullyConnected && strCommand != NetMsgType::VERACK) return;
Dispatch(strCommand);

I might be convinced that a slimmed-down version of this that only performed ^^ checks before dispatching would be reasonable.

fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke) Pull request description: `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see #9608). It was closed because it wasn't clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix #13162) This patch does exactly that. Also note that this patch is a subset of previous approaches such as #9608 and #10145. Review suggestion: `git diff HEAD~ --function-context` Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201

maflcko · 2018-08-25T16:31:22Z

Closing for now. Let me know when you want to continue working on this

JeremyRubin · 2018-09-16T18:43:41Z

@MarcoFalke my read was that there was not consensus that this approach was welcome.

If it's worth refreshing, I'd be happy to.

fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke) Pull request description: `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see bitcoin#9608). It was closed because it wasn't clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix bitcoin#13162) This patch does exactly that. Also note that this patch is a subset of previous approaches such as bitcoin#9608 and bitcoin#10145. Review suggestion: `git diff HEAD~ --function-context` Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201

fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke) Pull request description: `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see bitcoin#9608). It was closed because it wasn't clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix bitcoin#13162) This patch does exactly that. Also note that this patch is a subset of previous approaches such as bitcoin#9608 and bitcoin#10145. Review suggestion: `git diff HEAD~ --function-context` Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201 More of 13946

fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke) Pull request description: `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see bitcoin#9608). It was closed because it wasn't clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix bitcoin#13162) This patch does exactly that. Also note that this patch is a subset of previous approaches such as bitcoin#9608 and bitcoin#10145. Review suggestion: `git diff HEAD~ --function-context` Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201

fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke) Pull request description: `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see bitcoin#9608). It was closed because it wasn't clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix bitcoin#13162) This patch does exactly that. Also note that this patch is a subset of previous approaches such as bitcoin#9608 and bitcoin#10145. Review suggestion: `git diff HEAD~ --function-context` Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201 Signed-off-by: cevap <dev@i2pmail.org>

fanquake added the P2P label Apr 3, 2017

JeremyRubin force-pushed the netprocessing_enum_rebased branch 2 times, most recently from be96a9e to edf6f03 Compare April 4, 2017 21:02

JeremyRubin mentioned this pull request Apr 5, 2017

Net: Divide ProcessMessage in smaller functions #9608

Closed

Separate Contextual checks and handling in net_processing.cpp.

1b389b0

JeremyRubin force-pushed the netprocessing_enum_rebased branch from edf6f03 to 1b389b0 Compare September 5, 2017 20:39

ryanofsky reviewed Sep 6, 2017

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

maflcko mentioned this pull request Aug 12, 2018

p2p: Clarify control flow in ProcessMessage #13946

Merged

maflcko closed this Aug 25, 2018

laanwj removed the Needs rebase label Oct 24, 2019

maflcko mentioned this pull request Apr 5, 2020

scripted-diff: Replace strCommand with msg_type #18533

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Separate Contextual checks and handling & switch on enum in net_processing.cpp #10145

Separate Contextual checks and handling & switch on enum in net_processing.cpp #10145

Uh oh!

Conversation

JeremyRubin commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Apr 3, 2017

Uh oh!

theuni commented Apr 3, 2017

Uh oh!

JeremyRubin commented Apr 3, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Apr 4, 2017

Uh oh!

laanwj commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 4, 2017

Uh oh!

JeremyRubin commented Apr 4, 2017

Uh oh!

jtimon commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Sep 5, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

theuni commented Sep 22, 2017

Uh oh!

JeremyRubin commented Sep 23, 2017

Uh oh!

theuni commented Sep 25, 2017

Uh oh!

maflcko commented Aug 25, 2018

Uh oh!

JeremyRubin commented Sep 16, 2018

Uh oh!

Uh oh!

JeremyRubin commented Apr 3, 2017 •

edited

Loading

JeremyRubin commented Apr 3, 2017 via email •

edited

Loading

gmaxwell commented Apr 3, 2017 •

edited

Loading

laanwj commented Apr 4, 2017 •

edited

Loading

jtimon commented Apr 5, 2017 •

edited

Loading