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

Abandon transactions whose ancestors have been double spent #2818

Merged
merged 3 commits into from Feb 7, 2024

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Feb 6, 2024

When a transaction is directly double-spent, bitcoind is able to automatically detect it and free up wallet inputs that are used in double-spent transactions. However, when a transaction becomes invalid because one of its ancestors is double-spent, bitcoind is not able to detect it and will keep wallet inputs locked forever.

The only cases where this can happen are:

  • splice transactions when a commitment transaction that is not based on the latest splice is confirmed
  • anchor transactions in the above case, or when a different version of the commitment confirms (e.g. remote commit while we're trying to CPFP ou local commit)

When we detect that this happened, we abandon the corresponding transactions, which ensures that we don't end up with unusable liquidity in our wallet.

We also remove cases where we eagerly abandoned transactions, which could lead us to double-spend ourselves inadvertently because the abandoned transaction could still confirm while we reused its inputs in another transaction (which is a real issue when using 0-conf).

When a transaction is directly double-spent, bitcoind is able to
automatically detect it and free up wallet inputs that are used
in double-spent transactions. However, when a transaction becomes
invalid because one of its ancestors is double-spent, bitcoind is
not able to detect it and will keep wallet inputs locked forever.

The only cases where this can happen are:

- splice transactions when a commitment transaction that is not
  based on the latest splice is confirmed
- anchor transactions in the above case, or when a different
  version of the commitment confirms (e.g. remote commit while
  we're trying to CPFP ou local commit)

When we detect that this happened, we abandon the corresponding
transactions, which ensures that we don't end up with unusable
liquidity in our wallet.

We also remove cases where we eagerly abandoned transactions,
which could lead us to double-spend ourselves inadvertently
because the abandoned transaction could still confirm while
we reused its inputs in another transaction (which is a real
issue when using 0-conf).
@codecov-commenter
Copy link

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (e66e6d2) 85.86% compared to head (0378fc4) 85.89%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2818      +/-   ##
==========================================
+ Coverage   85.86%   85.89%   +0.02%     
==========================================
  Files         216      217       +1     
  Lines       18228    18296      +68     
  Branches      772      793      +21     
==========================================
+ Hits        15652    15715      +63     
- Misses       2576     2581       +5     
Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.41% <100.00%> (+0.02%) ⬆️
...ala/fr/acinq/eclair/blockchain/OnChainWallet.scala 100.00% <ø> (ø)
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 88.09% <ø> (ø)
...r/acinq/eclair/blockchain/fee/OnChainFeeConf.scala 95.00% <ø> (ø)
...q/eclair/channel/publish/ReplaceableTxFunder.scala 85.56% <100.00%> (+0.59%) ⬆️
...clair/channel/publish/ReplaceableTxPublisher.scala 71.33% <100.00%> (-0.10%) ⬇️
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala 99.69% <100.00%> (+0.01%) ⬆️
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala 99.67% <100.00%> (+0.01%) ⬆️
.../scala/fr/acinq/eclair/payment/PaymentPacket.scala 91.45% <100.00%> (+1.27%) ⬆️
...la/fr/acinq/eclair/payment/relay/NodeRelayer.scala 100.00% <100.00%> (ø)
... and 10 more

... and 4 files with indirect coverage changes

@t-bast t-bast merged commit 3d8dc88 into master Feb 7, 2024
1 check passed
@t-bast t-bast deleted the abandon-invalidated-transactions branch February 7, 2024 14:50
@achow101
Copy link

achow101 commented Feb 13, 2024

Someone brought this up to me today, and I'm thinking that this is a problem that should be solved in Bitcoin Core as well so that workarounds with abandontransaction are not needed. However, looking at how conflicts are handled in the Bitcoin Core wallet, I think this was already fixed in 26.0?

Edit: Is the issue that the parent transaction that ends up being conflicted does not belong to the wallet?

@t-bast
Copy link
Member Author

t-bast commented Feb 14, 2024

It would be awesome if the Bitcoin Core wallet automatically handled that for us, but it looks hard, because in those cases the ancestor transaction that ends up being double-spent indeed doesn't belong to the wallet. Bitcoin Core has no good reason to track that transaction, so it just looks like the child transaction is orphaned, but it cannot know whether it's permanent or temporary (if the ancestors are later reintroduced in the mempool because they were simply evicted).

On the lightning side, we do have more information because we need to track those ancestors, so it feels ok to handle those cases ourselves. To do it on the Bitcoin Core side, I believe you'd have to always track all the ancestors of any wallet transaction, even ancestors that are external to the wallet, and properly detect when they're double-spent: do you think that should be Bitcoin Core's responsibility?

@achow101
Copy link

Yes, with a parent unrelated to the wallet, this may be difficult for Bitcoin Core to handle. I've opened bitcoin/bitcoin#29435 to discuss possible solutions in Core.

@Eunovo
Copy link

Eunovo commented Feb 26, 2024

To do it on the Bitcoin Core side, I believe you'd have to always track all the ancestors of any wallet transaction, even ancestors that are external to the wallet, and properly detect when they're double-spent: do you think that should be Bitcoin Core's responsibility?

All ancestors or just the unconfirmed ancestors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants