Final Preparation for main.cpp Split #9183

TheBlueMatt commented at 3:34 AM on November 18, 2016: member

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.

fanquake added the label Refactoring on Nov 18, 2016

instagibbs commented at 3:30 PM on November 18, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/9183/commits/c53fc42cd7c57c75940e6ec593973e3f0ad69374

jonasschnelli commented at 10:02 AM on November 23, 2016: contributor

Core Review ACK c53fc42cd7c57c75940e6ec593973e3f0ad69374. Maybe Squash.

in src/main.cpp:None in 680316f6c0 outdated

6037 | +            hashLastBlock = header.GetHash();
6038 | +        }
6039 | +        }
6040 | +
6041 | +        CValidationState state;
6042 | +        if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) { // We should call this without cs_main

theuni commented at 11:02 PM on November 23, 2016:

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

TheBlueMatt commented at 11:46 PM on November 23, 2016:

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.

theuni commented at 6:41 PM on December 1, 2016:

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"

Expose AcceptBlockHeader through main.h 4a6b1f36b7

Split ::HEADERS processing into two separate cs_main locks

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

63fd101c52

in src/main.cpp:None in c5bf7b8669 outdated

3699 | +bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex)
3700 | +{
3701 | +    {
3702 | +        LOCK(cs_main);
3703 | +        for (const CBlockHeader& header : headers) {
3704 | +            if (!AcceptBlockHeader(header, state, chainparams, ppindex)) {

theuni commented at 11:07 PM on November 23, 2016:

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.

TheBlueMatt commented at 11:48 PM on November 23, 2016:

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.

TheBlueMatt force-pushed on Nov 24, 2016

TheBlueMatt commented at 12:30 AM on November 24, 2016: member

Rebased after merge of #8930.

sipa commented at 7:57 PM on November 25, 2016: member

utACK 711fb0bb192e6b9a6ac8c53c35f12be6d7216df7

morcos commented at 7:17 PM on November 28, 2016: member

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 commented at 8:40 PM on November 28, 2016: member

utACK 711fb0b

sipa commented at 2:40 AM on December 1, 2016: member

@theuni Comments?

in src/main.cpp:None in 711fb0bb19 outdated

5776 | @@ -5776,19 +5777,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5777 |                  connman.PushMessage(pfrom, NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256());
5778 |              return true;
5779 |          }
5780 | +        }
5781 |  
5782 |          CBlockIndex *pindex = NULL;
5783 |          CValidationState state;
5784 | -        if (!AcceptBlockHeader(cmpctblock.header, state, chainparams, &pindex)) {
5785 | +        if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { // We should call this without cs_main

theuni commented at 6:42 PM on December 1, 2016:

same

theuni commented at 6:49 PM on December 1, 2016: member

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

Use exposed ProcessNewBlockHeaders from ProcessMessages a8b936df20

Use ProcessNewBlockHeaders in CMPCTBLOCK processing 58a215ce8c

Document cs_main status when calling into PNB or PNBH 2c8c57e72f

TheBlueMatt force-pushed on Dec 1, 2016

TheBlueMatt commented at 7:03 PM on December 1, 2016: member

Fixed @theuni's documentation comments.

in src/main.cpp:None in a8b936df20 outdated

6044 | +            if (state.IsInvalid(nDoS)) {
6045 | +                if (nDoS > 0) {
6046 | +                    LOCK(cs_main);
6047 | +                    Misbehaving(pfrom->GetId(), nDoS);
6048 |                  }
6049 | +                return error("invalid header received");

jtimon commented at 7:15 PM on December 1, 2016:

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

TheBlueMatt commented at 8:55 PM on December 1, 2016:

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

in src/main.cpp:None in 58a215ce8c outdated

5776 | @@ -5776,19 +5777,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5777 |                  connman.PushMessage(pfrom, NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256());
5778 |              return true;
5779 |          }
5780 | +        }
5781 |  
5782 |          CBlockIndex *pindex = NULL;
5783 |          CValidationState state;
5784 | -        if (!AcceptBlockHeader(cmpctblock.header, state, chainparams, &pindex)) {
5785 | +        if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) {

jtimon commented at 8:33 PM on December 1, 2016:

why {} around cmpctblock.header ?

TheBlueMatt commented at 8:35 PM on December 1, 2016:

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 commented at 8:34 PM on December 1, 2016: contributor

utACK 2c8c57e besides my little nits.

sipa commented at 11:45 PM on December 1, 2016: member

utACK 2c8c57e72fe32cac909278312955145632da6d82

sipa merged this on Dec 2, 2016

sipa closed this on Dec 2, 2016

sipa referenced this in commit dc6dee41f7 on Dec 2, 2016

in src/main.cpp:None in 2c8c57e72f

6043 | +            hashLastBlock = header.GetHash();
6044 | +        }
6045 | +        }
6046 | +
6047 | +        CValidationState state;
6048 | +        if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) {

rebroad commented at 1:56 AM on December 5, 2016:

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

TheBlueMatt commented at 1:58 AM on December 5, 2016:

To mirror the naming of ProcessNewBlock.

codablock referenced this in commit 8b91205b02 on Jan 16, 2018

codablock referenced this in commit c3a1b7691a on Jan 16, 2018

codablock referenced this in commit b2fb586dec on Jan 17, 2018

andvgal referenced this in commit 7199c994cd on Jan 6, 2019

CryptoCentric referenced this in commit 62397a9dea on Feb 25, 2019

DrahtBot locked this on Sep 8, 2021