Extend signetchallenge to set target block spacing #29365
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29365. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Huge Concept ACK, this is great |
src/kernel/chainparams.cpp
Outdated
There was a problem hiding this comment.
My initial worry when seeing this PR was that it would touch mainnet consensus code in order to support these parameters. But clearly it doesn't, which is nice.
src/chainparams.cpp
Outdated
There was a problem hiding this comment.
Any specific reason why the params length must be 9?
There was a problem hiding this comment.
The format of params is extendable in case more fields are added in the future. It is encoded as a concatenation of (field_id, value) tuples, protobuf style. Currently there is only one field defined pow_target_spacing, whose field_id is 0x01 and the lendth of encoding is 8 (int64_t). So valid values of params are:
- empty string (checked in
ifblock above) - 0x01 followed by 8 bytes of pow_target_spacing (9 bytes in total)
If length is not 0 and not 9, the value can not be parsed.
There was a problem hiding this comment.
@starius Might be useful to leave comment explaining why the params size is limited this way. Anyone looking to add a param in the future will see that they can change it.
There was a problem hiding this comment.
@Eunovo That's a good idea, thanks! I added the text of my comment above to the code and pushed in a separate temporal commit to track changes.
There was a problem hiding this comment.
I would expect to see a loop here that parses the byte vector:
- read byte -- if its unknown (not
01, currently) throw an InitError - for byte ==
01read the next 8 bytes to set the value - repeat for all future extensions
|
Concept ACK |
|
Tested ACK |
starius
force-pushed
the
signet-blockitme-in-challenge
branch
from
c6f832f to
64e9332
Compare
|
Rebased to resolve conflict. |
edilmedeiros
left a comment
edilmedeiros
left a comment
There was a problem hiding this comment.
Concept ACK.
I prefer the approach in #27446 but I can't say this would not be the bitcoin style of using the existing script machinery to implement so a logic.
I would love to see an accompanying BIP to upgrade BIP 325, since the signet behavior is standardized there. In practice, this change in core will potentially make this implementation an "undocumented standard".
I personally would prefer the BIP discussion to happen before merging, specially to define which params would be included in the configurable set and see if anyone proposes a better encoding. I'm not suggesting, though, that this doesn't get merged if consensus form around this proposal. I was messing around recently to set a faster signet for instructional purposes, this seem to be the perfect fit.
I'm still to give the code a better look and to test it locally.
test/functional/feature_signet.py
Outdated
| self.num_nodes = 6 | ||
| self.setup_clean_chain = True | ||
| shared_args1 = ["-signetchallenge=51"] # OP_TRUE | ||
| shared_args1 = ["-signetchallenge=6a4c09011e000000000000004c0151"] # OP_TRUE, target_spacing=30. |
There was a problem hiding this comment.
I don't feel like changing this test is a good idea since the proposed change is supposed to be backward-compatible. Better would be to add an additional functional test just for the purpose of testing the new behavior while keeping the old functional test as a guard rail.
There was a problem hiding this comment.
Thanks for the proposal! I reworked the change of feature_signet.py test. Instead of modifying the existing test case, I added a new one with the wrapped signet challenge.
|
Tested ACK One thing I noticed is that |
|
@starius could you possibly rebase this onto latest? |
starius
force-pushed
the
signet-blockitme-in-challenge
branch
from
64e9332 to
f6d419e
Compare
|
I rebased the PR against latest Also I noticed that Makefile based testing was replaced with CMake based approach. I added new C++ file with tests (chainparams_tests.cpp) to src/test/CMakeLists.txt instead of src/Makefile.test.include. |
|
You're a legend - thank you. |
starius
force-pushed
the
signet-blockitme-in-challenge
branch
2 times, most recently
from
1f28c0a to
a59e9fe
Compare
Inspired by bitcoin#27446, this commit implements the proposal detailed in the comment bitcoin#27446 (comment). Rationale. Introduce the ability to configure a custom target time between blocks in a custom Bitcoin signet network. This enhancement enables users to create a signet that is more conducive to testing. The change enhances the flexibility of signet, rendering it more versatile for various testing scenarios. For instance, I am currently working on setting up a signet with a 30-second block time. However, this caused numerous difficulty adjustments, resulting in an inconsistent network state. Regtest isn't a viable alternative for me in this context since we prefer defaults to utilize our custom signet when configured, without impeding local regtest development. Implementation. If the challenge format is "OP_RETURN PUSHDATA<params> PUSHDATA<actual challenge>", the actual challenge from the second data push is used as the signet challenge, and the parameters from the first push are used to configure the network. Otherwise the challenge is used as is. Under the previous rules, such a signet challenge would always evaluate to false, suggesting that it is likely not in use by anyone. Updating bitcoind to a version that includes this change will not cause any disruptions - existing signet challenges will retain their original meaning without alteration. The only parameter currently available is "target_spacing" (default 600 seconds). To set it, place "0x01<target_spacing as uint64_t, little endian>" in the params. Empty params are also valid. If other network parameters are added in the future, they should use "0x02<option 2 value>", "0x03<option 3 value>", etc., following the protobuf style. Two public functions were added to chainparams.h: - ParseWrappedSignetChallenge: Extracts signet params and signet challenge from a wrapped signet challenge. - ParseSignetParams: Parses <params> bytes of the first data push. Function ReadSigNetArgs calls ParseWrappedSignetChallenge and ParseSignetParams to implement the new meaning of signetchallenge. The description of the flag -signetchallenge was updated to reflect the changes. A new unit tests file, chainparams_tests.cpp, was added, containing tests for ParseWrappedSignetChallenge and ParseSignetParams. The test signet_parse_tests from the file validation_tests.cpp was modified to ensure proper understanding of the new logic. In the functional test feature_signet.py, a test case was added with the value of -signetchallenge set to the wrapped challenge, setting spacing to 30 seconds and having the actual challenge OP_TRUE. The Signet miner was updated, introducing a new option --target-spacing with a default of 600 seconds. It must be set to the value used by the network. Example. I tested this commit against Mutinynet, a signet running on a custom fork of Bitcoin Core, implementing 30s target spacing. I successfully synchronized the blockchain using the following config: signet=1 [signet] signetchallenge=6a4c09011e000000000000004c25512102f7561d208dd9ae99bf497273e16f389bdbd6c4742ddb8e6b216e64fa2928ad8f51ae addnode=45.79.52.207:38333 dnsseed=0 The content of this wrapped challenge: 6a OP_RETURN 4c OP_PUSHDATA1 09 (length of signet params = 9) 011e00000000000000 (signet params: 0x01, pow_target_spacing=30) 4c OP_PUSHDATA1 25 (length of challenge = 37) 512102f7561d208dd9ae99bf497273e16f389bdbd6c4742ddb8e6b216e64fa2928ad8f51ae (original Mutinynet challenge, can be found here: https://blog.mutinywallet.com/mutinynet/ )
starius
force-pushed
the
signet-blockitme-in-challenge
branch
from
a59e9fe to
41728b5
Compare
1440000bytes
left a comment
1440000bytes
left a comment
There was a problem hiding this comment.
utACK 41728b5
Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
It is possible to split the commit (and the commit message) into multiple parts:
Do you think, that such commit structure would make sense? |
|
That sounds reasonable at first glance. Each commit has to pass the tests. Ideally anytime you introduce a new function you would also add at least one test, and/or use that function in existing code (to show that it doesn't break any test). |
pinheadmz
left a comment
pinheadmz
left a comment
There was a problem hiding this comment.
code reviewed at 41728b5
Built and tested on arm64/macos. Played with the feature locally using 10 and 30 second block times. A few comments below, my main concern being a mismatch between the python miner and the custom consensus rules. Didn't have time to try and mine 2, 016 signet blocks to check that difficulty adjusted as expected, but otherwise everything seems to work as expected.
Consensus code is touched, but only for signet ;-)
| argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); | ||
| argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); | ||
| argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); | ||
| argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge); in case -signetchallenge is in the form of 'OP_RETURN PUSHDATA<params> PUSHDATA<actual challenge>', then <actual challenge> is used as a challenge and <params> is used to set parameters of signet; currently the only supported parameter is target spacing, the format of <params> to set it is 01<8 bytes value of target spacing, seconds, little endian>", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); |
There was a problem hiding this comment.
currently the only supported parameter is target spacing, the format of to set it is 01<8 bytes value of target spacing, seconds, little endian>
I think you can remove this, it could get carried away if more parameters are added in the future. You could just link to verbose documentation in signet/README.md or reccomend using signet/miner, which leads me to a second suggestion:
Add a new command to signet/miner like generate, solvepsbt etc... maybe makechallenge that accepts arguments like --target-spacing and --actual-challenge or even --pubkey and spits back out a hex string to start your blockchain with.
You could also then pass --challenge to the generate command and the mining script will just parse all the consensus parameters automatically from that. I think that will reduce the possibilty of mistakes and make the whole thing easier to put together.
| std::vector<unsigned char> params; | ||
| std::vector<unsigned char> challenge; | ||
| ParseWrappedSignetChallenge(*val, params, challenge); | ||
| ParseSignetParams(params, options); | ||
| options.challenge.emplace(challenge); |
There was a problem hiding this comment.
Why not pass options and args to one function ParseSignetChallenge() that calls the two functions you have, and fills in the SigNetOptions members where applicable? In other words, why write data to these vectors?
| const uint64_t value = uint64_t(bytes[0]) | uint64_t(bytes[1])<<8 | uint64_t(bytes[2])<<16 | uint64_t(bytes[3])<<24 | | ||
| uint64_t(bytes[4])<<32 | uint64_t(bytes[5])<<40 | uint64_t(bytes[6])<<48 | uint64_t(bytes[7])<<56; |
There was a problem hiding this comment.
Don't we have a serializer helper method for this kind of thing?
There was a problem hiding this comment.
i would probably make parsing work by doing a SERIALIZE_METHODS on a struct, and have any extensions in the future be done by failing to deserialize unknown versions...
src/chainparams.cpp
Outdated
There was a problem hiding this comment.
I would expect to see a loop here that parses the byte vector:
- read byte -- if its unknown (not
01, currently) throw an InitError - for byte ==
01read the next 8 bytes to set the value - repeat for all future extensions
|
Crashed with long time spacing (are you sure you need 2^64 seconds?)
log: |
Great catch! I think this is related to this place in There is a code which divides these numbers in
I'll try to reproduce this crash in
I don't think we need such high values :-) |
|
I think I discovered a bug in existing test diff --git a/test/functional/feature_signet.py b/test/functional/feature_signet.py
index 02e37f0fdd..86ab3202b8 100755
--- a/test/functional/feature_signet.py
+++ b/test/functional/feature_signet.py
@@ -87,7 +87,10 @@ class SignetBasicTest(BitcoinTestFramework):
check_getmininginfo(node_idx=3, signet_idx=1)
check_getmininginfo(node_idx=4, signet_idx=2)
- self.generate(self.nodes[0], 1, sync_fun=self.no_op)
+ while not self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(self.nodes[0:2])):
+ pass
+ self.wait_until(lambda: self.nodes[0].getblockcount() == 1)
+ self.wait_until(lambda: self.nodes[1].getblockcount() == 1)
self.log.info("pregenerated signet blocks check")(The patched branch can be found here. It is the patch above applied to the The loop is needed because one
This made the test flaky. Here is one of the fail logs I see that node 0 successfully generated the block and updated its tip: but node 1 for some reason got only block header and not the block itself: Then How to debug this further? Is this a genuine bug or my change above is wrong? |
|
What commit is that failure from? |
|
@pinheadmz The failure is from commit starius@60a7042 |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Are you still working on this? Are any of the Concept ACKers interested in actually reviewing this? |
|
Concept NACK I like the idea of custom target block spacing, e.g. for testing Stratum v2 with faster blocks, but I don't like the complexity of a wrapped signet challenge. Why not just add |
| throw std::runtime_error(strprintf("signet params must have length %d, got %d.", 1+8, params.size())); | ||
| } | ||
| if (params[0] != 0x01) { | ||
| throw std::runtime_error(strprintf("signet params[0] must be 0x01, got 0x%02x.", params[0])); |
There was a problem hiding this comment.
I would change this to read "Unknown Signet Challenge Version %d, versions [0x01] supported"
and read the first field as a VarInt or CompactSize.
| // - 0x01 followed by 8 bytes of pow_target_spacing (9 bytes in total). | ||
| // If length is not 0 and not 9, the value can not be parsed. | ||
|
|
||
| if (params.size() != 1 + 8) { |
There was a problem hiding this comment.
this should come after the version check
|
Concept ACK. We use something like this for testing and it's quite helpful. it's nice being able to ship a parameter pack in a single hex for a signet. I think the future version flexibility can be simplified, reducing @Sjors concern of the wrapper complexity. A good ol' serialize_methods chunk can probably make this trivial :) e.g.: SERIALIZE_METHODS(WrappedSignetChallenge, obj)
{
READWRITE(obj.version);
if (obj.version == 0) {
READWRITE(obj./*...*/);
// ....
} else {
if (ser_action.ForRead()) {
throw std::ios_base::failure(
strprintf("Attempted to Read Unknown WrappedSignetChallenge", obj.version))
}
if (ser_action.ForWrite()) {
assert(0 && "Unknown Version Generated By This Node -- please add serialization code to support new versions");
}
}
|
Yes. I'm addressing all the feedback. I'll publish new version soon. |
Address comment bitcoin#29365 (comment) by pinheadmz: > I would expect to see a loop here that parses the byte vector: > read byte -- if its unknown (not 01, currently) throw an InitError > for byte == 01 read the next 8 bytes to set the value > repeat for all future extensions
15 participants
Inspired by #27446, this PR implements the proposal detailed in the comment #27446 (comment).
Rationale
Introduce the ability to configure a custom target time between blocks in a custom Bitcoin signet network. This enhancement enables users to create a signet that is more conducive to testing. The change enhances the flexibility of signet, rendering it more versatile for various testing scenarios. For instance, I am currently working on setting up a signet with a 30-second block time. However, this caused numerous difficulty adjustments, resulting in an inconsistent network state. Regtest isn't a viable alternative for me in this context since we prefer defaults to utilize our custom signet when configured, without impeding local regtest development.
Implementation
If the challenge format is
OP_RETURN PUSHDATA<params> PUSHDATA<actual challenge>, the actual challenge from the second data push is used as the signet challenge, and the parameters from the first push are used to configure the network. Otherwise the challenge is used as is.Under the previous rules, such a signet challenge would always evaluate to
false, suggesting that it is likely not in use by anyone. Updating bitcoind to a version that includes this change will not cause any disruptions — existing signet challenges will retain their original meaning without alteration.The only parameter currently available is "target_spacing" (default 600 seconds). To set it, place
0x01<target_spacing as uint64_t, little endian>in the params. Empty params are also valid. If other network parameters are added in the future, they should use0x02<option 2 value>,0x03<option 3 value>, etc., following the protobuf style.Two public functions were added to
chainparams.h:ParseWrappedSignetChallenge: Extracts signet params and signet challenge from a wrapped signet challenge.ParseSignetParams: Parses<params>bytes of the first data push.Function
ReadSigNetArgscallsParseWrappedSignetChallengeandParseSignetParamsto implement the new meaning of signetchallenge.The description of the flag
-signetchallengewas updated to reflect the changes.A new unit tests file,
chainparams_tests.cpp, was added, containing tests forParseWrappedSignetChallengeandParseSignetParams.The test
signet_parse_testsfrom the filevalidation_tests.cppwas modified to ensure proper understanding of the new logic.In the functional test
feature_signet.py, a test case was added with the value of-signetchallengeset to the wrapped challenge, setting spacing to 30 seconds and having the actual challengeOP_TRUE.The Signet miner was updated, introducing a new option
--target-spacingwith a default of 600 seconds. It must be set to the value used by the network.Example
I tested this PR against Mutinynet, a signet running on a custom fork of Bitcoin Core, implementing 30s target spacing. I successfully synchronized the blockchain using the following config:
The content of this wrapped challenge: