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

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

Contributors
luke-jr MarcoFalke ajtowns decryp2kanon sipa kallewoof

Labels
Validation

Milestone
0.21.0