When we receive a v3 transaction that would bust a mempool transaction’s descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it’s more incentive compatible and to avoid DoS).
Delving post with more background and motivation: https://delvingbitcoin.org/t/sibling-eviction-for-v3-transactions/472
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, sdaftuar, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
179@@ -180,6 +180,21 @@ std::optional<std::pair<std::string, CTransactionRef>> ApplyV3Rules(const CTrans
180 std::any_of(children.cbegin(), children.cend(),
181 [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;});
182 if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) {
183+ // Allow sibling eviction for v3 transaction: if another child already exists, even if
184+ // we don't conflict with it, consider evicting it under RBF rules. We rely on v3 rules
0 // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
54@@ -55,11 +55,11 @@ static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR
55 * @param[in] vsize The sigop-adjusted virtual size of ptx.
56 * @returns an error string if any v3 rule was violated, otherwise std::nullopt.
57 */
58-std::optional<std::string> ApplyV3Rules(const CTransactionRef& ptx,
59- const CTxMemPool::setEntries& mempool_ancestors,
60- unsigned int num_in_pkg_ancestors,
61- const std::set<Txid>& direct_conflicts,
62- int64_t vsize);
63+std::optional<std::pair<std::string, CTransactionRef>> ApplyV3Rules(const CTransactionRef& ptx,
971@@ -967,7 +972,19 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
972 CTxMemPool::setEntries collective_ancestors = *ancestors;
973 collective_ancestors.merge(ws.m_ancestors_of_in_package_ancestors);
974 if (const auto err{ApplyV3Rules(ws.m_ptx, collective_ancestors, ws.m_num_in_package_ancestors, ws.m_conflicts, ws.m_vsize)}) {
975- return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-rule-violation", err->first);
976+ // Disabled within package validation.
977+ if (err->second != nullptr && !args.m_package_submission) {
ApplyV3 directly to reduce conceptual sprawl
Do you mean all of the sibling eviction logic? That doesn’t work because we’d only enforce that the tx pays enough to evict siblings OR conflicting transactions; it needs to pay for both.
If you mean the logic adding stuff to m_conflicts, m_iters_conflicting, etc., it’s because I don’t like in-out parameters.
ApplyV3Rules would be using the contents beforehand to decide whether a child would be replaced, and then potentially modifying it in a way that looks like the child would replace it.
1041 "too many potential replacements", *err_string);
1042 }
1043 // Enforce Rule #2.
1044 if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) {
1045+ if (ws.m_sibling_eviction_failure_string.has_value()) {
1046+ return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-violation and cannot bypass because replacement-adds-unconfirmed",
Assume, also see #29306 (review)
very compact!
alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much
1031 return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1032 "too many potential replacements", *err_string);
1033 }
1034 // Enforce Rule #2.
1035 if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) {
1036+ if (ws.m_sibling_eviction_failure_string.has_value()) {
Assume() to check that there’s no sibling eviction going on in this scenario?
Assume
I was thinking this PR is ready to leave Draft status – is there anything you’re waiting on before doing that?
I’m not sure how important it is to return the v3 error messages if sibling RBF fails. It seems to me that if we just declare that attempting sibling eviction is part of the v3 validation rules, then we should treat failure to get in due to insufficient fees as an RBF failure and not as a v3 failure. (It would also simplify the implementation, so it’s possible I’m slightly blinded by aesthetics! Curious what others think.)
I haven’t reviewed the test yet, so I will still do that and do some testing of my own, but my first pass read is that this looks pretty good.
220+ // Allow sibling eviction for v3 transaction: if another child already exists, even if
221+ // we don't conflict with it, consider evicting it under RBF rules. We rely on v3 rules
222+ // only permitting 1 descendant, as otherwise we would need to have logic for deciding
223+ // which descendant to evict. Skip if this isn't true, e.g. if this transaction has
224+ // extra descendants due to a reorg.
225+ const bool consider_sibling_eviction{children.size() == 1};
parent_entry->GetCountWithDescendants() == 2 so no grandchildren and no other siblings.
alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much I’m not sure how important it is to return the v3 error messages if sibling RBF fails.
I’ve changed it to just add “(including sibling eviction)” when that is what’s happening. The diff is less invasive this way, but let me know if still too clunky. I just want to avoid confusing users (“wait, why am I getting an RBF error if I didn’t try to replace anything?”).
114@@ -115,7 +115,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
115 const auto expected_error_str{strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
116 tx_v2_from_v3->GetHash().ToString(), tx_v2_from_v3->GetWitnessHash().ToString(),
117 mempool_tx_v3->GetHash().ToString(), mempool_tx_v3->GetWitnessHash().ToString())};
118- BOOST_CHECK(*SingleV3Checks(tx_v2_from_v3, *ancestors_v2_from_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v3)) == expected_error_str);
->second
nullptr. Also added unit tests for whether sibling is returned when {1p1c, 1p2c, 3gen} is in mempool.
84@@ -85,7 +85,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
85 const auto parent_info = [&] {
86 if (mempool_ancestors.size() > 0) {
87 auto& mempool_parent = *mempool_ancestors.begin();
88- Assume(mempool_parent->GetCountWithDescendants() == 1);
ATMPArgs::m_allow_replacement to not hit it in a multi-testmempoolaccept.
311@@ -312,7 +312,8 @@ def test_mempool_sibling(self):
312 version=3
313 )
314 expected_error_mempool_sibling = f"v3-rule-violation, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit"
315- assert_raises_rpc_error(-26, expected_error_mempool_sibling, node.sendrawtransaction, tx_has_mempool_sibling["hex"])
316+ expected_error_mempool_sibling_no_eviction = f"insufficient fee (including sibling eviction), rejecting replacement"
533+ fee_rate=DEFAULT_FEE*50
534+ )
535+ expected_error_2siblings = f"v3-rule-violation, tx {tx_with_multi_children['txid']} (wtxid={tx_with_multi_children['wtxid']}) would exceed descendant count limit"
536+ assert_raises_rpc_error(-26, expected_error_2siblings, node.sendrawtransaction, tx_with_sibling3["hex"])
537+
538+ # However, an RBF (with conflicting inputs) is possible
0 # However, an RBF (with conflicting inputs) is possible even if the resulting cluster exceeds size 2
214@@ -214,10 +215,19 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTra
215 std::any_of(children.cbegin(), children.cend(),
216 [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;});
217 if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) {
218+ // Allow sibling eviction for v3 transaction: if another child already exists, even if
219+ // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
220+ // only permitting 1 descendant, as otherwise we would need to have logic for deciding
221+ // which descendant to evict. Skip if this isn't true, e.g. if the transaction has
222+ // multiple children or the sibling also has descendants due to a reorg.
223+ const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2};
969+ // Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
970+ // included in RBF checks.
971+ ws.m_conflicts.insert(err->second->GetHash());
972+ // Adding the sibling to m_iters_conflicting here means that it doesn't count towards
973+ // RBF Carve Out above. This is correct, since removing to-be-replaced transactions from
974+ // the descendant count is done separately in ApplyV3Rules for v3 transactions.
0 // the descendant count is done separately in SingleV3Checks for v3 transactions.
221+ // Allow sibling eviction for v3 transaction: if another child already exists, even if
222+ // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
223+ // only permitting 1 descendant, as otherwise we would need to have logic for deciding
224+ // which descendant to evict. Skip if this isn't true, e.g. if the transaction has
225+ // multiple children or the sibling also has descendants due to a reorg.
226+ const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2};
tighten checks a bit further to make sure this is incentive compatible?
0 const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2 &&
1 children.begin()->get().GetCountWithAncestors() == 2};
I don’t think this matters much either way, since this is only a case that can be arrived at via reorg, but I just wanted to note that I think it would be fine to attempt sibling eviction in the case where the other child has some other parent.
This is a scenario we want to avoid in general with v3, because that transaction could be an RBF pin, because such a transaction can pay a large fee but have a low mining score due to the other parent(s). However, there’s no downside to attempting the replacement in this case, in the sense that if our RBF validation succeeds, then the topology constraint on the incoming transaction, combined with the restriction that the child we’re evicting has no children of its own, should guarantee that the replacement is incentive compatible.
Excerpt from #29319:
Replacing CPFP carveout (with a new sibling-replacement policy)
We can provide a way to achieve the goals of the CPFP carveout rule even if the existing carevout rule were to be dropped. As explained in this post, the use-case contemplated by the CPFP carveout rule was one where a single transaction might have two spendable outputs, each spendable by a different party, and that either party should be able to spend their output without hitting global topology limits, provided that their spending transaction was bounded in size and had no other unconfirmed parents. If the v3 policy rule were to be applied to the LN commitment transactions described in that post, then neither of the two outputs that are spendable could be used to hit the limits that carveout is designed to bypass.
So if we couple the v3 policy with an RBF rule that would allow one spend of a v3 transaction to replace an existing lower-feerate spend of that same parent – something we call sibling eviction and has a draft implementation in #29306 – then I believe we will have enacted a set of policies that replicate the CPFP carveout use case.
After spending 1h30 reviewing the 4 major LN implementations (CLN, LND, Eclair, LDK):
commit_tx_boost / spend_anchor in lightningd/anchorspend.c).SubscribeMempoolSpent() in chainnntfs/bitcoindnotify/bitcoind.go) does not subscribe to spend of the funding outpoint in all the contractcourt/ logic, only for HTLC timeout resolution L952 in htlc_timeout_resolver.go.checkAnchorPreconditions in ReplaceableTxPrePublisher.scala, there is no guarantee that the commitment outpoint txid they’re passing to getmempoolentry to check there is a remote commitment tx state in the mempool is synchronized, the 2 both valid commitment txn should be checked forsendrawtransaction is outside the library scope, no feeding of any mempool event to onchaintx.r and channelmonitor.rs.So unless my understanding is incorrect all major Lightning implementations have failed to implement CPFP carve-out correctly, a policy mechanism as old as 2019. As such the alternative upgrade path laid out in #29319 can be privilegied, i.e having Lightning commitment transaction uplifting to a single P2WSH or P2TR output with 2 branches, one for the holder, one for the counterparty commitment transaction.
There is no rational on the Bitcoin Core side, that we’re adding additional complexity like sibling eviction as proposed by this PR. The same design goal as laid out in #29319 (always RBF on concurrent state) can be achieved by user-side only changes. I did write a test, a chain of junk child transaction can be replaced at the commitment transaction output level (the 2nd descendant), see following line: https://github.com/ariard/bitcoin/commit/6337b978e77a4e1f93bb009958db7c9a619323df#diff-7cc77e2ea399e289803d6e7a3cc1f9ee147f5447f2762d1c7cb33da69c4d1ba9R224
@glozow @sdaftuar @instagibbs can you re-explain why this PR is needed given LN commitment transactions can achieve the same tx-relay propagation benefits without this change ?
There is functional alternative implemented here https://github.com/bitcoin/bitcoin/commit/6337b978e77a4e1f93bb009958db7c9a619323df as a proof-of-concept. No change to Bitcoin Core needed. v3 is enough even if the 1000 vb limit is weak in practice.
I think there was a misunderstanding on the necessity CPFP carve-out as implemented by #16421. The original motivation was here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
Eclair head commit 5d6a1db: within checkAnchorPreconditions in ReplaceableTxPrePublisher.scala, there is no guarantee that the commitment outpoint txid they’re passing to getmempoolentry to check there is a remote commitment tx state in the mempool is synchronized, the 2 both valid commitment txn should be checked for
You only checked part of the code. What actually matters here is rather the “watch” we set of the funding outpoint, which makes us spend from whatever commit (local/remote/next-remote) is seen in our mempool: https://github.com/ACINQ/eclair/blob/5d6a1db9fb39ed004a157fe79aa6aed748df8ed9/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala#L1576
This will leverage the carve-out rule if our peer has published their commitment and attached a chain of transactions to their anchor output (but it of course won’t if we didn’t detect the remote commit and instead published our local commit).
There is no rational on the Bitcoin Core side, that we’re adding additional complexity like sibling eviction as proposed by this PR.
The complexity is extremely minor, and improves the mempool. The worry I’d have would be offering this feature(which is useful for a number of reasons), and then “revoking it” in some theoretical future V3 update that doesn’t stick to 1p1c topology.
The same design goal as laid out in #29319 (always RBF on concurrent state) can be achieved by user-side only changes
Not all protocols are willing or able to conflict with the parent transaction, which is what this feature is for.
You only checked part of the code. What actually matters here is rather the “watch” we set of the funding outpoint, which makes us spend from whatever commit (local/remote/next-remote) is seen in our mempool: https://github.com/ACINQ/eclair/blob/5d6a1db9fb39ed004a157fe79aa6aed748df8ed9/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala#L1576
Yeah I saw you were monitoring both current remote commitment (remoteCommitPublished) and next remote commitment (nextRemoteCommitPublished) in CMD_BUMP_FORCE_CLOSE_FEE so I thought you were managing well the case where you can have any of 2 valid remote commitment transaction in your local mempool. However in checkAnchorPreconditions you’re only checking one of them as a getmempoolentry() (IIUC RepleaceableTxPrePublisher) ? I think remote commit tx N can replace your remote commit tx N-1 between your WatchFundingSpentTriggered and the attempt to broadcast a CPFP. There is there is no “lock” guarantee on the mempool state, so I believe you should broadcast 2 CPFP for each remote in a blind fashion.
The complexity is extremely minor, and improves the mempool. The worry I’d have would be offering this feature(which is useful for a number of reasons), and then “revoking it” in some theoretical future V3 update that doesn’t stick to 1p1c topology.
Can you lay out why this feature is useful for a number of reasons ? AFAICT, it’s only designed to be used in Lightning context (?), where the commitment transaction types are trying to leverage CPFP carve-out. From my understanding, any tx-relay propagation security benefit you get with sibling eviction, you will get it with package-relay + package-RBF (not even bandwidth savings of announcing parent tx-announcement ?).
At the very best the only motivation, I can see it as a temporary measure until package-relay is deployed, though it’s already on the list of priorities for 28.0.
Not all protocols are willing or able to conflict with the parent transaction, which is what this feature is for.
Proposal on my side is just to have a single output for fee-bumping with 2 alternative branches, there is no assumption laid out on conflicting with the parent transaction. By the way which are “all protocols” this feature is for ? It’s not documented on the delving bitcoin post, a part of LN.
Additionally, I did extend the test_v3_sibling_eviction new test here https://github.com/bitcoin/bitcoin/commit/04fdc0a77f70a998a433a3839c807422bc2e3bfa.
Sibling eviction adds a new replacement cycling vector, where you can from now on throw out of network mempools your counterparty honest medium-feerate CPFP with a “sibling-eviction” higher absolute fee / feerate, and then evict this “malicious” CPFP with a conflict on one of its spent UTXO, not related to the Lightning channel. From my understanding, this is a security regression from current carve-out mechanism, where at least the 2 anchor outputs spent are protected from concurrent spending. Only assuming ability to broadcast transactions in network mempools from the attacker, not advanced tx-relay topology trick. (To be fair, the alternative proposal I was laying out i.e 1 single PT2R with alternative branches is obviously affected by the same concern).
My reasonable recommendation is to skip “sibling eviction” directly and rather focus on package relay support. Now, if people wishes to move forward with this, then deprecate CPFP carve-out (which is downgrading theoretical LN security in a world where CPFP broadcast in implemented correctly by the majority of LN implementations), I don’t really care. My time is better allocated to break more substantial things, sibling eviction is too easy.
So unless my understanding is incorrect all major Lightning implementations have failed to implement CPFP carve-out
It seems your understanding is incorrect, given #29306 (comment) and #29319 (comment)
can you re-explain why this PR is needed given LN commitment transactions can achieve the same tx-relay propagation benefits without this change ?
Can you lay out why this feature is useful for a number of reasons ? AFAICT, it’s only designed to be used in Lightning context (?), where the commitment transaction types are trying to leverage CPFP carve-out.
As the delving post says, it’s beneficial to accept a new high feerate child of a v3 tx - without busting the descendant limits - instead of being stuck with a less incentive-compatible descendant that we received earlier.
293@@ -291,7 +294,12 @@ def test_v3_ancestors_package_and_mempool(self):
294
295 @cleanup(extra_args=["-acceptnonstdtxn=1"])
296 def test_mempool_sibling(self):
297- self.log.info("Test that v3 transaction cannot have mempool siblings")
298+ """
299+ When a transaction has a mempool sibling, it may be eligible for sibling eviction.
this isn’t true, you can use submitpackage when the parent is going to be de-duplicated to get sibling eviction.
Please change this and add a test :)
args.m_package_submission as you mentioned offline, so fixed that.
966+ if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
967+ // Disabled within package validation.
968+ if (err->second != nullptr && !args.m_package_submission && args.m_allow_replacement) {
969+ // Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
970+ // included in RBF checks.
971+ ws.m_conflicts.insert(err->second->GetHash());
m_conflicts and m_iters_conflicting should change. They’re currently documented as /** Txids of mempool transactions that this transaction directly conflicts with. */
. Is that still accurate if we add transactions that don’t conflict due to spending the same outputs?
However in checkAnchorPreconditions you’re only checking one of them as a getmempoolentry() (IIUC RepleaceableTxPrePublisher) ?
Yes, but there are potentially 3 instances of that actor running in parallel, one for each commitment transaction. So one of them will match what is in our mempool and will correctly sweep the anchor.
looks like you have a real test failure on a non-final commit
Should be fixed now
965+ // Even though just checking direct mempool parents for inheritance would be sufficient, we
966+ // check using the full ancestor set here because it's more convenient to use what we have
967+ // already calculated.
968+ if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
969+ // Disabled within package validation.
970+ if (err->second != nullptr && args.m_allow_replacement) {
Yes, but there are potentially 3 instances of that actor running in parallel, one for each commitment transaction. So one of them will match what is in our mempool and will correctly sweep the anchor.
Do you have test coverage that if there is replacement of current N commitment transaction with N+1 commitment transaction, your parallel actor re-broadcast well the corresponding anchor transaction ? I even think the correct model should be to have a parallel actor for each version of the counterparty revoked transaction (and as such txid), as we considered in the past for pinning.
As the delving post says, it’s beneficial to accept a new high feerate child of a v3 tx - without busting the descendant limits - instead of being stuck with a less incentive-compatible descendant that we received earlier.
This does not address my concern here on the introduction of a new replacement cycling vector.
Do you have test coverage that if there is replacement of current N commitment transaction with N+1 commitment transaction, your parallel actor re-broadcast well the corresponding anchor transaction ?
Yes, as long as we see any commitment in our mempool, we’ll spend it. If we don’t see it in our mempool, we cannot do anything though. I think we should stop spamming this PR though, lightning implementation details aren’t very useful here…
Yes, as long as we see any commitment in our mempool, we’ll spend it. If we don’t see it in our mempool, we cannot do anything though.
Better if you have test coverage. Not answering the fact that you might have to CPFP any past revoked commitment transaction which is not a new concern related to pinning (cf. scenario 2a)). Here lightning implementations details matters as sibling eviction is proposed to replace carve-out, which itself was justified for “contracting applications” (it’s already marked here: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L905).
This does not address my concern #29306 (comment) on the introduction of a new replacement cycling vector.
No conceptual answer on the introduction of a new replacement cycling vector neutralizing all security benefits for time-sensitive use-cases (either timevalue or loss of funds) :(
I’ll go to hack a tx-relay jamming toolkit and experiment under real-world conditions. Best process to prove my claim on the introduction of a new RC attack vector claim.
Feel free to move ahead with merging this, though if it’s broken “I would have told you so” :)
