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.

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

[rpc] Fix pruneheight help description in getblockchaininfo #11366

Closed
wants to merge 1 commit into from
Closed

[rpc] Fix pruneheight help description in getblockchaininfo #11366

wants to merge 1 commit into from

Conversation

esotericnonsense
Copy link
Contributor

This is a bit nitpicky, but the field isn't included in the JSON at all unless fPruneMode is set.
Alternatively we could return 0 (and remove 'pruned', which would then be pointless, but someone might be relying on that).

@practicalswift
Copy link
Contributor

utACK 9da936232743a2235dade998be7f01ce0cf1b709. Clarity is good.

@jnewbery
Copy link
Contributor

utACK 9da936232743a2235dade998be7f01ce0cf1b709

Also consider in this PR moving the if (fPruneMode) code block up in the function so that it's immediately below the pruned line. That seems more logical and the return object would be ordered the same as the help text.

@promag promag mentioned this pull request Sep 19, 2017
Merged
promag
promag reviewed Sep 20, 2017
View reviewed changes
Copy link
Member

@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.

Nit, I would reword commit and PR to something along [rpc] Fix pruneheight help description in getblockchaininfo.

Otherwise utACK 9da9362.

src/rpc/blockchain.cpp Outdated
@@ -1136,7 +1136,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
" \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
" \"chainwork\": \"xxxx\" (string) total amount of work in active chain, in hexadecimal\n"
" \"pruned\": xx, (boolean) if the blocks are subject to pruning\n"
" \"pruneheight\": xxxxxx, (numeric) lowest-height complete block stored\n"
" \"pruneheight\": xxxxxx, (numeric) lowest-height complete block stored (if pruning is enabled)\n"
Copy link
Member

Choose a reason for hiding this comment

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

Update to (only present if pruning is enabled). See this example for reference.

@esotericnonsense esotericnonsense changed the title Trivial: RPC/getblockchaininfo: pruneheight is only present in pruned mode [rpc] Fix pruneheight help description in getblockchaininfo Sep 20, 2017
@esotericnonsense
Copy link
Contributor Author

Done and done, tested latest commit, appears in correct order.

promag
promag reviewed Sep 20, 2017
View reviewed changes
src/rpc/blockchain.cpp
@@ -1180,6 +1180,14 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
obj.push_back(Pair("verificationprogress", GuessVerificationProgress(Params().TxData(), chainActive.Tip())));
obj.push_back(Pair("chainwork", chainActive.Tip()->nChainWork.GetHex()));
obj.push_back(Pair("pruned", fPruneMode));
if (fPruneMode)
Copy link
Member

Choose a reason for hiding this comment

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

Since this is touched, update as per developer notes? { in this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #11367

src/rpc/blockchain.cpp
if (fPruneMode)
{
CBlockIndex *block = chainActive.Tip();
while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
Copy link
Member

Choose a reason for hiding this comment

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

Add { }.

Copy link
Member

Choose a reason for hiding this comment

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

Remove first condition block and add assert(block); above.

@esotericnonsense
Copy link
Contributor Author

esotericnonsense commented Sep 20, 2017 •
edited
Loading

Suggest merging this with #11367 to avoid PR dependencies?

@esotericnonsense esotericnonsense mentioned this pull request Sep 20, 2017
Merged
@esotericnonsense
Copy link
Contributor Author

Changes are now included in #11367.

laanwj added a commit that referenced this pull request Sep 25, 2017
@laanwj
Merge #11370: [test] Add getblockchaininfo functional test
2d85899 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to #11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
@esotericnonsense esotericnonsense deleted the 2017-09-getblockchaininfo-docs branch September 30, 2017 00:57
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
2d99b73 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
f376b46 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
2329b8e 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
1f29f1c 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
15562d8 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
81461b7 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
4b6d804 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 26, 2021
@laanwj
Merge bitcoin#11370: [test] Add getblockchaininfo functional test
553a447 
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa)
fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa)

Pull request description:

  Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366).

Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@promag promag promag left review comments

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@esotericnonsense @practicalswift @jnewbery @promag @fanquake