Only relay Taproot spends if next block has it active #20165

sipa commented at 4:23 AM on October 16, 2020: member

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.

in src/policy/policy.cpp:188 in 6d56873186 outdated

 182 | @@ -183,6 +183,9 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 183 |              if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) {
 184 |                  return false;
 185 |              }
 186 | +        } else if (whichType == TxoutType::WITNESS_V1_TAPROOT) {
 187 | +            // Don't allow Taproot spends unless Taproot is active.
 188 | +            if (!taproot_active) return false;

kallewoof commented at 4:25 AM on October 16, 2020:

            return taproot_active;

(Unless there is an assumption that there will be more stuff added below.)

sipa commented at 4:28 AM on October 16, 2020:

No, because we need to check all inputs. There can't be any return true's inside the loop body.

kallewoof commented at 4:40 AM on October 16, 2020:

D'oh, yep.

sipa commented at 4:25 AM on October 16, 2020: member

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 commented at 4:26 AM on October 16, 2020: member

utACK 6d568731862b1d888fa8db9effb3663dfe8e0e46

DrahtBot added the label TX fees and policy on Oct 16, 2020

DrahtBot added the label Validation on Oct 16, 2020

Sjors commented at 9:57 AM on October 16, 2020: member

utACK 6d56873

naumenkogs commented at 7:30 AM on October 19, 2020: member

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 commented at 6:50 PM on October 19, 2020: member

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 commented at 2:55 AM on October 20, 2020: member

@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 commented at 9:41 AM on October 20, 2020: member

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

sipa added this to the milestone 0.21.0 on Oct 20, 2020

MarcoFalke commented at 12:02 PM on October 21, 2020: member

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

in test/functional/feature_taproot.py:1454 in 6d56873186 outdated

1450 | @@ -1451,7 +1451,7 @@ def run_test(self):
1451 |  
1452 |          # Pre-taproot activation tests.
1453 |          self.log.info("Pre-activation tests...")
1454 | -        self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[1, 2, 2, 2, 2, 3])
1455 | +        self.test_spenders(self.nodes[0], spenders_taproot_inactive() + spenders_taproot_inactive() + spenders_taproot_inactive(), input_counts=[1, 2, 2, 2, 2, 3])

jnewbery commented at 4:48 PM on October 27, 2020:

I don't understand what this change is doing.

sipa commented at 5:00 PM on October 27, 2020:

It's running every test twice.

What motivated this: before this change, there are only 10 tests, the majority of which were non-standard. As a transaction is only standard when (at least) all its inputs are, this meant that almost all test transactions were expected to be nonstandard (as they contain more than one input). This means not much is tested: having one test incorrectly marked as standard would often not be caught, as it'd get combined with a nonstandard one. Having multiple instances of every test improves that somewhat.

jnewbery commented at 7:42 PM on October 27, 2020:

ok, in that case I think this would be a bit more obvious:

diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
index ca523572bc..5984ea4161 100755
--- a/test/functional/feature_taproot.py
+++ b/test/functional/feature_taproot.py
@@ -1143,6 +1143,9 @@ def spenders_taproot_inactive():
     add_spender(spenders, "inactive/scriptpath_valid_opsuccess", key=sec, tap=tap, leaf="op_success", standard=False, inputs=[getter("sign")])
     add_spender(spenders, "inactive/scriptpath_valid_opsuccess", key=sec, tap=tap, leaf="op_success", standard=False, inputs=[getter("sign")], sighash=bitflipper(default_sighash))
 
+    # Repeat each test 3 times so that any input which is non-standard is more likely to be caught
+    spenders *= 3
+
     return spenders
 
 # Consensus validation flags to use in dumps for tests with "legacy/" or "inactive/" prefix.
@@ -1451,7 +1454,7 @@ class TaprootTest(BitcoinTestFramework):
 
         # Pre-taproot activation tests.
         self.log.info("Pre-activation tests...")
-        self.test_spenders(self.nodes[0], spenders_taproot_inactive() + spenders_taproot_inactive() + spenders_taproot_inactive(), input_counts=[1, 2, 2, 2, 2, 3])
+        self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[1, 2, 2, 2, 2, 3])

sipa commented at 10:54 PM on October 30, 2020:

I've changed the approach, and moved it to a different commit.

jnewbery commented at 4:55 PM on October 27, 2020: member

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.

in src/policy/policy.h:101 in 6d56873186 outdated

  97 | @@ -98,7 +98,7 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
  98 |       * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
  99 |       * @return True if all inputs (scriptSigs) use only standard transaction forms
 100 |       */
 101 | -bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
 102 | +bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active = false);

MarcoFalke commented at 11:06 AM on October 28, 2020:

bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active);

I don't really like modifying code to accommodate tests. This is just asking for a mishap down the road, and at best someone else will have to clean it up anyway. It seems safer to have all callers pass down the bool explicitly.

MarcoFalke commented at 12:42 PM on October 28, 2020:

Ok, assuming that taproot activates, this code will go away either way, so consider my comment a style-nit.

sipa commented at 10:54 PM on October 30, 2020:

I've made the argument mandatory.

MarcoFalke commented at 11:07 AM on October 28, 2020: member

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 👉

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 2b92f56206799ab5e32a77050772e4eae3c8dec7e90cd81d50688289a0789af3 -

</details>

sipa force-pushed on Oct 30, 2020

Only relay Taproot spends if next block has it active 525cbd425e

Increase feature_taproot inactive test coverage 3d0556d410

sipa force-pushed on Oct 30, 2020

Sjors commented at 8:42 AM on October 31, 2020: member

utACK 3d0556d41087f945ed0a47a5d770076ad42ce432

MarcoFalke commented at 8:33 AM on November 2, 2020: member

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 995b925e09b8d1e7f45089547fb58deb2eb37a0a2f9be02f451aa790b742d490 -

</details>

jnewbery commented at 8:47 AM on November 2, 2020: member

utACK 3d0556d41087f945ed0a47a5d770076ad42ce432

MarcoFalke merged this on Nov 2, 2020

MarcoFalke closed this on Nov 2, 2020

in src/policy/policy.h:99 in 3d0556d410

  94 | @@ -95,10 +95,11 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType);
  95 |  bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason);
  96 |      /**
  97 |       * Check for standard transaction types
  98 | -     * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
  99 | +     * @param[in] mapInputs       Map of previous transactions that have outputs we're spending
 100 | +     * @param[in] taproot_active  Whether or taproot consensus rules are active (used to decide whether spends of them are permitted)

jonatack commented at 10:55 AM on November 2, 2020:

525cbd425 omit "or" or s/or/or not/

in src/validation.cpp:694 in 3d0556d410

 689 | @@ -690,7 +690,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 690 |      }
 691 |  
 692 |      // Check for non-standard pay-to-script-hash in inputs
 693 | -    if (fRequireStandard && !AreInputsStandard(tx, m_view)) {
 694 | +    const auto& params = args.m_chainparams.GetConsensus();
 695 | +    auto taproot_state = VersionBitsState(::ChainActive().Tip(), params, Consensus::DEPLOYMENT_TAPROOT, versionbitscache);

jonatack commented at 11:12 AM on November 2, 2020:

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

jonatack commented at 11:24 AM on November 2, 2020: member

Concept ACK, posthumous ACK 3d0556d41087f945ed0a47a5d770076ad42ce432

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

DrahtBot locked this on Feb 15, 2022