Skip to content

Add electrum support to lightning-transaction-sync - #2685

Merged
TheBlueMatt merged 10 commits into
lightningdevkit:mainfrom
tnull:2022-12-add-electrum-sync
Nov 27, 2023
Merged

Add electrum support to lightning-transaction-sync#2685
TheBlueMatt merged 10 commits into
lightningdevkit:mainfrom
tnull:2022-12-add-electrum-sync

Conversation

@tnull

@tnulltnull commented Oct 25, 2023

Copy link
Copy Markdown
Contributor

Closes#2010.

This PR implements an ElectrumSyncClient that allows syncing LDK via the Confirm/Filter interfaces based on BDK's electrum-client crate.

We furthermore improve the logging in EsploraSyncClient and ElectrumSyncClient, and extend our test coverage.

Currently in draft as I want to do some further cleanup and experimentation.

@tnull
tnull marked this pull request as draft October 25, 2023 14:40
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch 3 times, most recently from eadc4ba to dfcc2f5CompareOctober 27, 2023 10:28
@codecov-commenter

codecov-commenter commented Oct 27, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (70ea110) to head (31ea90e).
⚠️ Report is 4936 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs0.00%6 Missing ⚠️
lightning/src/chain/onchaintx.rs0.00%5 Missing ⚠️
lightning/src/chain/chainmonitor.rs0.00%3 Missing ⚠️
lightning/src/ln/channel.rs83.33%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #2685 +/- ##
==========================================
- Coverage 88.55% 88.53% -0.02% 
==========================================
Files 113 113 Lines 89323 89334 +11 Branches 89323 89334 +11 ==========================================
- Hits 79097 79090 -7 - Misses 7860 7875 +15 - Partials 2366 2369 +3 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch 4 times, most recently from c684ae0 to 75ad902CompareOctober 30, 2023 09:07
@tnull

Copy link
Copy Markdown
ContributorAuthor

Did some further cleanup, should be ready for review now.

@tnull
tnull marked this pull request as ready for review October 30, 2023 09:13
@tnulltnull added this to the 0.0.119 milestone Oct 30, 2023
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch from 75ad902 to e38abe0CompareOctober 30, 2023 17:27

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still have a bit to get through

Comment threadlightning-transaction-sync/src/esplora.rs
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch from e38abe0 to 74cafd3CompareNovember 7, 2023 09:11
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs
Comment threadlightning-transaction-sync/src/esplora.rs
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch from 74cafd3 to 5ba1c45CompareNovember 8, 2023 10:38

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly looks good. Feel free to squash fixups. Will take a final pass after.

Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch from 5ba1c45 to ce31cf2CompareNovember 8, 2023 15:50
@tnull

tnull commented Nov 8, 2023

Copy link
Copy Markdown
ContributorAuthor

Squashed previous fixups and addressed the pending comments.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM after a couple minor comments

Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch from ce31cf2 to d9f6d41CompareNovember 10, 2023 08:48
@tnull

Copy link
Copy Markdown
ContributorAuthor

Squashed fixups and included the following changes:

diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs
index a37d017fd..a002fc8e2 100644
--- a/lightning-transaction-sync/src/electrum.rs+++ b/lightning-transaction-sync/src/electrum.rs@@ -320,13 +320,13 @@ where
if let Some(history) = script_history.iter()
.filter(|h| h.tx_hash == txid).max_by_key(|x| x.height)
- {- let prob_conf_height = history.height;- let block_header = self.client.block_header(- prob_conf_height as usize)?;- if block_header.block_hash() == block_hash {- // Skip if the tx is still confirmed in the block in question.- continue;- }+ {+ let prob_conf_height = history.height;+ let block_header = self.client.block_header(+ prob_conf_height as usize)?;+ if block_header.block_hash() == block_hash {+ // Skip if the tx is still confirmed in the block in question.+ continue;
}
+ }
}
}
@@ -404,11 +404,6 @@ where
for mut bytes in merkle_res.merkle {
bytes.reverse();
- let next_hash = Sha256d::from_slice(&bytes).map_err(|_| {- log_error!(self.logger,- "Failed due to the server sending us bogus transaction data. This should not happen. Please verify server integrity."- );- InternalError::Failed- })?;-+ // unwrap() safety: `bytes` has len 32 so `from_slice` can never fail.+ let next_hash = Sha256d::from_slice(&bytes).unwrap();
let (left, right) = if index % 2 == 0 {
(cur, next_hash)

jkczyz
jkczyz previously approved these changes Nov 10, 2023
Comment threadlightning-transaction-sync/Cargo.toml Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs
Comment threadlightning-transaction-sync/tests/integration_tests.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
@tnull

Copy link
Copy Markdown
ContributorAuthor

Moved logging and updating SyncState back to the call sites and renamed the method:

> git diff-tree -U2 a48cf50b8 d609b5d37diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs
index 9d39ed87b..4dbd53609 100644
--- a/lightning-transaction-sync/src/electrum.rs+++ b/lightning-transaction-sync/src/electrum.rs@@ -113,7 +113,9 @@ where
// Double-check the tip hash. If it changed, a reorg happened since
// we started syncing and we need to restart last-minute.
- if self.check_has_chain_moved(&mut sync_state, &mut tip_header,- &mut tip_height)?+ if self.check_update_tip(&mut tip_header, &mut tip_height)?
{
+ log_debug!(self.logger,+ "Encountered inconsistency during transaction sync, restarting.");+ sync_state.pending_sync = true;
continue;
}
@@ -145,7 +147,9 @@ where
// Double-check the tip hash. If it changed, a reorg happened since
// we started syncing and we need to restart last-minute.
- if self.check_has_chain_moved(&mut sync_state, &mut tip_header,- &mut tip_height)?+ if self.check_update_tip(&mut tip_header, &mut tip_height)?
{
+ log_debug!(self.logger,+ "Encountered inconsistency during transaction sync, restarting.");+ sync_state.pending_sync = true;
continue;
}
@@ -186,5 +190,7 @@ where
}
- fn check_has_chain_moved(&self, sync_state: &mut SyncState, cur_tip_header: &mut BlockHeader, cur_tip_height: &mut u32) -> Result<bool, InternalError> {+ fn check_update_tip(&self, cur_tip_header: &mut BlockHeader, cur_tip_height: &mut u32)+ -> Result<bool, InternalError>+ {
let check_notification = self.client.block_headers_subscribe()?;
let check_tip_hash = check_notification.header.block_hash();
@@ -203,8 +209,4 @@ where
*cur_tip_header = check_notification.header;
*cur_tip_height = check_notification.height as u32;
- log_debug!(self.logger,- "Encountered inconsistency during transaction sync, restarting.");- sync_state.pending_sync = true;-
Ok(true)
} else {

jkczyz
jkczyz previously approved these changes Nov 15, 2023
Comment threadlightning-transaction-sync/src/electrum.rs Outdated
@tnull
tnullforce-pushed the 2022-12-add-electrum-sync branch 2 times, most recently from 85c0d77 to c8dca25CompareNovember 16, 2023 11:05
@tnull

Copy link
Copy Markdown
ContributorAuthor

Now included two more commits that set pending_sync when our last-minute tip checks failed and would have us error out (both for Esplora and Electrum).

Comment threadlightning-transaction-sync/src/electrum.rs Outdated
Comment threadlightning-transaction-sync/src/electrum.rs
@tnull

Copy link
Copy Markdown
ContributorAuthor

I just double-checked that there are no version conflicts/notable API changes messing with this after the bumps in #2740. Have a rebased branch ready-to-go, so happy to land this after #2740.

TheBlueMatt
TheBlueMatt previously approved these changes Nov 22, 2023

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should be good to go. I dunno if @jkczyz had any other thoughts, though.

We previously included the block hash, but it's also useful to include
the height under which we expect the respective transaction to be
confirmed.
We give some more information while reducing the log levels to make the
logging less spammy.
We also convert one safe-to-unwrap case from returning an error to
unwrapping the value.
In particular, we now test `register_output` functionality, too.
@tnull

Copy link
Copy Markdown
ContributorAuthor

Rebased on main after #2740 landed.

Now that we upgraded `esplora-client` to 0.6 we can use
`async-https-rustls` instead of manually overriding the `reqwest`
dependency.
@tnull

Copy link
Copy Markdown
ContributorAuthor

Introduced a small follow-up commit to the rebase. As we've now upgraded to use esplora-client 0.6, we can make use of the introduced async-https-rustls feature.

@TheBlueMatt
TheBlueMatt merged commit fa0d015 into lightningdevkit:mainNov 27, 2023
@tnulltnull mentioned this pull request Nov 27, 2023
Sign up for freeto 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.

Transaction sync crate follow-ups

5 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz@optout21