The PR branch HEAD was fa3e0da06 at the time of this review club meeting.
Notes
Policy or transaction relay policy describes which transactions are
considered for P2P relay by a node. Generally, any transactions added to the
transaction memory pool are valid according to consensus and policy rules.
While all nodes on the network are assumed to agree on consensus rules, they
might not agree on policy rules. For example, at times of high transaction
throughput, some peers might reject consensus-valid transactions with
insufficient fee.
Moreover, policy rules often change between software releases. For example if
a node receives a transaction that requires validation rules of a consensus
change it is not yet aware of, it may reject the loose transaction out of
caution even though the transaction will be valid in a block.
This pull request changes a policy rule of Taproot-spending transactions.
Previously it was only possible to send Taproot-spends after the Taproot
deployment activated, now it is possible at any time.
Which places use the status of the Taproot deployment? Which of those
are policy related? Hint: Search for Consensus::DEPLOYMENT_TAPROOT, for
example with git grep 'Consensus::DEPLOYMENT_TAPROOT'.
Which function was responsible to check if a transaction has a Taproot
input? Hint: Start looking in
MemPoolAccept::PreChecks.
How does the pull request change the function? Is the return value or
the function signature changed?
Does this pull request change the handling of consensus-invalid
transactions?
Are there any (theoretical) issues with the change? If yes, give examples of
adverse effects. Hint: Is the Taproot activation set in stone? What happens
if a wallet creates a transaction with Taproot inputs before Taproot is
active?
(bonus question) Is it theoretically possible for a mainnet chain to exist
that has Taproot not activated or activated at a different block height?
(bonus question) Does this change affect miners running this code? Assume
that the miner is running on a chain that has Taproot not active. Would the
miner attempt to include the transaction in a block? Hint: Look at
CreateNewBlock.
<MarcoFalke> Which places use the status of the Taproot deployment? Which of those are policy related? Hint: Search for Consensus::DEPLOYMENT_TAPROOT, for example with git grep 'Consensus::DEPLOYMENT_TAPROOT'.
<stickies-v> It was used for policy in MemPoolAccept::PreChecks. It is/was also used for what seems non-policy in GetBlockScriptFlags and ChainImpl::isTaprootActive.
<MarcoFalke> So to summarize the 4 places I found: GetBlockScriptFlags (consensus-related), AreInputsStandard (mempool policy), getblockchaininfo (RPC), isTaprootActive (wallet RPC)
<stickies-v> The function signature is changed because the last argument `taproot_active` is removed. The return type is unchanged. The function does not check anymore if taproot has been activated. As long as the longest chain has a taproot activation block height of 709,632, there should be no behaviour change.
<MarcoFalke> So to summarize the behavior change of the function: Previously AreInputsStandard returned false when it encountered a taproot spend, now it returns true if all inputs are otherwise standard.
<stickies-v> MarcoFalke : it would only return false if taproot was not actually activated yet, right? I.e. the current master branch would still return true at this point in time when there are taproot inputs?
<MarcoFalke> Are there any (theoretical) issues with the change? If yes, give examples of adverse effects. Hint: Is the Taproot activation set in stone? What happens if a wallet creates a transaction with Taproot inputs before Taproot is active?
<stickies-v> Taproot outputs that predate taproot activation height can be spent by anyone. In case of a huge reorg (going back to before block 709,632) to a chain that has no or a later taproot activation block, taproot outputs can be stolen.
<OliverOffing> Taproot activation is hardcoded at a blockheight, so theoretically a 51% attack could rewrite history back to before Taproot was activated
<MarcoFalke> OliverOffing: Currently the taproot activation is not (yet) hardcoded at a blockheight. While it has a minimum activatio height, the status is determined by version bits signalling in the chain.
<raj_> Yup.. but I can't just claim a taproot utxo out there with invalid sig because no miner would mine it, and most of the nodes are updated right now..
<raj_> MarcoFalke, hmm makes sense.. So is the reasoning for "disabling spend until activation" is like, we wanna make sure a taproot witness should only appear on chain when large majority of the network already knows how to deal with it?
<raj_> michaelfolkson, I think its about policy. I am not sure, but if a wallet did create a taproot spend before activation, it won't be propagated in the network..
<MarcoFalke> raj_: Good q. In theory it should be possible to include spends *before* activation (with or without witness), that is "invalid" or "valid". Though, obviously this is not safe (in the same way that sending to a taproot address is not safe in the first place). Though, it would be really messy relay-wise (as everyones mempool or just txs looked different).
<MarcoFalke> So to summarize the answer of negative effects if this patch was run with taproot being active: (1) One issue is that the wallet now allows import of taproot descriptors at any time. If someone were to send to those descriptors without taproot being active on the chain, miners could claim the outputs.
<MarcoFalke> Obviously sending to future (unenforced) witness programs is an issue already, but exposing it through the wallet is an actual concern for us.
<MarcoFalke> (2) Another issue is that txs with taproot inputs (sent by the wallet, RPC or over P2P) while *segwit* is inactive would fill the mempool without being mined.
<MarcoFalke> Also, let's jump into the next q: Is it theoretically possible for a mainnet chain to exist that has Taproot not activated or activated at a different block height?
<stickies-v> yes I it's theoretically possible, if we reorg back long enough so that we don't have enough taproot signaling blocks to activate speedy trial?
<MarcoFalke> Ok last question: Does this change affect miners running this code? Assume that the miner is running on a chain that has Taproot not active. Would the miner attempt to include the transaction in a block? Hint: Look at CreateNewBlock.
<MarcoFalke> stickies-v: timeout is given in "time" and min height is given in block height. While the two are expected to not overlap, with massive amounts of POW, I think they can be made to overlap
<raj_> MarcoFalke, it seems it doesn't affect.. We only check for segwit deployment. So as taproot is already part of segwit schemes so its already covered?