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

TheBlueMatt commented at 7:01 pm on November 3, 2016: member

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).

fanquake added the label Refactoring on Nov 4, 2016

in src/main.cpp: in da6ee5f4ec outdated

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

sipa commented at 10:05 pm on November 4, 2016:

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.

TheBlueMatt commented at 0:16 am on November 7, 2016:

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

in src/main.cpp: in 7ef1da44a9 outdated

6079             // need it even though it is not a candidate for a new best tip.
6080-            forceProcessing |= MarkBlockAsReceived(block.GetHash());
6081+            forceProcessing |= MarkBlockAsReceived(hash);
6082+            // mapBlockSource is only used for sending reject messages and DoS scores,
6083+            // so the race between here and cs_main in ProcessNewBlock is fine.
6084+            if (!mapBlockSource.count(hash))

sipa commented at 11:48 pm on November 4, 2016:

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)

TheBlueMatt commented at 0:32 am on November 7, 2016:

Fixed

in src/main.cpp: in 5f0c20b72f outdated

5904-            // BIP 152 permits peers to relay compact blocks after validating
5905-            // the header only; we should not punish peers if the block turns
5906-            // out to be invalid.
5907-            ProcessNewBlock(state, chainparams, &block, true, NULL, &fNewBlock);
5908-            int nDoS;
5909-            if (state.IsInvalid(nDoS)) {

sipa commented at 0:11 am on November 5, 2016:

So glad to see this duplication go!

sipa commented at 0:12 am on November 5, 2016: member

Concept ACK

TheBlueMatt force-pushed on Nov 7, 2016

sipa commented at 10:27 pm on November 7, 2016: member

Needs rebase.

TheBlueMatt force-pushed on Nov 7, 2016

TheBlueMatt commented at 10:52 pm on November 7, 2016: member

Rebased.

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

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

e2e069dabc

Remove pfrom parameter from ProcessNewBlock

This further decouples ProcessNewBlock from networking/peer logic.

7c98ce584e

Replace CValidationState param in ProcessNewBlock with BlockChecked ae22357607

in src/main.cpp: in 444f3d681e outdated

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

theuni commented at 11:03 pm on November 8, 2016:

Stale comment? Or am I not seeing the race?

TheBlueMatt commented at 1:18 am on November 9, 2016:

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).

sipa commented at 2:32 am on November 11, 2016:

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.

theuni commented at 5:13 am on November 11, 2016:

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.

TheBlueMatt force-pushed on Nov 9, 2016

TheBlueMatt commented at 7:22 pm on November 9, 2016: member

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 commented at 2:37 am on November 11, 2016: member

utACK ae223576077448bd4ec250c73f5d8fe5e9a9ac7d

theuni commented at 5:20 am on November 11, 2016: member

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

in src/rpc/mining.cpp: in ae22357607

770-        if (!sc.found)
771-            return "inconclusive";
772-        state = sc.state;
773-    }
774-    return BIP22ValidationResult(state);
775+    if (!sc.found)

jtimon commented at 7:58 pm on November 17, 2016:

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

sipa commented at 9:04 pm on November 17, 2016:

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.

TheBlueMatt commented at 11:12 pm on November 17, 2016:

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.

in src/main.cpp: in ae22357607

3806+        }
3807     }
3808 
3809     NotifyHeaderTip();
3810 
3811+    CValidationState state; // Only used to report errors, not invalidity - ignore it

jtimon commented at 8:00 pm on November 17, 2016:

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

sdaftuar commented at 8:53 pm on November 17, 2016:

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).

jtimon commented at 10:15 pm on November 17, 2016:

Not a big deal anyway.

jtimon commented at 8:18 pm on November 17, 2016: contributor

ut ACK ae22357 apart from the couple of small nits.

sdaftuar commented at 8:51 pm on November 17, 2016: member

utACK ae223576077448bd4ec250c73f5d8fe5e9a9ac7d

sipa merged this on Nov 17, 2016

sipa closed this on Nov 17, 2016

sipa referenced this in commit 9346f84299 on Nov 17, 2016

rebroad commented at 8:11 am on November 21, 2016: contributor

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

jtimon commented at 10:37 pm on November 21, 2016: contributor

@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 commented at 3:19 pm on November 22, 2016: contributor

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

sipa commented at 10:59 pm on November 23, 2016: member

@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.

MarcoFalke locked this on Sep 8, 2021

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