Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block#24629
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Code review ACK 1ab501b67fd4f9cf215ff18e3243fdc9a8b51173, now returns the last pruned block height.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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
force-pushed
the
bugfix_rpc_prunebc_retval
branch
from
1ab501b to
558aef5
Compare
|
Fixed tests, |
|
Release notes could be something like: (Not putting in the branch, so it can be cleanly merged into 23.x which has already moved release notes to wiki) |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Code review ACK 558aef5345eca89766c5001aa1b534c46fd327f7, needs rebase though.
luke-jr
force-pushed
the
bugfix_rpc_prunebc_retval
branch
from
558aef5 to
5375051
Compare
|
Dropped the conflicting getblockchaininfo doc change for now |
|
Approach ACK, reviewing concurrently with #21726 |
|
LGTM, will review after rebase |
…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
…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
…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
force-pushed
the
bugfix_rpc_prunebc_retval
branch
from
5375051 to
e593ae0
Compare
|
Rebased |
|
utACK e593ae0 |
7 participants
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)