Ensure transactions_confirmed is idempotent #1861
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the next commit we'll add some checks that redundant transactions aren't confirmed in different blocks, which would cause test_htlc_ignore_latest_remote_commitment to fail. Here we fix it to avoid the issue.
TheBlueMatt
force-pushed
the
2022-11-tx-connection-idempotency
branch
from
a16d4ca to
a005bf0
Compare
In `ChannelMonitor`s, when a transaction containing a spend of a revoked remote output reaches 6 confs, we may have no other tracking of that txid remaining. Thus, if we see that transaction again (because a user duplicatively confirms it), we'll generate a redundant spendable output event for it. Here we simply explicitly track all txids of transactions which confirm with a spendable output, allowing us to check this condition in the next commit.
TheBlueMatt
force-pushed
the
2022-11-tx-connection-idempotency
branch
from
a005bf0 to
315d120
Compare
Codecov ReportBase: 90.61% // Head: 90.95% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1861 +/- ##
==========================================
+ Coverage 90.61% 90.95% +0.34%
==========================================
Files 90 91 +1
Lines 47623 50270 +2647
Branches 47623 50270 +2647
==========================================
+ Hits 43152 45723 +2571
- Misses 4471 4547 +76
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
tnull
reviewed
Nov 21, 2022
dunxen
reviewed
Nov 21, 2022
TheBlueMatt
force-pushed
the
2022-11-tx-connection-idempotency
branch
from
315d120 to
a65a816
Compare
tnull
reviewed
Nov 23, 2022
In many complexity-reduced implementations of chain syncing using esplora `transactions_confirmed` may be called redundantly for transactions which were already confirmed. To ensure this is idempotent we add two new `ConnectionStyle`s in our tests which (a) call `transactions_confirmed` twice for each call, ensuring simple idempotency is ensured and (b) call `transactions_confirmed` once for each historical block every time we're connecting a new block, ensuring we're fully idempotent even if every call is repeated constantly. In order to actually behave correctly this requires a simple already-confirmed check in `ChannelMonitor`, which is included.
At the end of our `monitor_tests`, which test `ChannelMonitor` `SpendableOutputs` and claimable `Balance`s, add new checks that ensure that, if we're using the new `ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks`, we can replay the full chain without getting redundant events or `Balance`s.
TheBlueMatt
force-pushed
the
2022-11-tx-connection-idempotency
branch
from
a65a816 to
cd315d5
Compare
|
Squashed without changes. |
dunxen
approved these changes
Nov 24, 2022
tnull
approved these changes
Nov 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In many complexity-reduced implementations of chain syncing using
esplora
transactions_confirmedmay be called redundantly fortransactions which were already confirmed. To ensure this is
idempotent we add two new
ConnectionStyles in our tests which(a) call
transactions_confirmedtwice for each call, ensuringsimple idempotency is ensured and (b) call
transactions_confirmedonce for each historical block every time we're connecting a new
block, ensuring we're fully idempotent even if every call is
repeated constantly.
In order to actually behave correctly this requires a simple
already-confirmed check in
ChannelMonitor, which is included.Example #53,637,290 of the old adage "that which isn't tested is broken".