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

1. 
   luke-jr commented at 6:41 pm on March 21, 2022: member

   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)

2. 
   DrahtBot commented at 7:29 pm on March 21, 2022: member

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

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label RPC/REST/ZMQ on Mar 21, 2022
4. in src/rpc/blockchain.cpp:1159 in 1ab501b67f outdated

   1025@@ -1026,7 +1026,7 @@ static RPCHelpMan pruneblockchain()
   1026     while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
   1027         block = block->pprev;
   1028     }
   1029-    return uint64_t(block->nHeight);
   1030+    return int64_t(block->nHeight) - 1;
   

   ---

   ---

   

   promag commented at 8:53 pm on March 21, 2022:

   How about

   0while (block && (block->nStatus & BLOCK_HAVE_DATA)) {
   1    block = block->pprev;
   2}
   3return block ? return uint64_t(block->nHeight) : 0;
   

   ---

   

   ryanofsky commented at 10:09 pm on March 21, 2022:

   In commit “Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block” (1ab501b67fd4f9cf215ff18e3243fdc9a8b51173)

   How about

   0while (block && (block->nStatus & BLOCK_HAVE_DATA)) {
   1    block = block->pprev;
   2}
   3return block ? return uint64_t(block->nHeight) : 0;
   

   19610999595ec557fc602c8cefb930c2dfc1ad67 from #21726 is adding a GetFirstStoredBlock function, and current commit would seem to conflict less with that

5. 
   promag commented at 9:02 pm on March 21, 2022: member

   Code review ACK 1ab501b67fd4f9cf215ff18e3243fdc9a8b51173, now returns the last pruned block height.
6. 
   ryanofsky commented at 10:14 pm on March 21, 2022: member

   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.

7. luke-jr force-pushed on Mar 21, 2022
8. 
   luke-jr commented at 11:09 pm on March 21, 2022: member

   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)
9. 
   luke-jr commented at 0:06 am on March 22, 2022: member

   Release notes could be something like:

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

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

10. luke-jr closed this on Mar 22, 2022

    ---

11. luke-jr reopened this on Mar 22, 2022

    ---

12. DrahtBot added the label Needs rebase on Mar 22, 2022
13. 
    promag commented at 9:16 am on March 22, 2022: member

    Code review ACK 558aef5345eca89766c5001aa1b534c46fd327f7, needs rebase though.
14. luke-jr force-pushed on Mar 22, 2022
15. 
    luke-jr commented at 1:39 pm on March 22, 2022: member

    Dropped the conflicting getblockchaininfo doc change for now
16. DrahtBot removed the label Needs rebase on Mar 22, 2022
17. 
    jonatack commented at 2:45 pm on April 5, 2022: member

    Approach ACK, reviewing concurrently with #21726
18. DrahtBot added the label Needs rebase on Apr 6, 2022
19. in src/rpc/blockchain.cpp:1159 in 5375051abc outdated

    1155@@ -1156,7 +1156,7 @@ static RPCHelpMan pruneblockchain()
    1156     while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    1157         block = block->pprev;
    1158     }
    1159-    return uint64_t(block->nHeight);
    1160+    return int64_t(block->nHeight) - 1;
    

    ---

    ---

    

    jonatack commented at 12:33 pm on April 6, 2022:

    nit, braced init seems to be the current preferred approach

    0    return int64_t{block->nHeight} - 1;
    

20. 
    jonatack commented at 12:33 pm on April 6, 2022: member

    ACK modulo rebase
21. 
    fjahr commented at 12:17 pm on April 17, 2022: member

    LGTM, will review after rebase
22. 
    ryanofsky commented at 8:09 pm on April 20, 2022: member

    Code review ACK 5375051abccc551cdf389edc74c223e4e7f93e7e. Just test fixes since last review
23. MarcoFalke referenced this in commit 1511c9efb4 on May 16, 2022
24. sidhujag referenced this in commit 9498eabc5e on May 28, 2022
25. Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual 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 #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.

    e593ae07c4
26. luke-jr force-pushed on Jun 3, 2022
27. 
    luke-jr commented at 7:21 am on June 3, 2022: member

    Rebased
28. DrahtBot removed the label Needs rebase on Jun 3, 2022
29. ryanofsky approved
30. 
    ryanofsky commented at 5:47 pm on June 6, 2022: member

    Code review ACK e593ae07c4fb41a26c95dbd03301607fc5b4d5e2. 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!
31. 
    fjahr commented at 10:17 pm on June 6, 2022: member

    utACK e593ae07c4fb41a26c95dbd03301607fc5b4d5e2
32. MarcoFalke merged this on Jun 7, 2022
33. MarcoFalke closed this on Jun 7, 2022

    ---

34. sidhujag referenced this in commit 5bc795857e on Jun 8, 2022
35. DrahtBot locked this on Jun 7, 2023

Contributors
luke-jr DrahtBot promag ryanofsky jonatack fjahr

Labels
RPC/REST/ZMQ