Skip to content
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.

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

[rpc] Correct reconsiderblock help text, add test #15057

Merged
merged 1 commit into from Jan 7, 2019

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Dec 29, 2018

Rework documentation and test to match the implementation

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1608-qaAssert branch Dec 29, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2018

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

Coverage

Coverage Change (pull 15057, a830d1c4d3eeac4c869c9a061d64e2722a1a252a) Reference (master, a1fd876)
Lines -0.0129 % 87.3755 %
Functions -0.0452 % 84.5100 %
Branches -0.0079 % 51.3984 %

Updated at: 2018-12-29T18:13:51.889322.

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2018

utACK

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

IIRC if you have a chain A B C D E and invalidate D then invalidate B, then reconsider B you'll end back up on E.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1608-qaAssert branch to fa38d3d Jan 1, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

Added a test for this as well (and fixed up the help text). Maybe the method should have been called reconsiderchain with the description "Reconsiders all chains the given block is in"

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

utACK fa38d3d

@promag
Copy link
Member

left a comment

Some comments/questions, other than that looks great.

self.nodes[1].invalidateblock(blocks[-1])
self.nodes[1].invalidateblock(blocks[-2])
assert_equal(self.nodes[1].getbestblockhash(), blocks[-3])
# Reconsider only the previous tip

This comment has been minimized.

Copy link
@promag

promag Jan 2, 2019

Member

s/previous/initial/

blocks = self.nodes[1].generatetoaddress(10, ADDRESS_BCRT1_UNSPENDABLE)
assert_equal(self.nodes[1].getbestblockhash(), blocks[-1])
# Invalidate the two blocks at the tip
self.nodes[1].invalidateblock(blocks[-1])

This comment has been minimized.

Copy link
@promag

promag Jan 2, 2019

Member

remove?

blocks = self.nodes[1].generatetoaddress(10, ADDRESS_BCRT1_UNSPENDABLE)
assert_equal(self.nodes[1].getbestblockhash(), blocks[-1])
# Invalidate the two blocks at the tip
self.nodes[1].invalidateblock(blocks[-2])

This comment has been minimized.

Copy link
@promag

promag Jan 2, 2019

Member

remove?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 3, 2019

Author Member

Why? This is exactly what the test should be testing for. See #15057 (comment)

This comment has been minimized.

Copy link
@promag

promag Jan 3, 2019

Member

Yap sorry.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

utACK fa38d3d, don't mind the above nits..

Candidate for backport?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

I'd say no. User-facing it is only a minor documentation fixup of a method that should not casually be used in production.

@laanwj laanwj merged commit fa38d3d into bitcoin:master Jan 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 7, 2019

Merge #15057: [rpc] Correct reconsiderblock help text, add test
fa38d3d [rpc] Correct reconsiderblock help text, add test (MarcoFalke)

Pull request description:

  Rework documentation and test to match the implementation

Tree-SHA512: d0adef6b054a341bcc1cb87783a4e4cf9be124ba6812e1ac88246a5e01b2861a8071b12dba880b2b428c37da3fa860bfec3fe3e5fbb7c28696872113faa84a9f

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1608-qaAssert branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.