Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

Add a electrum syncing example #77

Merged
merged 5 commits into from
Jan 6, 2023

Conversation

rajarshimaitra
Copy link
Collaborator

This PR adds an electrum syncing example.. Fixes #72

The electrum sync is done in two steps.

  • wallet_txid_scan returns a candidate KeychainScan with TxGraph data.
  • The candidate is compared with tracker's SparseChain, and list of new tx data to fetch is derived from the ChangeSet.
  • New transactions are fetched in batch and added into the KeychainScan update.
  • Apply the final KeychainScan on tracker and DB.

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #77 (6051596) into master (fbc88da) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #77   +/-   ##
=======================================
  Coverage   59.39%   59.39%           
=======================================
  Files           8        8           
  Lines         266      266           
=======================================
  Hits          158      158           
  Misses        108      108           
Impacted Files Coverage Δ
bdk_chain/src/chain_graph.rs 0.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rajarshimaitra rajarshimaitra force-pushed the electrum-example branch 3 times, most recently from ea42c4b to 3542c3d Compare December 7, 2022 11:37
@rajarshimaitra
Copy link
Collaborator Author

Restructured the commit to handle #72 requirements.

  • Make Chaingraph consistent, by adding few assertions at the apply_update() API.
  • Make the electrum sync in two steps. Get all the txids, find new ones, fetch tx data, apply update..

@rajarshimaitra
Copy link
Collaborator Author

Me and @evanlinjin tend to disagree on the approach of the consistency assertion.. While to me it seems the best place to apply it is the update API, Evan thinks there could be a better approach.. Opening this up for discussion and reviews..

@rajarshimaitra rajarshimaitra force-pushed the electrum-example branch 3 times, most recently from 03b7d94 to 605abf8 Compare December 11, 2022 14:37
@rajarshimaitra
Copy link
Collaborator Author

Rebased on current master, and updated the example syncing with recent invariants around ChainGraph. Similar to RPC syncing, the first stage returns a SparseChain. The nex to_fetch_tx id set is derived using determine_changeset between the returned sparsechain and existing sparaechain within the tracker..

@rajarshimaitra rajarshimaitra mentioned this pull request Dec 11, 2022
Copy link
Collaborator

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Good work so far!

bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Collaborator Author

rajarshimaitra commented Dec 14, 2022

Updated with the above suggestions.

  • No default reorg depth mechanism. Takes in a ref of existing chain, and inserts checkpoint until the last common check point is found. And then push the current tip into update. In case of reorg, all checkpoints after the last common block is added into the update structure, and while SparseChain::apply_update new checkpoints will replace old checkpoints. No error is thrown.

  • No default stop gap.

  • A new function added in the main to collect all the common code between Scan and Sync.

  • Transaction fetching is done opportunistically, for only txids that we haven't seen before. For transactions that only changed in Index tx data is fetched from existing TxGraph of the tracker.

Q: What to do when we only have TxNode::Partial in graph, and index changes of that transaction? 2 possible options.

  • We can fetch such transactions again from the blockchain before applying update.
  • Have mechanism to insert_tx() with TXNode::Partial

@LLFourn
Copy link
Owner

LLFourn commented Dec 14, 2022

Q: What to do when we only have TxNode::Partial in graph, and index changes of that transaction? 2 possible options.

* We can fetch such transactions again from the blockchain before applying update.

* Have mechanism to `insert_tx()` with `TXNode::Partial`

Haven't reviewed just comment: we don't keep the index for TxNode::Partial.

@rajarshimaitra rajarshimaitra force-pushed the electrum-example branch 2 times, most recently from ba92c75 to 056979e Compare December 14, 2022 10:33
@rajarshimaitra
Copy link
Collaborator Author

Haven't reviewed just comment: we don't keep the index for TxNode::Partial.

Done..

Copy link
Owner

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

Thanks @rajarshimaitra. Looks great. A few minor requests to finish this off. The main thing that's missing is to protect against re-org during sync.

bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Collaborator Author

It seems like there is still a bit of duplicate code. Isn't it the case that everything after getting the sparse chain and applying the last active indexes will be the same?

Yes.. I tried to add this into a separate function but that's making the code more complicated, because many trait bounds needs to be satisfied for the KeychainStore::append_changeset(), which are handled in bdk_cli.. It still could be done, but after doing it it made the code more complex than it needs to be, and thought would be detriment for readers reading this as example, and its only removing 3 lines. So kept it as it is for now..

bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
Copy link
Owner

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

I just took another pass at this. The list of things I've proposed to fix here is final now I think.

bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
@rajarshimaitra rajarshimaitra force-pushed the electrum-example branch 2 times, most recently from 837b0e7 to e582098 Compare December 22, 2022 08:15
@rajarshimaitra
Copy link
Collaborator Author

Thanks @LLFourn for the comments.. Updated as suggested and used the new API.. modulo #77 (comment).. Code looks much simpler..

@rajarshimaitra rajarshimaitra force-pushed the electrum-example branch 2 times, most recently from 42e151d to b60d4e1 Compare December 22, 2022 09:03
Copy link
Owner

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

Look great @rajarshimaitra. Main things:

bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Collaborator Author

@LLFourn updated with the suggestions, module #77 (comment).

Let me know if this looks good and you want the the commit history squashed..

@rajarshimaitra
Copy link
Collaborator Author

Sqaushed and rebased on master..

@LLFourn
Copy link
Owner

LLFourn commented Jan 5, 2023

I just did some work to get this working:

  • break at the right time in the loop for stop gap
  • add signet electrum server
  • print out scnning progress
  • Make client configurable
  • Stop returning the sparse chain from inflate_changeset because it
    makes it harder to ? the error and you can just clone it if you still
    need it on failure.

Check it out @rajarshimaitra.

This is working and probably ready to merge with final review from @evanlinjin.

rajarshimaitra and others added 4 commits January 6, 2023 10:53
- break at the right time in the loop
- add signet electrum server
- print out syncing progress
- Make client configurable
- Stop returning the sparse chain from inflate_changeset because it
  makes it harder to ? the error and you can just clone it if you still
  need it on failure.
@rajarshimaitra
Copy link
Collaborator Author

ACK 3531907

Thanks, @LLFourn for the fixes.. Code looks much better..

Added a nit fix..

Testing out the full thing..

@evanlinjin evanlinjin mentioned this pull request Jan 6, 2023
@evanlinjin
Copy link
Collaborator

@rajarshimaitra You forced-pushed away the changes...

@rajarshimaitra
Copy link
Collaborator Author

Holy shit..

@evanlinjin
Copy link
Collaborator

@rajarshimaitra Fixed.

@rajarshimaitra
Copy link
Collaborator Author

Sorry dude.. Intended to just push a nit.. Fat fingered..

Copy link
Collaborator

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 3531907

Just some nits

bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Show resolved Hide resolved
bdk_electrum_example/src/electrum.rs Outdated Show resolved Hide resolved
bdk_electrum_example/src/main.rs Show resolved Hide resolved
* Remove unnecessary code
* Check `index` is greater than `last_active_index` before changing
  `last_active_index`
@evanlinjin evanlinjin merged commit b271db7 into LLFourn:master Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChainGraph API for electrum
4 participants