policy: enable sibling eviction for v3 transactions #29306
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
very compact!
alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
72a8433 to
96eadd1
Compare
There was a problem hiding this comment.
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.
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
96eadd1 to
8ee900f
Compare
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?"). |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
looking good, will do some testing
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
8ee900f to
8549fca
Compare
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
8549fca to
ac7527f
Compare
|
ACK apart from @instagibbs' nit here: #29306 (comment) |
src/policy/v3_policy.cpp
Outdated
There was a problem hiding this comment.
tighten checks a bit further to make sure this is incentive compatible?
| const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2}; | |
| const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2 && | |
| children.begin()->get().GetCountWithAncestors() == 2}; |
There was a problem hiding this comment.
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.
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
ac7527f to
77b0b8c
Compare
|
Additionally, I did extend the 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. |
It seems your understanding is incorrect, given #29306 (comment) and #29319 (comment)
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. |
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. |
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
77b0b8c to
75788ee
Compare
|
looks like you have a real test failure on a non-final commit |
Should be fixed now |
glozow
force-pushed
the
2024-01-sibling-eviction
branch
from
75788ee to
1342a31
Compare
| // already calculated. | ||
| if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { | ||
| // Disabled within package validation. | ||
| if (err->second != nullptr && args.m_allow_replacement) { |
There was a problem hiding this comment.
For package RBF I'll be switching this boolean on in multi-tx setting, so I'll likely have to modify this. It's correct as-is.
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 |
This does not address my concern here on the introduction of a new replacement cycling vector. |
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... |
|
ACK 1342a31 |
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).
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” :) |
|
ACK 1342a31 |
30a0113 [doc] update bips.md for 431 (glozow) 9dbe6a0 [test] wallet uses CURRENT_VERSION which is 2 (glozow) 539404f [policy] make v3 transactions standard (glozow) 052ede7 [refactor] use TRUC_VERSION in place of 3 (glozow) Pull request description: Make `nVersion=3` (which is currently nonstandard on mainnet) standard. Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873 See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool. ACKs for top commit: sdaftuar: utACK 30a0113 achow101: ACK 30a0113 instagibbs: utACK 30a0113 murchandamus: ACK 30a0113 ismaelsadeeq: ACK 30a0113 🛰️ Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
8 participants
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