Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
morehouse opened this issue Jan 26, 2024 · 6 comments · Fixed by #8667
Closed

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

morehouse opened this issue Jan 26, 2024 · 6 comments · Fixed by #8667
Labels
brainstorming Long term ideas/discussion/requests for feedback mempool P2 should be fixed if one has time security General label for issues/PRs related to the security of the software utxo sweeping

Comments

@morehouse
Copy link
Collaborator

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.

@morehouse morehouse added security General label for issues/PRs related to the security of the software utxo sweeping mempool labels Jan 26, 2024
@Roasbeef
Copy link
Member

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.

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
Copy link
Collaborator Author

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.

@saubyk saubyk added brainstorming Long term ideas/discussion/requests for feedback P2 should be fixed if one has time labels Jan 31, 2024
@morehouse
Copy link
Collaborator Author

@yyforyongyu Is this something we can add to #8424, or would you rather make this change separately?

@Crypt-iQ
Copy link
Collaborator

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

@yyforyongyu
Copy link
Member

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorming Long term ideas/discussion/requests for feedback mempool P2 should be fixed if one has time security General label for issues/PRs related to the security of the software utxo sweeping
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants