-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce block-by-block API to bdk::Wallet
and add RPC wallet example
#1172
Introduce block-by-block API to bdk::Wallet
and add RPC wallet example
#1172
Conversation
2dc9575
to
ef8a2ab
Compare
@evanlinjin apology for the delay on this one. I'm doing all necessary changes today. |
ff5ecca
to
45ddedc
Compare
45ddedc
to
3b6b8e2
Compare
Is the intention to have the user supply their own descriptor? Or are we hardcoding the descriptor? |
I think we should hardcode the descriptor as we do in other wallet example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did successfully run the regtest example, and after modifying wallet_rpc/src/main.rs
to correctly parse the arg vector, I ran the example on both testnet and signet. I had to play with setting an appropriate fallback height to find the wallet txs, and on one occasion the program sat hanging while applying blocks, but I didn't investigate why. I wonder if this example might also be adapted to run on regtest, but that can be saved for future work
3b6b8e2
to
7a24c1a
Compare
@ValuedMammal, I have added a note for other reviewers to check if the program hangs while applying blocks. |
a0e45bc
to
d97875b
Compare
adds Wallet methods `set_lookahead_for_all`, `apply_block_relevant`, `batch_insert_relevant_unconfirmed`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK d97875b
4fbfabb
to
fad6f94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the wallet use a default lookahead limit other than 0
. I expect it would be quite surprising to the user of the high-level Wallet
interface to see that he has to explicitly call set_lookahead
on the wallet for it to be able to notice a utxo received on next_deriv_index + 1.
Doesn't need to be in this PR, but wanted to raise it here since it's tightly related to the functionality introduced.
I think 1_000
would be a good default. This is what the Bitcoin Core wallet uses.
EDIT: this can actually be tackled in parallel of this PR. I've done so here: #1229.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM but I'd like to ask whether the changes to the file store actually result in a significant performance improvement? I think the issue is a design bug in the bincode API which is fixed in the v2 release which returns the amount of data read from the reader. Maybe we can just wait for that instead of making this more complicated.
@LLFourn wrapping the file in For the counting reader, we still need to back track on any type of error. To backtrack, we still need the position before the read. It's either we seek to get the position, or we store the position. Side note, maybe |
f9d07fa
to
db58b8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall this is good. It's nice to see everything come together in wallet_rpc
in an elegant fashion, can confirm the examples work as intended.
In b486ecc 'feat(wallet): introduce block-by-block api', the commit description could be updated (still referring to methods process_block
, etc).
We should perhaps include more links to example crates in the workspace README (wallet_rpc
, example_esplora
, example_bitcoind_rpc
).
|
||
/// An error that may occur when applying a block to [`Wallet`]. | ||
#[derive(Debug)] | ||
pub enum ApplyBlockError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to use this for either apply_block
or apply_block_connected_to
?
/// This method takes in an iterator of `(tx, last_seen)` where `last_seen` is the timestamp of | ||
/// when the transaction was last seen in the mempool. This is used for conflict resolution | ||
/// when there is conflicting unconfirmed transactions. The transaction with the later | ||
/// `last_seen` is prioritied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: prioritied
} | ||
ApplyHeaderError::CannotConnect(err) => err, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this function return ApplyHeaderError
, or is mapping the err serving some greater purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should probably be called ApplyHeaderConnectedTo
error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to ignore: I found this example a bit difficult to follow. It might be better suited for a website tutorial, but if we decide to keep it, I made a few edits in terms of style ValuedMammal/bdk@0caeb79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, can you do a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, can you do a PR?
Onto this branch or on master?
crates/chain/src/local_chain.rs
Outdated
/// The `header` will be transformed into checkpoints - one for the current block and one for | ||
/// the previous block. Note that a genesis header will be transformed into only one checkpoint | ||
/// (as there are no previous blocks). The checkpoints will be applied to the chain via | ||
/// [`apply_update`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation doesn't tell us what the method is for and is written in the passive voice which makes it harder to read. It should start by saying that it's going to insert the block hash into the chain along with the previous block if it's not already there. The documentation shouldn't mention apply_update
-- the reader doesn't care and if we change this then the documentation will be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK db58b8a
These are convenience methods to transform a header into checkpoints to update the `LocalChain` with. Tests are included.
* methods `process_block` and `process_unconfirmed_txs` are added * amend stage method docs Co-authored-by: Vladimir Fomene <[email protected]> Co-authored-by: 志宇 <[email protected]>
Previously, emissions are purely blocks + the block height. This means emitted blocks can only connect to previous-adjacent blocks. Hence, sync must start from genesis and include every block.
Previously, `apply_block_relevant` used `batch_insert_relevant` which allows inserting non-topologically-ordered transactions. However, transactions from blocks are always ordered, so we can avoid looping through block transactions twice (as done in `batch_insert_relevant`). Additionally, `apply_block_relevant` now takes in a reference to a `Block` instead of consuming the `Block`. This makes sense as typically very few of the transactions in the block are inserted.
Co-authored-by: Vladimir Fomene <[email protected]> Co-authored-by: 志宇 <[email protected]>
db58b8a
to
8ec65f0
Compare
self-ACK: a4f28c0 I added one commit to improve the documentation of |
I would like to use this :) |
Me too! cc. @kcalvinalvin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a4f28c0
51bd01b fix(file_store): recover file offset after read (志宇) 66dc34e refactor(file_store): Use BufReader but simplify (LLFourn) c871764 test(file_store): `last_write_is_short` (志宇) a3aa8b6 feat(file_store)!: optimize `EntryIter` by reducing syscalls (志宇) Pull request description: ### Description `EntryIter` performance is improved by reducing syscalls. The underlying file reader is wrapped with `BufReader` (to reduce calls to `read` and `seek`). Two new tests are introduced. One ensures correct behavior when the last changeset write is too short. The other ensures the next write position is correct after a short read. ### Notes to the reviewers This is extracted from #1172 as suggested by #1172 (review). ### Changelog notice Changed * `EntryIter` performance is improved by reducing syscalls. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: LLFourn: ACK 51bd01b Tree-SHA512: 9c25f9f2032cb2d551f3fe4ac62b856ceeb69a388f037b34674af366c55629a2eaa2b90b1ae4fbd425415ea8d02f44493a6c643b4b1a57f4507e87aa7ade3736
Description
Introduce block-by-block API for
bdk::Wallet
. Awallet_rpc
example is added to demonstrate syncingbdk::Wallet
with thebdk_bitcoind_rpc
chain-source crate.The API of
bdk_bitcoind_rpc::Emitter
is changed so the receiver knows how to connect to the block emitted.Notes to the reviewers
Changelog notice
Added
Wallet
methods to apply full blocks (apply_block
andapply_block_connected_to
) and a method to apply a batch of unconfirmed transactions (apply_unconfirmed_txs
).CheckPoint::from_block_ids
convenience method.LocalChain
methods to apply a block header (apply_header
andapply_header_connected_to
).LocalChain
can apply updates that are shorter than original. This will happen during reorgs if we sync wallet withbdk_bitcoind_rpc::Emitter
.Fixed
InsertTxError
now implementsstd::error::Error
.All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: