validation: Tidy up ValidationState interface by jnewbery · Pull Request #15921 · bitcoin/bitcoin

jnewbery · 2019-04-29T16:52:20Z

Carries out some remaining tidy-ups remaining after PR 15141:

split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
various minor code style tidy-ups to the ValidationState class
remove the useless ret parameter from ValidationState::Invalid()
remove the now unused first_invalid parameter from ProcessNewBlockHeaders()
remove the fMissingInputs parameter from AcceptToMemoryPool(), and deal with missing inputs the same way as other errors by using the TxValidationState object.

Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:

Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for in the commands below.

git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^

After that it's possible to easily see the mechanical changes with:

git log -p -n1 -U0 --word-diff-regex=. <CommitHash>

jnewbery · 2019-04-29T16:53:20Z

~~This is built on #15141. Only commit [validation] Add CValidationState subclasses onwards are for review in this PR.~~

#15141 is merged. This is ready for review.

promag

Concept ACK, looks like a good refactor and improvement to add ValidationState subclasses.

JeremyRubin · 2019-04-30T05:58:01Z

Concept ACK.

I'd like to see Invalid and ilk return void instead of always false, and clean up the call sites correspondingly. This adds a lines here and there, but I think it improves the readability of the code to see a false literal returned explicitly.

I also somewhat think that ideally there should be a third state class which handles Corruption cases for 'system state' or something. This covers the notion that the issue is not the fault of the block or the transaction, it is an issue with our local state or implementation. Not sure it's worth the churn though, two is already a huge improvement.

laanwj · 2019-04-30T19:33:00Z

Concept ACK

ajtowns · 2019-05-03T00:02:19Z

I also somewhat think that ideally there should be a third state class which handles Corruption cases for 'system state' or something. This covers the notion that the issue is not the fault of the block or the transaction,

The current split is more about "what was being tested" not "what was at fault" -- that's easy to deal with via types, because you know what's being tested at compile time; but you don't know what's going to have been at fault at compile time, so I think dealing with that via the type system (ie as a third state class) wouldn't work.

DrahtBot · 2019-05-04T19:58:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17268 (Epoch Mempool by JeremyRubin)
#17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
#16981 (Improve runtime performance of --reindex by LarryRuane)
#16974 (Walk pindexBestHeader back to ChainActive().Tip() if it is invalid by TheBlueMatt)
#16658 (validation: Rename CheckInputs to CheckInputScripts by jnewbery)
#16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
#13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

src/validation.h

laanwj · 2019-10-27T18:44:39Z

~~ACK dcf5003ca0d8927f3e2457a32394a70ed683f01e~~
ACK 3004d5a

mzumsande · 2019-10-29T01:52:27Z

src/consensus/validation.h

Your old answer to to an old comment indicates that at some point you wanted to make the base class ValidationState abstract. However, the class has a (non-pure) virtual destructor and no pure virtual functions, so instantiation of ValidationState should still be possible. Is this working as intended?

fjahr

Code review ACK dcf5003. Also built and ran tests locally.

I agree with @JeremyRubin that Invalid should return void ideally. Also, have you thought about moving the punishment logic into its own class and file? It's not much of a difference in terms of LoC but reducing net_processing.cpp in size would not be a bad thing and it feels like that logic could deserve its own place due to its importance.

fjahr · 2019-10-29T12:16:15Z

src/consensus/tx_check.cpp

nit: the & was moved from the type to the variable name. For consistency I would keep it at end of the type. That's also how it is still declared in the header file.

jnewbery · 2019-10-29T14:07:13Z

Thanks for the reviews @mzumsande and @fjahr . I've added a couple of fixup commits that address your comments. Let me know if you like them and I'll squash into the appropriate commits.

@mzumsande - I had to add a consensus/validation.cpp file to define the ValidationState destructor. Let me know if you know of a better way to do this.

mzumsande · 2019-10-29T15:55:18Z

For me, inline ValidationState::~ValidationState() {}; in the header works as well to make the class abstract (the inline prevents multiple definition linker errors). I am not sure about the cause for the MSVC/AppVeyor error of the current implementation.

jnewbery · 2019-10-29T17:00:23Z

For me, inline ValidationState::~ValidationState() {}; in the header works

Thanks. Much better!

fjahr · 2019-10-29T17:08:35Z

Thanks for the reviews @mzumsande and @fjahr . I've added a couple of fixup commits that address your comments. Let me know if you like them and I'll squash into the appropriate commits.

@mzumsande - I had to add a consensus/validation.cpp file to define the ValidationState destructor. Let me know if you know of a better way to do this.

Looks good to me!

Split CValidationState into TxValidationState and BlockValidationState to store validation results for transactions and blocks respectively.

Minor style fixups and comment updates. This is purely a style change. There is no change in behavior.

This is in preparation for the next commit, which removes the useless `ret` parameter from ValidationState::Invalid(). error() is simply a convenience wrapper that calls LogPrintf and returns false. Call LogPrintf explicitly and substitute the error() call for a false bool literal.

ValidationState::Invalid() takes a parameter `ret` which is returned to the caller. All call sites set this to false. Remove the `ret` parameter and just return false always.

…ckHeaders() No callers use the returned value in first_invalid. Remove it from the function signature and don't set it in the function.

Handle this failure in the same way as all other failures: call Invalid() with the reasons for the failure.

jnewbery · 2019-10-29T19:46:54Z

squashed fixup commits.

ryanofsky

Code review ACK 3004d5a. Just whitespace change and pure virtual destructor added since last review.

amitiuttarwar · 2019-10-29T23:41:08Z

code review ACK 3004d5a. Also built & ran tests locally.

fjahr · 2019-10-29T23:52:24Z

Code review ACK 3004d5a . Only nit style change and pure virtual destructor added since my last review.

jonatack · 2019-10-30T14:44:12Z

There oughta be a rule about merging review club PRs right before the meeting :)

laanwj · 2019-10-30T14:46:59Z

@jonatack whoops …
nah, reviewing doesn't necessarily stop after merging 😄

mzumsande · 2019-10-30T14:50:33Z

Ok, will still mention my code-review ACK for 3004d5a that I was just about to write when this got merged :-)

maflcko · 2019-10-30T14:55:52Z

nah, reviewing doesn't necessarily stop after merging

Agree. The only downside is that review comments (with the reviewer's name) can not be included in the merge commit.

jonatack · 2019-10-30T16:05:37Z

Could possibly use a review club tag 🏷️

jonatack

Built at 3004d5a, ran tests, running bitcoind tailing the log.

Code review progress:

[validation] Add CValidationState subclasses
[validation] Tidy Up ValidationResult class
[validation] Remove error() calls from Invalid() calls
[validation] Remove useless ret parameter from Invalid()
[validation] Remove unused first_invalid parameter from ProcessNewBlo…
[validation] Remove fMissingInputs from AcceptToMemoryPool()

Interesting reviewer tip in the PR description; result here (without the right colors).

laanwj · 2019-10-30T18:47:03Z

Could possibly use a review club tag label

Done

jonatack · 2019-10-30T19:02:19Z

Perhaps helpful context for future readers/reviewers, here are two comments motivating a27a295 (CValidationState subclasses) from discussion in #15141:

sipa: Rewrite DoS interface between validation and net_processing #15141 (comment)
theBlueMatt: Rewrite DoS interface between validation and net_processing #15141 (comment)

jonatack · 2019-10-30T19:04:16Z

Could possibly use a review club tag label

Done

Thanks!

practicalswift · 2019-11-27T22:20:45Z

Reviewers of this PR are encouraged to review PR #17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have") which fixes a quite serious bug introduced in this PR. Luckily it was caught before being part of any release :)

jnewbery force-pushed the 2019-04-pr15141-cleanups branch from bca1b5f to edcbe6c Compare April 29, 2019 16:57

DrahtBot added Mempool Mining P2P RPC/REST/ZMQ Tests Utils/log/libs Validation Wallet Needs rebase labels Apr 29, 2019

promag reviewed Apr 29, 2019

View reviewed changes

fanquake added Refactoring and removed Mempool Mining P2P Tests Utils/log/libs labels Apr 30, 2019

jnewbery mentioned this pull request Apr 30, 2019

Rewrite DoS interface between validation and net_processing #15141

Merged

jnewbery force-pushed the 2019-04-pr15141-cleanups branch from edcbe6c to 4a51148 Compare May 4, 2019 14:40

DrahtBot removed the Needs rebase label May 4, 2019

jnewbery force-pushed the 2019-04-pr15141-cleanups branch 2 times, most recently from 2b794dc to 4b4d081 Compare May 4, 2019 15:53

practicalswift reviewed May 7, 2019

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label May 8, 2019

jnewbery force-pushed the 2019-04-pr15141-cleanups branch from 4b4d081 to 8d88c86 Compare May 8, 2019 17:32

mzumsande reviewed Oct 29, 2019

View reviewed changes

fjahr approved these changes Oct 29, 2019

View reviewed changes

jnewbery added 6 commits October 29, 2019 15:46

[validation] Add CValidationState subclasses

a27a295

Split CValidationState into TxValidationState and BlockValidationState to store validation results for transactions and blocks respectively.

[validation] Tidy Up ValidationResult class

067981e

Minor style fixups and comment updates. This is purely a style change. There is no change in behavior.

[validation] Remove useless ret parameter from Invalid()

7204c64

ValidationState::Invalid() takes a parameter `ret` which is returned to the caller. All call sites set this to false. Remove the `ret` parameter and just return false always.

[validation] Remove unused first_invalid parameter from ProcessNewBlo…

c428622

…ckHeaders() No callers use the returned value in first_invalid. Remove it from the function signature and don't set it in the function.

[validation] Remove fMissingInputs from AcceptToMemoryPool()

3004d5a

Handle this failure in the same way as all other failures: call Invalid() with the reasons for the failure.

ryanofsky approved these changes Oct 29, 2019

View reviewed changes

jonatack reviewed Oct 30, 2019

View reviewed changes

practicalswift mentioned this pull request Nov 27, 2019

net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have #17624

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Apr 27, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

Conversation

jnewbery commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Apr 30, 2019

Uh oh!

laanwj commented Apr 30, 2019

Uh oh!

ajtowns commented May 3, 2019

Uh oh!

DrahtBot commented May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 29, 2019

Uh oh!

mzumsande commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Oct 29, 2019

Uh oh!

fjahr commented Oct 29, 2019

Uh oh!

jnewbery commented Oct 29, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Oct 30, 2019

Uh oh!

laanwj commented Oct 30, 2019

Uh oh!

mzumsande commented Oct 30, 2019

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

jonatack commented Oct 30, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 30, 2019

Uh oh!

jonatack commented Oct 30, 2019

Uh oh!

jonatack commented Oct 30, 2019

Uh oh!

practicalswift commented Nov 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jnewbery commented Apr 29, 2019 •

edited

Loading

jnewbery commented Apr 29, 2019 •

edited

Loading

DrahtBot commented May 4, 2019 •

edited

Loading

laanwj commented Oct 27, 2019 •

edited

Loading

mzumsande commented Oct 29, 2019 •

edited

Loading

amitiuttarwar commented Oct 29, 2019 •

edited

Loading

fjahr commented Oct 29, 2019 •

edited

Loading