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

Tx-Sync: Track spent WatchedOutputs and re-add if unconfirmed #2946

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 19, 2024

Fixes #2734.

  • Previously, we would track a spending transaction but wouldn't account for it being reorged out of the chain, in which case we wouldn't monitor the WatchedOutputs until they'd be reloaded on restart.
    In the first commit, we keep any WatchedOutputs around until their spends are sufficiently confirmed and only prune them after ANTI_REORG_DELAY.

  • Moreover, we dedup any ConfirmedTxs entries before handing them to Confirm::transactions_confirmed. Previously, we would just push to the confirmed_txs Vec, leading to redundant Confirm::transactions_confirmed calls, especially now that we re-confirm previously disconnected spends.
    In the second commit, we ensure that we don't push additional ConfirmedTx entries if already one with matching Txid is present. This not only gets rid of the spurious transactions_confirmed calls (which are harmless), but
    more importantly saves us from issuing unnecessary network calls, which improves latency.

@tnull tnull added this to the 0.0.122 milestone Mar 19, 2024
@tnull tnull requested a review from TheBlueMatt March 19, 2024 14:38
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (2c9dbb9) to head (b71c6e2).
Report is 27 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
+ Coverage   89.18%   90.01%   +0.83%     
==========================================
  Files         117      117              
  Lines       95541    99416    +3875     
  Branches    95541    99416    +3875     
==========================================
+ Hits        85205    89489    +4284     
+ Misses       7840     7714     -126     
+ Partials     2496     2213     -283     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tnull tnull force-pushed the 2024-03-txsync-readd-reorged-output-spends branch 2 times, most recently from 8ae8f02 to 0c8d233 Compare March 19, 2024 14:53
Previously, we would track a spending transaction but wouldn't account
for it being reorged out of the chain, in which case we wouldn't monitor
the `WatchedOutput`s until they'd be reloaded on restart.

Here, we keep any `WatchedOutput`s around until their spends are
sufficiently confirmed and only prune them after `ANTI_REORG_DELAY`.
@tnull tnull force-pushed the 2024-03-txsync-readd-reorged-output-spends branch 2 times, most recently from b1e51db to 6ded069 Compare March 19, 2024 16:02
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM.

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Looks good modulo the comments.

lightning-transaction-sync/src/esplora.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2024-03-txsync-readd-reorged-output-spends branch from 7253062 to 68c75dd Compare March 21, 2024 07:38
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to squash.

@@ -342,15 +342,19 @@ where
// unwrap() safety: len() > 0 is checked above
let pos = *indexes.first().unwrap() as usize;
if let Some(tx) = maybe_await!(self.client.get_tx(&txid))? {
let txid = tx.txid();
if tx.txid() != *txid {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: may as well pass txid by value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Previously, we would just push to the `confirmed_txs` `Vec`, leading to
redundant `Confirm::transactions_confirmed` calls, especially now that
we re-confirm previously disconnected spends.

Here, we ensure that we don't push additional `ConfirmedTx` entries if
already one with matching `Txid` is present. This not only gets rid of
the spurious `transactions_confirmed` calls (which are harmless), but
more importantly saves us from issuing unnecessary network calls, which
improves latency.
@tnull tnull force-pushed the 2024-03-txsync-readd-reorged-output-spends branch from d7dafc9 to b71c6e2 Compare March 21, 2024 16:41
@tnull
Copy link
Contributor Author

tnull commented Mar 21, 2024

Squashed commits and included the following changes:

> git diff-tree -U2 d7dafc94 b71c6e2f6
diff --git a/lightning-transaction-sync/src/esplora.rs b/lightning-transaction-sync/src/esplora.rs
index e7952d6c2..538918ada 100644
--- a/lightning-transaction-sync/src/esplora.rs
+++ b/lightning-transaction-sync/src/esplora.rs
@@ -274,5 +274,5 @@ where
                                continue;
                        }
-                       if let Some(confirmed_tx) = maybe_await!(self.get_confirmed_tx(&txid, None, None))? {
+                       if let Some(confirmed_tx) = maybe_await!(self.get_confirmed_tx(*txid, None, None))? {
                                confirmed_txs.push(confirmed_tx);
                        }
@@ -297,5 +297,5 @@ where
                                                if let Some(confirmed_tx) = maybe_await!(self
                                                        .get_confirmed_tx(
-                                                               &spending_txid,
+                                                               spending_txid,
                                                                spending_tx_status.block_hash,
                                                                spending_tx_status.block_height,
@@ -320,5 +320,5 @@ where
        #[maybe_async]
        fn get_confirmed_tx(
-               &self, txid: &Txid, expected_block_hash: Option<BlockHash>, known_block_height: Option<u32>,
+               &self, txid: Txid, expected_block_hash: Option<BlockHash>, known_block_height: Option<u32>,
        ) -> Result<Option<ConfirmedTx>, InternalError> {
                if let Some(merkle_block) = maybe_await!(self.client.get_merkle_block(&txid))? {
@@ -335,5 +335,5 @@ where
                        let mut indexes = Vec::new();
                        let _ = merkle_block.txn.extract_matches(&mut matches, &mut indexes);
-                       if indexes.len() != 1 || matches.len() != 1 || matches[0] != *txid {
+                       if indexes.len() != 1 || matches.len() != 1 || matches[0] != txid {
                                log_error!(self.logger, "Retrieved Merkle block for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid);
                                return Err(InternalError::Failed);
@@ -343,5 +343,5 @@ where
                        let pos = *indexes.first().unwrap() as usize;
                        if let Some(tx) = maybe_await!(self.client.get_tx(&txid))? {
-                               if tx.txid() != *txid {
+                               if tx.txid() != txid {
                                        log_error!(self.logger, "Retrieved transaction for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid);
                                        return Err(InternalError::Failed);
@@ -350,10 +350,10 @@ where
                                if let Some(block_height) = known_block_height {
                                        // We can take a shortcut here if a previous call already gave us the height.
-                                       return Ok(Some(ConfirmedTx { tx, txid: *txid, block_header, pos, block_height }));
+                                       return Ok(Some(ConfirmedTx { tx, txid, block_header, pos, block_height }));
                                }

                                let block_status = maybe_await!(self.client.get_block_status(&block_hash))?;
                                if let Some(block_height) = block_status.height {
-                                       return Ok(Some(ConfirmedTx { tx, txid: *txid, block_header, pos, block_height }));
+                                       return Ok(Some(ConfirmedTx { tx, txid, block_header, pos, block_height }));
                                } else {
                                        // If any previously-confirmed block suddenly is no longer confirmed, we found

@TheBlueMatt TheBlueMatt merged commit 650caa0 into lightningdevkit:main Mar 21, 2024
13 of 16 checks passed
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.

tx-sync: Re-register watched outputs after spend has been reorged out of chain
4 participants