bdk icon indicating copy to clipboard operation
bdk copied to clipboard

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

Open LagginTimes opened this issue 1 year ago • 3 comments

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

#1416 should be merged before this PR, as it will fix a problem with Wallet::insert_tx. An expect statement is used in Wallet::insert_tx in this PR to serve as a temporary workaround.

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:

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

New Features:

  • [ ] I've added tests for the new feature
  • [x] I've added docs for the new feature

LagginTimes avatar Jun 25 '24 11:06 LagginTimes

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

notmandatory avatar Jun 25 '24 19:06 notmandatory

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

I'm okay with this!

LagginTimes avatar Jun 26 '24 16:06 LagginTimes

@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.

notmandatory avatar Jun 27 '24 15:06 notmandatory

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

notmandatory avatar Jul 08 '24 02:07 notmandatory

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 avatar Jul 09 '24 12:07 evanlinjin

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

evanlinjin avatar Jul 09 '24 12:07 evanlinjin