Skip to content

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

Merged
maflcko merged 1 commit intobitcoin:masterfrom
luke-jr:signet_taproot_fix
Oct 21, 2020
Merged

Bugfix: chainparams: Add missing (always enabled) Taproot deployment for Signet#20157
maflcko merged 1 commit intobitcoin:masterfrom
luke-jr:signet_taproot_fix

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Oct 15, 2020

Is there a way we can trigger compiler warnings if a deployment is undefined?

@maflcko
Copy link
Member

maflcko commented Oct 15, 2020

How is this bug observable?

@luke-jr
Copy link
Member Author

luke-jr commented Oct 15, 2020

I didn't try, but in theory the deployment params are going to be uninitialized and could be anything?

@laanwj laanwj added this to the 0.21.0 milestone Oct 15, 2020
maflcko
maflcko reviewed Oct 15, 2020
View reviewed changes
@decryp2kanon
Copy link
Contributor

decryp2kanon commented Oct 18, 2020 •
edited
Loading

taproot is already activated on global(?) signet? how to activate manually on my own custom signet?

@luke-jr luke-jr force-pushed the signet_taproot_fix branch from cb2bf18 to b645e99 Compare October 20, 2020 13:31
@luke-jr luke-jr force-pushed the signet_taproot_fix branch from b645e99 to 2d5793c Compare October 20, 2020 13:32
@decryp2kanon
Copy link
Contributor

decryp2kanon commented Oct 20, 2020

utACK 2d5793c

@sipa
Copy link
Member

sipa commented Oct 20, 2020

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

@luke-jr luke-jr changed the title Bugfix: chainparams: Add missing (disabled) Taproot deployment for Signet Bugfix: chainparams: Add missing (always enabled) Taproot deployment for Signet Oct 20, 2020
@kallewoof
Copy link
Contributor

kallewoof commented Oct 21, 2020

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

Yep!

@maflcko
Copy link
Member

maflcko commented Oct 21, 2020

review ACK 2d5793c

@maflcko
Copy link
Member

maflcko commented Oct 21, 2020

Master:

==2885080== Thread 23 b-qt-rpcconsole:
==2885080== Conditional jump or move depends on uninitialised value(s)
==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)
==2885080==    by 0x3F8E11: operator() (blockchain.cpp:1357)
...

@maflcko
Copy link
Member

maflcko commented Oct 21, 2020

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

    "taproot": {
      "type": "bip9",
      "bip9": {
        "status": "active",
        "start_time": -1,
        "timeout": 9223372036854775807,
        "since": 0
      },
      "height": 0,
      "active": true
    }

@maflcko maflcko merged commit 0f86e7f into bitcoin:master Oct 21, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2020
Merge bitcoin#20157: Bugfix: chainparams: Add missing (always enabled…
3dfb40b 
…) Taproot deployment for Signet

2d5793c Bugfix: chainparams: Add missing (disabled) Taproot deployment for Signet (Luke Dashjr)

Pull request description:

  Is there a way we can trigger compiler warnings if a deployment is undefined?

ACKs for top commit:
  decryp2kanon:
    utACK 2d5793c
  MarcoFalke:
    review ACK 2d5793c

Tree-SHA512: 135cefae0f8dc552b0f682c2b87cabca7a4716290a36410a55968850e803a5049234e3cc597c8ef8d7917ae5d5ea3fb851e160df171b6793114c6bc01c5ea3e7
@ajtowns ajtowns mentioned this pull request Oct 22, 2020
Merged
@practicalswift practicalswift mentioned this pull request Nov 11, 2020
Merged
@ajtowns ajtowns mentioned this pull request Jan 11, 2022
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ajtowns ajtowns ajtowns left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Validation

Projects

None yet

Milestone

0.21.0

Development

Successfully merging this pull request may close these issues.

8 participants

@luke-jr @maflcko @decryp2kanon @sipa @kallewoof @ajtowns @laanwj @DrahtBot