Signet challenges are currently only available via getblocktemplate RPC.
getblockchaininfo and getmininginfo both provide inadequate information to distinguish signets. Since these are the RPCs used to determine the current network, they should also provide the signet challenge for signets.
Test coverage is included in test/functional/feature_signet.py.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31531.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, zaidmstrr, sipa, achow101 |
| Concept ACK | pinheadmz, i-am-yuvi, BrandonOdiwuor, kallewoof |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
concept ACK, I've needed the signet challenge before from RPC (to determine network magic bytes for warnet) and it seems like it should be available from a pure "getter" rather than GBT which also requires parameters like "rules"
30 | self.setup_clean_chain = True 31 | - shared_args1 = ["-signetchallenge=51"] # OP_TRUE 32 | + 33 | + self.signet_challenge1 = '51' # OP_TRUE 34 | + # signet 2 uses the default challenge 35 | + self.signet_challenge2 = '512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae'
nit: could make this a constant: SIGNET_DEFAULT_CHALLENGE = ... at the top.
Resolved in 4a21c49e26e78a2820b9603cf765710323efd700
28 | self.chain = "signet" 29 | self.num_nodes = 6 30 | self.setup_clean_chain = True 31 | - shared_args1 = ["-signetchallenge=51"] # OP_TRUE 32 | + 33 | + self.signet_challenge1 = '51' # OP_TRUE
nit: could make self.signet_challenges an array, which is a more common pattern in the tests.
Resolved in 4a21c49e26e78a2820b9603cf765710323efd700
95 | + assert_equal(mining_info3['chain'], 'signet') 96 | + assert_equal(mining_info3['signet_challenge'], self.signet_challenge3) 97 | 98 | self.generate(self.nodes[0], 1, sync_fun=self.no_op) 99 | 100 | +
nit: stray new blank line
Resolved in 4a21c49e26e78a2820b9603cf765710323efd700
@A-Manning thanks, can you squash the commits?
cc @maflcko
50 | + 51 | + self.signets = [ 52 | + SignetParams(challenge='51'), # OP_TRUE 53 | + SignetParams(), # default challenge 54 | + # default challenge as a 2-of-2, which means it should fail 55 | + SignetParams(challenge='522103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae')
nit: wrong indentation and unnecessary blank line
57 | 58 | self.extra_args = [ 59 | - shared_args1, shared_args1, 60 | - shared_args2, shared_args2, 61 | - shared_args3, shared_args3, 62 | + self.signets[0].shared_args, self.signets[0].shared_args,
I don't understand why this was duplicated in the original version. @kallewoof do you remember?
Makes no sense to me. I read through the entire thread in #18267 where this was added, and it looks like I put it there right at the end before it was merged, so it may have slipped under the radar.
Anyway, my suggestion: remove the duplication. Try the tests. If the tests fail, we know this at least has a purpose.
Oh I get it now. There's two nodes for each signet, six in total. So this is fine.
3 sets of pairs of nodes where each pair shares their set of args? OK, that sounds reasonable.
79 | - assert_equal(mining_info['chain'], 'signet') 80 | - assert 'currentblocktx' not in mining_info 81 | - assert 'currentblockweight' not in mining_info 82 | - assert_equal(mining_info['networkhashps'], Decimal('0')) 83 | - assert_equal(mining_info['pooledtx'], 0) 84 | + # mining info for node on signet 0
Maybe also add a check_getmininginfo helper?
Added in edc34044362628f2994cb5fa5de74ca49fce0fd1
78 | - assert_equal(mining_info['chain'], 'signet') 79 | - assert 'currentblocktx' not in mining_info 80 | - assert 'currentblockweight' not in mining_info 81 | - assert_equal(mining_info['networkhashps'], Decimal('0')) 82 | - assert_equal(mining_info['pooledtx'], 0) 83 | + def check_getmininginfo(node_idx, signet_idx):
Thanks for adding the helper. But now it's inconsistent with the check_getblockchaininfo helper. No strong preference for which style to use.
Maybe make it def check_getmininginfo(self, node_idx, signet_idx): and check_getblockchaininfo(self, node_idx, signet_idx) and put them above run_test.
Both helper functions are internal to run_test in ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e.
check_getmininginfo makes specific assertions on values for fields such as blocks, which may change during a test. It is best to keep it as localized as possible, since it is unlikely to be broadly useful.
Concept ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
Both the RPC seems to be giving enough information about the signet challenges!! Thanks @A-Manning for this!!
I have used custom signet(OP_TRUE) on Linux x86_64:
getblockchaininfo:
{
"chain": "signet",
"blocks": 0,
"headers": 0,
"bestblockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
"difficulty": 0.001126515290698186,
"time": 1598918400,
"mediantime": 1598918400,
"verificationprogress": 1,
"initialblockdownload": true,
"chainwork": "000000000000000000000000000000000000000000000000000000000049d414",
"size_on_disk": 293,
"pruned": false,
"signet_challenge": "512102ee856c56a5aaadd1656f849bafa4c9dacc86a2878fe546c6189185f842ae2c1851ae",
"warnings": [
"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
]
}
getmininginfo:
{
"blocks": 0,
"difficulty": 0.001126515290698186,
"networkhashps": 0,
"pooledtx": 0,
"chain": "signet",
"signet_challenge": "512102ee856c56a5aaadd1656f849bafa4c9dacc86a2878fe546c6189185f842ae2c1851ae",
"warnings": [
"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
]
}
The test also seems to pass:
./build/test/functional/test_runner.py feature_signet.py
Temporary test directory at /tmp/test_runner_₿_🏃_20241221_004806
Remaining jobs: [feature_signet.py]
1/1 - feature_signet.py passed, Duration: 1 s
TEST | STATUS | DURATION
feature_signet.py | ✓ Passed | 1 s
ALL | ✓ Passed | 1 s (accumulated)
Runtime: 1 s
ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
Concept ACK
1306 | @@ -1307,6 +1307,7 @@ RPCHelpMan getblockchaininfo() 1307 | {RPCResult::Type::NUM, "pruneheight", /*optional=*/true, "height of the last block pruned, plus one (only present if pruning is enabled)"}, 1308 | {RPCResult::Type::BOOL, "automatic_pruning", /*optional=*/true, "whether automatic pruning is enabled (only present if pruning is enabled)"}, 1309 | {RPCResult::Type::NUM, "prune_target_size", /*optional=*/true, "the target size used by pruning (only present if automatic pruning is enabled)"}, 1310 | + {RPCResult::Type::STR_HEX, "signet_challenge", /*optional=*/true, "the block challenge (aka. block script), in hexadecimal (only present if the current network is a signet)"},
When used as an argument it is -signetchallenge, perhaps use that here as well.
signet_challenge is consistent with getblocktemplate
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
Indeed the general pattern seems to be that for RPC we use argument_name whereas for startup arguments we use -argumentname.
423 | @@ -424,6 +424,7 @@ static RPCHelpMan getmininginfo() 424 | {RPCResult::Type::NUM, "networkhashps", "The network hashes per second"}, 425 | {RPCResult::Type::NUM, "pooledtx", "The size of the mempool"}, 426 | {RPCResult::Type::STR, "chain", "current network name (" LIST_CHAIN_NAMES ")"}, 427 | + {RPCResult::Type::STR_HEX, "signet_challenge", /*optional=*/true, "The block challenge (aka. block script), in hexadecimal (only present if the current network is a signet)"},
See above note about -signetchallenge.
signet_challenge is consistent with getblocktemplate
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
Concept ACK
Tested ACK ecaa786
I tested the changes on the default signet and the result provided by both getblockchaininfo and getmininginfo RPC includes signet_challenge field. I also manually reviewed the new code for any errors and missing statements.
The updated test test/functional/feature_signet.py also works fine.
utACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e