Extend signetchallenge to set target block spacing #29365

pull starius wants to merge 1 commits into bitcoin:master from starius:signet-blockitme-in-challenge changing 10 files +320 −19
  1. starius commented at 4:36 pm on February 1, 2024: none

    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 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 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:

    0signet=1
    1[signet]
    2signetchallenge=6a4c09011e000000000000004c25512102f7561d208dd9ae99bf497273e16f389bdbd6c4742ddb8e6b216e64fa2928ad8f51ae
    3addnode=45.79.52.207:38333
    4dnsseed=0
    

    The content of this wrapped challenge:

    06a OP_RETURN
    14c OP_PUSHDATA1
    209 (length of signet params = 9)
    3011e00000000000000 (signet params: 0x01, pow_target_spacing=30)
    44c OP_PUSHDATA1
    525 (length of challenge = 37)
    6512102f7561d208dd9ae99bf497273e16f389bdbd6c4742ddb8e6b216e64fa2928ad8f51ae
    7(original Mutinynet challenge)
    
  2. DrahtBot commented at 4:36 pm on February 1, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29365.

    Reviews

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30611 (validation: write chainstate to disk every hour by andrewtoth)

    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.

  3. benthecarman commented at 7:02 pm on February 1, 2024: contributor
    Huge Concept ACK, this is great
  4. in src/kernel/chainparams.cpp:464 in c6f832f521 outdated
    340@@ -341,7 +341,7 @@ class SigNetParams : public CChainParams {
    341         consensus.CSVHeight = 1;
    342         consensus.SegwitHeight = 1;
    343         consensus.nPowTargetTimespan = 14 * 24 * 60 * 60; // two weeks
    344-        consensus.nPowTargetSpacing = 10 * 60;
    345+        consensus.nPowTargetSpacing = options.pow_target_spacing;
    


    Sjors commented at 11:22 am on February 6, 2024:
    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.
  5. in src/chainparams.cpp:84 in c6f832f521 outdated
    68+void ParseSignetParams(const std::vector<uint8_t>& params, CChainParams::SigNetOptions& options) {
    69+    if (params.empty()) {
    70+        return;
    71+    }
    72+
    73+    if (params.size() != 1 + 8) {
    


    Eunovo commented at 12:52 pm on February 13, 2024:
    Any specific reason why the params length must be 9?

    starius commented at 1:41 pm on February 13, 2024:

    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 if block 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.


    Eunovo commented at 5:05 am on February 21, 2024:
    @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.

    starius commented at 3:17 pm on February 21, 2024:
    @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.
  6. TheCharlatan commented at 1:13 pm on February 13, 2024: contributor
    Concept ACK
  7. Eunovo commented at 5:05 am on February 21, 2024: none
    Tested ACK Tried different signetchallenge inputs and got the expected results.
  8. DrahtBot added the label Needs rebase on Feb 26, 2024
  9. starius force-pushed on Feb 26, 2024
  10. starius commented at 8:14 pm on February 26, 2024: none
    Rebased to resolve conflict.
  11. DrahtBot removed the label Needs rebase on Feb 26, 2024
  12. maflcko requested review from ajtowns on Feb 27, 2024
  13. DrahtBot added the label CI failed on Apr 20, 2024
  14. DrahtBot removed the label CI failed on Apr 23, 2024
  15. in test/functional/feature_signet.py:31 in 557f7db468 outdated
    27@@ -28,7 +28,7 @@ def set_test_params(self):
    28         self.chain = "signet"
    29         self.num_nodes = 6
    30         self.setup_clean_chain = True
    31-        shared_args1 = ["-signetchallenge=51"]  # OP_TRUE
    32+        shared_args1 = ["-signetchallenge=6a4c09011e000000000000004c0151"]  # OP_TRUE, target_spacing=30.
    


    edilmedeiros commented at 3:13 pm on May 14, 2024:
    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.

    starius commented at 2:59 am on December 31, 2024:
    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.
  16. edilmedeiros commented at 3:25 pm on May 14, 2024: contributor

    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.

  17. cryptoquick commented at 4:48 am on May 28, 2024: none

    Tested ACK Works great against the Mutinynet node with the provided configuration, and the approach seems appropriate

    One thing I noticed is that progress=1.000000 is displayed on every block from the start… This isn’t normal, but it might be a quirk of Mutinynet.

  18. DrahtBot added the label Needs rebase on Jun 12, 2024
  19. Zk2u commented at 1:09 pm on September 12, 2024: none
    @starius could you possibly rebase this onto latest?
  20. starius force-pushed on Sep 12, 2024
  21. starius commented at 4:19 pm on September 12, 2024: none

    I rebased the PR against latest master. Manually resolved a small code conflict in src/chainparams.cpp and a large one in contrib/signet/miner. In new OOP version of contrib/signet/miner my change is much smaller :)

    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.

  22. DrahtBot removed the label Needs rebase on Sep 12, 2024
  23. DrahtBot added the label CI failed on Sep 13, 2024
  24. Zk2u commented at 11:21 am on September 13, 2024: none
    You’re a legend - thank you.
  25. DrahtBot removed the label CI failed on Sep 15, 2024
  26. Zk2u commented at 8:45 am on September 18, 2024: none

    Hi @starius, we’ve been testing this internally but can’t seem to get our blocks below the 2m29s interval. Here’s the signet challenge:

    06a4c090105000000000000004c25512102efc19c310fd60351b84273392d5bc2c58746bd3e8deca043623c0a16065ec2ec51ae
    

    Or:

    06a
    14c
    209
    301 0500000000000000  (signet params: 0x01, pow_target_spacing=5)
    44c
    525
    6512102efc19c310fd60351b84273392d5bc2c58746bd3e8deca043623c0a16065ec2ec51ae
    

    Calibrated nbits via:

    0../../contrib/signet/miner --cli="./bitcoin-cli" calibrate --grind-cmd="./bitcoin-util grind" --seconds=5
    

    Running the miner as:

    0../../contrib/signet/miner --cli="./bitcoin-cli -datadir=$datadir" generate --descriptor="tr(.../86h/0h/0h/0/*)#..." --grind-cmd="./bitcoin-util grind" --nbits=1d4f149d --ongoing --target-spacing=5
    

    Screenshot 2024-09-18 at 09 44 39

    Are we doing something wrong here?

    Edit: the miner script is not quite correct. While Generate’s class is doing the target spacing, the do_generate function wasn’t actuallly passing the target spacing argument into generate’s constructor so it was effecitvely ignoring it. My 5 second blocks are running every second now, though I’m hoping it autoadjusts after the difficulty adjustment period.

    Edit 2: 5 second block miner is working with this command + patch

     0diff --git a/contrib/signet/miner b/contrib/signet/miner
     1index 13468ff3ee..778e9b5d65 100755
     2--- a/contrib/signet/miner
     3+++ b/contrib/signet/miner
     4@@ -389,7 +389,7 @@ def do_generate(args):
     5     ultimate_target = nbits_to_target(int(args.nbits,16))
     6
     7     gen = Generate(multiminer=my_blocks, ultimate_target=ultimate_target, poisson=args.poisson, max_interval=args.max_interval,
     8-                   standby_delay=args.standby_delay, backup_delay=args.backup_delay, set_block_time=args.set_block_time, poolid=poolid)
     9+                   standby_delay=args.standby_delay, backup_delay=args.backup_delay, set_block_time=args.set_block_time, poolid=poolid, target_spacing=args.target_spacing)
    10
    11     mined_blocks = 0
    12     bestheader = {"hash": None}
    
    0../../contrib/signet/miner --cli="./bitcoin-cli -datadir=$datadir" generate --descriptor="tr(.../86h/0h/0h/0/*)#..." --grind-cmd="./bitcoin-util grind" --min-nbits --ongoing --target-spacing=5
    
  27. torkelrogstad commented at 6:52 pm on September 28, 2024: contributor
    Concept ACK
  28. starius force-pushed on Sep 29, 2024
  29. starius commented at 4:51 pm on September 29, 2024: none
    @Zk2u Thanks for the patch! I applied it.
  30. i-am-yuvi commented at 8:07 pm on December 20, 2024: none

    Great work!! @starius Concept ACK

    This would be a great help for Warnet Game since now you can easily configure the block creation time. Will review the code very soon!!

  31. pinheadmz commented at 8:21 pm on December 20, 2024: member

    This would be a great help for Warnet Game since now you can easily configure the block creation time. Will review the code very soon!!

    Indeed, this is how we do it now: https://github.com/bitcoin-dev-project/battle-of-galen-erso/blob/15ac31e0ec78dae716afc150f62e5cbc0fb7bd2e/scenarios/stub_orphan.py#L25-L31

  32. i-am-yuvi commented at 3:53 pm on December 21, 2024: none

    This would be a great help for Warnet Game since now you can easily configure the block creation time. Will review the code very soon!!

    Indeed, this is how we do it now: https://github.com/bitcoin-dev-project/battle-of-galen-erso/blob/15ac31e0ec78dae716afc150f62e5cbc0fb7bd2e/scenarios/stub_orphan.py#L25-L31

    Interesting!! Thanks for sharing 🔥

  33. DrahtBot added the label CI failed on Dec 22, 2024
  34. i-am-yuvi commented at 11:07 am on December 26, 2024: none

    #31446

    This should fix the issue with CI failure!!

  35. i-am-yuvi approved
  36. i-am-yuvi commented at 12:00 pm on December 26, 2024: none

    Tested ACK 6ce1b0ed7b8e4115f5ffed4c51c1d4eb0d6d5ddf

    Tested using various signet challenges(30s, 1min, 5 mins, 30mins, etc), worked as expected.

  37. DrahtBot requested review from edilmedeiros on Dec 26, 2024
  38. DrahtBot requested review from Eunovo on Dec 26, 2024
  39. DrahtBot requested review from TheCharlatan on Dec 26, 2024
  40. starius commented at 4:01 pm on December 26, 2024: none
    I’ll rebase after #31468 is merged to make CI green.
  41. DrahtBot removed the label CI failed on Dec 27, 2024
  42. DrahtBot added the label Needs rebase on Dec 30, 2024
  43. starius force-pushed on Dec 31, 2024
  44. starius commented at 2:58 am on December 31, 2024: none
    Rebased the PR, including #31468 and resolving the conflict in test/functional/feature_signet.py.
  45. starius force-pushed on Dec 31, 2024
  46. DrahtBot added the label CI failed on Dec 31, 2024
  47. DrahtBot commented at 3:02 am on December 31, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35003952940

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  48. starius force-pushed on Dec 31, 2024
  49. DrahtBot removed the label Needs rebase on Dec 31, 2024
  50. starius force-pushed on Dec 31, 2024
  51. starius force-pushed on Dec 31, 2024
  52. DrahtBot removed the label CI failed on Dec 31, 2024
  53. in test/functional/feature_signet.py:105 in c6bc0bb1f7 outdated
    102-        height = 0
    103-        for block in signet_blocks:
    104-            assert_equal(self.nodes[2].submitblock(block), None)
    105-            height += 1
    106-            assert_equal(self.nodes[2].getblockcount(), height)
    107+        for node_idx in [2, 6]:
    


    pinheadmz commented at 3:47 pm on January 2, 2025:
    Why change this? Why not also include nodes 0 & 1?

    starius commented at 5:51 pm on January 2, 2025:

    I verify that nodes accept blocks mined using the default signet challenge. This test includes one default signet node (2) and another node (6) configured with an extended signet challenge that uses the actual script OP_TRUE (accepts all blocks).

    I added a comment with this explanation to the code.

  54. pinheadmz commented at 3:51 pm on January 2, 2025: member
    concept ACK
  55. pinheadmz commented at 4:15 pm on January 2, 2025: member

    This change is backward-compatible, as per the old rules, such a signet challenge would always evaluate to false, rendering it unused.

    I don’t completely agree with this, since starting an old version of bitcoind with (for example) -signetchallenge=6a4c0901ff000000000000004c0151 would create an incompatible node. It would have a different network magic bytes and I think the OP_RETURN challenge would cause it to reject all blocks.

  56. starius commented at 5:40 pm on January 2, 2025: none

    This change is backward-compatible, as per the old rules, such a signet challenge would always evaluate to false, rendering it unused.

    I don’t completely agree with this, since starting an old version of bitcoind with (for example) -signetchallenge=6a4c0901ff000000000000004c0151 would create an incompatible node. It would have a different network magic bytes and I think the OP_RETURN challenge would cause it to reject all blocks.

    The change is backward-compatible in the sense that if bitcoind is updated to a version with this change, nothing will break - no existing signet challenge should have its meaning altered.

    You are correct that starting an old node with an extended signet challenge will result in it rejecting all blocks. However, I believe this outcome is actually beneficial: it would alert the node owner to the issue, prompting them to update their software.

    I updated that part of the description to make it more clear: “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.”.

    It would have a different network magic bytes.

    This isn’t accurate. The network magic bytes are derived solely from the real challenge (51 (OP_TRUE) in the case of 6a4c0901ff000000000000004c0151). Additionally, getblockchaininfo and getmininginfo both return 51, not the extended challenge, as verified in the feature_signet.py test.

  57. pinheadmz commented at 5:42 pm on January 2, 2025: member

    This isn’t accurate.

    I meant, if you passed the extended challenge to an old version of bitcoin it would compute different magic bytes than if you passed the same challenge to an upgraded node.

  58. Extend signetchallenge to set target block spacing
    Inspired by https://github.com/bitcoin/bitcoin/pull/27446, this commit
    implements the proposal detailed in the comment
    https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1516600820.
    
    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/ )
    7416b18392
  59. starius force-pushed on Jan 2, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me