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] getblockchaininfo: Loop through the bip9 soft fork deployments instead of hard coding #10874

Merged
merged 1 commit into from Nov 30, 2017
Merged

[RPC] getblockchaininfo: Loop through the bip9 soft fork deployments instead of hard coding #10874

merged 1 commit into from Nov 30, 2017

Conversation

achow101
Copy link
Member

Instead of hard coding which deployment statistics should be listed in the getblockchaininfo output, loop through the available deployments (except testdummy) when displaying their deployment info.

sipa
sipa reviewed Jul 19, 2017
View reviewed changes
src/rpc/blockchain.cpp Outdated
@@ -1185,8 +1185,11 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
softforks.push_back(SoftForkDesc("bip34", 2, tip, consensusParams));
softforks.push_back(SoftForkDesc("bip66", 3, tip, consensusParams));
softforks.push_back(SoftForkDesc("bip65", 4, tip, consensusParams));
BIP9SoftForkDescPushBack(bip9_softforks, "csv", consensusParams, Consensus::DEPLOYMENT_CSV);
BIP9SoftForkDescPushBack(bip9_softforks, "segwit", consensusParams, Consensus::DEPLOYMENT_SEGWIT);
for (int BIP9SoftFork = Consensus::DEPLOYMENT_CSV; BIP9SoftFork != Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++BIP9SoftFork) {
Copy link
Member

@sipa sipa Jul 19, 2017 •
edited

Choose a reason for hiding this comment

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

Why convert to int and back to DeploymentPos? Nevermind, enums don't suport ++.

@jonasschnelli
Copy link
Contributor

utACK 4048d77fb462268f374edd420a827d91be8af11f

@jnewbery
Copy link
Contributor

I'm not sure this change is worth it when there are just two bip9 softforks. This is a net +ve LOC change for the same functionality.

Perhaps worth revisiting if we get more bip9 softforks in future, or am I missing some benefit?

@achow101
Copy link
Member Author

It's fine to do this in the future when we have more soft forks. I had only made the change since I needed to add stats about BIP 91 to one of my branches and the fact that the forks needed to be hard coded in to be displayed with getblockchaininfo bothered me.

@promag
Copy link
Member

promag commented Oct 31, 2017

ACK 4048d77.

Nit, make it so that the name is accessed in BIP9SoftForkDescPushBack(), so the loop would be:

for (int pos = Consensus::DEPLOYMENT_CSV; pos != Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++pos) {
    BIP9SoftForkDescPushBack(bip9_softforks, consensusParams, static_cast<Consensus::DeploymentPos>(pos));
}

@laanwj
Copy link
Member

laanwj commented Nov 9, 2017

Nit, make it so that the name is accessed in BIP9SoftForkDescPushBack(), so the loop would be:

I also prefer that to casting to int and back in the loop header.

@laanwj
Copy link
Member

laanwj commented Nov 30, 2017

Ping @achow101

@laanwj laanwj added Refactoring and removed Docs labels Nov 30, 2017
@achow101
Copy link
Member Author

pong @laanwj

@laanwj
Copy link
Member

laanwj commented Nov 30, 2017 •
edited

@achow101 can you respond to @promag's nit? If you don't want to do it that's fine just say so, but if you don't respond to review comments your PR will be in limbo.

@achow101
Copy link
Member Author

Done.

laanwj
laanwj reviewed Nov 30, 2017
View reviewed changes
src/rpc/blockchain.cpp Outdated
BIP9SoftForkDescPushBack(bip9_softforks, "segwit", consensusParams, Consensus::DEPLOYMENT_SEGWIT);
for (int pos = Consensus::DEPLOYMENT_CSV; pos != Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++pos) {
BIP9SoftForkDescPushBack(bip9_softforks, consensusParams, static_cast<Consensus::DeploymentPos>(pos));

Copy link
Member

Choose a reason for hiding this comment

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

please don't add an empty line here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, removed.

@promag
Copy link
Member

promag commented Nov 30, 2017

Thanks, much better.

utACK 8cf8b06.

@laanwj
Copy link
Member

laanwj commented Nov 30, 2017

utACK e4d0af4

@laanwj laanwj merged commit e4d0af4 into bitcoin:master Nov 30, 2017
laanwj added a commit that referenced this pull request Nov 30, 2017
@laanwj
Merge #10874: [RPC] getblockchaininfo: Loop through the bip9 soft for…
9e38d35 
…k deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
@maflcko maflcko mentioned this pull request Jul 24, 2019
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 23, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
da77fb8 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 28, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
a227cc8 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
a4d625f 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
2c4f38d 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Mar 31, 2020
@laanwj @UdjinM6
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
0aa7645 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 1, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10874: [RPC] getblockchaininfo: Loop through the bip9 s…
5842251 
…oft fork deployments instead of hard coding

e4d0af4 Loop through the bip9 soft fork deployments instead of hard coding (Andrew Chow)

Pull request description:

  Instead of hard coding which deployment statistics should be listed in the `getblockchaininfo` output, loop through the available deployments (except testdummy) when displaying their deployment info.

Tree-SHA512: 87e503bcf5e0fd379940d5e53320b9cbb4b47d647c66246d46f47c09a941f135e6ce1e8b75dad441ed4c22c3f41992dfde7717414be1d71c771d4ff8fe0e1936
@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

@laanwj laanwj laanwj left review comments

@sipa sipa sipa left review comments

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@achow101 @jonasschnelli @jnewbery @promag @laanwj @sipa @maflcko