policy: Treat taproot as always active (TX fees and policy)

https://github.com/bitcoin/bitcoin/pull/23512

Host: MarcoFalke  -  PR author: MarcoFalke

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.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. 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'.

  3. Which function was responsible to check if a transaction has a Taproot input? Hint: Start looking in MemPoolAccept::PreChecks.

  4. How does the pull request change the function? Is the return value or the function signature changed?

  5. Does this pull request change the handling of consensus-invalid transactions?

  6. 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?

  7. (bonus question) Is it theoretically possible for a mainnet chain to exist that has Taproot not activated or activated at a different block height?

  8. (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.

Meeting Log

  117:00 <MarcoFalke> Welcome to the review club today!
  217:00 <stickies-v> Hi everyone!
  317:00 <svav> Hi
  417:00 <emzy> Hi
  517:00 <oliver-offing> Hi all, first time here. Excited!
  617:00 <seaona> hi!
  717:01 <MarcoFalke> Welcome oliver-offing and every else here for the first time.
  817:01 <raj_> Hello..
  917:01 <ekzyis> hi :)
 1017:01 <Azor> Hi everyone
 1117:01 <MarcoFalke> Ok, let's get started. Did you review the PR? y/n?
 1217:01 <ekzyis> y
 1317:01 <raj_> y
 1417:02 <emzy> n
 1517:02 <seaona> y
 1617:02 <stickies-v> y
 1717:02 <oliver-offing> n
 1817:02 <svav> n
 1917:02 <MarcoFalke> nice, seeing quite a few yes
 2017:03 <MarcoFalke> So let's jump right into the first question :)
 2117:03 <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'.
 2217:03 <Kaizen_Kintsugi> Hi
 2317:04 <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.
 2417:04 <raj_> I think there are two places where its used. Once for script flag calculation, and other for input standardness test?
 2517:04 <MarcoFalke> raj_: Correct, though there was one more (isTaprootActive)
 2617:04 <raj_> Ah right..
 2717:04 <seaona> check input standards
 2817:05 <MarcoFalke> stickies-v: Correct. Anyone knows what isTaprootActive is used for?
 2917:05 <stickies-v> it was used by the rpc client, I believe for getblockchaininfo?
 3017:05 <raj_> Its used to determine weather taproot activation is reached given a specific block height?
 3117:06 <MarcoFalke> Oh, I meant which part of the codebase is using it :)
 3217:06 <MarcoFalke> (The caller)
 3317:06 <stickies-v> hmm no sorry not for getblockchainactive, just to check if the wallet could import taproot descriptors already
 3417:06 <MarcoFalke> stickies-v: getblockchaininfo is also using it, so you are still right
 3517:07 <Kaizen_Kintsugi> it looks like chain interfaces?
 3617:07 <MarcoFalke> Kaizen_Kintsugi: Yes, isTaprootActive is part of the chain interface
 3717:07 <MarcoFalke> The chain interface is there for multiprocess and usually called by the wallet or GUI
 3817:08 <MarcoFalke> In this case it is called by a wallet RPC
 3917:08 <MarcoFalke> So to summarize the 4 places I found: GetBlockScriptFlags (consensus-related), AreInputsStandard (mempool policy), getblockchaininfo (RPC), isTaprootActive (wallet RPC)
 4017:08 <MarcoFalke> Which function was responsible to check if a transaction has a Taproot input? Hint: Start looking in MemPoolAccept::PreChecks.
 4117:09 <raj_> It was the now modified AreInputStandard function..
 4217:09 <seaona> bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active)
 4317:09 <stickies-v> `Solver` parses the script type, `AreInputsStandard` checks the output of Solver to check if it was a taproot script
 4417:09 <MarcoFalke> raj_: seaona: Correct
 4517:09 <MarcoFalke> stickies-v: Correct
 4617:10 <MarcoFalke> How does the pull request change the function? Is the return value or the function signature changed?
 4717:10 <ekzyis> it removes the flag if taproot is active, since now it is always active
 4817:11 <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.
 4917:11 <seaona> it won't return false on the assert:
 5017:11 <seaona> if (!taproot_active) return false;
 5117:11 <seaona> as it's removed. Also the function argument for checking input standards
 5217:12 <MarcoFalke> For anyone following, we are currently looking at https://github.com/bitcoin/bitcoin/blob/38b2a0a3f933fef167274851acaad0fd9104302a/src/validation.cpp#L727 and https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/policy/policy.h#L111 + policy.cpp
 5317:13 <raj_> quick question: in testnet the taproot activation height is 0. https://github.com/bitcoin/bitcoin/blob/64059b78f59e45cc4200ca76d0af8c6dff8a20d4/src/chainparams.cpp#L211
 5417:14 <raj_> What does that mean? Its always active in testnet?
 5517:14 <MarcoFalke> seaona: Correct. stickies-v: I'd say the function itself does change behavior. We'll get into the behavior changes a bit later.
 5617:14 <MarcoFalke> raj_: Yes, taproot is always active in regtest and signet
 5717:14 <raj_> Ok.. thanks..
 5817:16 <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.
 5917:16 <MarcoFalke> Does this pull request change the handling of consensus-invalid transactions?
 6017:16 <raj_> I think it doesn't. The PR only affects policy changes..
 6117:17 <oliver44> No, only changes transaction relay policy
 6217:17 <MarcoFalke> raj_: Yes. It only changes tx relay (and wallet behavior)
 6317:17 <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?
 6417:17 <seaona> I am not sure about this one. As far as I see,  the taproot_active bool is removed from the args, nothing else affecting?
 6517:18 <stickies-v> and agreed, this PR doesn't seem to affect any consensus behaviour
 6617:18 <MarcoFalke> stickies-v: Good question! The function in current master may return false if taproot_active was set to false.
 6717:19 <MarcoFalke> Can anyone explain why taproot_active might be set to false?
 6817:19 <svav> If a node has not been upgraded?
 6917:19 <ekzyis> if someone has not upgraded his node yet?
 7017:20 <MarcoFalke> Let's assume we are running the current master branch.
 7117:20 <seaona> basically if the deployment is not performed?
 7217:20 <seaona>  DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);
 7317:20 <raj_> MarcoFalke, for unupdated nodes, and when activation height is not reached for updated nodes..
 7417:20 <MarcoFalke> seaona: Yes. At what time would this happen in normal operation?
 7517:20 <stickies-v> ah or when the node hasn't synced to current chaintip?
 7617:21 <raj_> initial block download..
 7717:21 <MarcoFalke> yes, during IBD (initial block download) the node syncs the chain and might not be on the current tip
 7817:21 <seaona> aha I see
 7917:21 <stickies-v> right, so that explains the behaviour change I didn't catch earlier
 8017:22 <ekzyis> interesting, was already useful to be here. (also first timer here)
 8117:22 <MarcoFalke> stickies-v: Was good to have your question to clarify it.
 8217:23 <raj_> probably a stupid question, why do we disable spending before activation? but don't disable utxo creation..
 8317:23 <MarcoFalke> raj_: Good question. This is also the next question.
 8417:23 <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?
 8517:24 <ekzyis> the transaction is not relayed?
 8617:24 <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.
 8717:25 <raj_> If a non majority hashrate is only updated at that time, can that cause chain split?
 8817:25 <OliverOffing> Taproot activation is hardcoded at a blockheight, so theoretically a 51% attack could rewrite history back to before Taproot was activated
 8917:26 <MarcoFalke> ekzyis: Yes, you are on the right path. (The full answer is a bit tricky, though)
 9017:26 <ekzyis> okay, looking at the other answers, I thought my answer was way too simple :D
 9117:27 <ekzyis> but aren't they also correct?
 9217:27 <MarcoFalke> stickies-v: correct. Though, sending to taproot *outputs* is always standard (This was changed in a previous release)
 9317:27 <MarcoFalke> raj_: I think there is no risk of a chain split with just this change
 9417:29 <raj_> MarcoFalke, yup right..
 9517:29 <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.
 9617:29 <raj_> Maybe another thing is, before activation a taproot utxo can be spent by anyone if majority hashrate haven't updated yet?
 9717:30 <MarcoFalke> raj_: Right, but this is the case on current master already.
 9817:31 <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..
 9917:32 <MarcoFalke> for reference, it was made standard in https://github.com/bitcoin/bitcoin/pull/15846
10017:33 <michaelfolkson> [17:14:02] <raj_> What does that mean? Its always active in testnet? (sorry to skip back, I arrived late)
10117:33 <michaelfolkson> Has this change in this PR already been applied to testnet then?
10217:34 <MarcoFalke> raj_: On a chain without taproot active (that is, without taproot rules being enforced), you *can* steal taproot utxo
10317:34 <MarcoFalke> Only as a miner, though
10417:34 <raj_> michaelfolkson, I think taproot spending was always standard in testnet, and there was no policy rule..
10517:35 <MarcoFalke> michaelfolkson: The discussed pr changes only relay policy and wallet RPC logic, not the activation status of a deployment.
10617:35 <MarcoFalke> michaelfolkson: taproot is always active on regtest and signet
10717:35 <MarcoFalke> On main and testnet3 it was deployed by version bits signalling
10817:35 <michaelfolkson> But not always active on testnet right?
10917:35 <michaelfolkson> Gotcha, thanks
11017:36 <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?
11117:36 <Kaizen_Kintsugi> ah I get it now
11217:37 <stickies-v> raj_ I think you don't need majority of network, just majority of miners
11317:38 <raj_> stickies-v, yes thats correct..
11417:40 <michaelfolkson> Sorry, confused. You talking about policy here or disabling ability to spend in the Core wallet?
11517:41 <michaelfolkson> Oh you're talking about the wallet
11617:42 <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..
11717:42 <raj_> Thats what this PR changes..
11817:43 <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).
11917:45 <MarcoFalke> Also, it wouldn't be a safe soft fork deployment if miners would generally include spends that they can't validate
12017:48 <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.
12117:49 <michaelfolkson> Just for clarity, this PR only makes changes for when you are still syncing the chain (IBD). In the run up to activation the wallet did allow you to send to a Taproot address but policy didn't relay that transaction https://bitcoin.stackexchange.com/questions/107186/should-the-bitcoin-core-wallet-or-any-wallet-prevent-users-from-sending-funds
12217:49 <michaelfolkson> Maybe that is obvious to people
12317:49 <MarcoFalke> Obviously sending to future (unenforced) witness programs is an issue already, but exposing it through the wallet is an actual concern for us.
12417:50 <Kaizen_Kintsugi> good to know
12517:50 <Kaizen_Kintsugi> damn these are good learning xps
12617:50 <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.
12717:50 <MarcoFalke> <- on this one I am not 100% sure (haven't tested it, but I read the code)
12817:50 <Kaizen_Kintsugi> that sounds like it could be expoited as an attack?
12917:51 <MarcoFalke> Kaizen_Kintsugi: Can you clarify?
13017:51 <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?
13117:51 <Kaizen_Kintsugi> COuld someone spam txs with tr inputs to fill up a mempool?
13217:52 <MarcoFalke> To clarify, I mean a *valid* mainnet chain
13317:52 <Kaizen_Kintsugi> MarcoFalke: I guess no, tr activation is controled by blockheight and version bits?
13417:52 <MarcoFalke> Kaizen_Kintsugi: Yes, those txs will fill up your mempool and the mempool of nodes that run this patch.
13517:53 <raj_> I would incline towards no.. That seems like a chain split recipe. All the nodes should agree on when a taproot block is valid?
13617:53 <MarcoFalke> Hint: Think about large reorgs
13717:53 <seaona> I am not sure. Why couldn't it be at a different block height?
13817:54 <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?
13917:54 <seaona> it's not hardcoded
14017:54 <michaelfolkson> All nodes should agree on when Taproot rules are being enforced
14117:54 <MarcoFalke> seaona: stickies-v: Correct. It could both be at a *different* block (a later block) or happen *not at all*.
14217:55 <stickies-v> the minimum activation height is hardcoded in core, but that could also easily be changed with a forked version of Core?
14317:55 <MarcoFalke> Which "property "would the chain need to have to be considered valid?
14417:55 <MarcoFalke> stickies-v: Yes, it could be changed, but that wouldn't be safe at all and also violate the BIP (thus be invalid)
14517:56 <MarcoFalke> I am only asking about valid chains
14617:56 <stickies-v> ah, then how could the activation height be different in practice?
14717:58 <MarcoFalke> stickies-v: It could be at a later point in time if miners went back and mined a different chain where they started signalling later
14817:58 <michaelfolkson> They would need to start mining on a block months deep though so in reality not viable
14917:59 <MarcoFalke> So they mine enough blocks to be *above* the minimum activation height and *below* the vb timeout
15017:59 <raj_> MarcoFalke, in that case they could also go back a not signal at all right? Like segwit? :D
15117:59 <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.
15217:59 <stickies-v> ah I thought timeout was before min activation height for speedy trial, thanks for clearing that up
15318:00 <michaelfolkson> If there is a months long re-org that is almost an Armageddon scenario
15418:00 <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
15518:01 <MarcoFalke> To answer the last question: I think the miner would include the spend, unless segwit was not active.
15618:02 <sipa> let's call the amount of pow that requires "1 powwow"
15718:02 <MarcoFalke> Let's wrap up the meeting
15818:02 <MarcoFalke> #endmeeting
15918:02 <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?
16018:02 <stickies-v> sipa ACK
16118:03 <Kaizen_Kintsugi> Thanks for hosting. Learned a lot today
16218:03 <raj_> MarcoFalke, thanks for hosting.. great meeting..
16318:03 <MarcoFalke> raj_: Yes, that is what I read from the mining code. The miner is happy to include taproot spends as soon as segwit (v0) is active.
16418:03 <seaona> thank you!! Very interesting meeting
16518:03 <svav> Thanks
16618:03 <dariusparvin> Thanks MarcoFalke!
16718:03 <stickies-v> thanks a lot MarcoFalke for hosting and everyone for the discussion!
16818:03 <MarcoFalke> Thanks for everyone who attended. Great answers and discussions!
16918:04 <OliverOffing> Yeah thanks MarcoFalke and all. Learned a lot!
17018:04 <ekzyis> yes, thank you very much!
17118:04 <michaelfolkson> Thanks MarcoFalke!
17218:08 <OliverOffing> See you all next week