Move message processing to new 'procmsg' module.

jgarzik commented at 4:51 am on August 7, 2014: contributor

Compiles and passes tests. Could use some refinement.

Compiles and passes tests. Could use some refinement.

8c84a55f83

laanwj added the label Improvement on Aug 7, 2014

Create a MessageEngine wrapper class for message processing 451e44a77b

Split up P2P message processing into MessageEngine methods 78edf5cb8c

procmsg: Un-indent msg processing code one level

Code was left indented for diff-reading purposes in previous commit.

aa9ad2895e

jgarzik commented at 5:45 pm on August 7, 2014: contributor

Updated such that each message is a method. It is recommended to review each commit rather than the sum of all commits, as this patch series follows the “equivalent transformation” method of refactoring code into a more useful form.

This should be suitable for updating to use signals or similar (possibly moving back outside of the class if necessary).

BitcoinPullTester commented at 5:58 pm on August 7, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4646_aa9ad2895e32dcaedba5e1511f5edead4664c029/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jgarzik added the label Priority Low on Aug 8, 2014

jgarzik added the label P2P on Aug 8, 2014

jtimon commented at 10:42 am on September 3, 2014: contributor

Oh, yes, please! untested and undiffed ACK. The diffed ack will come later. Maybe the last un-indent commit can be replaced with a “Pass clang to the new files”. Now that we have a way to automatically format the code, it can be useful for after-diff-sensible commits like that one, should be faster to review just by reproducing it locally.

sipa commented at 1:00 pm on September 3, 2014: member

Idea ACK, but this code is pretty much in flux still (askfor/headersfirst/sendmessages/…); I’d propose to delay this until close to the next major release.

jgarzik closed this on Sep 3, 2014

jtimon commented at 6:43 pm on October 18, 2014: contributor

After headersfirst has been merged, can we reopen this?

laanwj reopened this on Oct 19, 2014

sipa commented at 8:13 am on October 19, 2014: member

No objection against just moving things like this, but things like #4831 conflict probably with less, and actually go further, as it actually splits processing up completely, rather than just moving it as a whole.

sipa commented at 8:14 am on October 19, 2014: member

Things like ping/pong handling and alert handling can probably be split off in a way similar to #4831.

in src/main.cpp: in aa9ad2895e

16@@ -17,6 +17,7 @@
17 #include "txmempool.h"
18 #include "ui_interface.h"
19 #include "util.h"
20+#include "procmsg.h"

Diapolo commented at 10:42 am on October 19, 2014:

Nit: Alphabetical ordering.

in src/main.h: in aa9ad2895e

22@@ -23,6 +23,7 @@
23 #include <exception>
24 #include <map>
25 #include <set>
26+#include <list>

Diapolo commented at 10:43 am on October 19, 2014:

Nit: Same here.

in src/procmsg.cpp: in aa9ad2895e

0@@ -0,0 +1,1374 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT/X11 software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include <map>

Diapolo commented at 10:44 am on October 19, 2014:

Nit: And for this whole block. Also just MIT license.

in src/procmsg.cpp: in aa9ad2895e

89+        ++nEvicted;
90+    }
91+    return nEvicted;
92+}
93+
94+

Diapolo commented at 10:44 am on October 19, 2014:

Suggestion: remove the unneeded newlines here.

in src/procmsg.cpp: in aa9ad2895e

179+                            typedef std::pair<unsigned int, uint256> PairType;
180+                            BOOST_FOREACH(PairType& pair, merkleBlock.vMatchedTxn)
181+                                if (!pfrom->setInventoryKnown.count(CInv(MSG_TX, pair.second)))
182+                                    pfrom->PushMessage("tx", block.vtx[pair.first]);
183+                        }
184+                        // else

Diapolo commented at 10:45 am on October 19, 2014:

Suggestion: Could be a one line-comment?

in src/procmsg.cpp: in aa9ad2895e

263+    }
264+
265+    if (strCommand == "version") {
266+        if (!MsgVersion(pfrom, vRecv, nTimeReceived))
267+            return false;
268+

Diapolo commented at 10:45 am on October 19, 2014:

Nit: Unneeded newline.

in src/procmsg.h: in aa9ad2895e

0@@ -0,0 +1,41 @@
1+#ifndef __BITCOIN_PROCMSG_H__

Diapolo commented at 10:46 am on October 19, 2014:

Misses license header!

in src/procmsg.h: in aa9ad2895e

0@@ -0,0 +1,41 @@
1+#ifndef __BITCOIN_PROCMSG_H__
2+#define __BITCOIN_PROCMSG_H__
3+
4+#include <string>

Diapolo commented at 10:46 am on October 19, 2014:

Nit: Ordering.

Diapolo commented at 10:47 am on October 19, 2014: none

Suggestion: Why not let the clang-script fix coding style for this?

jtimon commented at 6:00 am on October 23, 2014: contributor

I really like how https://github.com/jgarzik/bitcoin/commit/78edf5cb8cf65caf38080ac57040469cc9c6957f (Split up P2P message processing into MessageEngine methods) improves the readability of the p2p messages.

jgarzik commented at 1:34 pm on October 23, 2014: contributor

I felt that this was a good step towards the direction we want to go longer term, which is registering processing methods (signals/hooks), so that the message processing implementation is more de-coupled from network message dispatch.

It sounded like @sipa did not like this as an intermediate step, though.

sipa commented at 3:37 pm on October 23, 2014: member

I would absolutely be in favor of this as intermediate step if not for the review overhead and the fact that it will break existing pull requests that already go further.

Afterwards I think we can definitely do this for the non-trivial pieces after the trivial ones have been moved out.

jtimon commented at 1:22 pm on June 21, 2015: contributor

Needs rebase… Would this be easier to merge if it only contained one commit equivalent to https://github.com/jgarzik/bitcoin/commit/78edf5cb8cf65caf38080ac57040469cc9c6957f but doing the separation directly in main.cpp and using regular functions instead of method of a new class? They can even be static inline functions in which case the commit should produce an identical build unless I’m missing something (meaning no risk with low review overhead).

jgarzik commented at 6:01 pm on July 23, 2015: contributor

Closing. I still think this is worth merging and @jtimon seems to agree. However it has not gained sufficient momentum to get merged for whatever reason.

As usual, it is easy to re-open a PR if this turns out to be in error, ACKs suddenly appear, etc. Applying the “close old PRs, easy to reopen” pattern.

This code movement change is too large to continue rebasing if it is not getting merged.

jgarzik closed this on Jul 23, 2015

jtimon commented at 1:31 am on July 24, 2015: contributor

I really think this kind of PR opens the door to many modularity improvements. And I understand that code movements are a burden to reviewers because they need to keep up to date with them after they’re merged. This is a very big move on main.cpp that everybody is touching. But I seriously don’t understand why breaking breaking an overly-nested switch into new functions of a too-large function like ProcessMessage is so difficult when (as you have proven) it can be done with a minimal and review-friendly single commit (https://github.com/jgarzik/bitcoin/commit/78edf5cb8cf65caf38080ac57040469cc9c6957f ). I believe this can be done in a way that results in an identical gitian just prefixing all the new functions with static inline, but I’ve never tested it. When I tried to do something similar in #5153 for EvalScript @gmaxwell tested it and the build hash wasn’t identical. I didn’t add that prefix because, I thought, theoretically, the compiler should be smart enough to tell by itself, but as @sipa explained me later, non-static (or in an anonymous namespace) things are effectively “public” to the compiler, because it doesn’t have a per-file way to tell whether you’re using extern somewhere else or not.

I really think we should break both EvalScript and ProcessMessage with an identical-build single commit. With an identical build you don’t even need any tested ACK, utACKS should be enough. If I remember correctly @petertodd started a bounty on a python script to check whether the last commit produces an identical build hash or not. I proposed it and I didn’t needed the bounty as motivation, but I never did it.This is the tutorial by @laanwj that I was going to use: #4180 (comment)

jgarzik commented at 5:20 am on July 24, 2015: contributor

@jtimon I agree. It is however difficult to produce identical hashes due to minor compiler build differences such as the ones that sipa mentions.

You could probably perform some #include tricks to accomplish the first file movement step with equivalent hashes, if you keep ProcessMessage() intact. Once the individual code blocks move to their own functions/methods, the hashes will unavoidably differ.

A line-by-line source code comparison tool would be better than a hash check.

sipa commented at 5:34 am on July 24, 2015: member

I have nothing against changes like this, except I think that:

The resulting temporary state is worse than the start due to circular dependencies (you don’t claim it is, but:)
It does not help anything towards achieving the IMHO correct solution, which is separate handler modules, like @laanwj’s inv/ask transaction handling. In fact, it interferes with it.

I would like to see moving bits of processing out to well-encapsulated handler modules, and would have expected such changes to have happened by now.

jtimon commented at 1:14 pm on July 24, 2015: contributor

You could probably perform some #include tricks to accomplish the first file movement step with equivalent hashes, if you keep ProcessMessage() intact.

I was talking about doing the equivalent to that commit without moving anything first.

Once the individual code blocks move to their own functions/methods, the hashes will unavoidably differ.

Shouldn’t static inline be enough to in practice keep the code where it was (in ProcessMessage)?

A line-by-line source code comparison tool would be better than a hash check.

We already have that with git. And by not correcting the indentation at first, the commit can be very readable (specially if the new class is not introduced). But the identical hash eliminates the need for testing, which is nice. @sipa would you oppose to just doing the function separation in main.cpp ? That wouldn’t create any circular dependencies and I believe it would help with the later modularization that you want (apart from making ProcessMessage much, much, much more readable [I don’t use eclipse anymore but I believe some checkstyle plugins can segfault just by trying to analyze this function {if you first disable the typical “too long function” check, obviously}</bad joke>]).

jtimon commented at 1:48 pm on July 24, 2015: contributor

I’m talking about something along this lines: https://github.com/bitcoin/bitcoin/compare/bitcoin:master...jtimon:process-message-0.12.99 But I now realize that for the build to be identical the return true; at the end of every new function should be introduced before that commit. That little preparation would also be trivial to review, but I’m not sure having an identical build commit would be a great benefit anymore…

EDIT: Updated the branch, now there’s a preparation commit and a second one that I believe should result in an identical hash, so please, correct me if I’m wrong. @laanwj assuming it produces an identical hash, would that make it more mergeable or it would be better to squash it with the preparation commit from the beginning?

sipa commented at 2:02 pm on July 24, 2015: member

No problem with splitting up processing.

jtimon commented at 3:07 pm on July 24, 2015: contributor

@sipa Great, I will eventually open what I started there then, although I believe it would be good to make more things like #6163 first, so that we don’t have to work more later (having to add more parameters to the new functions).

Thoughts on the identical build? Could https://github.com/bitcoin/bitcoin/commit/d8e5ff46d31680c1b90abb1d04a7580a2ba48888 theoretically produce an identical build? Is it worth it (given that you need a previous commit at least for the return trues)?

sipa commented at 3:24 pm on July 24, 2015: member

I think that for refactors like this, you shouldn’t aim for identical builds.

The only way to achieve that is by forcing the compiler to inline things, which for such large blocks of code is probably a bad idea.

jtimon commented at 10:16 pm on July 25, 2015: contributor

Thanks, @sipa that’s useful. But I don’t see why inlining all those new static functions is a bad idea. First, it is no worse than what we currently have, and second, the inline can be removed in a fixup! commit (to be squashed together after “testing”, just like the preparation commit).

In any case, maybe is still not worth it, but what about doing something similar in consensus code, more concretely in EvalScript? To remind you, #5153 was considered too risky (https://github.com/bitcoin/bitcoin/pull/5153#issuecomment-60885271).

sipa commented at 10:35 pm on July 25, 2015: member

@jtimon I mean forcing the compiler to inline things - just putting the ‘inline’ keyword may not be enough for that (it may need compiler flags, for example), and even then, the order in the binary may end up being different.

Just saying that IMHO that’s not a good use of your time, but if you think you can make the build identical, so much the better.

I’m much less concerned about changes here than in script evaluation, as getting some state/variables/processing wrong in message handling should result in immediate failure (some messages not being processed), in script evaluation it could lead to very subtle differences.

jgarzik commented at 0:38 am on July 26, 2015: contributor

Basically the inlining doesn’t produce any value. It doesn’t generate identical hashes. Might as well make them separate functions/methods.

jtimon commented at 10:14 pm on July 26, 2015: contributor

Thank you very much both of you: this is very helpful. When (if nobody does it before me) I reopen the discussed subset of this PR, I will forget about the identical build. I guess I was too much worried about reusing the IsIdenticalBuild tool for #5153 (comment) , but this seems definitely less risky and more of a priority.

But, as said, I would like to see more of those Params() disappear, and some of those const CChainParams& and const CPolicy& moving up first. The most current and finished thing I have on this will be in https://github.com/jtimon/bitcoin/commits/process-message-0.12.99 until I open it as a PR in case you want to take a look or cherry pick for finishing.

MarcoFalke locked this on Sep 8, 2021

Move message processing to new ‘procmsg’ module. #4646