Skip to content

Bugfix: fix pruneblockchain returned prune height #15991

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
jonasschnelli merged 2 commits into from
Jun 13, 2019
Merged

Bugfix: fix pruneblockchain returned prune height #15991

jonasschnelli merged 2 commits into from
Jun 13, 2019

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 9, 2019

The help of pruneblockchain tells us that the return value is Height of the last block pruned.,... but the implementation naively returns the provided input height and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

This fixes the return value to actually return the correct prune height.

@jonasschnelli jonasschnelli added this to the 0.18.1 milestone May 9, 2019
promag
promag approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK 2b3849d.

Please see 87af0b4 in https://github.com/promag/bitcoin/tree/pr15991, where the highest pruned block is returned from FindFilesToPruneManual.

Edit: on a second thought your code is more suitable for backport.

@promag
Copy link
Contributor

promag commented May 12, 2019

Tested ACK 2b3849d, but should fix the extended test test/functional/feature_pruning.py, which gives the following error:

2019-05-12T21:49:52.136000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/joao/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 175, in main
    self.run_test()
  File "./test/functional/feature_pruning.py", line 462, in run_test
    self.manual_test(3, use_timestamp=False)
  File "./test/functional/feature_pruning.py", line 312, in manual_test
    prune(100)
  File "./test/functional/feature_pruning.py", line 289, in prune
    assert_equal(ret, expected_ret)
  File "/Users/joao/Projects/bitcoin/test/functional/test_framework/util.py", line 39, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(0 == 100)

With this change I think it makes sense to compare the result with node.getblockchaininfo()['prunedheight'].

@maflcko
Copy link
Member

maflcko commented May 12, 2019

utACK 2b3849d

@promag Thanks for running the test ;)

@maflcko maflcko closed this May 15, 2019
@maflcko maflcko reopened this May 15, 2019
@maflcko
Copy link
Member

maflcko commented May 17, 2019

Travis fails in pruning: https://travis-ci.org/bitcoin/bitcoin/builds/533034514

@maflcko
Copy link
Member

maflcko commented May 23, 2019

@jonasschnelli are you still working on this?

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 24, 2019

I haven't started working on the prune test fix... I have plans to do this the next week. But happy to pull in a commit if someone else wants to fix it.

@promag
Copy link
Contributor

promag commented May 25, 2019

@jonasschnelli promag/bitcoin@22ff1bbe7 is an option, if accepted should be squashed.

@jonasschnelli jonasschnelli force-pushed the 2019/05/pruning_test branch from 2b3849d to e0cbbaa Compare May 29, 2019 07:12
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 29, 2019

Added 22ff1bb from @promag repo (test fix). I don't think it needs squashing.

@maflcko
Copy link
Member

maflcko commented May 29, 2019

@jonasschnelli Are you sure you pushed the correct branch?

@maflcko
Copy link
Member

maflcko commented Jun 5, 2019

@jonasschnelli My previous ACK was 2b3849d, which is a small bugfix. The current pull request adds a config option, which does not qualify as a bugfix that should be backported.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2019

I don't think it needs squashing.

The test will fail otherwise, breaking git bisect.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 7, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@jonasschnelli jonasschnelli force-pushed the 2019/05/pruning_test branch from e0cbbaa to f402012 Compare June 11, 2019 08:22
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Jun 11, 2019

There was a rebase issue. Thanks, @MarcoFalke for reporting. Fixed.

@maflcko
Copy link
Member

maflcko commented Jun 11, 2019

ACK f402012

@jonasschnelli jonasschnelli merged commit f402012 into bitcoin:master Jun 13, 2019
jonasschnelli added a commit that referenced this pull request Jun 13, 2019
@jonasschnelli
Merge #15991: Bugfix: fix pruneblockchain returned prune height
431d81b 
f402012 fixup: Fix prunning test (João Barbosa)
97f517d Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)

Pull request description:

  The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

  This fixes the return value to actually return the correct prune height.

ACKs for commit f40201:
  MarcoFalke:
    ACK f402012

Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 13, 2019
@jonasschnelli @sidhujag
Merge bitcoin#15991: Bugfix: fix pruneblockchain returned prune height
98ee6d7 
f402012 fixup: Fix prunning test (João Barbosa)
97f517d Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)

Pull request description:

  The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

  This fixes the return value to actually return the correct prune height.

ACKs for commit f40201:
  MarcoFalke:
    ACK f402012

Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 14, 2019
@jonasschnelli
Fix RPC/pruneblockchain returned prune height
c80a498 
Github-Pull: bitcoin#15991
Rebased-From: 97f517d
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 14, 2019
@promag
fixup: Fix prunning test
592016b 
Github-Pull: bitcoin#15991
Rebased-From: f402012
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019
@jonasschnelli
Fix RPC/pruneblockchain returned prune height
68e3989 
Github-Pull: bitcoin#15991
Rebased-From: 97f517d
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019
@promag
fixup: Fix prunning test
094a467 
Github-Pull: bitcoin#15991
Rebased-From: f402012
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019
@jonasschnelli @Bushstar
Fix RPC/pruneblockchain returned prune height
69b1963 
Github-Pull: bitcoin#15991
Rebased-From: 97f517d
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019
@promag @Bushstar
fixup: Fix prunning test
adcc1bb 
Github-Pull: bitcoin#15991
Rebased-From: f402012
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 8, 2020
@jonasschnelli @majcosta
[backport#15991] Bugfix: fix pruneblockchain returned prune height
9ffd982 
Summary:
fixup: Fix prunning test (João Barbosa)
Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)

Pull request description:

  The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

  This fixes the return value to actually return the correct prune height.

---

Backport of Core [[bitcoin/bitcoin#15991 | PR15991]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7389
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 28, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#15991: Bugfix: fix pruneblockchain returned prune height
70c4c85 
f402012 fixup: Fix prunning test (João Barbosa)
97f517d Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)

Pull request description:

  The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

  This fixes the return value to actually return the correct prune height.

ACKs for commit f40201:
  MarcoFalke:
    ACK f402012

Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 18, 2021
@jonasschnelli @Munkybooty
Merge bitcoin#15991: Bugfix: fix pruneblockchain returned prune height
34d1a74 
f402012 fixup: Fix prunning test (João Barbosa)
97f517d Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)

Pull request description:

  The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).

  This fixes the return value to actually return the correct prune height.

ACKs for commit f40201:
  MarcoFalke:
    ACK f402012

Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@promag promag promag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

0.18.1

Development

Successfully merging this pull request may close these issues.

4 participants

@jonasschnelli @promag @maflcko @DrahtBot