`LocalChain` with hardwired genesis block by evanlinjin · Pull Request #1178 · bitcoindevkit/bdk

evanlinjin · 2023-10-15T16:10:32Z

closes #1079
closes #1107

Description

Many methods of TxGraph require a chain_tip: BlockId input to use against a ChainOracle implementation. This is used to ask the ChainOracle implementation whether a certain block exists in the chain identified by the chain_tip. This guarantees that the TxGraph methods will return a consistent history of transactions.

However, the ChainOracle trait's get_chain_tip method returns an option of BlockId. It becomes unclear what to do when get_chain_tip returns None.

This PR changes the ChainOracle::get_chain_tip method to always return a BlockId (no Option). LocalChain now hardwires the genesis block in order to implement ChainOracle.

bdk::Wallet and bdk_file_store::Store are changed to have separate constructor methods for initializing a fresh instance and recovering a previous instance from persistence.

Notes to the reviewers

Changelog notice

Changed ChainOracle::get_chain_tip method to return a BlockId instead of an Option of a BlockId.
Refactored LocalChain so that the genesis BlockId is hardwired. This way, the ChainOracle::get_chain_tip implementation can always return a tip.
Add is_empty method to PersistBackend. This returns true when there is no data in the persistence.
Changed Wallet::new to initialize a fresh wallet only.
Added Wallet::load to restore an instance of a wallet.
Replaced Store::new with separate methods to create/open the database file.

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

crates/bdk/README.md

crates/bdk/src/wallet/mod.rs

crates/chain/src/genesis.rs

crates/chain/src/persist.rs

crates/bdk/src/wallet/mod.rs

LLFourn

ACK 632b806

vladimirfomene · 2023-11-06T12:58:57Z

crates/bitcoind_rpc/src/lib.rs

-    ///
-    /// `start_height` is the block height to start emitting blocks from.
-    pub fn from_height(client: &'c C, start_height: u32) -> Self {
+    /// TODO


I wonder what we can do so that we don't forget to write this doc.

Looks like docs here should be something like:

Suggested change

/// TODO

/// Construct a new [`Emitter`] with the given RPC `client`, `last_cp` and `start_height`.

///

/// `last_cp` is the check point used to find the latest block which is still part of the best chain.

/// `start_height` is the block height to start emitting blocks from.

vladimirfomene

Great work Evan! I just have some nits.

vladimirfomene · 2023-11-07T07:08:41Z

crates/bdk/src/wallet/mod.rs

 impl<D> Wallet<D> {
-    /// Create a wallet from a `descriptor` (and an optional `change_descriptor`) and load related
-    /// transaction data from `db`.
+    /// Initialize an empty [`Wallet`].


What does empty here mean? From my understanding, it means the wallet has an empty state (empty IndexedTxGraph, empty localchain). We might want to improve the doc here.

vladimirfomene · 2023-11-07T07:26:50Z

crates/bdk/src/wallet/mod.rs

+        Self::load_from_changeset(descriptor, change_descriptor, db, changeset)
+    }
+
+    fn load_from_changeset<E: IntoWalletDescriptor>(


nits: I will change the name of this method to init_from_changeset. Feel free to ignore if it doesn't resonate.

vladimirfomene · 2023-11-07T07:59:02Z

crates/file_store/src/store.rs

+    C: Append + serde::Serialize + serde::de::DeserializeOwned,
 {
-    /// Creates a new store from a [`File`].
+    /// Create a new [`Store`] file in write-only mode; error if the file exists.


Is it really in write only mode given that the file is also given read option on line 65

notmandatory

ACK e342a8c

Looks good, nice to have that chain tip Option removed!

notmandatory · 2023-11-13T01:16:59Z

crates/bitcoind_rpc/src/lib.rs

-    ///
-    /// `start_height` is the block height to start emitting blocks from.
-    pub fn from_height(client: &'c C, start_height: u32) -> Self {
+    /// TODO


Looks like docs here should be something like:

Suggested change

/// TODO

/// Construct a new [`Emitter`] with the given RPC `client`, `last_cp` and `start_height`.

///

/// `last_cp` is the check point used to find the latest block which is still part of the best chain.

/// `start_height` is the block height to start emitting blocks from.

notmandatory · 2023-11-13T16:54:25Z

This should also close #1107 .

notmandatory · 2023-11-15T22:07:23Z

@evanlinjin looks like this one has a conflict so needs a rebase before I can merge it.

This ensures that `LocalChain` will always have a tip. The `ChainOracle` trait's `get_chain_tip` method no longer needs to return an option.

`Wallet::new` now creates a new wallet. `Wallet::load` loads an existing wallet. The network type is now recoverable from persistence. Error types have been simplified.

These methods try to load wallet from persistence and initializes the wallet instead if non-existant. An internal helper method `create_signers` is added to reuse code. Documentation is also improved.

`PersistBackend::is_empty` is removed. Instead, `load_from_persistence` returns an option of the changeset. `None` means persistence is empty. This is a better API than a separate method. We can now differentiate between a persisted single changeset and nothing persisted. `Store::aggregate_changeset` is refactored to return a `Result` instead of a tuple. A new error type (`AggregateChangesetsError`) is introduced to include the partially-aggregated changeset in the error. This is a more idiomatic API.

evanlinjin · 2023-11-15T23:17:00Z

@notmandatory don't merge yet, I want to address comments and make clippy happier.

LLFourn reviewed Oct 15, 2023

View reviewed changes

crates/bdk/README.md Outdated Show resolved Hide resolved

LLFourn reviewed Oct 16, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory reviewed Oct 16, 2023

View reviewed changes

crates/chain/src/genesis.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the issue/1079 branch from 6dadc40 to 01ee8c3 Compare October 16, 2023 14:10

nondiremanuel added this to the 1.0.0-alpha.3 milestone Oct 22, 2023

nondiremanuel mentioned this pull request Oct 22, 2023

W43 Lib Team Call bitcoindevkit/.github#83

Closed

33 tasks

evanlinjin self-assigned this Oct 23, 2023

evanlinjin force-pushed the issue/1079 branch 6 times, most recently from 7c6164d to eb39808 Compare October 27, 2023 07:31

evanlinjin changed the title ~~WIP: LocalChain with hardwired genesis block~~ LocalChain with hardwired genesis block Oct 27, 2023

This comment was marked as resolved.

Sign in to view

evanlinjin force-pushed the issue/1079 branch from 97c78e7 to 7e0cd98 Compare October 30, 2023 22:48

evanlinjin marked this pull request as ready for review October 31, 2023 02:09

evanlinjin requested review from LLFourn and notmandatory October 31, 2023 03:00

LLFourn reviewed Oct 31, 2023

View reviewed changes

crates/chain/src/persist.rs Outdated Show resolved Hide resolved

evanlinjin requested a review from LLFourn November 1, 2023 02:41

LLFourn reviewed Nov 2, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved

LLFourn approved these changes Nov 2, 2023

View reviewed changes

evanlinjin mentioned this pull request Apr 7, 2025

Wallet's persistence should have a fixed "header" section bitcoindevkit/bdk_wallet#125

Open

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

evanlinjin force-pushed the issue/1079 branch from 632b806 to 63e06fd Compare November 6, 2023 03:39

vladimirfomene reviewed Nov 6, 2023

View reviewed changes

vladimirfomene reviewed Nov 7, 2023

View reviewed changes

vladimirfomene mentioned this pull request Nov 7, 2023

Syncing/Rescanning super issue #1195

Closed

notmandatory mentioned this pull request Nov 12, 2023

Add wallet sync_request and full_scan_request functions #1194

Closed

8 tasks

notmandatory approved these changes Nov 13, 2023

View reviewed changes

notmandatory added module-blockchain module-wallet labels Nov 13, 2023

evanlinjin added 4 commits November 16, 2023 06:41

feat!: LocalChain with hardwired genesis checkpoint

5998a22

This ensures that `LocalChain` will always have a tip. The `ChainOracle` trait's `get_chain_tip` method no longer needs to return an option.

feat(chain, file_store): add is_empty method to PersistBackend trait

7d5f31f

feat(bdk)!: have separate methods for creating and loading Wallet

6cf3963

`Wallet::new` now creates a new wallet. `Wallet::load` loads an existing wallet. The network type is now recoverable from persistence. Error types have been simplified.

chore(file_store): rm empty test file

7c6cbc4

evanlinjin force-pushed the issue/1079 branch from e342a8c to b216585 Compare November 15, 2023 23:06

evanlinjin added 7 commits November 16, 2023 07:07

feat(wallet)!: add new_or_load methods

d294e2e

These methods try to load wallet from persistence and initializes the wallet instead if non-existant. An internal helper method `create_signers` is added to reuse code. Documentation is also improved.

feat(file_store)!: have separate methods for creating and opening Store

24994a3

chore(examples): use Wallet::new_or_load method where appropriate

1886dc4

test(file_store): add construction method tests

96f1d94

test(bdk): add tests for wallet constructor methods

c3265e2

feat(bdk): changeset's Append impl checks that network is consistent

79b84be

evanlinjin force-pushed the issue/1079 branch from b216585 to 79b84be Compare November 15, 2023 23:07

evanlinjin added 2 commits November 16, 2023 07:17

chore: make clippy happy

9a250ba

docs(bitcoind_rpc): update docs for Emitter::new

f1b112e

notmandatory merged commit bc8d6a3 into bitcoindevkit:master Nov 16, 2023

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

ValuedMammal mentioned this pull request Nov 20, 2023

fix: more typos #1223

Closed

8 tasks

notmandatory mentioned this pull request Jan 3, 2024

Release 1.0.0-alpha.3 #1254

Closed

12 tasks

Conversation

evanlinjin commented Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirfomene Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirfomene Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

vladimirfomene Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

vladimirfomene Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Nov 13, 2023

Uh oh!

notmandatory commented Nov 15, 2023

Uh oh!

evanlinjin commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

evanlinjin commented Oct 15, 2023 •

edited

Loading