Final Preparation for main.cpp Split #9183

TheBlueMatt · 2016-11-18T03:34:53Z

Based on #8930, the only thing left to do after this to split main.cpp into main and net_processing is a single move-only commit.

After #8930, the only remaining changes are tweaks to pull header-processing-logic out of net code and into a function exposed through main.h.

instagibbs · 2016-11-18T15:30:32Z

utACK c53fc42

jonasschnelli · 2016-11-23T10:02:25Z

Core Review ACK c53fc42cd7c57c75940e6ec593973e3f0ad69374.
Maybe Squash.

theuni · 2016-11-23T23:02:40Z

src/main.cpp

+        }
+
+        CValidationState state;
+        if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) { // We should call this without cs_main


Seems if one fails we don't actually know which? Or is there some async event?

Thats true, though I dont think any code actually needs to know which currently? If its needed at some point its easy to add an async event and/or additional return value.

Stale comment? As far as I can tell, cs_main is not locked here.

Not sure if this means "TODO: Don't call without cs_main" or "make sure not to call this with cs_main"

theuni · 2016-11-23T23:07:29Z

src/main.cpp

+    {
+        LOCK(cs_main);
+        for (const CBlockHeader& header : headers) {
+            if (!AcceptBlockHeader(header, state, chainparams, ppindex)) {


I think this is probably fine now, but reusing state and assuming that it's only interesting if AcceptBlockHeader fails seems like it's asking for future bugs.

Hmm...I'm sure this isnt the only place we do this. Would definitely need an audit to figure out all the places this is true if we ever want to change state meaning.

This will allow NotifyHeaderTip to be called from an AcceptBlockHeader wrapper function without holding cs_main.

TheBlueMatt · 2016-11-24T00:30:05Z

Rebased after merge of #8930.

sipa · 2016-11-25T19:57:04Z

utACK 711fb0bb192e6b9a6ac8c53c35f12be6d7216df7

morcos · 2016-11-28T19:17:21Z

cautious ACK

This looks good to me, I code reviewed, and did some light testing. I was a bit hesitant about moving NotifyHeaderTip before the rest of the header processing, but that seems to be mostly async behavior.

sdaftuar · 2016-11-28T20:40:34Z

utACK 711fb0b

sipa · 2016-12-01T02:40:10Z

@theuni Comments?

theuni · 2016-12-01T18:42:41Z

src/main.cpp


        CBlockIndex *pindex = NULL;
        CValidationState state;
-        if (!AcceptBlockHeader(cmpctblock.header, state, chainparams, &pindex)) {
+        if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { // We should call this without cs_main


theuni · 2016-12-01T18:49:58Z

utACK 711fb0bb192e6b9a6ac8c53c35f12be6d7216df7. The comment nits can be fixed later.

TheBlueMatt · 2016-12-01T19:03:57Z

Fixed @theuni's documentation comments.

jtimon · 2016-12-01T19:15:29Z

src/main.cpp

                }
+                return error("invalid header received");


maybe print the state here (ie use FormatStateMessage(state), maybe func too) ?

AcceptBlockHeader already does a bunch of printing. And while this could maybe be cleaned up, probably a topic for another PR, I think.

jtimon · 2016-12-01T20:33:33Z

src/main.cpp


        CBlockIndex *pindex = NULL;
        CValidationState state;
-        if (!AcceptBlockHeader(cmpctblock.header, state, chainparams, &pindex)) {
+        if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) {


why {} around cmpctblock.header ?

the {} creates the vector that ProcessNewBlockHeaders takes, instead of a single header as AcceptBlockHeader does (note that it makes a copy of the header, but nbd, I think).

jtimon · 2016-12-01T20:34:51Z

utACK 2c8c57e besides my little nits.

sipa · 2016-12-01T23:45:15Z

utACK 2c8c57e

2c8c57e Document cs_main status when calling into PNB or PNBH (Matt Corallo) 58a215c Use ProcessNewBlockHeaders in CMPCTBLOCK processing (Matt Corallo) a8b936d Use exposed ProcessNewBlockHeaders from ProcessMessages (Matt Corallo) 63fd101 Split ::HEADERS processing into two separate cs_main locks (Matt Corallo) 4a6b1f3 Expose AcceptBlockHeader through main.h (Matt Corallo)

rebroad · 2016-12-05T01:56:01Z

src/main.cpp

+        }
+
+        CValidationState state;
+        if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) {


Why name it such when the headers won't necessarily be new?

To mirror the naming of ProcessNewBlock.

Lost while backporting bitcoin#9183

fanquake added the Refactoring label Nov 18, 2016

laanwj mentioned this pull request Nov 21, 2016

Manual block file pruning. #7871

Merged

theuni reviewed Nov 23, 2016

View reviewed changes

TheBlueMatt added 2 commits November 23, 2016 16:29

Expose AcceptBlockHeader through main.h

4a6b1f3

Split ::HEADERS processing into two separate cs_main locks

63fd101

This will allow NotifyHeaderTip to be called from an AcceptBlockHeader wrapper function without holding cs_main.

TheBlueMatt force-pushed the net_processing_5 branch from c53fc42 to 711fb0b Compare November 24, 2016 00:29

theuni reviewed Dec 1, 2016

View reviewed changes

TheBlueMatt added 3 commits December 1, 2016 11:03

Use exposed ProcessNewBlockHeaders from ProcessMessages

a8b936d

Use ProcessNewBlockHeaders in CMPCTBLOCK processing

58a215c

Document cs_main status when calling into PNB or PNBH

2c8c57e

TheBlueMatt force-pushed the net_processing_5 branch from 711fb0b to 2c8c57e Compare December 1, 2016 19:03

jtimon reviewed Dec 1, 2016

View reviewed changes

sipa merged commit 2c8c57e into bitcoin:master Dec 2, 2016

rebroad reviewed Dec 5, 2016

View reviewed changes

OlegGirko mentioned this pull request Aug 2, 2017

Backport Bitcoin PR#9183: Final Preparation for main.cpp Split dashpay/dash#1561

Merged

codablock added a commit to codablock/dash that referenced this pull request Jan 17, 2018

Add missing comments to ProcessNewBlock and ProcessNewBlockHeaders

b2fb586

Lost while backporting bitcoin#9183

gladcow mentioned this pull request Mar 1, 2018

Backport compact blocks functionality from bitcoin dashpay/dash#1966

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Add missing comments to ProcessNewBlock and ProcessNewBlockHeaders

7199c99

Lost while backporting bitcoin#9183

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019

Add missing comments to ProcessNewBlock and ProcessNewBlockHeaders

62397a9

Lost while backporting bitcoin#9183

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

Final Preparation for main.cpp Split #9183

Final Preparation for main.cpp Split #9183

Uh oh!

Conversation

TheBlueMatt commented Nov 18, 2016

Uh oh!

instagibbs commented Nov 18, 2016

Uh oh!

jonasschnelli commented Nov 23, 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!

TheBlueMatt commented Nov 24, 2016

Uh oh!

sipa commented Nov 25, 2016

Uh oh!

morcos commented Nov 28, 2016

Uh oh!

sdaftuar commented Nov 28, 2016

Uh oh!

sipa commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Dec 1, 2016

Uh oh!

TheBlueMatt commented Dec 1, 2016

Uh oh!

jtimon Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtimon Dec 1, 2016 •

edited

Loading

jtimon commented Dec 1, 2016 •

edited

Loading