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

validation: Tidy up ValidationState interface #15921

Merged
merged 6 commits into from Oct 30, 2019

Conversation

@jnewbery
Copy link
Member

jnewbery commented Apr 29, 2019

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Apr 29, 2019

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.

Copy link
Member

promag left a comment

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

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Apr 30, 2019

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

This comment has been minimized.

Copy link
Member

laanwj commented Apr 30, 2019

Concept ACK

@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented May 3, 2019

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.

@jnewbery jnewbery force-pushed the jnewbery:2019-04-pr15141-cleanups branch from edcbe6c to 4a51148 May 4, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 4, 2019
@jnewbery jnewbery force-pushed the jnewbery:2019-04-pr15141-cleanups branch 2 times, most recently from 2b794dc to 4b4d081 May 4, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented May 4, 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:

  • #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 Outdated Show resolved Hide resolved
@jnewbery jnewbery force-pushed the jnewbery:2019-04-pr15141-cleanups branch from 4b4d081 to 8d88c86 May 8, 2019
@fjahr
fjahr approved these changes Oct 29, 2019
Copy link
Contributor

fjahr left a comment

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.

@@ -7,28 +7,28 @@
#include <primitives/transaction.h>
#include <consensus/validation.h>

bool CheckTransaction(const CTransaction& tx, CValidationState& state)
bool CheckTransaction(const CTransaction& tx, TxValidationState &state)

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 29, 2019

Contributor

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Contributor

mzumsande commented Oct 29, 2019

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 jnewbery force-pushed the jnewbery:2019-04-pr15141-cleanups branch from 2fc16d7 to 92584fe Oct 29, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Oct 29, 2019

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

Thanks. Much better!

@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Oct 29, 2019

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!

jnewbery added 6 commits Apr 28, 2019
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.
Split CValidationState into TxValidationState and BlockValidationState
to store validation results for transactions and blocks respectively.
@jnewbery jnewbery force-pushed the jnewbery:2019-04-pr15141-cleanups branch from 92584fe to 3004d5a Oct 29, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Oct 29, 2019

squashed fixup commits.

Copy link
Contributor

ryanofsky left a comment

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

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor

amitiuttarwar commented Oct 29, 2019

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

@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Oct 29, 2019

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

laanwj added a commit that referenced this pull request Oct 30, 2019
3004d5a [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c64 [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e [validation] Tidy Up ValidationResult class (John Newbery)
a27a295 [validation] Add CValidationState subclasses (John Newbery)

Pull request description:

  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 <CommitHash> in the commands below.

  ```sh
  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:

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

ACKs for top commit:
  laanwj:
    ACK 3004d5a
  amitiuttarwar:
    code review ACK 3004d5a. Also built & ran tests locally.
  fjahr:
    Code review ACK 3004d5a . Only nit style change and pure virtual destructor added since my last review.
  ryanofsky:
    Code review ACK 3004d5a. Just whitespace change and pure virtual destructor added since last review.

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
@laanwj laanwj merged commit 3004d5a into bitcoin:master Oct 30, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnewbery jnewbery deleted the jnewbery:2019-04-pr15141-cleanups branch Oct 30, 2019
@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Oct 30, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 30, 2019

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

@mzumsande

This comment has been minimized.

Copy link
Contributor

mzumsande commented Oct 30, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member

jonatack commented Oct 30, 2019

Could possibly use a review club tag 🏷

Copy link
Member

jonatack left a comment

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 laanwj added the Review club label Oct 30, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 30, 2019

Could possibly use a review club tag label

Done

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Oct 30, 2019

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

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Oct 30, 2019

Could possibly use a review club tag label

Done

Thanks!

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.