Only relay Taproot spends if next block has it active by sipa · Pull Request #20165 · bitcoin/bitcoin

sipa · 2020-10-16T04:23:25Z

There should be no change to mempool transaction behavior for witness v1 transactions as long as no activation is defined. Until that point, we should treat the consensus rules as under debate, and for soft-fork safety, that means spends should be treated as non-standard.

It's possible to go further: don't relay them unless the consensus rules are actually active for the next block. This extends non-relay to the period where a deployment is defined, started, locked in, or failed. I see no downsides to this, and the code change is very simple.

sipa · 2020-10-16T04:25:35Z

A suggestion that is not included here is rejecting the creation of witness v1 spends during a period preceding activation to protect users sending to such outputs too early. It's easy to add if desired, but perhaps less necessary.

@sdaftuar Doing this for the entire defined/started/lockedin period adds additional non-homogeneity to network relay policy. @ajtowns suggested doing it only during during the lockedin period, to provide a short but still coordinated window of additional protection.

kallewoof

utACK 6d568731862b1d888fa8db9effb3663dfe8e0e46

src/policy/policy.cpp

Sjors · 2020-10-16T09:57:42Z

utACK 6d56873

naumenkogs · 2020-10-19T07:30:09Z

utACK 6d568731862b1d888fa8db9effb3663dfe8e0e46

Could you elaborate on what this means:

@ajtowns suggested doing it only during during the lockedin period, to provide a short but still coordinated window of additional protection.

sdaftuar · 2020-10-19T18:50:57Z

Concept ACK the change suggested in this PR.

A suggestion that is not included here is rejecting the creation of witness v1 spends during a period preceding activation to protect users sending to such outputs too early. It's easy to add if desired, but perhaps less necessary.

@sdaftuar Doing this for the entire defined/started/lockedin period adds additional non-homogeneity to network relay policy. @ajtowns suggested doing it only during during the lockedin period, to provide a short but still coordinated window of additional protection.

I'm not sure I entirely see a reason for concern over the non-homogeneity of network relay policy with respect to anyone-can-spend outputs; in my view, taproot-enabled software (by which I mean software that is aware of all the taproot consensus changes, necessarily including activation criteria for enforcing those rules) should differ from other software with how it treats v1 addresses, because it is the only software that actually knows what they mean.

One way to think about this is that if the community is using bitcoind to determine whether it's reasonable to relay a transaction creating a v1 output, that having the taproot-enabled-version of bitcoind correctly say "no" prior to activation is a useful thing to do. In the absence of us implementing that behavior, how would a user implement such footgun protection on their own?

Arguably, we could ship a taproot-enabled version of bitcoind that relayed such transactions anyway, if the user requested (eg by command-line option or something); but none of the reasons that come to mind right now of why someone might want to use such an option seem very good to me.

sipa · 2020-10-20T02:55:10Z

@naumenkogs Some background, this was discussed on the last IRC dev meeting, where I brought up two things we can do:

Making sure spending of witnesses isn't standard until an activation is defined (as the node shouldn't behave differently up to that point)
Making sure creating witness v1 outputs are not allowed to protect users potentially accidentally creating anyonecanspend outputs if they do it too soon, for some period of time prior to activation.

The PR here only does the first, as I think there are a few trade-offs to consider regarding the second.

ajtowns · 2020-10-20T09:41:53Z

Concept ACK -- I think it makes sense to not do any "create witness v1 outputs" protection until an activation method is defined.

Making spends non-standard is valuable for signet (where standardness and consensus aren't all that different, so making something standard but not consensus-enforced means it can be relied on in practice).

maflcko · 2020-10-21T12:02:35Z

Concept ACK will review when I am done reading through the other taproot changes

jnewbery

Concept ACK. Presumably this code can be reverted once taproot has been activated for some time?

I don't understand one change in the functional test.

test/functional/feature_taproot.py

src/policy/policy.h

maflcko · 2020-10-28T11:07:06Z

I checked out the test and ran it against master, without failure. Similarly running master against this test didn't result in a failure either.

Concept ACK on not changing policy for creation of v1 outputs

ACK 6d568731862b1d888fa8db9effb3663dfe8e0e46 👉

Show signature and timestamp

Signature:

Timestamp of file with hash 2b92f56206799ab5e32a77050772e4eae3c8dec7e90cd81d50688289a0789af3 -

Sjors · 2020-10-31T08:42:51Z

utACK 3d0556d

maflcko · 2020-11-02T08:33:34Z

review ACK 3d0556d 🏓
did not review the test, but verified that it fails on master-bitcoind

Show signature and timestamp

Signature:

Timestamp of file with hash 995b925e09b8d1e7f45089547fb58deb2eb37a0a2f9be02f451aa790b742d490 -

jnewbery · 2020-11-02T08:47:21Z

utACK 3d0556d

jonatack

Concept ACK, posthumous ACK 3d0556d

Verified the updated test fails on pre-merge master with

  File "test/functional/feature_taproot.py", line 1457, in run_test
    self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[1])
  File "test/functional/feature_taproot.py", line 1426, in test_spenders
    assert_raises_rpc_error(-26, None, node.sendrawtransaction, tx.serialize().hex(), 0)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 123, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
AssertionError: No exception raised

jonatack · 2020-11-02T10:55:50Z

src/policy/policy.h

     * Check for standard transaction types
-     * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
+     * @param[in] mapInputs       Map of previous transactions that have outputs we're spending
+     * @param[in] taproot_active  Whether or taproot consensus rules are active (used to decide whether spends of them are permitted)


525cbd4 omit "or" or s/or/or not/

jonatack · 2020-11-02T11:12:51Z

src/validation.cpp

    // Check for non-standard pay-to-script-hash in inputs
-    if (fRequireStandard && !AreInputsStandard(tx, m_view)) {
+    const auto& params = args.m_chainparams.GetConsensus();
+    auto taproot_state = VersionBitsState(::ChainActive().Tip(), params, Consensus::DEPLOYMENT_TAPROOT, versionbitscache);


525cbd4 perhaps s/auto/ThresholdState/ like validation.cpp::L1834 and mining.cpp::L820

kallewoof reviewed Oct 16, 2020

View reviewed changes

src/policy/policy.cpp Outdated Show resolved Hide resolved

DrahtBot added TX fees and policy Validation labels Oct 16, 2020

sipa added this to the 0.21.0 milestone Oct 20, 2020

jnewbery reviewed Oct 27, 2020

View reviewed changes

test/functional/feature_taproot.py Outdated Show resolved Hide resolved

maflcko reviewed Oct 28, 2020

View reviewed changes

src/policy/policy.h Outdated Show resolved Hide resolved

sipa force-pushed the 202010_taproot_policy branch from 6d56873 to 5482545 Compare October 30, 2020 22:42

sipa added 2 commits October 30, 2020 15:52

Only relay Taproot spends if next block has it active

525cbd4

Increase feature_taproot inactive test coverage

3d0556d

sipa force-pushed the 202010_taproot_policy branch from 5482545 to 3d0556d Compare October 30, 2020 22:53

maflcko merged commit c5ec036 into bitcoin:master Nov 2, 2020

jonatack reviewed Nov 2, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

sipa commented Oct 16, 2020

Uh oh!

sipa commented Oct 16, 2020

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Oct 16, 2020

Uh oh!

naumenkogs commented Oct 19, 2020

Uh oh!

sdaftuar commented Oct 19, 2020

Uh oh!

sipa commented Oct 20, 2020

Uh oh!

ajtowns commented Oct 20, 2020

Uh oh!

maflcko commented Oct 21, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 28, 2020

Uh oh!

Sjors commented Oct 31, 2020

Uh oh!

maflcko commented Nov 2, 2020

Uh oh!

jnewbery commented Nov 2, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants