Skip to content

bdk_electrum: Handle negative heights properly#1837

Merged
ValuedMammal merged 2 commits intobitcoindevkit:masterfrom
evanlinjin:fix/electrum_negative_height
Feb 20, 2025
Merged

bdk_electrum: Handle negative heights properly#1837
ValuedMammal merged 2 commits intobitcoindevkit:masterfrom
evanlinjin:fix/electrum_negative_height

Conversation

@evanlinjin
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin commented Feb 16, 2025

Description

This fixes a problem with bdk_electrum making bogus transaction_get_merkle calls to the Electrum server.

In electrum, heights returned alongside txs may be 0 or -1. 0 means the tx is unconfirmed. -1 means the tx is unconfirmed and spends an unconfirmed tx.

Previously, the codebase assumed that heights cannot be negative and used a i32 as usize cast (which would lead to the casted height being 18446744073709551615). Then the codebase will try to call transaction_get_merkle with the overflowed height.

Notes to the reviewers

The test introduced in this PR does not fail with the old codebase. What ends up happening is that transaction_get_merke will be called with the overflow height and the Electrum server will return with "merkle not found".

To see the failure, you can change the height as usize casts to use .try_into().expect() then you will see a panic.

These changes should be applied as 1.0.1 and 1.1.1 fixes.

Changelog notice

  • Fix bdk_electrum handling of negative spk-history height.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • I've added tests to reproduce the issue which are now passing

* [ ] This pull request breaks the existing API
* [ ] I'm linking the issue being fixed by this PR

@evanlinjin evanlinjin force-pushed the fix/electrum_negative_height branch from d300ba4 to ada073b Compare February 16, 2025 05:49
@evanlinjin evanlinjin marked this pull request as ready for review February 16, 2025 06:01
@evanlinjin evanlinjin self-assigned this Feb 16, 2025
@evanlinjin evanlinjin added the bug Something isn't working label Feb 16, 2025
@evanlinjin evanlinjin added this to the 2.0.0 milestone Feb 16, 2025
Copy link
Copy Markdown
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.

Nice work. Especially with the test.

Copy link
Copy Markdown
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.

ACK 2afdae5

@evanlinjin evanlinjin force-pushed the fix/electrum_negative_height branch from 2afdae5 to 85f97c7 Compare February 20, 2025 13:13
In electrum, heights returned alongside txs may be 0 or -1. 0 means the
tx is unconfirmed. -1 means the tx is unconfirmed and spends an
unconfirmed tx.

Previously, the codebase assumed that heights cannot be negative and
used a `i32 as usize` cast (which would lead to panic if the i32 is
negative).
Is an spk history contains a tx that spends another unconfirmed tx, the
Electrum API will return the tx with a negative height. This should
succeed and not panic.
@evanlinjin evanlinjin force-pushed the fix/electrum_negative_height branch from 85f97c7 to 161d715 Compare February 20, 2025 14:03
Copy link
Copy Markdown
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.

ACK 161d715

@ValuedMammal ValuedMammal merged commit ad21685 into bitcoindevkit:master Feb 20, 2025
23 checks passed
@notmandatory notmandatory modified the milestones: 2.0.0, 1.2.0 Feb 28, 2025
evanlinjin added a commit that referenced this pull request Feb 28, 2025
800f358 feat!: Improve spk-based syncing flow (志宇)
ee52745 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 800f358

Tree-SHA512: 85af8452ac60c4a8087962403fd10c5c67592d3711f7665ae09a2d9c48a868583a41e54f28d639e47bd264ccf95d9254efc8d0d6248c8bcc9343c8290502e061
@ValuedMammal ValuedMammal moved this to Done in BDK Chain Mar 10, 2025
@ValuedMammal ValuedMammal mentioned this pull request Apr 3, 2025
41 tasks
@ValuedMammal ValuedMammal mentioned this pull request Apr 30, 2025
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants