search

lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.69k stars 2.08k forks source link

[feature]: Disable sweeper batching for anchor spends #8433

Closed morehouse closed 6 months ago

morehouse commented 9 months ago

The CPFP carve-out requires anchor spends to have only one unconfirmed ancestor (i.e. the commitment transaction). Thus, if the sweeper attempts to batch anchor spends, the CPFP carve-out rules are violated and we're vulnerable to basic commitment pinning attacks.

Roasbeef commented 9 months ago

So with https://github.com/lightningnetwork/lnd/pull/8345 we should no longer be attempting to publish (which may lock up UTXOs in the wallet) anything that violates default policy rules.

Aside from that, IIUC, wouldn't the bitcoind APIs reject such publish attempts in the first place as they don't pass the normal mempool checks?

morehouse commented 9 months ago

So with #8345 we should no longer be attempting to publish (which may lock up UTXOs in the wallet) anything that violates default policy rules.

Keeping the UTXOs unlocked when publishing fails is good, but it doesn't fix the problem. We still need to actually publish a transaction that doesn't fail, otherwise the commitment transaction will have low fee and can fail to confirm before HTLC deadlines.

Aside from that, IIUC, wouldn't the bitcoind APIs reject such publish attempts in the first place as they don't pass the normal mempool checks?

Our batched anchor spends would only be rejected by bitcoind if our counterparty has saturated the descendant limit for the commitment transaction. Then our anchor spend would exceed the limit, and also not satisfy the CPFP carve-out rules, and therefore be rejected.

It's also possible that the descendant limit is reached in miner mempools, but some of those descendants haven't propagated to our bitcoind mempool yet. In this case, our bitcoind would actually accept our batched anchor spends, while they would fail to propagate to miners.

morehouse commented 8 months ago

@yyforyongyu Is this something we can add to https://github.com/lightningnetwork/lnd/pull/8424, or would you rather make this change separately?

Crypt-iQ commented 8 months ago

This is also needed for when we upgrade to v3 transactions or if imbued v3 becomes a thing

yyforyongyu commented 8 months ago

This should happen after #8424, and should be straightforward as we can skip the aggregation by adding a NoAggregation implementation. On the other hand, we'll need to update btcwallet and btcd to make use of v3.

Crypt-iQ commented 8 months ago

With imbued v3 (new v2 transactions having v3 logic applied to them), we won't need to update btcwallet or btcd for the v3 logic to apply; it will just happen.