Return wallet events when applying updates and blocks (3.0 milestone)

[notmandatory]

Description

Cherry-pick of commits from #310 and #336.

Notes to the reviewers

See #319 (comment) for proposed follow-up work.

Changelog notice

No changes from release/2.x branch.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• This pull request breaks the existing API

[ValuedMammal]

Hey @notmandatory thanks for your work on this! I left some comments related to code quality. As always, nits are non-blocking.

[ValuedMammal]

Any reason not to apply the update and use the resulting changeset to produce a summary of what changed in the Wallet?

[notmandatory]

The resulting Wallet::ChangeSet only contains the new blocks, tx, and anchors found after applying the sync update but don't show how this new data changes the canonical status of existing tx.

[notmandatory]

I added this as an "Option 4" to the ADR 0003.

[ValuedMammal]

We make a logical assumption that the information in the events matches the data that is persisted via the ChangeSet, so my thinking is that an adequate test for this feature should include checking that the events do in fact agree with the newly staged changes. I may look into adding a test for it.

[ValuedMammal]

I think it would be helpful to know which blocks were connected and/or disconnected.

[notmandatory]

Do you mean a list of connected/disconnected block ids? I didn't add any other info here because @tnull didn't need it for LDK's use case. Do you have some use or user in mind for this extra info?

[ValuedMammal]

Yeah I mean the block IDs. It's just a more complete way of describing how the chain tip changed.

[notmandatory]

@ValuedMammal are you thinking of something like adding connected and disconnected fields to this variant:

    /// The latest chain tip known to the wallet changed.
    ChainTipChanged {
        /// Previous chain tip.
        old_tip: BlockId,
        /// New chain tip.
        new_tip: BlockId,
        /// Newly connected blocks.
        connected: Vec<BlockId>,
        /// Newly disconnected blocks.
        disconnected: Vec<BlockId>,
    },

I'm still not clear on how this info would be used by a wallet user since it's more of an internal detail.

[ValuedMammal]

I've seen a similar thing used in LDK, for example Cache and Listen, but don't know if it's also applicable in this context.

[ValuedMammal]

nit: Please use a for loop for this.

[notmandatory]

I don't mind using a for loop, but just curious what the benefit is, is it just easier to read?

[notmandatory]

Claude says the main reasons to use for instead of for_each are:

1. early termination, ie. can call break in a for loop
2. using mutable variable inside the for loop
3. readability

We don't need 1 or 2 in this code and 3 is debatable so I'm going to stick with the for_each unless anyone has another reason to use for. The performance should be identical.

[coveralls]

Pull Request Test Coverage Report for Build 18893642494

Details

• 158 of 161 (98.14%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 85.16%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/wallet/event.rs	79	82	96.34%

Totals
Change from base Build 18891447990:	0.2%
Covered Lines:	7110
Relevant Lines:	8349

---

💛 - Coveralls

[notmandatory]

Addressed initial comments from @ValuedMammal and rebased on master branch.

Next steps:

• remove Wallet::apply_update_events and change Wallet::apply_update to return events instead
• investigate returning events from Wallet::apply_block
• add examples for getting affected UTXOs/addresses from tx related WalletEvents

[notmandatory]

I will cherry-pick and incorporate commits from #336 for apply_block_events and apply_block_connected_to_events when it's merged.

[notmandatory]

Finished cherry-picking from #336 and rebased on master branch.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.81%. Comparing base (de48edc) to head (54b7a3c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   86.30%   86.81%   +0.50%     
==========================================
  Files          24       25       +1     
  Lines        8346     8464     +118     
==========================================
+ Hits         7203     7348     +145     
+ Misses       1143     1116      -27     

Flag	Coverage Δ
rust	86.81% <100.00%> (+0.50%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[notmandatory]

Refactored to return events from original apply_update, apply_block* functions, removed *_events functions, see 66cedb4.

[notmandatory]

Added one more commit with some docs improvements and refactored out helper method based on suggestions from Claude LLM 🤖.

[notmandatory]

I haven't added an example for getting affected UTXOs/addresses from a Transaction related WalletEvents. This is a bit out of scope for this PR and should get it's own PR, either to add an example or add a helper function to Wallet to do it.

old

[notmandatory]

I haven't added an example for getting affected UTXOs/addresses from a Transaction related WalletEvents. This is a bit out of scope for this PR and should get it's own PR, either to add an example or add a helper function to Wallet to do it.

My thinking is to provide a new helper method Wallet::send_and_received_txouts to accomplish this. See: bitcoindevkit/bdk#2081

[ValuedMammal]

In 95e681a: f Duplicate event logic rather than business logic

If this is a fixup commit I guess it can be squashed into the previous one.

[ValuedMammal]

This is a bit out of scope for this PR and should get it's own PR, either to add an example or add a helper function to Wallet to do it.

On second thought isn't this precisely what the user is asking for in issue #6, suggesting we should have something like WalletEvent::UtxoSent, and WalletEvent::UtxoReceived?

[ValuedMammal]

I couldn't track down the original comment, but may be worth having a Wallet::apply_wallet_update that returns a clone of the newly staged ChangeSet for users who might want to generate events without enduring multiple rounds of tx-graph canonicalization.

[evanlinjin]

Thanks for working on this!

Since this is a 3.0 milestone, we will have CanonicalView available. I would store the latest CanonicalView as a field in Wallet and update it every time an update is applied.

Events can be obtained by diff-ing the new view against the old view.

impl CanonicalView {
    /// Not implemented.
    pub fn diff(other: &Self) -> Diff { todo!() }
}

If apply_update returns the old view, we can do this:

let old_view = wallet.apply_update(update)?;
let diff = old_view.diff(wallet.view() /* current view */);

Then diff can be treated as "events".

Here are some benefits to this:

• This feature will be available for callers that don't use bdk_wallet.
• You aren't forced to do the event processing (it's opt-in).
• Only canonicalize once (after applying update).

In other words, I propose implementing this in bdk_chain as a method on CanonicalView. Let me know what you think.

[evanlinjin]

I think it's useful to include the specific types of events that the users want emitted. This way, we can make a better judgement on how exactly we should implement it.

Edit: I see you have the event types listed in option 2, maybe it's best to include it in this section here?

[tnull]

In other words, I propose implementing this in bdk_chain as a method on CanonicalView. Let me know what you think.

Hmm, do I understand correctly that you propose to revert the API changes from #310, i.e., no longer expose WalletEvents? While surely doable, I want to note that that would mean a good bit of churn for us, as we just started migrating our internals to the APIs introduced in bdk_wallet v2.2 / v2.3.

[notmandatory]

Rebased this one on latest master.

[evanlinjin]

In other words, I propose implementing this in bdk_chain as a method on CanonicalView. Let me know what you think.

Hmm, do I understand correctly that you propose to revert the API changes from #310, i.e., no longer expose WalletEvents? While surely doable, I want to note that that would mean a good bit of churn for us, as we just started migrating our internals to the APIs introduced in bdk_wallet v2.2 / v2.3.

No. Essentially moving WalletEvents to bdk_chain (probably renaming it to something else). WalletEvents will be a diff between two CanonicalViews.

However, I did suggest some API changes - one benefit of my suggestion is you can choose exactly where and when you diff.

// Look you can apply multiple things and then diff later!
let old_view = wallet.apply_update(update1)?;
let _ = wallet.apply_update(update2)?;
// Look, we switch to a block-based chain source in the middle.
let _ = wallet.apply_block(&block, height)?;

// We diff now! Capturing the changes made by update1 + update2 + block.
let diff = old_view.diff(wallet.view() /* current view */);

Maybe we don't even need apply_x methods to return the old CanonicalView.

let old_view = wallet.view();
wallet.apply_update(update1)?;
wallet.apply_update(update2)?;
wallet.apply_block(&block, height)?;
let diff = old_view.diff(wallet.view());

I would argue this is probably even cleaner.

If the bdk_chain::CanonicalViewDiff thing departs too much from the current WalletEvent enum, we can have a helper method to convert it to WalletEvent.

[tnull]

No. Essentially moving WalletEvents to bdk_chain (probably renaming it to something else). WalletEvents will be a diff between two CanonicalViews.

Makes sense!

Maybe we don't even need apply_x methods to return the old CanonicalView.

Not sure about the apply_ methods per se, but I do agree that it might be easier to just make the wallet_events helper a public utility method (cf. #374). That would obviate the need for all the specific API variants that do return WalletEvents.

[notmandatory]

No. Essentially moving WalletEvents to bdk_chain (probably renaming it to something else). WalletEvents will be a diff between two CanonicalViews.

However, I did suggest some API changes - one benefit of my suggestion is you can choose exactly where and when you diff.
...

If the bdk_chain::CanonicalViewDiff thing departs too much from the current WalletEvent enum, we can have a helper method to convert it to WalletEvent.

I like the concept of giving Wallet users more control of when events are created and making the functionality available to bdk_chain users. Wouldn't the diff for let diff = old_view.diff(wallet.view()); be a Vec of events? Or if not couldn't what ever it is be converted into events?

My main concern is that this is making the wallet API and steps to get events more complicated for the simple/common case where a users only wanting events after applying an update or block. But I'll start taking a look at how to create my current events from two CanonicalViews.

One solution could be to keep the current apply_(update|block)_events APIs as is, and leave the plain apply_(update|block) functions to return nothing. Then add a Wallet function that returns the current CanonicalView and a function that generates events for the diff of two CanonicalViews.

[notmandatory]

I've rolled back this PR to only cherry-pick the commits from #310 and #336.

Now all this PR does is add from release/2.x branch:

• Wallet::apply_update_events
• Wallet::apply_block_events
• Wallet::apply_block_connected_to_events

After bdk 0.24 is released I can do follow-up PRs to:

• take advantage of the CanonicalView in the generation of events
• (maybe) rename WalletEvents to TransactionEvents and move it to bdk_chain with CanonicalView diff logic
• add Wallet::canonical_view function for users who want to manage snapshots and event generation themselves

[tnull]

Now all this PR does is add from release/2.x branch:

• Wallet::apply_update_events
• Wallet::apply_block_events
• Wallet::apply_block_connected_to_events

Hmm, if you're gonna keep the _events APIs, it would be great for consistency to add the variants for mempool-related updates (cf. #374). But, as mentioned there, I'd also be happy with the _events variants getting dropped if you prefer to just expose the helper method.