Skip to content

validation: count blocks correctly for check level < 3 #13431

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
Jul 1, 2018
Merged

validation: count blocks correctly for check level < 3 #13431

maflcko merged 1 commit into from
Jul 1, 2018

Conversation

@kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Jun 11, 2018 •
edited
Loading

As noted in #13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because pindexState is never updated.

Post-commit ./bitcoin-cli verifychain 1 3:


2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)

Pre-commit ./bitcoin-cli verifychain 1 3:

2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)

@kallewoof kallewoof mentioned this pull request Jun 11, 2018
Merged
@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch from ad9cf86 to d5244f5 Compare June 11, 2018 07:28
@Empact
Copy link
Contributor

Empact commented Jun 11, 2018

I think you can move the assignment to the for loop increment statement, because pindex isn't assigned anywhere else within the loop, and it's irrelevant if the ShutdownRequested early return occurs. This assumes the coin check is irrelevant to the update.

@kallewoof
Copy link
Contributor Author

kallewoof commented Jun 11, 2018 •
edited
Loading

What about (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA) case? It would be incorrectly set to the wrong value if this occurred. Also would have to make pindex available outside the for loop.

Edit: First statement is not right, second is. I guess it's okay to make pindex available, though.

Edit: Done.

@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch from d5244f5 to 2da0159 Compare June 11, 2018 07:45
@Empact
Copy link
Contributor

Empact commented Jun 11, 2018

Check out the shadowing on 4056.

@kallewoof
Copy link
Contributor Author

kallewoof commented Jun 11, 2018

Good catch. Shouldn't affect the outcome, but bad for readability. Fixing.

@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch from 2da0159 to c995014 Compare June 11, 2018 07:58
maflcko
maflcko reviewed Jun 13, 2018
View reviewed changes
src/validation.cpp Outdated
Copy link
Member

@maflcko maflcko Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you moved pindex to the outer scope and made it visible here, they are already the same (both of them are set to pindex->pprev at the end of the previous for loop)

At this point pindexState is only used to calculate the number of blocks, so it could make sense to just remove it altogether and calculate the number of blocks through a index_lowest or num_blocks.

@maflcko
Copy link
Member

maflcko commented Jun 13, 2018

utACK c995014f492bc9e2d8273b10a19659f9dfc4b182. Feel free to ignore the style-nit. (Though, if you fix it, you might as well do a rebase on your other fix, which was already merged.)

@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch 2 times, most recently from 179b201 to 9f55e73 Compare June 15, 2018 02:20
@kallewoof
Copy link
Contributor Author

kallewoof commented Jun 15, 2018 •
edited
Loading

@MarcoFalke Sorry for delay. Good point. I removed pindexState. I had to add a block counter as the check level >= 4 case would move pindex, but I think it makes sense.

Also rebased on master.

@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch from 9f55e73 to 95aee05 Compare June 15, 2018 02:29
@kallewoof kallewoof changed the title validation: update pindexState for check level < 3 validation: count blocks correctly for check level < 3 Jun 15, 2018
@kallewoof kallewoof force-pushed the verifydb_pindexstate_lvl0-2 branch from 95aee05 to f618ebc Compare June 15, 2018 04:27
@maflcko
Copy link
Member

maflcko commented Jun 15, 2018

utACK f618ebc

@Empact
Copy link
Contributor

Empact commented Jun 15, 2018

utACK f618ebc

@sipa
Copy link
Member

sipa commented Jun 30, 2018

utACK f618ebc

@fanquake
Copy link
Member

fanquake commented Jul 1, 2018

Quickly tested using src/bitcoind -checkblocks=3 -checklevel=1
master (10ffca7):

2018-07-01T08:40:22Z init message: Verifying blocks...
2018-07-01T08:40:22Z Verifying last 3 blocks at level 1
2018-07-01T08:40:22Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-07-01T08:40:22Z No coin database inconsistencies in last 0 blocks (0 transactions)
2018-07-01T08:40:22Z  block index            4211ms

f618ebc:

2018-07-01T08:46:50Z init message: Verifying blocks...
2018-07-01T08:46:50Z Verifying last 3 blocks at level 1
2018-07-01T08:46:50Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-07-01T08:46:50Z No coin database inconsistencies in last 3 blocks (0 transactions)
2018-07-01T08:46:50Z  block index            3865ms

src/bitcoind -checkblocks=3 -checklevel=4

2018-07-01T08:49:43Z init message: Verifying blocks...
2018-07-01T08:49:43Z Verifying last 3 blocks at level 4
2018-07-01T08:49:43Z [0%]...[16%]...[33%]...[50%]...[DONE].
2018-07-01T08:49:43Z No coin database inconsistencies in last 3 blocks (901 transactions)
2018-07-01T08:49:43Z  block index            3359ms

@maflcko maflcko merged commit f618ebc into bitcoin:master Jul 1, 2018
maflcko pushed a commit that referenced this pull request Jul 1, 2018
Merge #13431: validation: count blocks correctly for check level < 3
954f4a9 
f618ebc validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in #13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96
@kallewoof kallewoof deleted the verifydb_pindexstate_lvl0-2 branch July 1, 2018 10:30
@Bushstar Bushstar mentioned this pull request Jul 2, 2018
Merged
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 27, 2019
Merge #13431: validation: count blocks correctly for check level < 3
b925970 
Summary:
f618ebc validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 24, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
c77e011 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 24, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
acd6561 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 24, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
207d21d 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 24, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
266d6d9 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 24, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
a78d9dc 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
jonspock pushed a commit to devaultcrypto/devault that referenced this pull request Dec 26, 2019
@jonspock
Merge #13431: validation: count blocks correctly for check level < 3
3f4b54a 
Summary:
f618ebc4e4 validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin/bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96

Backport of Core PR13431
bitcoin/bitcoin#13431

Depends on D4103

Test Plan:
  make check
  ./bitcoind --printtoconsole
  ./bitcoin-cli verifychain 1 3

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4102
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 8, 2020
@PastaPastaPasta
Merge bitcoin#13431: validation: count blocks correctly for check lev…
55d74a6 
…el < 3

f618ebc validation: count blocks correctly for check level < 3 (Karl-Johan Alm)

Pull request description:

  As noted in bitcoin#13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because `pindexState` is never updated.

  Post-commit `./bitcoin-cli verifychain 1 3`:
  ```

  2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
  2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)
  ```

  Pre-commit `./bitcoin-cli verifychain 1 3`:
  ```
  2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
  2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
  2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)
  ```

Tree-SHA512: 3d82ed26665162c9615fb0e6e91a46ed4d229a5e6797c6c420e6b0bf1be6e5e02401c6e9a93b7a5aec503a2650d8c20d1b45fe300a922379e4cef8ee26e18d96
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kallewoof @Empact @maflcko @sipa @fanquake