-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Add getdeploymentinfo RPC #23508
Add getdeploymentinfo RPC #23508
Conversation
Example output:
(signalling results split into sections of 144 blocks by hand so it's easier to see on github; 683423 is the last block in taproot's first signalling period) |
8202c88
to
9f5a6fd
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
4469411
to
8a48fe4
Compare
My first thought was that |
8a48fe4
to
684821c
Compare
Changed from |
Concept ACK this is really cool! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 684821c (MacOS 11.3)
➜ bitcoin git:(ajtowns) ✗ ./src/bitcoin-cli getdeploymentinfo
{
"*": {
"hash": "00000000000001f0c73d20c4843c1eac8658b5f49e2d80032ff1feaeb543fe35",
"height": 207120
},
"bip34": {
"type": "buried",
"active": false,
"height": 227931
},
"bip66": {
"type": "buried",
"active": false,
"height": 363725
},
"bip65": {
"type": "buried",
"active": false,
"height": 388381
},
"csv": {
"type": "buried",
"active": false,
"height": 419328
},
"segwit": {
"type": "buried",
"active": false,
"height": 481824
},
"taproot": {
"type": "bip9",
"active": false,
"bip9": {
"start_time": 1619222400,
"timeout": 1628640000,
"since": 0,
"min_activation_height": 709632,
"status": "defined",
"status-next": "defined"
}
}
}
➜ bitcoin git:(ajtowns) ✗ ./src/bitcoin-cli getdeploymentinfo | jq '."*", .taproot'
{
"hash": "0000000000000228e53941db17bc6eb44079f4e1677f39eb8afc73335842da68",
"height": 210280
}
{
"type": "bip9",
"active": false,
"bip9": {
"start_time": 1619222400,
"timeout": 1628640000,
"since": 0,
"min_activation_height": 709632,
"status": "defined",
"status-next": "defined"
}
}
Functional tests:
Outputs
➜ bitcoin git:(ajtowns) ✗ ./test/functional/feature_cltv.py 2021-11-15T10:58:34.803000Z TestFramework (INFO): Initializing test directory /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_0ijd3ade 2021-11-15T10:58:35.463000Z TestFramework (INFO): Mining 109 blocks 2021-11-15T10:58:35.536000Z TestFramework (INFO): Test that invalid-according-to-CLTV transactions can still appear in a block 2021-11-15T10:58:35.595000Z TestFramework (INFO): Test that blocks must now be at least version 4 2021-11-15T10:58:35.706000Z TestFramework (INFO): Test that invalid-according-to-CLTV transactions cannot appear in a block 2021-11-15T10:58:36.268000Z TestFramework (INFO): Test that a version 4 block with a valid-according-to-CLTV transaction is accepted 2021-11-15T10:58:36.384000Z TestFramework (INFO): Stopping nodes 2021-11-15T10:58:36.606000Z TestFramework (INFO): Cleaning up /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_0ijd3ade on exit 2021-11-15T10:58:36.606000Z TestFramework (INFO): Tests successful ➜ bitcoin git:(ajtowns) ✗ ./test/functional/feature_dersig.py 2021-11-15T10:58:43.438000Z TestFramework (INFO): Initializing test directory /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_83lf0cfx 2021-11-15T10:58:44.087000Z TestFramework (INFO): Mining 100 blocks 2021-11-15T10:58:44.185000Z TestFramework (INFO): Test that a transaction with non-DER signature can still appear in a block 2021-11-15T10:58:44.251000Z TestFramework (INFO): Test that blocks must now be at least version 3 2021-11-15T10:58:44.362000Z TestFramework (INFO): Test that transactions with non-DER signatures cannot appear in a block 2021-11-15T10:58:44.478000Z TestFramework (INFO): Test that a block with a DERSIG-compliant transaction is accepted 2021-11-15T10:58:44.590000Z TestFramework (INFO): Stopping nodes 2021-11-15T10:58:44.915000Z TestFramework (INFO): Cleaning up /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_83lf0cfx on exit 2021-11-15T10:58:44.915000Z TestFramework (INFO): Tests successful ➜ bitcoin git:(ajtowns) ✗ ./test/functional/rpc_blockchain.py 2021-11-15T10:58:52.598000Z TestFramework (INFO): Initializing test directory /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_x6_f3kdy 2021-11-15T10:58:53.138000Z TestFramework (INFO): Generate 200 blocks after the genesis block in ten-minute steps 2021-11-15T10:58:54.405000Z TestFramework (INFO): Test getblockchaininfo 2021-11-15T10:58:56.405000Z TestFramework (INFO): Test getdeploymentinfo 2021-11-15T10:58:56.406000Z TestFramework (INFO): Test getchaintxstats 2021-11-15T10:58:56.468000Z TestFramework (INFO): Test gettxoutsetinfo works for blockchain with just the genesis block 2021-11-15T10:58:56.642000Z TestFramework (INFO): Test gettxoutsetinfo returns the same result after invalidate/reconsider block 2021-11-15T10:58:56.741000Z TestFramework (INFO): Test gettxoutsetinfo hash_type option 2021-11-15T10:58:56.948000Z TestFramework (INFO): Test getblockheader 2021-11-15T10:58:56.952000Z TestFramework (INFO): Test getdifficulty 2021-11-15T10:58:56.952000Z TestFramework (INFO): Test getnetworkhashps 2021-11-15T10:58:56.953000Z TestFramework (INFO): Test stopping at height 2021-11-15T10:59:00.749000Z TestFramework (INFO): Test waitforblockheight 2021-11-15T10:59:00.991000Z TestFramework (INFO): Test that getblock with verbosity 1 doesn't include fee 2021-11-15T10:59:00.992000Z TestFramework (INFO): Test that getblock with verbosity 2 and 3 includes expected fee 2021-11-15T10:59:00.995000Z TestFramework (INFO): Test that getblock with verbosity 1 and 2 does not include prevout 2021-11-15T10:59:00.997000Z TestFramework (INFO): Test that getblock with verbosity 3 includes prevout 2021-11-15T10:59:00.998000Z TestFramework (INFO): Test that getblock with verbosity 2 and 3 still works with pruned Undo data 2021-11-15T10:59:00.998000Z TestFramework (INFO): Test getblock with invalid verbosity type returns proper error message 2021-11-15T10:59:01.154000Z TestFramework (INFO): Stopping nodes 2021-11-15T10:59:01.426000Z TestFramework (INFO): Cleaning up /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_x6_f3kdy on exit 2021-11-15T10:59:01.426000Z TestFramework (INFO): Tests successful ➜ bitcoin git:(ajtowns) ✗ ./test/functional/rpc_signrawtransaction.py 2021-11-15T10:59:13.712000Z TestFramework (INFO): Initializing test directory /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_k4wwnsd2 2021-11-15T10:59:16.139000Z TestFramework (INFO): Test valid raw transaction with one input 2021-11-15T10:59:16.140000Z TestFramework (INFO): Test script verification errors 2021-11-15T10:59:16.142000Z TestFramework (INFO): Test signing transaction to P2SH-P2WSH addresses without wallet 2021-11-15T10:59:17.743000Z TestFramework (INFO): Test with a P2PKH script as the witnessScript 2021-11-15T10:59:18.321000Z TestFramework (INFO): Test with a P2PK script as the witnessScript 2021-11-15T10:59:18.881000Z TestFramework (INFO): Test OP_1NEGATE (0x4f) satisfies BIP62 minimal push standardness rule 2021-11-15T10:59:18.882000Z TestFramework (INFO): Test correct error reporting when trying to sign a locked output 2021-11-15T10:59:19.810000Z TestFramework (INFO): Test signing a fully signed transaction does nothing 2021-11-15T10:59:20.496000Z TestFramework (INFO): Test signing a transaction containing a fully signed CSV input 2021-11-15T10:59:21.050000Z TestFramework (INFO): Test signing a transaction containing a fully signed CLTV input 2021-11-15T10:59:21.736000Z TestFramework (INFO): Stopping nodes 2021-11-15T10:59:22.606000Z TestFramework (INFO): Cleaning up /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test_k4wwnsd2 on exit 2021-11-15T10:59:22.606000Z TestFramework (INFO): Tests successful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 684821c on Ubuntu 21.10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remain in getblockchaininfo
, at least as deprecated, for now
684821c
to
797b853
Compare
Updated. |
Concept ACK. I was first skeptical about moving this to a new RPC, but being able to pass in a block hash to evaluate the status as of an arbitrary block is a great idea. |
797b853
to
4662cf3
Compare
Rebased past #23703 and tweaked the commit descriptions slightly. For testing on mainnet, the following little shell snippet shows some interesting block heights: EARLY="0 1"
BURIED="227930 363724 388380 419327 481823"
TAPROOT="681406 681407 681408 681409 683423 685439 687455 689471 691487 693503 695519 697535 699551 701567 703583 705599 707615 709631 711647"
for a in $EARLY $BURIED $TAPROOT; do ./bitcoin-cli getdeploymentinfo $(./bitcoin-cli getblockhash $a); done | less Note that |
Needs release note wrt deprecation. |
bf61392
to
a380922
Compare
@Sjors okay, done that, and also deduplicated the help text between getblockchaininfo and getdeploymentinfo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK a380922
Small steps to making this stuff less confusing... (see also btcsuite/btcd#1700 (comment))
@@ -1605,8 +1607,18 @@ static RPCHelpMan getdeploymentinfo() | |||
LOCK(cs_main); | |||
CChainState& active_chainstate = chainman.ActiveChainstate(); | |||
|
|||
const CBlockIndex* tip = active_chainstate.m_chain.Tip(); | |||
CHECK_NONFATAL(tip); | |||
const CBlockIndex* tip; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7f15c18: this variable should probably be called block
. I suggest also renaming the tip
parameter of DeploymentInfo(
and the active_chain_tip
parameter of SoftForkDescPushBack
to block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but leaving this for later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK a380922
Reviewed code and tested many historical statistics around the Taproot deployment.
Code review and lightly tested ACK a380922 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP, reviewed the first 3 commits and the release note. A few minor comments to pick/choose.
@@ -2716,6 +2807,7 @@ static const CRPCCommand commands[] = | |||
{ "blockchain", &getblockheader, }, | |||
{ "blockchain", &getchaintips, }, | |||
{ "blockchain", &getdifficulty, }, | |||
{ "blockchain", &getdeploymentinfo, }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fd82613 this line should perhaps be before getdifficulty
, not sure, feel free to ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't in alphabetical order, and I don't think it quite fits in with any of the other categories of functions (info about the tip, mempool stuff, stats, queries about a particular block; and the functions aren't really grouped by category anyway), so not seeing much benefit to moving it.
return RPCHelpMan{"getdeploymentinfo", | ||
"Returns an object containing various state info regarding soft-forks.", | ||
{ | ||
{"blockhash", RPCArg::Type::STR_HEX, RPCArg::Default{"chain tip"}, "The block hash at which to query fork state"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7f15c18 current naming style for new rpc arguments, and a default description that (maybe) looks a little less like "chain tip" could be a value
Arguments:
1. blockhash (string, optional, default="chain tip") The block hash at which to query fork state
{"blockhash", RPCArg::Type::STR_HEX, RPCArg::Default{"chain tip"}, "The block hash at which to query fork state"}, | |
{"block_hash", RPCArg::Type::STR_HEX, RPCArg::Default{"value of chain tip"}, "The block hash at which to query fork state"}, |
It would be good to have tests for invoking getdeploymentinfo with a valid and an invalid block hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is reused in the deprecated getblockchaininfo
part, so let's not rename these things. Except for new items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's true; the functionality is nicely reused. Correcting myself, the argument is only in the new RPC, so it could follow current naming style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, why are we introducing new naming styles that are incompatible with the existing names, especially where they're part of an API? 🤮
Opened #24187 for non-code-changing followups. |
e5f0356 rpc/blockchain: rename getdeploymentinfo tip/active_chain_tip to blockindex (Anthony Towns) fbab43f rpc/blockchain: a constant craving (Anthony Towns) 5179656 trivial: comment tweaks (Anthony Towns) 32f04e6 rpc documentation improvements (Anthony Towns) 555eafa doc: getdeploymentinfo release notes tweaks (Anthony Towns) Pull request description: Documentation, comments and trivial code changes to followup bitcoin#23508. ACKs for top commit: Sjors: utACK e5f0356 Tree-SHA512: 4e854a8453588901edb887504f7bfa100cc32df2e99654a5e5970032a0bd63259ba0582479e15bc09ef4792c6672715007f89eb1a7b2d7e229433a678cde9f44
e5f0356 rpc/blockchain: rename getdeploymentinfo tip/active_chain_tip to blockindex (Anthony Towns) fbab43f rpc/blockchain: a constant craving (Anthony Towns) 5179656 trivial: comment tweaks (Anthony Towns) 32f04e6 rpc documentation improvements (Anthony Towns) 555eafa doc: getdeploymentinfo release notes tweaks (Anthony Towns) Pull request description: Documentation, comments and trivial code changes to followup bitcoin#23508. ACKs for top commit: Sjors: utACK e5f0356 Tree-SHA512: 4e854a8453588901edb887504f7bfa100cc32df2e99654a5e5970032a0bd63259ba0582479e15bc09ef4792c6672715007f89eb1a7b2d7e229433a678cde9f44
…haininfo a01b92a doc: add release notes about removal of the `deprecatedrpc=softforks` flag (Sebastian Falbesoner) 8c5533c rpc: remove deprecated "softforks" field from getblockchaininfo (Sebastian Falbesoner) Pull request description: Information on soft fork status has been moved from the `getblockchaininfo` RPC to the `getdeploymentinfo` RPC in #23508. The "softfork" result in `getblockchaininfo` was still available for 23.0 with the `-deprecatedrpc=softforks` configuration option, but this can be fully removed now for the next release (24.0). ACKs for top commit: shaavan: ACK a01b92a ajtowns: ACK a01b92a Tree-SHA512: 692d9d02fdf0b3c18376644a85b24b57efebf612738084c01ef47d47e41861e773688613a808e81f10ab6eec340de00eef96987a1e34d612aaf7f0a0b134d89e
…tblockchaininfo a01b92a doc: add release notes about removal of the `deprecatedrpc=softforks` flag (Sebastian Falbesoner) 8c5533c rpc: remove deprecated "softforks" field from getblockchaininfo (Sebastian Falbesoner) Pull request description: Information on soft fork status has been moved from the `getblockchaininfo` RPC to the `getdeploymentinfo` RPC in bitcoin#23508. The "softfork" result in `getblockchaininfo` was still available for 23.0 with the `-deprecatedrpc=softforks` configuration option, but this can be fully removed now for the next release (24.0). ACKs for top commit: shaavan: ACK a01b92a ajtowns: ACK a01b92a Tree-SHA512: 692d9d02fdf0b3c18376644a85b24b57efebf612738084c01ef47d47e41861e773688613a808e81f10ab6eec340de00eef96987a1e34d612aaf7f0a0b134d89e
In this commit, we add a check during normal node construction to see if the backend node supports Taproot. If it doesn't, then we want to shutdown and force the user to take note. To check if the node supports Taproot, we'll first try the normal getblockchaininfo call. If this works, cool, then we can rely on the value. If it doesn't, then we'll fall back to the getdeploymentinfo call which was added in a recent version of bitcoind [1]. Newer versions of bitcoind might also have this call, and the getblockchaininfo call, but not actually populate the softforks field [2]. In this case, we'll fall back, and we also account for the case when the getblockchaininfo RPC is removed all together. [1]: bitcoin/bitcoin#23508 [2]: bitcoin/bitcoin#25114 Fixes lightningnetwork#6773
In this commit, we add a check during normal node construction to see if the backend node supports Taproot. If it doesn't, then we want to shutdown and force the user to take note. To check if the node supports Taproot, we'll first try the normal getblockchaininfo call. If this works, cool, then we can rely on the value. If it doesn't, then we'll fall back to the getdeploymentinfo call which was added in a recent version of bitcoind [1]. Newer versions of bitcoind might also have this call, and the getblockchaininfo call, but not actually populate the softforks field [2]. In this case, we'll fall back, and we also account for the case when the getblockchaininfo RPC is removed all together. [1]: bitcoin/bitcoin#23508 [2]: bitcoin/bitcoin#25114 Fixes lightningnetwork#6773
a8250e3 doc: add release note about `/rest/deploymentinfo` (brunoerg) 5c96020 doc: add `/deploymentinfo` in REST-interface (brunoerg) 3e44bee test: add coverage for `/rest/deploymentinfo` (brunoerg) 9149703 rest: add `/deploymentinfo` (brunoerg) Pull request description: #23508 added a new RPC named `getdeploymentinfo`, it moved the softfork section from `getblockchaininfo` into this new one. In the REST interface, we have an endpoint named`/rest/chaininfo.json` (which refers to `getblockchaininfo`), so, this PR adds a new REST endpoint named `/deploymentinfo` which refers to `getdeploymentinfo`. You can use it by passing a block hash, e.g: '/rest/deploymentinfo/<BLOCKHASH>.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block. ACKs for top commit: jonatack: re-ACK a8250e3 rebase-only since my last review at c65f82b achow101: ACK a8250e3 stickies-v: re-ACK a8250e3 Tree-SHA512: 0735183b6828d51a72ed0e2be5a09b314ac4693f548982c6e9adaa0ef07a55aa428d3b2d1b1de70b83169811a663a8624b686166e5797f624dcc00178b9796e6
a8250e3 doc: add release note about `/rest/deploymentinfo` (brunoerg) 5c96020 doc: add `/deploymentinfo` in REST-interface (brunoerg) 3e44bee test: add coverage for `/rest/deploymentinfo` (brunoerg) 9149703 rest: add `/deploymentinfo` (brunoerg) Pull request description: bitcoin#23508 added a new RPC named `getdeploymentinfo`, it moved the softfork section from `getblockchaininfo` into this new one. In the REST interface, we have an endpoint named`/rest/chaininfo.json` (which refers to `getblockchaininfo`), so, this PR adds a new REST endpoint named `/deploymentinfo` which refers to `getdeploymentinfo`. You can use it by passing a block hash, e.g: '/rest/deploymentinfo/<BLOCKHASH>.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block. ACKs for top commit: jonatack: re-ACK a8250e3 rebase-only since my last review at c65f82b achow101: ACK a8250e3 stickies-v: re-ACK bitcoin@a8250e3 Tree-SHA512: 0735183b6828d51a72ed0e2be5a09b314ac4693f548982c6e9adaa0ef07a55aa428d3b2d1b1de70b83169811a663a8624b686166e5797f624dcc00178b9796e6
The aim of this PR is to improve the ability to monitor soft fork status. It first moves the softfork section from getblockchaininfo into a new RPC named getdeploymentinfo, which is then also able to query the status of forks at an arbitrary block rather than only at the tip. In addition, bip9 status is changed to indicate the status of the given block, rather than just for the next block, and an additional field is included to indicate whether each block in the signalling period signaled.