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

Test bitcoind wallet behavior during mempool eviction #2817

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

t-bast
Copy link
Member

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

When transactions are being evicted from the mempool, we want to make sure that bitcoind keeps the corresponding wallet inputs locked to avoid accidentally double-spending ourselves. We update our unit tests to be closer to common scenarios (chain of unconfirmed splice txs, commit txs replaced by the remote version, anchor tx RBF and htlc txs conflicts).

These tests confirm that:

  • if our wallet transaction is directly double-spent by another transactions (either a wallet transaction or an external one), bitcoind automatically frees up inputs when the competing transaction confirms
  • if our wallet transaction is invalidated by one of its parents being double-spent, bitcoind keeps wallet inputs locked, and we have to call abandontransaction ourselves to free up utxos

@t-bast t-bast requested review from sstone and pm47 February 6, 2024 06:51
When transactions are being evicted from the mempool, we want to make
sure that bitcoind keeps the corresponding wallet inputs locked to
avoid accidentally double-spending ourselves. We update our unit test
to be closer to common scenarios (chain of unconfirmed splice txs and
commit txs replaced by the remote version).

This test confirms that bitcoind keeps wallet inputs locked, and we
have to call `abandontransaction` to free up utxos used in txs that
have been permanently double-spent.
@t-bast t-bast force-pushed the bitcoind-pruning-tests branch from 01acbbc to 57f3242 Compare February 6, 2024 07:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

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

Comparison is base (e66e6d2) 85.86% compared to head (e92291f) 85.90%.
Report is 4 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    #2817      +/-   ##
==========================================
+ Coverage   85.86%   85.90%   +0.04%     
==========================================
  Files         216      217       +1     
  Lines       18228    18296      +68     
  Branches      772      793      +21     
==========================================
+ Hits        15652    15718      +66     
- Misses       2576     2578       +2     
Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.41% <100.00%> (+0.02%) ⬆️
...r/acinq/eclair/blockchain/fee/OnChainFeeConf.scala 95.00% <ø> (ø)
...q/eclair/channel/publish/ReplaceableTxFunder.scala 85.05% <100.00%> (+0.07%) ⬆️
.../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%> (ø)
.../scala/fr/acinq/eclair/payment/relay/Relayer.scala 89.13% <100.00%> (ø)
...air/payment/send/CompactBlindedPathsResolver.scala 100.00% <100.00%> (ø)
...scala/fr/acinq/eclair/payment/send/Recipient.scala 98.63% <100.00%> (+2.79%) ⬆️
...a/fr/acinq/eclair/wire/protocol/PaymentOnion.scala 99.36% <ø> (+3.82%) ⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 56.43% <80.00%> (+0.36%) ⬆️
... and 3 more

... and 13 files with indirect coverage changes

@t-bast t-bast marked this pull request as draft February 6, 2024 08:53
@t-bast t-bast marked this pull request as ready for review February 6, 2024 09:29
When one of our wallet transactions is directly double-spent, the
bitcoin wallet detects that and automatically frees up wallet inputs
from the permanently double-spent wallet transaction. This is great,
that means we only need to manually call `abandontransaction` for
transactions that have been invalidated because one of their parents
has been double-spent.
@t-bast t-bast force-pushed the bitcoind-pruning-tests branch from 6b5a770 to ae0c312 Compare February 6, 2024 09:48
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't imagine how much fun you had writing these tests!

@t-bast t-bast merged commit 599f9af into master Feb 13, 2024
1 check passed
@t-bast t-bast deleted the bitcoind-pruning-tests branch February 13, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants