Skip to content

Conversation

@evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Feb 16, 2025

Description

#1811 introduces the evicted-at concept. While adding this to TxUpdate, I realized there were some shortcomings in our full-scan & sync flow:

  • Chain sources that use TxUpdate to convey updates cannot choose to add transactions without a seen_at timestamp as the TxGraph::apply_update_at logic adds this timestamp for all unanchored txs if the seen_at parameter is specified. Purposefully adding uncanonical txs is useful for wallets that want to know about replaced txs.
  • The TxGraph::apply_update_at logic is hard to reason about. TxUpdate::seen_ats already has the seen_at timestamp per tx, but apply_update_at() also takes in a seen_at timestamp`.
  • TxUpdate::seen_ats currently forces us to only have one seen-at per tx as it's a map of txid -> seen_at. However, in the future we want to add a first-seen timestamp to TxGraph which is basically the earliest seen-at timestamp introduced so we may want to have multiple seen_ats per tx. The other issue is if we merge TxUpdates, we will loose data. I.e. we can only keep the last_seen or the first_seen.

These problems were exacerbated when introducing evicted-at. In the old design, the chain-source has no concept of sync time (as sync time was introduced after-the-fact when applying the TxUpdate). Therefore the introduced TxUpdate::evicted field was a HashSet<Txid> (no timestamps) and relied on the TxGraph::apply_upate_at logic to introduce the evicted-at timestamp. All this makes the sync logic harder to understand. What happens if the seen_at input is None? What happens if updates were applied out of order? What happens when we merge TxUpdates before applying?

The following changes are made in this PR to simplify the sync/full-scan logic to be easier to understand and robust:

  • The sync_time is added to the SyncRequest and FullScanRequest. sync_time is mandatory and is added as an input of builder(). If the std feature is enabled, the builder_now() is available which uses the current timestamp. The chain source is now responsible to add this sync_time timestamp as seen_at for mempool txs. Non-canonical and non-anchored txs can be added without the seen_at timestamp.
  • TxUpdate::seen_ats is now a HashSet of (Txid, u64). This allows multiple seen_ats per tx. This is also a much easier to use API as the chain source can just insert into this HashSet without checking previous data.
  • TxGraph::apply_update_at is removed as we no longer needs to introduce a fallback seen_at timestamp after-the-fact. TxGraph::apply_update is no longer std-only and the logic of applying updates is greatly simplified.

Additionally, TxUpdate is updated to be #[non_exhaustive] for backwards-compatibility protection.

Notes to the reviewers

This is based on #1837. Merge that first.

These are breaking changes to bdk_core. It needs to be breaking to properly fix all the issues.

As mentioned by @notmandatory, bdk_wallet changes will be added to a new PR once the new bdk_wallet repo is ready. But the PR won't be merged until we're ready for a bdk_wallet 2.0.

Changelog notice

  • Change FullScanRequest::builder and SyncRequest::builder methods to depend on feature = "std". This is because requests now have a start_time, instead of specifying a seen_at when applying the update.
  • Add FullScanRequest::builder_at and SyncRequest::builder_at methods which are the non-std version of the ..Request::builder methods.
  • Change TxUpdate::seen_ats field to be a HashSet of (Txid, u64). This allows a single update to have multiple seen_ats per tx.
  • Change TxUpdate to be non-exhaustive.
  • Remove apply_update_at as we no longer need to apply with a timestamp after-the-fact.

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 No tests needed as it's more of a refactor.

  • I've added docs for the new feature

@evanlinjin evanlinjin added this to the 2.0.0 milestone Feb 16, 2025
@evanlinjin evanlinjin marked this pull request as ready for review February 16, 2025 07:47
@evanlinjin evanlinjin added the api A breaking API change label Feb 16, 2025
Copy link
Collaborator

@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 left a couple of documentation comments but this LGTM.

@evanlinjin evanlinjin self-assigned this Feb 16, 2025
@evanlinjin evanlinjin force-pushed the tx_update_cleanup branch 2 times, most recently from 64b4f14 to 6bb7afa Compare February 20, 2025 15:45
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Approach ACK

@evanlinjin evanlinjin force-pushed the tx_update_cleanup branch 5 times, most recently from abf2bc1 to 8682487 Compare February 27, 2025 04:07
Copy link
Collaborator

@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.

ACK 8682487

Copy link
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

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

ACK 8682487

Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

Nit: typo in commit message.

This may, if we introduce new fields to TxUdpate, they can be minor
non-breaking updates.

Otherwise LGTM.

@evanlinjin evanlinjin force-pushed the tx_update_cleanup branch 2 times, most recently from 68f668b to 61bcd12 Compare February 28, 2025 01:20
@evanlinjin
Copy link
Member Author

Rebased on #1860 and removed the changes to bdk_wallet.

If we introduce new fields to `TxUpdate`, they can be minor
non-breaking updates.
* Change `TxUpdate::seen_ats` to be a `HashSet<(Txid, u64)>` so we can
  introduce multiple timestamps per tx. This is useful to introduce both
  `first_seen` and `last_seen` timestamps to `TxGraph`. This is also a
  better API for chain-sources as they can just insert timestamps into
  the field without checking previous values.
* Change sync/full-scan flow to have the request structure introduce the
  sync time instead of introducing the timestamp when apply the
  `TxUpdate`. This simplifies the `apply_update{_at}` logic and makes
  `evicted_at` easier to reason about (in the future).
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

utACK 800f358

Note to other reviewers: bdk_wallet changes will be added to a new PR once the new bdk_wallet repo is ready. But the PR won't be merged until we're ready for a bdk_wallet 2.0.

@evanlinjin evanlinjin merged commit b26ff89 into bitcoindevkit:master Feb 28, 2025
23 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Feb 28, 2025
@ValuedMammal ValuedMammal mentioned this pull request Apr 3, 2025
41 tasks
@ValuedMammal ValuedMammal mentioned this pull request Apr 30, 2025
39 tasks
notmandatory added a commit to bitcoindevkit/bdk_wallet that referenced this pull request May 8, 2025
e125014 feat(examples): Change `example_wallet_rpc` to detect evictions. (志宇)
12b00e0 feat(wallet): Add method `apply_evicted_txs` (valued mammal)
a247215 deps!: update bdk_chain to 0.22.0 (valued mammal)
f882945 ci: Unpin some dependencies (valued mammal)

Pull request description:

  This PR updates `bdk_chain` dependency to the latest version 0.22.0

  ### Notes to the reviewers

  This PR doesn't make use of `CanonicalizationParams` for any advanced use cases - instead we rely on the default params for all tx-graph queries.

  fixes #202
  fixes #224

  ### Changelog notice

  - deps: bump `bdk_chain` to 0.22.0
  - deps: bump `bdk_file_store` to 0.20.0

  #### Added

  - Added `start_sync_with_revealed_spks_at`
  - Added `start_full_scan_at`
  - Added `apply_evicted_txs`

  #### Changed

  - `start_sync_with_revealed_spks` is feature gated by "std", as it uses the system time to provide the time of the sync and also the tx last-seen.
  - `apply_update` no longer requires std, as the timestamp is provided in the sync / full scan request
  - `start_full_scan` is also gated behind "std"
  - `start_sync_*` now includes the expected SPK history (via `list_expected_spk_txids`)

  #### Removed

  - Removed `apply_update_at`

  #### BREAKING

  - `bdk_core::TxUpdate` is non-exhaustive bitcoindevkit/bdk#1838
  - For the optional file_store feature, the Load variant of the `FileStoreError` is changed to hold a new inner type bitcoindevkit/bdk#1684

  #### Changes to persisted data

  The `tx_graph::ChangeSet` gained a new default-able field `last_evicted` to persist the fact that a tx is no longer part of the canonical chain.

  ### 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
  * [x] I've added docs for the new feature
  * [x] This pull request breaks the existng API
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK e125014

Tree-SHA512: 91f77e4268aac0aaee2503a3fdca13754fc291159b9f37b0997be8c011b02b8ac16b4b6a7edbbb731005c83b37d438666fb803dbe18a6b9496b11f7e3e1ab155
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants