Page MenuHomePhabricator

Merge #15921: validation: Tidy up ValidationState interface
ClosedPublic

Authored by jasonbcox on Jul 8 2020, 22:56.

Details

Summary

3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [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 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
  code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
  Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
  Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36

Backport of Core PR15921 and PR17624 (small fix to 15921)

Followup to PR15141

Some differences reviewers will encounter:

  1. RejectCodes (such as REJECT_INVALID) do not appear in the original PR due to an out-of-order backport, but we currently still support them, so it was retained.
  2. Some files (notably tests) contain refactors not present in the original PR. This is due to a few out-of-order backports but otherwise harmless.

Depends on D6923, D6929, D6930

Test Plan

ninja check check-functional-extended

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jasonbcox planned changes to this revision.

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

src/validation.cpp
1703 ↗(On Diff #22094)

Added a code block to limit scope of txfee and tx_state. These are temporary variables not used beyond this point. tx_state in particular has a high chance of being accidentally used in the CheckInputs() block further below, but the limited scope prevents this entirely.

Fix some unnecessary variable renames

deadalnix requested changes to this revision.Jul 9 2020, 00:46
deadalnix added a subscriber: deadalnix.

Mixing automate changes with manual ones makes this basically impossible to review. You need to split this up.

This revision now requires changes to proceed.Jul 9 2020, 00:46

Rebased on the partial backports. This patch should only be the remaining
changes directly related to ValidationState.

This revision is now accepted and ready to land.Jul 14 2020, 14:31