bdk_electrum: Handle negative heights properly#1837
Merged
ValuedMammal merged 2 commits intobitcoindevkit:masterfrom Feb 20, 2025
Merged
bdk_electrum: Handle negative heights properly#1837ValuedMammal merged 2 commits intobitcoindevkit:masterfrom
bdk_electrum: Handle negative heights properly#1837ValuedMammal merged 2 commits intobitcoindevkit:masterfrom
Conversation
d300ba4 to
ada073b
Compare
4 tasks
LLFourn
approved these changes
Feb 16, 2025
Collaborator
LLFourn
left a comment
There was a problem hiding this comment.
Nice work. Especially with the test.
2afdae5 to
85f97c7
Compare
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.
85f97c7 to
161d715
Compare
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
39 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This fixes a problem with
bdk_electrummaking bogustransaction_get_merklecalls 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 usizecast (which would lead to the casted height being 18446744073709551615). Then the codebase will try to calltransaction_get_merklewith 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_merkewill 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 usizecasts to use.try_into().expect()then you will see a panic.These changes should be applied as
1.0.1and1.1.1fixes.Changelog notice
bdk_electrumhandling of negative spk-history height.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes:
* [ ] This pull request breaks the existing API* [ ] I'm linking the issue being fixed by this PR