Net: Divide ProcessMessage in smaller functions #9608

jtimon · 2017-01-21T05:51:10Z

As in #9579, indentation and further documentation can be done later to avoid further disruption on the same PR. Maybe at different times for different groups of functions.
The main goal of this PR ProcessMessage to make it more readable and maintainable (ie, changes to ProcessMessage should in principle be easier to review after this PR). Brings ProcessMessage to 142 lines.

This should be relatively easy to review.

gmaxwell · 2017-01-21T19:31:12Z

This should use a switch statement, not a long if/else cascade. (the extra rescan and whatever checks can be moved inside their respective functions).

sipa · 2017-01-21T19:35:21Z

You can't switch/case on strings.

gmaxwell · 2017-01-21T19:51:53Z

Well damn, I thought it was an enum now.

gmaxwell · 2017-01-21T19:53:20Z

I feel like this just obscures the control flow. :-/

rebroad · 2017-01-23T07:24:01Z

What problem is this PR fixing?

dcousens · 2017-01-23T12:28:07Z

@rebroad algorithmic readability, it is simpler to reason about these algorithms, IMHO, by these smaller functions than it is otherwise.

concept ACK

I feel like this just obscures the control flow. :-/

How so?

jtimon · 2017-01-23T14:17:12Z

What problem is this PR fixing?

Right, readability and maintainability (ie ease of review for later changes on ProcessMessage). As noted, this can be indented later (although that obscures the git blame).
In other projects, having to read more than X nested control flows in function directly goes against the style and smaller functions can be always a solution to that. Others simply don't allow functions above N lines

IMO, before this PR ProcessMessage is both too long and has some lines with too many nestings. It is very hard for me to read this function as it, and I suspect I'm not the only one.

I feel like this just obscures the control flow. :-/

Yeah, how so?

sipa · 2017-01-23T16:54:41Z

(although that obscures the git blame).

Use git blame -w.

maflcko

Concept ACK

maflcko · 2017-01-23T18:09:02Z

src/net_processing.cpp

+        return MsgPong(pfrom, vRecv, nTimeReceived);
+    }
+    else if (strCommand == NetMsgType::FILTERLOAD)
+    {


According to our style guideline all those curly brackets are on the "wrong" line. If we want to stick to the current style guideline, new code should not violate it. If we don't want to stick with the current style guide, we should weaken in with regard to new lines.

You are completely right, I'm happy correcting this.

jtimon · 2017-01-23T21:32:06Z

Added a couple of commits to hopefully squash.

gmaxwell · 2017-01-26T08:49:41Z

How does it obscure the control flow? Because there is only one caller to these functions, and they're all called in order. But now looking at the function to considering its invariants you can no longer see where they are called from, what threads they run, what orders things happen in... As far as nesting goes... it hardly changes the nesting-- just reduces the indentation by one. It doesn't change the cyclomatic complexity of the code (which is usually the motivation against large functions, avoiding gnarly intertwined functions that are not testable).

I don't think it grievously hurts things, but I do not see the improvement and it feels a bit like movement for movement sake to me. No biggie.

jtimon · 2017-01-31T22:55:30Z

Needed rebase. Rebased on top of #9659 some changes to use more const can be made even if the separation is not done.

jnewbery · 2017-03-02T03:27:44Z

Concept ACK, as long as you change the function names to ProcessMsg[type](). You're proposing to refactor SendMessages() in #9579. I'd prefer that function to be split into SendMsg[type]() in that PR if you continue. At the moment you have a function here MsgFeefilter() for processing a received feefilter message and a function in #9579 called MsgFeefilter() for sending a feefilter message. That's not helpful for code taging and documentation.

jtimon · 2017-03-07T19:58:58Z

Fixed nits ( #9608 (comment) #9579 (comment) ) and rebased without needing it.

dcousens · 2017-03-07T22:49:17Z

once over utACK 1d7a16f

theuni · 2017-03-25T04:09:54Z

Concept ACK, but I don't see this as an improvement without separating control flow and dispatch. I believe that's part of @gmaxwell's concern as well?

Added that here: theuni@f1e4e28 (needs tests). This has the advantage of moving the rules for message acceptance into one place. If the message is deemed suitable, it can be dispatched with no ordering/dependency concerns.

jtimon · 2017-03-25T16:08:55Z

@theuni At a first glance that looks like an improvement in readability. I'm happy with that being done before or after this, probably before is less disruptive.

jtimon · 2017-04-04T15:45:41Z

Needed rebase.

JeremyRubin · 2017-04-05T01:29:59Z

@jtimon can you take a look at #10145 as it now does a similar thing.

jtimon · 2017-05-18T21:54:48Z

Needed rebase

jtimon · 2017-08-31T02:39:55Z

Needed rebase.

jtimon · 2018-02-08T19:15:27Z

I can reopen and rebase if there's interest, but it seems there has been no interest for a while, so closing.

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

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>

jtimon force-pushed the 2017-01-split-processmessages branch from 0a84625 to f896b20 Compare January 21, 2017 06:47

fanquake added the P2P label Jan 21, 2017

jtimon force-pushed the 2017-01-split-processmessages branch from 1934e8f to a7e24f2 Compare January 21, 2017 09:02

jtimon changed the title ~~WIP: split processmessages~~ Net: Divide ProcessMessage in smaller functions Jan 21, 2017

maflcko added the Refactoring label Jan 23, 2017

maflcko reviewed Jan 23, 2017

View reviewed changes

jtimon force-pushed the 2017-01-split-processmessages branch from a7e24f2 to fd7b7b2 Compare January 23, 2017 21:31

jtimon mentioned this pull request Jan 24, 2017

Net: Trivial-review: Make SendMessages easier to review #9579

Closed

jtimon force-pushed the 2017-01-split-processmessages branch from 9677569 to 03097e2 Compare January 31, 2017 22:51

jtimon force-pushed the 2017-01-split-processmessages branch 2 times, most recently from 0a966e5 to 951fde4 Compare February 7, 2017 20:39

jtimon force-pushed the 2017-01-split-processmessages branch from 951fde4 to 1d7a16f Compare March 7, 2017 19:57

dcousens approved these changes Mar 7, 2017

View reviewed changes

JeremyRubin mentioned this pull request Apr 3, 2017

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

Closed

jtimon force-pushed the 2017-01-split-processmessages branch from 1d7a16f to d985df2 Compare April 4, 2017 15:45

jtimon force-pushed the 2017-01-split-processmessages branch from d985df2 to b4a918d Compare May 18, 2017 21:53

jtimon force-pushed the 2017-01-split-processmessages branch from b4a918d to b9e617b Compare August 31, 2017 02:39

jtimon force-pushed the 2017-01-split-processmessages branch from b9e617b to 64333d6 Compare September 20, 2017 21:45

Net: Divide ProcessMessage into smaller functions

2723925

jtimon force-pushed the 2017-01-split-processmessages branch from 64333d6 to 2723925 Compare September 23, 2017 06:52

jtimon closed this Feb 8, 2018

maflcko mentioned this pull request Aug 12, 2018

p2p: Clarify control flow in ProcessMessage #13946

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Net: Divide ProcessMessage in smaller functions #9608

Net: Divide ProcessMessage in smaller functions #9608

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

sipa commented Jan 21, 2017 via email

Uh oh!

gmaxwell commented Jan 21, 2017

Uh oh!

gmaxwell commented Jan 21, 2017

Uh oh!

rebroad commented Jan 23, 2017

Uh oh!

dcousens commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Jan 23, 2017

Uh oh!

sipa commented Jan 23, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 23, 2017

Uh oh!

gmaxwell commented Jan 26, 2017

Uh oh!

jtimon commented Jan 31, 2017

Uh oh!

jnewbery commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Mar 7, 2017

Uh oh!

dcousens commented Mar 7, 2017

Uh oh!

theuni commented Mar 25, 2017

Uh oh!

jtimon commented Mar 25, 2017

Uh oh!

jtimon commented Apr 4, 2017

Uh oh!

JeremyRubin commented Apr 5, 2017

Uh oh!

jtimon commented May 18, 2017

Uh oh!

jtimon commented Aug 31, 2017

Uh oh!

jtimon commented Feb 8, 2018

Uh oh!

Uh oh!

jtimon commented Jan 21, 2017 •

edited

Loading

gmaxwell commented Jan 21, 2017 •

edited

Loading

dcousens commented Jan 23, 2017 •

edited

Loading

jnewbery commented Mar 2, 2017 •

edited

Loading