Decouple peer-processing-logic from block-connection-logic (#3) #9075

TheBlueMatt · 2016-11-03T19:01:03Z

Yet another step towards splitting main.cpp. After this and #8930 (which I will rebase after this is merged to clean it up a ton), we're ready to do the big code moves :).

This PR focuses on mapBlockSource, removing references to it from ProcessNewBlock and moving it to callsites, cleaning up logic in the process.

Built on #9026 to avoid conflicts and simplify the backporting (#9048).

sipa

Concept ACK

sipa · 2016-11-04T22:05:32Z

src/main.cpp

@@ -2976,8 +2976,6 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c
            if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, txConflicted, txChanged)) {
                if (state.IsInvalid()) {
                    // The block violates a consensus rule.
-                    if (!state.CorruptionPossible())


This does not actually remove a duplicate. If there is a reorganization, and some block within it is invalid, InvalidChainFound will be called with the tip of the chain, rather than the block that failed. This may have an effect on the large fork detection logic.

Eek! Youre right. Just removed the commit...it wasnt important anyway.

sipa · 2016-11-04T23:48:31Z

src/main.cpp

+            forceProcessing |= MarkBlockAsReceived(hash);
+            // mapBlockSource is only used for sending reject messages and DoS scores,
+            // so the race between here and cs_main in ProcessNewBlock is fine.
+            if (!mapBlockSource.count(hash))


Nit: You can use std::map::emplace here to avoid two searches (the bool returned in the pair tells you whether an insert occurred or now). You could also remember the created iterator and pass it to mapBlockSource.erase below. (and likewise above)

sipa · 2016-11-05T00:11:00Z

src/main.cpp

-            // out to be invalid.
-            ProcessNewBlock(state, chainparams, &block, true, NULL, &fNewBlock);
-            int nDoS;
-            if (state.IsInvalid(nDoS)) {


So glad to see this duplication go!

sipa · 2016-11-07T22:27:54Z

Needs rebase.

TheBlueMatt · 2016-11-07T22:52:49Z

Rebased.

theuni · 2016-11-08T23:03:19Z

src/main.cpp

@@ -5914,22 +5910,29 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
                // updated, reject messages go out, etc.
                MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer
                fBlockRead = true;
+                // mapBlockSource is only used for sending reject messages and DoS scores,


Stale comment? Or am I not seeing the race?

Race if multiple nodes send you something near the same time you might send the DoS/Reject to the "wrong" node, of course we dont really care because we'll get to them next time they send us a block (and someone will still get it).

I assumed this just referred to the fact that we drop the cs_main lock between here and the call to ProcessNewBlock below, so in theory mapBlockSource could be modified in between, and we'd lose the expected callbacks.

Thanks. After discussion on IRC, I'm good with this. Though the comment could be a bit more descriptive, it makes the issue seem like more than it is.

This only returned information in the case of CheckBlock failure, but breaks future changes.

This further decouples ProcessNewBlock from networking/peer logic.

TheBlueMatt · 2016-11-09T19:22:54Z

As mentioned on IRC, this now reverts #9087, which conflicted, but no one registered any serious desire to maintain its functionality since it was realtively trivial.

sipa · 2016-11-11T02:37:51Z

utACK ae22357

theuni · 2016-11-11T05:20:00Z

utACK ae22357 other than the comment nit. Obviously not a blocker though.

jtimon · 2016-11-17T19:58:41Z

src/rpc/mining.cpp

-        state = sc.state;
-    }
-    return BIP22ValidationResult(state);
+    if (!sc.found)


not sure why this shouldn't be if (fAccepted && !sc.found)

If it is not accepted, there will certainly have been a callback to signal what was invalid about it. Thus, not having had a signal implies it was accepted.

Technically this is a change in behavior...previously: if you got an error (disk space, etc) during connection you would print it in the BIP22 result function, after this patch (though, indeed, adding an fAccepted check wouldnt help) you will no longer print that, and your node will just exit (probably after retuning "success" from this function.

jtimon · 2016-11-17T20:00:50Z

src/main.cpp

    }

    NotifyHeaderTip();

+    CValidationState state; // Only used to report errors, not invalidity - ignore it


you don't need to duplicate CValidationState state, you can move the first declaration out of the locked context.

Personally I like making it clear that this CValidationState is unrelated to the other one; it was confusing in the old code that validation errors from different blocks might get put in the same CValidationState (which would get cleared out in ActivateBestChainStep).

Not a big deal anyway.

jtimon · 2016-11-17T20:18:48Z

ut ACK ae22357 apart from the couple of small nits.

sdaftuar · 2016-11-17T20:51:38Z

utACK ae22357

…ic (#3) ae22357 Replace CValidationState param in ProcessNewBlock with BlockChecked (Matt Corallo) 7c98ce5 Remove pfrom parameter from ProcessNewBlock (Matt Corallo) e2e069d Revert "RPC: Give more details when "generate" fails" (Matt Corallo)

rebroad · 2016-11-21T08:11:57Z

Is this change part of a roadmap that is documented somewhere?

jtimon · 2016-11-21T22:37:42Z

@rebroad It is part of a longer branch by @TheBlueMatt : https://github.com/TheBlueMatt/bitcoin/tree/net_processing_file which does enough decoupling to cleanly separate the p2p messages part out of main.o. But I think the PRs (and commits) stand on their own (I did less review on the previous steps). Perhaps @TheBlueMatt can give more details.

rebroad · 2016-11-22T15:19:12Z

@jtimon thanks. I also meant to ask: why is this being done? Is it because main.cpp is considered to be too large?

sipa · 2016-11-23T22:59:06Z

@rebroad Splitting main into several files would be trivial to do, if we don't care much about clean separation between the resulting files. This PR is part of an ongoing effort to separate network processing (currently spread over net, protocol, and main) and block validation with clear interfaces between them. The end result is not only splitting main into two files, but splitting it into two files that each have clearly-separated responsibilities.

TheBlueMatt mentioned this pull request Nov 3, 2016

Move orphan processing to ActivateBestChain #8930

Merged

fanquake added the Refactoring label Nov 4, 2016

sipa reviewed Nov 5, 2016

View reviewed changes

TheBlueMatt force-pushed the net_processing_4 branch 2 times, most recently from 2e30cb4 to 9ab2086 Compare November 7, 2016 00:32

TheBlueMatt force-pushed the net_processing_4 branch from 9ab2086 to dbaf9ee Compare November 7, 2016 22:52

theuni reviewed Nov 8, 2016

View reviewed changes

TheBlueMatt mentioned this pull request Nov 9, 2016

Fix block-connection performance regression #9014

Merged

TheBlueMatt added 3 commits November 8, 2016 17:39

Revert "RPC: Give more details when "generate" fails"

e2e069d

This only returned information in the case of CheckBlock failure, but breaks future changes.

Remove pfrom parameter from ProcessNewBlock

7c98ce5

This further decouples ProcessNewBlock from networking/peer logic.

Replace CValidationState param in ProcessNewBlock with BlockChecked

ae22357

TheBlueMatt force-pushed the net_processing_4 branch from dbaf9ee to ae22357 Compare November 9, 2016 19:22

jtimon reviewed Nov 17, 2016

View reviewed changes

sipa merged commit ae22357 into bitcoin:master Nov 17, 2016

sipa mentioned this pull request Nov 17, 2016

net: Decouple CConnman and message serialization #9128

Merged

OlegGirko mentioned this pull request Aug 1, 2017

Backport Bitcoin PR#9075: Decouple peer-processing-logic from block-connection-logic (#3) dashpay/dash#1560

Merged

gladcow mentioned this pull request Mar 1, 2018

Backport compact blocks functionality from bitcoin dashpay/dash#1966

Merged

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

Decouple peer-processing-logic from block-connection-logic (#3) #9075

Decouple peer-processing-logic from block-connection-logic (#3) #9075

Uh oh!

Conversation

TheBlueMatt commented Nov 3, 2016

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 7, 2016

Uh oh!

TheBlueMatt commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 9, 2016

Uh oh!

sipa commented Nov 11, 2016

Uh oh!

theuni commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Nov 17, 2016

Uh oh!

sdaftuar commented Nov 17, 2016

Uh oh!

rebroad commented Nov 21, 2016

Uh oh!

jtimon commented Nov 21, 2016

Uh oh!

rebroad commented Nov 22, 2016

Uh oh!

sipa commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sipa commented Nov 23, 2016 •

edited

Loading