Bugfix: chainparams: Add missing (always enabled) Taproot deployment for Signet #20157

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:signet_taproot_fix changing 1 files +5 −0
  1. luke-jr commented at 2:46 pm on October 15, 2020: member
    Is there a way we can trigger compiler warnings if a deployment is undefined?
  2. DrahtBot added the label Validation on Oct 15, 2020
  3. MarcoFalke commented at 6:27 pm on October 15, 2020: member
    How is this bug observable?
  4. luke-jr commented at 6:42 pm on October 15, 2020: member
    I didn’t try, but in theory the deployment params are going to be uninitialized and could be anything?
  5. laanwj added this to the milestone 0.21.0 on Oct 15, 2020
  6. in src/chainparams.cpp:336 in cb2bf18cd1 outdated
    330@@ -331,6 +331,11 @@ class SigNetParams : public CChainParams {
    331         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
    332         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
    333 
    334+        // Deployment of Taproot (BIPs 340-342)
    335+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
    336+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = 1199145601; // January 1, 2008
    


    MarcoFalke commented at 7:54 pm on October 15, 2020:
    0        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
    

    Should be always active


    ajtowns commented at 3:54 am on October 16, 2020:

    I think the ideal behaviour might be to have it always active on the default global signet, but optional for custom signets. Without the ability to disable it for custom signets, it’s not possible to make a custom signet that behaves the same as mainnet.

    Something like https://github.com/ajtowns/bitcoin/commit/4ceae373743043fac372a27244538a2ee64d0295 perhaps


    luke-jr commented at 4:32 pm on October 16, 2020:
    Well, testnet/regtest already have a way to configure these in the config file… maybe just copying/sharing that logic?

    ajtowns commented at 1:53 am on October 17, 2020:
    -vbparams is regtest-only, and I think more general than we’d want for something on signet. (I don’t think signalling activations on signet is ever actually useful, in particular)

    luke-jr commented at 1:31 pm on October 20, 2020:
    Made it always active for now. Adding a new option for custom signets goes beyond a bugfix.
  7. decryp2kanon commented at 6:12 pm on October 18, 2020: contributor
    taproot is already activated on global(?) signet? how to activate manually on my own custom signet?
  8. luke-jr force-pushed on Oct 20, 2020
  9. Bugfix: chainparams: Add missing (disabled) Taproot deployment for Signet 2d5793c016
  10. luke-jr force-pushed on Oct 20, 2020
  11. decryp2kanon commented at 6:52 pm on October 20, 2020: contributor
    utACK 2d5793c0161902730cde384dbdf3e3ba3e55c9e0
  12. sipa commented at 6:53 pm on October 20, 2020: member
    @kallewoof @ajtowns are you ok with having taproot always active on signet? @luke-jr Can you update the PR title as it’s no longer disabled.
  13. luke-jr renamed this:
    Bugfix: chainparams: Add missing (disabled) Taproot deployment for Signet
    Bugfix: chainparams: Add missing (always enabled) Taproot deployment for Signet
    on Oct 20, 2020
  14. kallewoof commented at 7:58 am on October 21, 2020: member

    @kallewoof @ajtowns are you ok with having taproot always active on signet?

    Yep!

  15. MarcoFalke commented at 8:05 am on October 21, 2020: member
    review ACK 2d5793c0161902730cde384dbdf3e3ba3e55c9e0
  16. MarcoFalke commented at 11:44 am on October 21, 2020: member

    Master:

    0==2885080== Thread 23 b-qt-rpcconsole:
    1==2885080== Conditional jump or move depends on uninitialised value(s)
    2==2885080==    at 0x3F94F4: BIP9SoftForkDescPushBack(UniValue&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Consensus::Params const&, Consensus::DeploymentPos) (blockchain.cpp:1220)
    3==2885080==    by 0x3F8E11: operator() (blockchain.cpp:1357)
    4...
    
  17. MarcoFalke commented at 12:19 pm on October 21, 2020: member

    Checked that this pull fixes the uninitialized read and also prints:

     0    "taproot": {
     1      "type": "bip9",
     2      "bip9": {
     3        "status": "active",
     4        "start_time": -1,
     5        "timeout": 9223372036854775807,
     6        "since": 0
     7      },
     8      "height": 0,
     9      "active": true
    10    }
    
  18. MarcoFalke merged this on Oct 21, 2020
  19. MarcoFalke closed this on Oct 21, 2020

  20. sidhujag referenced this in commit 3dfb40b982 on Oct 21, 2020
  21. DrahtBot locked this on Feb 15, 2022

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: 2024-10-08 16:12 UTC

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