Skip to content

Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block #24629

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
Jun 7, 2022
Merged

Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block #24629

maflcko merged 1 commit into from
Jun 7, 2022

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 21, 2022 •
edited
Loading

From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.

In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first unpruned block.

Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.

(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)

@luke-jr luke-jr mentioned this pull request Mar 21, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 21, 2022 •
edited
Loading

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

Conflicts

No conflicts as of last run.

promag
promag reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 1ab501b67fd4f9cf215ff18e3243fdc9a8b51173, now returns the last pruned block height.

ryanofsky
ryanofsky reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

In 1ab501b67fd4f9cf215ff18e3243fdc9a8b51173, feature_blockfilterindex_prune.py is failing, which is good, but does need to be fixed. Otherwise code review ACK.

It would also be good to have release notes with the information about when this was broken.

@luke-jr luke-jr force-pushed the bugfix_rpc_prunebc_retval branch from 1ab501b to 558aef5 Compare March 21, 2022 23:08
@luke-jr
Copy link
Member Author

luke-jr commented Mar 21, 2022 •
edited
Loading

Fixed tests, and also noticed the description of "pruneheight" in getblockchaininfo is technically inaccurate, so corrected it as well (suggestions for a better phrasing welcome)

@luke-jr
Copy link
Member Author

luke-jr commented Mar 22, 2022

Release notes could be something like:

- The return value of the `pruneblockchain` method had an off-by-one bug,
  returning the height of the block *after* the most recent pruned. This has
  been corrected, and it now returns the height of the last pruned block as
  documented.

(Not putting in the branch, so it can be cleanly merged into 23.x which has already moved release notes to wiki)

@luke-jr luke-jr closed this Mar 22, 2022
@luke-jr luke-jr reopened this Mar 22, 2022
promag
promag reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 558aef5345eca89766c5001aa1b534c46fd327f7, needs rebase though.

@luke-jr luke-jr force-pushed the bugfix_rpc_prunebc_retval branch from 558aef5 to 5375051 Compare March 22, 2022 13:39
@luke-jr
Copy link
Member Author

luke-jr commented Mar 22, 2022

Dropped the conflicting getblockchaininfo doc change for now

@luke-jr luke-jr mentioned this pull request Mar 22, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Apr 1, 2022
Merged
@jonatack
Copy link
Member

jonatack commented Apr 5, 2022

Approach ACK, reviewing concurrently with #21726

jonatack
jonatack reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK modulo rebase

@fjahr
Copy link
Contributor

fjahr commented Apr 17, 2022

LGTM, will review after rebase

ryanofsky
ryanofsky reviewed Apr 20, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 5375051. Just test fixes since last review

maflcko pushed a commit that referenced this pull request May 16, 2022
Merge #24640: Bugfix: RPC/blockchain: Correct description of getblock…
1511c9e 
…chaininfo's pruneheight result

06822f8 Bugfix: RPC/blockchain: Correct description of getblockchaininfo's pruneheight result (Luke Dashjr)

Pull request description:

  It is possible that lower blocks are complete due to being stored in the same file as blocks not yet eligible for pruning.

  Not really satisfied with this new description, so suggestions for better phasing welcome :)

  (Split out of #24629)

ACKs for top commit:
  theStack:
    Code-review ACK 06822f8

Tree-SHA512: 755a5a40d065ad77f4ac2c19c0b3502eceb3162034823ee7ce1668100d97e8a2bfb822ac381feb7afd13e653cd08a81d5fa505575531757457d6d22c909a6510
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022
Merge bitcoin#24640: Bugfix: RPC/blockchain: Correct description of g…
9498eab 
…etblockchaininfo's pruneheight result

06822f8 Bugfix: RPC/blockchain: Correct description of getblockchaininfo's pruneheight result (Luke Dashjr)

Pull request description:

  It is possible that lower blocks are complete due to being stored in the same file as blocks not yet eligible for pruning.

  Not really satisfied with this new description, so suggestions for better phasing welcome :)

  (Split out of bitcoin#24629)

ACKs for top commit:
  theStack:
    Code-review ACK 06822f8

Tree-SHA512: 755a5a40d065ad77f4ac2c19c0b3502eceb3162034823ee7ce1668100d97e8a2bfb822ac381feb7afd13e653cd08a81d5fa505575531757457d6d22c909a6510
@luke-jr
Bugfix: RPC/blockchain: pruneblockchain: Return the height of the act…
e593ae0 
…ual last pruned block

From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last
block pruned was returned, subject to a bug if there were blocks left unpruned
due to sharing files with later blocks.

In bitcoin#15991, this was "fixed" to the current implementation, introducing a new
bug: now, it returns the first *unpruned* block.

Since the user provides the parameter as a block to include in pruning, it
makes more sense to fix the behaviour to match the documentation.
@luke-jr luke-jr force-pushed the bugfix_rpc_prunebc_retval branch from 5375051 to e593ae0 Compare June 3, 2022 07:20
@luke-jr
Copy link
Member Author

luke-jr commented Jun 3, 2022

Rebased

ryanofsky
ryanofsky approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK e593ae0. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!

@fjahr
Copy link
Contributor

fjahr commented Jun 6, 2022

utACK e593ae0

@maflcko maflcko merged commit 581e2bd into bitcoin:master Jun 7, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 8, 2022
Merge bitcoin#24629: Bugfix: RPC/blockchain: pruneblockchain: Return …
5bc7958 
…the hei…
@bitcoin bitcoin locked and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@jonatack jonatack jonatack left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@luke-jr @DrahtBot @jonatack @fjahr @promag @ryanofsky @maflcko