[mempool] allow tx replacement by smaller witness #24007
Conversation
There was a problem hiding this comment.
Concept ACK, thanks for putting back on tracks witness replacement!
W.r.t 5%, I think that's good enough given we should be upper bounded by MAX_STANDARD_TX_WEIGHT, that should give us two-digit number of replacements as a DoS worst-case. What's your reasoning behind selecting this value ?
Maybe we would like to fine-tune to allow at least a 1-input/1-ouput Taproot spent, with a control block shorter by one node compared to the replaced transaction. I think one of the most probable witness replacement.
| bool validForFeeEstimation = !bypass_limits && !args.m_package_submission && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx); | ||
| // transaction has not necessarily been accepted to miners' mempools | ||
| // - this is not a witness replacement. Miners may not have witness replacement implemented | ||
| // (yet). TODO: re-include witness replacements in v25.0 |
There was a problem hiding this comment.
While I've seen protocol proposals leveraging same-txid-different-witness, I don't know any of them which is effectively deployed now (?). If they're and their usages is high enough to lead to a significant rate of witness conflicts, by that time I would expect this new replacement policy to be widely deployed in the miner ecosystem.
Of course, the majority of miners might run a different policy than the default Core's one. Though, until the contrary is corroborated by periodic probing of miner's mempools, I think we can reasonably hold the assumption they're.
So to simplify, I lean to account witness replacement in fee estimation as early as this PR.
What do you think ?
There was a problem hiding this comment.
The reasoning was based on #9519 - adding a witness replacement to the fee estimator isn't really accurate unless miners are also allowing witness replacements. I think it's reasonable to wait 1 or 2 releases for miners to update their policies? We just need to not forget to remove this bool. I imagine it'll take some time for it to be used, anyway.
|
We should probably use the same criteria for the feerate increase. Either this new criteria is safe to apply to other replacements, or it isn't safe. |
Just to clarify, what do you mean by "the same" criteria? Our current rules require an increase in absolute fees, which is not possible when the txid is the same. |
|
Concept ACK |
|
Concept ACK Would it be worth documenting the behaviour in a new section of doc/policy/mempool-replacements.md in this PR, or are we just reserving that for RBF? I'd need to try and convince myself whether 5% smaller witness is a reasonable anti-DoS or not. |
|
I understood the proposal by reading the functional test, however I couldn't imagine a real use case that could reduce the witness. Could you please give an example? |
There's some discussion of the motivations in #19645 but one simple example I can mention here involves Taproot. You may know that there are two ways to spend a Taproot output, the key path or the script path. The key path requires all participants to be in agreement with the spend and has a smaller witness. But suppose there is an absence of unanimous agreement, requiring broadcasting a transaction with an input using the script path (which has a large witness). While it's in the mempool, agreement is reached, and the transaction is sent again but this time using the key path. It's beneficial to the miner (in terms of feerate) and to the transaction participants (in terms of privacy and confirmation speed) if this second version of the transaction can replace the first, as this PR enables. I believe this can happen when L2s (such as LN) integrate the use of Taproot, but I don't understand the details. |
|
@LarryRuane Great, thank you so much! |
|
Hi @LarryRuane this PR needs a Rebase because of #21464 which intruduced mandatory parameters for UpdateTransactionsFromBlock() which needs now |
b15079a to
3452da0
Compare
|
Force pushed to improve the test comments, no functional change. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
cabb86b to
3a9a814
Compare
|
Force pushed to resolve hidden conflict due to #21464. |
|
Concept ACK |
Right. So for this to be consistent, those "current rules" would have to change. That's either safe or not. If not, then it isn't safe to do for this case either. |
|
@luke-jr agree, I should have clarified that we are indeed looking to propose a new set of RBF rules in general as well. I've been dragging my feet on it (sorry to everyone) but hopefully posting to ML soon. |
|
Concept ACK
Maybe other L2s but LN wouldn't use this (I don't think). In LN you might broadcast a unilateral close (sending to a timelocked output) because your counterparty is unresponsive. Then your counterparty comes back online and you agree to do a cooperative close to replace the unilateral close with no timelocked output. But the transactions aren't identical so you wouldn't be able to make use of this. I agree in principle that it makes no sense to prevent someone from using a smaller witness if for whatever reason they previously broadcast a larger witness than was necessary. But I also agree with Luke that it will be much easier to review these RBF PRs once we know what the updated RBF rules are. At the moment we are being asked to review PRs without knowing what the updated rules are. |
|
I'd still like to see a concrete use case (even just a proposal) before we consider merging support for this. This all seems very theoretical to me, and there are both code complexity concerns as well as anti-DoS concerns around using the network to relay transactions that never get mined. So if some protocol is relying on coordinating signatures for transactions by relaying messages on the bitcoin network rather than communicating over some other channel, I think we should be skeptical and carefully evaluate whether this makes sense. Does the bitcoin p2p network really need to serve as a communication medium for layer 2 protocols to construct efficient transactions?
I believe this proposal is equivalent to reducing the minrelayfee for witness-replacement transactions by approximately a factor of 20 -- you could imagine someone constructs a transaction with bloated witness which just meets the minrelayfee, and then repeatedly replaces it, dropping the size by 5% each time, resulting in using nearly 20x the bandwidth for the same total fee expended. Presumably the 5% number could be changed to accommodate a more (or less) conservative bandwidth limit, but I don't know how we should be picking it without knowing what use cases we are trying to support in the first place. So I tend to concept NACK, at least until we understand concretely what those use cases are. |
There was a problem hiding this comment.
Some small refactoring suggestions for the test:
--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -15,12 +15,10 @@ from test_framework.messages import (
CTxIn,
CTxInWitness,
CTxOut,
- sha256,
)
from test_framework.p2p import P2PTxInvStore
from test_framework.script import (
CScript,
- OP_0,
OP_ELSE,
OP_ENDIF,
OP_EQUAL,
@@ -29,9 +27,13 @@ from test_framework.script import (
OP_TRUE,
hash160,
)
+from test_framework.script_util import (
+ script_to_p2wsh_script,
+)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
+ assert_greater_than,
)
class MempoolWtxidTest(BitcoinTestFramework):
@@ -59,9 +61,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
parent.vin.append(CTxIn(COutPoint(int(txid, 16), 0), b""))
# Create a P2WSH output that the child that will be replaced will spend
- witness_program = sha256(witness_script)
- # This format (OP_0, <32-byte hash>) identifies this output as P2WSH
- script_pubkey = CScript([OP_0, witness_program])
+ script_pubkey = script_to_p2wsh_script(witness_script)
parent.vout.append(CTxOut(int(9.99998 * COIN), script_pubkey))
parent.rehash()
@@ -77,8 +77,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
# This simple P2WSH pubkey is trivial to spend, will conveniently allow
# the grandchild to spend the child's output without needing a signature
simple_witness_script = CScript([OP_TRUE])
- simple_witness_program = sha256(simple_witness_script)
- simple_script_pubkey = CScript([OP_0, simple_witness_program])
+ simple_script_pubkey = script_to_p2wsh_script(simple_witness_script)
# Create a new transaction with witness solving first (complicated) branch;
# in this case the witness contains the large_witness
@@ -104,7 +103,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
child_txid = child_original_txid
# Check the test itself; replacement transaction must be at least 5%
# smaller than the transaction being replaced.
- assert child_original.get_vsize() * 95 > child_replacement.get_vsize() * 100
+ assert_greater_than(child_original.get_vsize() * 95, child_replacement.get_vsize() * 100)
self.log.info("Submit child to the mempool")
txid_submitted = node.sendrawtransaction(child_original.serialize().hex())
3a9a814 to
ec9fc45
Compare
|
Force-pushed to incorporate #24007 (review), thanks, @theStack! |
|
Force-pushed rebase to 86ead0f |
af4bf1a to
d9b5b83
Compare
|
Force-pushed to d9b5b83 to fix CI failure |
|
Based on #24007 (comment) and discussion in https://bitcoincore.reviews/24007#l-181, marking this with "needs conceptual review" until more concrete use cases are given and the change has been discussed more thoroughly on e.g. the ML. |
|
I agree we need a more solid use case or cases, but just a general thought: Could rate-limiting mitigate the DoS attack surface? If an attacker submitted the same transaction with incrementally smaller witnesses 20 times, could the node not accept (or relay) the new transaction if the one it's replacing was accepted and relayed less than so many seconds ago? Maybe implement an exponential backoff policy by requiring an ever-greater delay before accepting the next version of the same transaction. It's true that the total required bandwidth would be the same, but if it's spread out over many minutes or hours, it would be less of a practical DoS threat. |
Currently, an existing mempool transaction can't be replaced by a tx that's identical except for its witness (same txid, different wtxid); the request is rejected as a duplicate. Change this behavior such that the mempool will replace the existing tx if the new tx has a smaller witness by at least 5% (an anti-Dos measure). Co-authored-by: glozow <gloriajzhao@gmail.com>
d9b5b83 to
04fe371
Compare
|
Force-pushed rebase to fix conflicts.
Good point. Is the following a possible solution?: Have the mempool keep track of the sum of the sizes of all versions of this transaction (same txid, different witnesses) that it has accepted and relayed, and require that this sum satisfy This would incentivize submitters to supply an initial fee large enough to cover the anticipated series of replacements. If it's not possible to submit another version of the transaction due to this constraint, it's not the end of the world; the existing (latest accepted and relayed) transaction remains in the mempool (and presumably will eventually get mined) -- this is no worse than today. Another benefit may be that we could eliminate the requirement that the replacement tx be at least 5% smaller than the existing tx. Instead, the only size constraint would be that the replacement is strictly smaller. A possible objection here is that an attacker could submit a series of transactions that are each one byte smaller than the previous, thus "using up" the entire |
|
One idea that comes to mind that would mitigate DoS but still provide utility in the way I think this might be intended to be used: Only allow going from a Tr script path to a Tr key path. This ensures that the witness replacement can only happen once, and prioritizes lazy cooperation. If APO is added later, another allowable replacement can be added for the 0x01 top branch script. |
|
@LarryRuane It's hard to evaluate what the right strategy is for mitigating potential network DoS in these cases, without first understanding the use cases we're trying to support. At the highest level, I'd say that there's a benefit to never writing the code or figuring out rules to support tx-replacement-via-smaller-witness if that meant that we just deter all possible protocols people might otherwise come up with which would use our public relay network unnecessarily. If people can figure out a way to relay the best version of their transactions the first time, that's a strict win for the public relay network. Expanding on that more: I think a really important idea is that we influence behavior on the network via the policy choices we implement and support. Inventing a standard in the absence of a use case is risky, both because we might encourage unintended behaviors which are bad for the network (and would otherwise have been avoided), and because we add more complexity to our code base without a corresponding benefit. On the other hand, if there is a use case / protocol where relaying alternate versions of the same txid might arise, we should just talk about what those are, because I would agree that we should find a way to support behaviors that users need and have economic value, to discourage the development of private relay to miners (which would be damaging to the ecosystem). So regarding your specific proposal: I'm really not sure; something like that might work? But I don't think it's worth adding that kind of complexity to our code when we don't know what specific use cases we're even trying to reason about. |
|
Closing as there doesn't seem to be much support for this change - it's still unclear whether any use cases exist. If something changes, please feel free to comment and we can revisit this at a later time. |
|
Regarding use cases: perhaps the potential coinjoin problem with annexes mentioned in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-June/021736.html can be mitigated with this PR. If large annexes can be replaced by smaller annexes (same txid, different wtxid), it may not be possible anymore for an attacker to inflate the annex and get it pinned. |
|
In the thread above, an idea came up that could be relevant for this PR in a world where the annex is enabled: In addition to the minimum reduction of 5%, a transaction with an empty annex is also always accepted as a replacement. This prevents an attacker from publishing with an annex that inflates the witness by 4.99% and preventing it from being replaced in the context of a protocol that doesn't use the annex at all. |
This PR is a reattempt of #19645, which has been abandoned.
Currently, an existing mempool transaction can't be replaced by a transaction that's identical except for its witness (same txid, different wtxid); the request is rejected as a duplicate. This PR changes this behavior such that the mempool will replace the existing transaction if the new transaction has a smaller witness by at least 5% (an anti-Dos measure). The replacement transaction can't increase the fee (since its inputs and outputs cannot change), but it necessarily has a higher fee rate than the existing transaction, since its overall size is lower. Miners should always prefer the replacement transaction.
An important difference between what this PR implements and the existing RBF (replace-by-fee, BIP125) is that the replacement can occur even if the original transaction is not marked as replaceable. This is because the replacement transaction will pay the same outputs as the transaction being replaced (identical txids ensures this), so the recipients of the existing transaction have no reason to fear not being paid by the replacement transaction. Unlike RBF, all of the existing transaction's descendants, if any, remain in the mempool.
Initially, we'll limit this to single transactions (not packages); this can be reconsidered later.