refactor(chain)!: remove min_confirmations parameter from CanonicalView::balance#2221
Draft
Dmenec wants to merge 8 commits into
Draft
refactor(chain)!: remove min_confirmations parameter from CanonicalView::balance#2221Dmenec wants to merge 8 commits into
min_confirmations parameter from CanonicalView::balance#2221Dmenec wants to merge 8 commits into
Conversation
It introduces the new `CanonicalizationTask` that's implements the canonicalization algorithm through a request/response pattern. Also, it introduces the new `ChainQuery` trait in `bdk_core`, which provides an interface for blockchain source/oracle query-based operations. Allowing sans-IO patterns for algorithm that needs a blockchain oracle, without the need for directly implement/handle I/O. Adds new API methods into `LocalChain`: `canonicalize` and `canonical_view`, adding same features as the existing `CanonicalIter` and it's APIs. Co-Authored-By: Claude <noreply@anthropic.com>
Updates the codebase to use the new convenience `LocalChain::canonical_view` method in order to generate the `CanonicalView`. Internally the convenience method follows the `sans-IO` approach, separating the canonicalization algorithm from i/o operations, and it's used as follows: 1. Create a new `CanonicalizationTask` with a `TxGraph`, by calling: `graph.canonicalization_task(params)` 2. Execute the canonicalization process with a chain oracle (e.g `LocalChain`, which implements `ChainOracle` trait), by calling: `chain.canonicalize(task, chain_tip)`
The codebase has been updated to the new `LocalChain::canonical_view` method. It's now safe to remove the `CanonicalIter` it's the old APIs relying on it, eg. `try_canonical_view`.
…`Canonical<A, P>` Separate concerns by splitting `CanonicalizationTask` into two phases: 1. `CanonicalTask` determines which transactions are canonical and why (`CanonicalReason`), outputting `CanonicalTxs<A>`. 2. `CanonicalViewTask` resolves reasons into `ChainPosition`s (confirmed vs unconfirmed), outputting `CanonicalView<A>`. Make `Canonical<A, P>`, `CanonicalTx<P>`, and `FullTxOut<P>` generic over the position type so the same structs serve both phases. Add `LocalChain::canonical_view()` convenience method for the common two-step pipeline. Renames: - `CanonicalizationTask` -> `CanonicalTask` - `CanonicalizationParams` -> `CanonicalParams` - `canonicalization_task()` -> `canonical_task()` - `FullTxOut::chain_position` -> `FullTxOut::pos` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…view_task` It moves the shared types: `CanonicalTx`, `Canonical`, `CanonicalView`, `CanonicalTxs` and other convenience methods into `canonical.rs`. Keep the phase-2 task (`CanonicalViewTask` in `canonical_view_task.rs`. Also, rename `FullTxOut` to `CanonicalTxOut`, and move it to `canonical.rs`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- add new `test_canonical_view_task.rs` to handle different scenarios of chain position resolution. - fixes the assumed canonical txs chain position resolution, especially for transitively assumed canonical transactions, where there's an anchored/confirmed descendant.
Co-authored-by: Dmenec <dmenec@proton.me>
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 builds on top of #2038.
Removes the
min_confirmationsparameter fromCanonicalView::balance.This is part of the redesign of the wallet
balanceAPI (see bitcoindevkit/bdk_wallet#451).Context
Trust Categorization
The goal is to fix how
balancedecides if a pending output is trusted or not. Right now the wallet decides this from the keychain (internal = trusted), which has some issues (bitcoindevkit/bdk_wallet#16, bitcoindevkit/bdk_wallet#273).The approach discussed in bitcoindevkit/bdk_wallet#431 describes creating the trust predicate by walking the tx ancestors. An unconfirmed output is trusted only if every unconfirmed ancestor spends inputs that are ours.
To achieve that, we need a few things together in chain:
CanonicalIterwith sans-IOCanonicalTask+ChainQuerytrait #2038 — the new CanonicalView, which lets us look up any tx by txid and get its canonical position.There is a small prototype test of this trust logic here: b0acd3b#diff-d3f18e9a77fd241fa351a0cbcd75e993d90819834ccde09b7f4247c153ff26eeR586-R607. It is just how the idea may work in wallet, not the final API.
Removing
min_confirmationsThe ancestor walk decides trust by checking each ancestor's canonical position. A
Confirmedancestor stops the walk, anUnconfirmedone is only trusted if all of its inputs are also ours. For this to be coherent,Confirmedhas to mean the same thing inbalanceas it does in the walk.On the previous approach the parameter
min_confirmationsbroke that. AConfirmedoutput below themin_confirmationsthreshold was reported aspending, even though the walk still saw it asConfirmed.Removing it,
balanceclassifies purely from the canonical position, andConfirmedmeans the same thing everywhere. This also alignsbalancewith the usage of every other method ofCanonicalViewwhich already reads positions relative to the view's tip and leaves interpretation to the caller.With the parameter gone, the way to ask for a confirmation threshold is though the tip the
CanonicalViewis build with. A view is always a snapshot of the chain seen from a given tip, so "how many confirmations does this output have" is really the same question as "which tip am I looking from". To require N confirmations, the view is built attip - N +1:Any tx whose block ends up above that tip is no longer in the chain from a view's perspective, so it falls back to
Unconfirmedand is classified by the trust predicate.The downside is that this moves some work to the caller. The tip used to build the view must be a block that the
LocalChainalready has, sincecanonical_viewchecks it against the chain's checkpoints. The chain is sparse, it does not keep every height, so a checkpoint that is not there can't be used. Before, the threshold was just a number checked against the current tip and nothing else was needed. Now, if the caller wants to measure from a height the chain never kept, it has to insert or fetch that block first.Notes to the reviewers
In the
Confirmedbranch of balance theif confirmations < min_confirmationspath is dropped, so a confirmed output can now only go toconfirmedorimmature, never to*_pending. Thependingcategories are now reached only from theUnconfirmedbranch.Changelog notice
min_confirmationsparameter fromCanonicalView::balance.Checklists
All Submissions: