Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 15, 2021

Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things:

  • Importing tr descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate tr descriptors by default (regardless of the taproot status) after commit 47fe744.
  • Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe744, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See Could the wallet count unconfirmed non-mempool change? #11887.

A similar change was done for segwit v0 in #13120 .

This effectively reverts commit c5ec036.

@sipa
Copy link
Member

sipa commented Nov 15, 2021

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23536 (Enforce Taproot script flags whenever WITNESS is set by MarcoFalke)
  • #23308 (Update basic multisig test/docs to use multipath descriptor by mjdietzx)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
  • #21702 (Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin)
  • #15719 (Wallet passive startup by ryanofsky)

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.

@maflcko maflcko force-pushed the 2111-policyTaprootActive branch from fa49d61 to fa3e0da Compare November 16, 2021 07:21
@bitcoin bitcoin deleted a comment from achow101 Nov 16, 2021
@maflcko
Copy link
Member Author

maflcko commented Nov 16, 2021

@achow101 Sorry, I accidentally deleted your comment. It was a comment on the docstring of Standard:

This comment is hard to parse, and it doesn't really explain each value in the enum. Could you please clarify?

Thanks, I've force pushed a new docstring.

@mjdietzx
Copy link
Contributor

Code Review ACK fa3e0da

@achow101
Copy link
Member

ACK fa3e0da

@sipa
Copy link
Member

sipa commented Nov 22, 2021

utACK fa3e0da

@gruve-p
Copy link
Contributor

gruve-p commented Nov 24, 2021

ACK fa3e0da

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review + tACK fa3e0da

@gunar
Copy link
Contributor

gunar commented Nov 24, 2021

Code Review + tACK fa3e0da

@maflcko maflcko merged commit 064c729 into bitcoin:master Nov 25, 2021
@maflcko maflcko deleted the 2111-policyTaprootActive branch November 25, 2021 07:21
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2021
fa3e0da policy: Treat taproot as always active (MarcoFalke)

Pull request description:

  Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things:

  * Importing `tr` descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate `tr` descriptors by default (regardless of the taproot status) after commit 47fe744.
  * Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe744, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See bitcoin#11887.

  A similar change was done for segwit v0 in bitcoin#13120 .

  This effectively reverts commit c5ec036.

ACKs for top commit:
  mjdietzx:
    Code Review ACK fa3e0da
  achow101:
    ACK fa3e0da
  sipa:
    utACK fa3e0da
  gruve-p:
    ACK bitcoin@fa3e0da
  gunar:
    Code Review + tACK fa3e0da
  rajarshimaitra:
    code review + tACK bitcoin@fa3e0da

Tree-SHA512: c6dc7a4e6c345bdec33f256847dc63906ab1696aa683ab9b32a79e715613950884ac3a1a7a44e95f31bb28e58dd64679a616175f7e152b21f5550f3337c8e622
fanquake added a commit that referenced this pull request Mar 11, 2022
40e871d [miner] always assume we can create witness blocks (glozow)

Pull request description:

  Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there.

ACKs for top commit:
  gruve-p:
    ACK 40e871d
  jnewbery:
    utACK 40e871d, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: #24421 (comment).
  achow101:
    ACK 40e871d
  theStack:
    Code-review ACK 40e871d

Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019
@bitcoin bitcoin locked and limited conversation to collaborators Mar 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants