Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return the AcceptBlock CValidationState directly in ProcessNewBlock #16279

Closed
wants to merge 4 commits into from

Conversation

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jun 24, 2019

This is a first step towards a series of cleanups leading to a (more-complete) #16175. It's just two rather-nice (IMO) cleanups.

In practice this means that CheckBlock+ContextualCheckBlock are
called with a passed-in CValidationState before we move onto
connecting the best chain. This makes conceptual sense as these
calls represent the DoS checks on a block (ie PoW and malleability)
which the caller almost certainly wants to know about right away
and shouldn't have to wait on a callback for (and other
validationinterface clients shouldn't care about someone submitting
bogus malleated blocks to PNB).

This also makes it much, much easier to move the best chain
activation logic to a background thread as it implies that if PNB
returns with a IsValid() CValidationState we don't need to care
about trying to process (non-malleated) copies of the block from
other peers.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jun 25, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17140 (test: Fix bug in blockfilter_index_tests. by jimpo)
  • #16411 (Signet support by kallewoof)
  • #15921 (validation: Tidy up ValidationState interface by jnewbery)
  • #15545 ([doc] explain why CheckBlock() is called before AcceptBlock by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

promag left a comment

Concept ACK.

ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
if (fNewBlock) {
CValidationState dos_state;
ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);

This comment has been minimized.

Copy link
@promag

promag Jun 25, 2019

Member

Does it make sense to have a ProcessAndNotifyNewBlock to wrap this repeated code?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jun 25, 2019

Author Contributor

I'd prefer to just wait and clean it up in a later PR, given I want to move towards doing this in the background anyway.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 28, 2019

Contributor

re: #16279 (comment)

In commit "Return the AcceptBlock CValidationState directly in ProcessNewBlock" (dd07ec7)

I'd prefer to just wait and clean it up in a later PR, given I want to move towards doing this in the background anyway

Seems to me deduplicating this code now would only simplify the later PR.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 2, 2019

Author Contributor

Right, but in the full branch I first clean up some cruft, and then merge the now-identical code instead of the currently-partially-identical code. Easier to just leave it as-is.

Copy link
Contributor

ryanofsky left a comment

Concept ACK. Changes look good and I should add a review ACK soon, but I need to dig in a little more to understand this better.

src/validation.h Show resolved Hide resolved
*
* Note that we guarantee that either the proof-of-work is valid on pblock, or
* (and possibly also) BlockChecked will have been called.
* If pblock connects, and has been malleated, state is guaranteed to be some

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 28, 2019

Contributor

In commit "Return the AcceptBlock CValidationState directly in ProcessNewBlock" (dd07ec7)

Could you clarify what malleated means here, or what case this is talking about? When is a block connected but also non valid?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 2, 2019

Author Contributor

Maybe the confusion was the use of the old term "malleated" instead of the "mutated" term used in ValidationInvalidReason. I changed references to mutated.

*
* May not be called in a
* validationinterface callback.
* If fForceProcessing is set, barring pruning and a desire to re-download a

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 28, 2019

Contributor

In commit "Return the AcceptBlock CValidationState directly in ProcessNewBlock" (dd07ec7)

I don't understand what this is saying. Is it just saying it would be wasteful to call with fForceProcessing set in this case, or something more? Would the recommendation be to call with fForceProcessing unset instead, or try to avoid calls entirely? Probably it would be clearer to say when fForceProcessing should be set rather than when it shouldn't be set (if I'm not just completely misunderstanding point of this).

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 2, 2019

Author Contributor

This is saying something very specific - if fForceProcessing is set (but deliberately not saying anything about when you should set it, as that is a rather complicated topic), unless your goal is to re-download a block, you never need to call ProcessNewBlock again with the block. That is to say, "this copy of the block is good, no need to try to find an alternate source", hence the somewhat verbose text. Feel free to suggest an alternative.

pfrom->nLastBlockTime = GetTime();
} else {
if (!dos_state.IsValid()) {
BlockChecked(*pblock, dos_state, connman);
}
LOCK(cs_main);
mapBlockSource.erase(pblock->GetHash());

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 28, 2019

Contributor

Why is mapBlockSource.erase call needed here? In not valid case, this should already happen in explicit BlockChecked call. In valid case, I assume it happens in BlockChecked validation callback?

This logic is messy and repetitive and seems like it could be simplified by moving more of it out of here into the BlockChecked() function, or breaking BlockChecked up into reusable parts.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 2, 2019

Author Contributor

This PR removes the (now redundant) BlockChecked call in case a block is completely bogus (ie !dos_state.IsValid()), so, no.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2019

Member

I think you're right that we can't get rid of this mapBlockSource.erase() call, but not for the reason you say. We still need to call it if the block is valid, but this isn't the first time we've received it (ie fNewBlock is returned as true). I think this logic would be easier to follow if structured as follows, with comments:

        ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
        if (!dos_state.IsValid()) {
            // The block failed anti-dos / mutation checks. Call BlockChecked() callback here.
            // This clears the block from mapBlockSource.
            BlockChecked(*pblock, dos_state, connman);
        } else if (!fNewBlock) {
            // Block was valid but we've seen it before. Clear it from mapBlockSource.
            LOCK(cs_main);
            mapBlockSource.erase(pblock->GetHash());
        } else {
            // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.
            pfrom->nLastBlockTime = GetTime();
        }

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

Hmm, yea, thats a bit cleaner, however, after #16323, which this was refactored out of, the original goes back to being cleaner, so would prefer to just leave it (also cause its fewer LoC changed).

ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
if (fNewBlock) {
CValidationState dos_state;
ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 28, 2019

Contributor

re: #16279 (comment)

In commit "Return the AcceptBlock CValidationState directly in ProcessNewBlock" (dd07ec7)

I'd prefer to just wait and clean it up in a later PR, given I want to move towards doing this in the background anyway

Seems to me deduplicating this code now would only simplify the later PR.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-06-pnb-dos-state branch 2 times, most recently from 31a501d to 8d3ca74 Jul 2, 2019
*
* Note that we guarantee that either the proof-of-work is valid on pblock, or
* (and possibly also) BlockChecked will have been called.
* If pblock connects, and has been mutation, state is guaranteed to be some

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 7, 2019

Member

s/has been mutation/has been mutated/

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 7, 2019

Member

what does "connects" mean here? If it's not valid how does it connect?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 8, 2019

Author Contributor

Good point, clarified it a bunch, though its pretty subtle. I hope its a bit better now.

src/validation.h Outdated Show resolved Hide resolved
@@ -3428,14 +3428,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
return true;
}

bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock)
bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, CValidationState& state, bool fForceProcessing, bool *fNewBlock)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 7, 2019

Member

let's rename it to dos_state for clarity since it seems all callers(aside from dummy usage) calls it that.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 8, 2019

Author Contributor

Done. Also fixed an issue where the FormatStateMessage in ActivateBestChain failure prints the state state, not the dummy_state state.

src/validation.cpp Show resolved Hide resolved
src/net_processing.cpp Show resolved Hide resolved
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-06-pnb-dos-state branch from 8d3ca74 to 1218179 Oct 8, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 8, 2019

looks like it doesn't compile anymore

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-06-pnb-dos-state branch from 808e110 to 3f88fbd Oct 8, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Oct 8, 2019

Right, just an issue triggered by the rebase-on-master testing. Rebased so should work now.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 8, 2019

Concept ACK. Will review when the build is fixed.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Oct 8, 2019

Built is fixed :)

pfrom->nLastBlockTime = GetTime();
} else {
if (!dos_state.IsValid()) {
BlockChecked(*pblock, dos_state, connman);
}
LOCK(cs_main);
mapBlockSource.erase(pblock->GetHash());

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2019

Member

I think you're right that we can't get rid of this mapBlockSource.erase() call, but not for the reason you say. We still need to call it if the block is valid, but this isn't the first time we've received it (ie fNewBlock is returned as true). I think this logic would be easier to follow if structured as follows, with comments:

        ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
        if (!dos_state.IsValid()) {
            // The block failed anti-dos / mutation checks. Call BlockChecked() callback here.
            // This clears the block from mapBlockSource.
            BlockChecked(*pblock, dos_state, connman);
        } else if (!fNewBlock) {
            // Block was valid but we've seen it before. Clear it from mapBlockSource.
            LOCK(cs_main);
            mapBlockSource.erase(pblock->GetHash());
        } else {
            // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.
            pfrom->nLastBlockTime = GetTime();
        }
LOCK(cs_main);
mapBlockSource.erase(pblock->GetHash());
}
LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
if (dos_state.IsValid()) {

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2019

Member

This seems like a behaviour change. If ConnectBlock() fails for this block, then pindex->nStatus will be updated with BLOCK_FAILED_VALID. Previously, that would prevent this branch from being executed, but now we're only looking at the return CValidationState from CheckBlock() and AcceptBlock().

If this is an intentional behaviour change, can you pull it out into a separate commit and comment on why in the commit log?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

Don't think it matters either way (though also don't think it was intentional per se). It's not so much an important change as it is an accidental tweak thats probably a bit better but shouldn't matter. Once we get the block we have to mark it as received, but we cannot do that if the block is malleated. If the block is invalid, we don't care too much - essentially anyone who sends us the block is gonna get the banhammer (if the block is not received via compact blocks).

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 15, 2019

Member

If it doesn't matter either way, can you leave it unchanged? Trying to pick through what exactly MarkBlockAsReceived() is doing and whether changing whether it's called or not is adding quite a lot of burden to review.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

IMO, this is much cleaner, even if the behavior proper doesn't matter. a) reaching into validation to figure out what happened after calling PNB is...a massive layer violation, this should be returned, not figured out by side-effects, b) it means one chunk of cs_main less (even if its only in the uncommon case), and c) it matters a lot for #16323.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 15, 2019

Member

ok, if it does matter, can you separate into its own commit, with a commit log saying why it matters a lot :)

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 16, 2019

Author Contributor

Two different definitions of "matter", I suppose. Anyway, I pulled it out on its own.

This comment has been minimized.

Copy link
@jonatack

jonatack Oct 16, 2019

Member

Tests that clarify and assert the points in this discussion would be reassuring.

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

I lean to agree that's a minor change. I'm not even sure there is a difference between the 2 checks as BLOCK_VALID_TRANSACTION is set in ReceivedBlockTransactions itself called at the end of AcceptBlock after all checks modifying state to invalid. Even it's lower the bar and increase the number of bogus blocks we download we're going to ban peer through the BlockChecked callback. So if banhammer works bogus downloads are going to cease soon.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 17, 2019

Author Contributor

There really is no material difference in behavior here. Its much cleaner code, but the behavior is the same. Hence the two different definitions of "matter".

@@ -132,7 +132,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
continue;
}
std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr))
CValidationState state;
if (!ProcessNewBlock(Params(), shared_pblock, state, true, nullptr) || !state.IsValid())

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2019

Member

I don't think || !state.IsValid() is necessary. ProcessNewBlock() cannot return true if state is invalid.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

Might very well be #16323 bleeding in, but hopefully that is required soon :).

* If the block pblock is built on is in our header tree, and pblock is a
* candidate for accepting to disk (either because it is a candidate for the
* best chain soon, or due to fForceProcessing being set), but pblock has been
* mutated, state is guaranteed to be some non-IsValid() state.
Comment on lines 210 to 213

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

It's a little difficult to parse this sentence. It may be easier to understand if written a little more succinctly as:

If the block that pblock is built on is in our header tree, and pblock is a
candidate for accepting to disk (either because it is a candidate for the
best chain or fForceProcessing is set) but has been mutated, then state is
guaranteed to be non-IsValid().

Does that look equivalent?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

Note that it doesn't have to be a candidate for the best chain, as it can meet the "more or same work" criteria, hence the word "soon". I took the drop of "due to", but am not a huge fan of dropping the redundant subject after the parenthetical (as then you have to re-read skipping the parenthetical).

if (ret) {
// Store to disk
ret = ::ChainstateActive().AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
}
if (!ret) {
GetMainSignals().BlockChecked(*pblock, state);
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(state));
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));
Comment on lines 3765 to +3770

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

Consider turning this into an if-else or if guard.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 15, 2019

Author Contributor

Hmm? Note that ret may get set to false in the if (ret) {} conditional above.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

Ah, missed that. Not sure if CheckBlock(...) && ::ChainstateActive().AcceptBlock(...) is any better, but feel free to leave it as is.

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 15, 2019

Contributor

I missed it too :) Maybe consider not reusing the ret variable but giving the return of AcceptBlock a different name. check_ret and accept_ret for example.

src/validation.h Outdated Show resolved Hide resolved
src/rpc/mining.cpp Outdated Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
src/rpc/mining.cpp Outdated Show resolved Hide resolved
9fdf05d resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-06-pnb-dos-state branch from 3f88fbd to eabc9a1 Oct 15, 2019
if (ret) {
// Store to disk
ret = ::ChainstateActive().AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
}
if (!ret) {
GetMainSignals().BlockChecked(*pblock, state);
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(state));
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

Ah, missed that. Not sure if CheckBlock(...) && ::ChainstateActive().AcceptBlock(...) is any better, but feel free to leave it as is.

}
if (!ret) {
GetMainSignals().BlockChecked(*pblock, state);

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

The commit message mentions this in an offhand sort of way. But it seems like this (i.e. removing the event notification) and the way callers handle the secondary return are the primary changes introduced in this commit. The commit log could better reflect this rather than describing the implementation mechanics (i.e. the 'how').

You could argue that removing the event notification should be a commit unto itself. Doing so would definitely make formulating the two commit summaries much easier.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 16, 2019

Author Contributor

Clarified the commit message to make it more clear. Not gonna bother splitting as it really is the same idea - we return the dos_state instead of passing it back via a callback.

}
if (!ret) {
GetMainSignals().BlockChecked(*pblock, state);
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(state));
return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 15, 2019

Contributor

Unrelated to this commit, the error here may be from either CheckBlock or AcceptBlock. Related to this commit, the returned CValidationState could also be from either rather than just AcceptBlock but the commit summary mentions only the latter.

I think this reflects a need for better naming / code organization rather than the need to make the commit message more verbose, however.

This comment has been minimized.

Copy link
@amitiuttarwar

amitiuttarwar Oct 15, 2019

Contributor

but since this commit is touching this code, might be worth updating error message to "AcceptBlock or CheckBlock FAILED..." to be more accurate?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 16, 2019

Author Contributor

Historically, CheckBlock was only called as a part of AcceptBlock, and I think we'd like to eventually go back there. See #9765 and the bitcoin-dev mail titled "Vulnerability relating to 64-byte transactions in Bitcoin Core 0.13 branch". (sorry, lots and lots of context to this code.....)

@@ -132,7 +132,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
continue;
}
std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr))
CValidationState state;

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 15, 2019

Contributor

This one could also be named dos_state, I think?

@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Oct 16, 2019

tested ACK eabc9a1

Reviewed code and ran tests + modified tests to try failure scenarios. While beyond the scope of this PR, something to note is that all the tests that were changed only tested the happy path. Something to improve in follow-ups, maybe after #16323.

TheBlueMatt added 3 commits Oct 16, 2019
In practice this means that CheckBlock+ContextualCheckBlock are
called with a passed-in CValidationState before we move onto
connecting the best chain. This makes conceptual sense as these
calls represent the DoS checks on a block (ie PoW and malleability)
which the caller almost certainly wants to know about right away
and shouldn't have to wait on a callback for.

Further, as other validationinterface clients shouldn't care about
someone submitting bogus malleated blocks to PNB, the BlockChecked
callback is no longer called in response to a malleated block (ie
it really does now mean "block has been checked as a part of
connecting it").

This also makes it much, much easier to move the best chain
activation logic to a background thread as it implies that if PNB
returns with a IsValid() CValidationState we don't need to care
about trying to process (non-malleated) copies of the block from
other peers.
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-06-pnb-dos-state branch from eabc9a1 to 2ec121f Oct 16, 2019
@etscrivner

This comment has been minimized.

Copy link
Contributor

etscrivner commented Oct 16, 2019

tested ACK 2ec121f.

Compiled and ran all of the tests. Ran a testnet node with this code for ~1 day without issue. Currently running one with the updated commits from today (will post if any issues).

Copy link
Member

jonatack left a comment

Concept ACK 2ec121f. Changes seem ~ok, build/tests now green, bitcoind runs without apparent issues afaict. Will need to dig deeper into the code and context to add a code review ACK.

@adamjonas

This comment has been minimized.

Copy link
Contributor

adamjonas commented Oct 16, 2019

tested ACK 2ec121f. Compiled and ran unit and functional tests.

@ariard
ariard approved these changes Oct 17, 2019
Copy link
Contributor

ariard left a comment

Code review ACK 2ec121f. Mostly suggestions to improve comments on a very mangled part of the codebase, had a look on following PRs, beyond expected performance improvements, it should be far easier to understand with a clean layer separation.

@@ -1227,6 +1227,9 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
/**
* Handle invalid block rejection and consequent peer banning, maintain which
* peers announce compact blocks.
* Called both in case of cursory DoS checks failing (implying the peer is likely
* sending us bogus data) and after full validation of the block fails (which may

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

Is second alternative of comment right ? "after full validation of the block fails" in fact it's called in ConnectTip also when block is valid. Also comment should said what function is doing "Punish peer if sending bogus data unless it's under BIP152 relaxation or eventually promote peer to announce blocks via compact blocks"

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 17, 2019

Author Contributor

The existing comment already says that.
"Handle invalid block rejection and consequent peer banning".

LOCK(cs_main);
mapBlockSource.erase(pblock->GetHash());
}
LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
if (dos_state.IsValid()) {

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

I lean to agree that's a minor change. I'm not even sure there is a difference between the 2 checks as BLOCK_VALID_TRANSACTION is set in ReceivedBlockTransactions itself called at the end of AcceptBlock after all checks modifying state to invalid. Even it's lower the bar and increase the number of bogus blocks we download we're going to ban peer through the BlockChecked callback. So if banhammer works bogus downloads are going to cease soon.

* barring pruning and a desire to re-download a pruned block, there should
* never be any reason to re-ProcessNewBlock any block with the same hash.
*
* May not be called in a validationinterface callback.

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

Could you clarify the reasons? I think it's due a lock conflict with cs_main but maybe you're anticipating on the introduction of cs_peerstate

* Performs initial sanity checks using the provided CValidationState before
* connecting any block(s). If you want to *possibly* get feedback on whether
* pblock is valid beyond just cursory mutation/DoS checks, you must install
* a CValidationInterface (see validationinterface.h) - this will have its

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

"The installed BlockChecked method will be called for any block completing validation. It may not be called for the provided pblock as this one not being part of the best chain. But if BlockChecked is called for it and its CValidationState is invalid, the invalidity reason will be different than the CValidationState got back from ProcessNewBlock`". Just a verbose attempt to underscores clearly the fact you will get 2 different set of invalidity reasons.

*
* @param[in] pblock The block we want to process.
* @param[out] state This may be set to an Error state if any error occurred processing them
* @param[in] fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
* @param[out] fNewBlock A boolean which is set to indicate if the block was first received via this call
* @returns If the block was processed, independently of block validity

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

The whole comment could be clearer by defining what we mean by processed. Like "A processed block is one saved to disk and submit as candidate to be part of the best chain." We can also add "passed the cursory mutation/Dos checks" if I'm correct will always return failed (non-processed) if block fail them. So both concepts are a bit mangled, as processed presume these preliminary checks as successful. For DoS storage risks, we don't want to process blocks without basic safeguards.

UnregisterValidationInterface(&sc);
if (!new_block && accepted) {
return "duplicate";
}
if (!dos_state.IsValid()) {
return BIP22ValidationResult(dos_state);

This comment has been minimized.

Copy link
@ariard

ariard Oct 17, 2019

Contributor

nit: slight behavior change as now we return error on weak validity check, but would say it's now more accurate as BlockChecked wasn't giving state of the block we wait for.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 17, 2019

Author Contributor

Ehh, getblocktemplate is a mess already. TBH who really cares since no one actually uses it except to get a block template and submit a block. No one uses the return value.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 17, 2019

Needs rebase
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Nov 12, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 12, 2019

It's very difficult for reviewers to understand the motivation and direction for this sequence of PRs, and consequently to help you move them forward toward merge. The OP for this PR states "It's just two rather-nice (IMO) cleanups.", but your final comment here is "would recommend against running with it without reviving those [other PRs]". Those comments seem to me to be in direct contraction with each other. If the changes here are rather-nice cleanups, that suggests to me that they're worth taking independently from #16323 and #16324.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Nov 12, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 13, 2019

Marking up for grabs

http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-12.html#l-455

<jnewbery> #16279 seems to have already had a lot of review. I liked everything except the last commit in that one, so I might grab those

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Nov 13, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 14, 2019

Rebased in #17479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.