-
Notifications
You must be signed in to change notification settings - Fork 113
Use BDK events in update_payment_store
#658
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
base: main
Are you sure you want to change the base?
Use BDK events in update_payment_store
#658
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
75ff700 to
d3f7855
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
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.
Excuse the delay here!
Unfortunately I don't think we can make the move until we get corresponding functionality for all chain sources, i.e., also for bitcoind/apply_block. Will raise that with the BDK folks to make some progress.
I now opened bitcoindevkit/bdk_wallet#336 to add the missing APIs we need. In the meantime we can see to get this as close to being mergeable as possible.
src/wallet/mod.rs
Outdated
| })?; | ||
|
|
||
| self.update_payment_store(&mut *locked_wallet).map_err(|e| { | ||
| let events_vec: Vec<WalletEvent> = events.into_iter().collect(); |
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 don't think this re-allocation is necessary?
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.
Thanks! I have removed the re-allocation
| ); | ||
| } | ||
|
|
||
| match locked_wallet.apply_block(block, height) { |
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.
Ugh, seems there is no corresponding apply_block_events method. I think we need that before actually moving forward here. Will raise it with the BDK folks.
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.
Alright
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 saw that the PR on BDK wallet has been merged and added to the next release milestone. This will be updated as soon as there is a new release on BDK wallet
src/wallet/mod.rs
Outdated
| } | ||
|
|
||
| self.payment_store | ||
| .list_filter(|p| { |
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.
Hmm, ugh, that's already slow right now, but will be prohibitively slow when we don't keep our entire payment store in-memory. I think we can't get around adding another persisted lookup table that tracks RBF-Txid to original-Txid.
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.
Yes, this is true, if we have another table, the lookup will be faster and the original Txid can be updated when the RBF-Txid for example, has a confirmed event from BDK
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.
@tnull, I created a new persisted ReplacedTransactionStore that maintains lookups from any txid in an RBF chain to its associated payment. Store entries are automatically cleaned up when any transaction in the chain confirms. This keeps lookups fast even with large payment histories.
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.
@tnull, I created a new persisted
ReplacedTransactionStorethat maintains lookups from any txid in anRBFchain to its associated payment. Store entries are automatically cleaned up when any transaction in the chain confirms. This keeps lookups fast even with large payment histories.
Huh, why do we need a whole other module/store for this? Let's just use a HashMap<Txid, Txid> and be done with it? Or do we need all that additionally tracked data somehow?
I guess we could use a DataStore implementation for this, but I don't quite see why we need to track ConfirmationStatus and latest_update_timestamp?
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.
Huh, why do we need a whole other module/store for this? Let's just use a
HashMap<Txid, Txid>and be done with it? Or do we need all that additionally tracked data somehow?
Yes, we need to track this data for faster lookup of replaced transaction IDs instead of a full iteration
I guess we could use a
DataStoreimplementation for this, but I don't quite see why we need to trackConfirmationStatusandlatest_update_timestamp?
You are right about the ConfirmationStatus and latest_update_timestamp, they were part of my original design, and as I proceeded with my implementation, I decided to clean the store instead upon confirmation of any of the transactions. I will go ahead and remove them. I also used a DataStore implementation for this.
d3f7855 to
a2c8a55
Compare
…sactions Replace the full transaction list scan in `update_payment_store` with handling of BDK's `WalletEvent` stream during sync. This leverages the new events in BDK 2.2, reduces redundant work, and prepares the foundation for reliable RBF/CPFP tracking via `WalletEvent::TxReplaced`.
3332e4e to
ecaae51
Compare
Introduce a new lookup `ReplacedTransactionStore` that maps old/replaced transaction IDs to their current replacement transaction IDs, enabling reliable tracking of replaced transactions throughout the replacement chain. Key changes: - Add persisted storage for RBF replacement relationships - Link transactions in replacement trees using payment IDs - Remove entire replacement chains from persistence when any transaction in the tree is confirmed
ecaae51 to
8609d97
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
This PR updates
update_payment_storeto use BDK 2.2’sWalletEventstream during sync instead of iterating over the full list of wallet transactions every time. The new event-based approach reduces redundant work and ensures the payment store stays in sync with only the changes that actually occurred.It also sets up the foundation for RBF support in #628 with
WalletEvent::TxReplaced. Since #628 depends on this event handling, this PR should be merged first.This PR will also address #452