feat(electrum)!: Update `bdk_electrum` to use merkle proofs #1489

LagginTimes · 2024-06-25T11:44:17Z

Fixes #980.

Description

This PR is the first step in reworking bdk_electrum to use merkle proofs. When we fetch a transaction, we now also obtain the merkle proof and block header for verification. We then insert an anchor only after validation that the transaction exists in a confirmed block. The loop logic that previously existed in full_scan to account for re-orgs has also been removed as part of this rework.

This is a breaking change because graph_updates now provide the full ConfirmationTimeHeightAnchor type. This removes the need for the ElectrumFullScanResult and ElectrumSyncResult structs that existed only to provide the option for converting the anchor type from ConfirmationHeightAnchor into ConfirmationTimeHeightAnchor.

Notes to the reviewers

Changelog notice

ConfirmationTimeHeightAnchor and ConfirmationHeightAnchor have been removed.
ConfirmationBlockTime has been introduced as a new anchor type.
bdk_electrum's full_scan and sync now return graph_updates with ConfirmationBlockTime.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

notmandatory · 2024-06-25T19:26:53Z

Since this doesn't touch the wallet APIs I moved it to the beta milestone.

LagginTimes · 2024-06-26T16:19:26Z

Since this doesn't touch the wallet APIs I moved it to the beta milestone.

I'm okay with this!

LLFourn

Thanks this simplifies a lot. Left a few comments around the anchors/merkle proof bit.

crates/electrum/src/bdk_electrum_client.rs

LLFourn · 2024-06-27T01:12:07Z

crates/electrum/src/bdk_electrum_client.rs

+            .transaction_get_merkle(&txid, confirmation_height as usize)
+        {
+            let header = self.inner.block_header(merkle_res.block_height)?;
+            let is_confirmed_tx = electrum_client::utils::validate_merkle_proof(


Is it possible to use rust bitcoin's merkle proof validator for this? Why does electrum have its own validation logic?

The validation logic being used right now was implemented as a utility in rust-electrum-client, which was needed for LDK's electrum integration:
bitcoindevkit/rust-electrum-client#122

Would using rust bitcoin be more advantageous in this scenario?

I have no idea. I am just curious why @tnull added that when there is presumably this algorithm is implemented in rust-bitcoin.

IIRC, Electrum's get_merkle/GetMerkleRes doesn't provide sufficient/slightly different data, so that we couldn't construct bitcoin::MerkleBlock/ PartialMerkleTree to use upstream's validation logic (which of course would have been our preferred way).

notmandatory · 2024-06-27T15:00:49Z

@LagginTimes oops, I didn't read all the comments before, looks like the wallet API will need to be updated so I've moved this back to the alpha milestone.

ValuedMammal

I did a first pass, mostly documentation nits. Sorry I was reviewing while you were pushing, so comments might be outdated

crates/chain/src/chain_data.rs

crates/chain/tests/test_tx_graph.rs

crates/electrum/src/bdk_electrum_client.rs

crates/electrum/tests/test_electrum.rs

notmandatory · 2024-07-08T02:09:54Z

@LagginTimes hey sorry I just merged some smaller PRs and now this one needs a rebase.

Both `bdk_electrum` and `bdk_esplora` now report the exact block that the transaction is in, which removes the need for having the old `ConfirmationTimeHeightAnchor` and `ConfirmationHeightAnchor`. This PR introduces a new, simpler anchor type that can be modified to support additional data in the future.

evanlinjin · 2024-07-09T12:54:52Z

I think we should not wait for complete tests before merging this PR. As long as the examples work fine it is good. This is for "beta" which does not need to be bug free. Also having this PR merged means we can work on the other anchor changes (which I need to create a ticket for).

evanlinjin · 2024-07-09T12:59:26Z

Also, the anchor changes that I am suggesting is going to conflict with ConfirmationBlockTime.

ValuedMammal

ACK 1a62488

ValuedMammal · 2024-07-09T15:12:29Z

crates/electrum/src/bdk_electrum_client.rs

-                    if unused_spk_count > stop_gap {
-                        return Ok(scanned_spks);
+                    unused_spk_count = unused_spk_count.saturating_add(1);
+                    if unused_spk_count >= stop_gap {


ValuedMammal · 2024-07-09T15:15:31Z

crates/electrum/src/bdk_electrum_client.rs

        // Prune `new_blocks` to only include blocks that are actually new.
-        .filter(|(height, _)| Some(*height) > agreement_height)
-        .map(|(height, hash)| BlockId { height, hash })
+        .filter(|(height, _)| Some(*<&u32>::clone(height)) > agreement_height)


nit, feel free to ignore

Suggested change

.filter(|(height, _)| Some(*<&u32>::clone(height)) > agreement_height)

.filter(|(&height, _)| Some(height) > agreement_height)

ValuedMammal · 2024-07-09T15:19:51Z

crates/electrum/tests/test_electrum.rs

@@ -19,6 +21,222 @@ fn get_balance(
    Ok(balance)
 }

+#[test]
+pub fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {


📌 If I recall, the name of this test is a relic from before the sync and full_scan concepts were developed. I might now call it sync_should_update_chain_and_tx_graph or similar, but not terribly important at the moment.

notmandatory

ACK 1a62488

Thanks for also adding the extra esplora tests for electrum!

LagginTimes force-pushed the electrum_spv branch from 303cd56 to 1e4ec2f Compare June 25, 2024 12:10

notmandatory assigned LagginTimes Jun 25, 2024

notmandatory added the api A breaking API change label Jun 25, 2024

notmandatory added this to the 1.0.0-alpha milestone Jun 25, 2024

notmandatory modified the milestones: 1.0.0-alpha, 1.0.0-beta Jun 25, 2024

LagginTimes marked this pull request as draft June 26, 2024 16:18

LagginTimes marked this pull request as ready for review June 26, 2024 16:57

LLFourn reviewed Jun 27, 2024

View reviewed changes

LLFourn mentioned this pull request Jun 27, 2024

Update bdk_electrum to use electrum's merkle proof API #980

Closed

notmandatory modified the milestones: 1.0.0-beta, 1.0.0-alpha Jun 27, 2024

LagginTimes force-pushed the electrum_spv branch 3 times, most recently from 180ee9f to feba152 Compare July 3, 2024 16:31

ValuedMammal reviewed Jul 3, 2024

View reviewed changes

LagginTimes force-pushed the electrum_spv branch 5 times, most recently from eb71012 to 756b8b1 Compare July 7, 2024 11:53

LagginTimes force-pushed the electrum_spv branch 2 times, most recently from 44df255 to ac3a388 Compare July 8, 2024 15:23

LagginTimes added 3 commits July 9, 2024 00:23

feat(electrum)!: Update bdk_electrum to use merkle proofs

d7f4ab7

test(electrum): Imported bdk_esplora tests into bdk_electrum

e761adf

LagginTimes force-pushed the electrum_spv branch from ac3a388 to 1a62488 Compare July 8, 2024 16:23

evanlinjin mentioned this pull request Jul 9, 2024

Remove Anchor trait and make anchors unique to (Txid, BlockId) #1507

Open

ValuedMammal approved these changes Jul 9, 2024

View reviewed changes

notmandatory approved these changes Jul 9, 2024

View reviewed changes

notmandatory merged commit d99b3ef into bitcoindevkit:master Jul 9, 2024
12 checks passed

ValuedMammal mentioned this pull request Jul 9, 2024

electrum: Should validate_merkle_for_anchor throw an error for missing or invalid proof? #1508

Closed

notmandatory mentioned this pull request Jul 20, 2024

Release 1.0.0-beta.1 #1520

Closed

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(electrum)!: Update `bdk_electrum` to use merkle proofs #1489

feat(electrum)!: Update `bdk_electrum` to use merkle proofs #1489

LagginTimes commented Jun 25, 2024 •

edited

Loading

notmandatory commented Jun 25, 2024

LagginTimes commented Jun 26, 2024 •

edited

Loading

LLFourn left a comment

LLFourn Jun 27, 2024

LagginTimes Jun 27, 2024 •

edited

Loading

LLFourn Jun 28, 2024

tnull Jun 28, 2024 •

edited

Loading

notmandatory commented Jun 27, 2024

ValuedMammal left a comment

notmandatory commented Jul 8, 2024

evanlinjin commented Jul 9, 2024

evanlinjin commented Jul 9, 2024

ValuedMammal left a comment

ValuedMammal Jul 9, 2024

ValuedMammal Jul 9, 2024

ValuedMammal Jul 9, 2024

notmandatory left a comment

	.filter(\|(height, _)\| Some(*<&u32>::clone(height)) > agreement_height)
	.filter(\|(&height, _)\| Some(height) > agreement_height)

feat(electrum)!: Update bdk_electrum to use merkle proofs #1489

feat(electrum)!: Update bdk_electrum to use merkle proofs #1489

Conversation

LagginTimes commented Jun 25, 2024 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

notmandatory commented Jun 25, 2024

LagginTimes commented Jun 26, 2024 • edited Loading

LLFourn left a comment

Choose a reason for hiding this comment

LLFourn Jun 27, 2024

Choose a reason for hiding this comment

LagginTimes Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

LLFourn Jun 28, 2024

Choose a reason for hiding this comment

tnull Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

notmandatory commented Jun 27, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

notmandatory commented Jul 8, 2024

evanlinjin commented Jul 9, 2024

evanlinjin commented Jul 9, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

ValuedMammal Jul 9, 2024

Choose a reason for hiding this comment

ValuedMammal Jul 9, 2024

Choose a reason for hiding this comment

ValuedMammal Jul 9, 2024

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

feat(electrum)!: Update `bdk_electrum` to use merkle proofs #1489

feat(electrum)!: Update `bdk_electrum` to use merkle proofs #1489

LagginTimes commented Jun 25, 2024 •

edited

Loading

LagginTimes commented Jun 26, 2024 •

edited

Loading

LagginTimes Jun 27, 2024 •

edited

Loading

tnull Jun 28, 2024 •

edited

Loading