]> Untitled Git - bdk/commitdiff
Merge bitcoindevkit/bdk#1838: Make full-scan/sync flow easier to reason about.
author志宇 <hello@evanlinjin.me>
Fri, 28 Feb 2025 03:50:58 +0000 (14:50 +1100)
committer志宇 <hello@evanlinjin.me>
Fri, 28 Feb 2025 03:53:08 +0000 (14:53 +1100)
800f3580f8518e15ccaf9622fdd2e5141a50d5e5 feat!: Improve spk-based syncing flow (志宇)
ee527454b0dc451ea44fd9410db3e396cb48817f feat(core)!: Make `TxUpdate` non-exhaustive (志宇)

Pull request description:

  ### 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_at`s per tx. The other issue is if we merge `TxUpdate`s, 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_at`s 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_at`s 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:

  * [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:

  ~* [ ] I've added tests for the new feature~ No tests needed as it's more of a refactor.

  * [x] I've added docs for the new feature

ACKs for top commit:
  notmandatory:
    utACK 800f3580f8518e15ccaf9622fdd2e5141a50d5e5

Tree-SHA512: 85af8452ac60c4a8087962403fd10c5c67592d3711f7665ae09a2d9c48a868583a41e54f28d639e47bd264ccf95d9254efc8d0d6248c8bcc9343c8290502e061


Trivial merge