refactor(chain)!: remove min_confirmations parameter from CanonicalView::balance

[Dmenec]

Description

This builds on top of #2038.

Removes the min_confirmations parameter from CanonicalView::balance.

This is part of the redesign of the wallet balance API (see bitcoindevkit/bdk_wallet#451).

Context

Trust Categorization

The goal is to fix how balance decides 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:

• refactor(chain,core)!: replace CanonicalIter with sans-IO CanonicalTask + ChainQuery trait #2038 — the new CanonicalView, which lets us look up any tx by txid and get its canonical position.
• feat(chain): add walk_ancestors_from_root to TxGraph #2218 — makes it possible to walk the ancestors of a tx from the transaction itself.
• this PR — removing min_confirmations so a confirmed tx with few confirmations is not wrongly treated as pending while walking ancestors.

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_confirmations

The ancestor walk decides trust by checking each ancestor's canonical position. A Confirmed ancestor stops the walk, an Unconfirmed one is only trusted if all of its inputs are also ours. For this to be coherent, Confirmed has to mean the same thing in balance as it does in the walk.

On the previous approach the parameter min_confirmations broke that. A Confirmed output below the min_confirmations threshold was reported as pending, even though the walk still saw it as Confirmed.

Removing it, balance classifies purely from the canonical position, and Confirmed means the same thing everywhere. This also aligns balance with the usage of every other method of CanonicalView which 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 CanonicalView is 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 at tip - N +1:

Building the view at tip - N + 1 makes a tx count as Confirmed exactly when it has at least N confirmations relative to the real tip.

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 Unconfirmed and 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 LocalChain already has, since canonical_view checks 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 Confirmed branch of balance the if confirmations < min_confirmations path is dropped, so a confirmed output can now only go to confirmed or immature, never to *_pending. The pending categories are now reached only from the Unconfirmed branch.

Changelog notice

• Breaking: Removed the min_confirmations parameter from CanonicalView::balance.

Checklists

All Submissions:

• I followed the contribution guidelines
• This pull request breaks the existing API

[nymius]

cc: @evanlinjin , what do you think? Just last commit: dec3133

[evanlinjin]

@nymius I'll have a look! Give me a bit of time, I just got informed for the incomplete Balance problem.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.60%. Comparing base (6d03fc3) to head (5a7c95c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2221      +/-   ##
==========================================
- Coverage   78.65%   78.60%   -0.06%     
==========================================
  Files          30       30              
  Lines        5909     5894      -15     
  Branches      279      277       -2     
==========================================
- Hits         4648     4633      -15     
  Misses       1185     1185              
  Partials       76       76              

Flag	Coverage Δ
rust	78.60% <100.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evanlinjin]

@Dmenec thank you for this PR.

Unfortunately, min_confirmations is different to canonicalizing with a lower tip. A transaction confirmed at the actual tip is not guaranteed to be the best canonicalization candidate at a lower tip. Canonicalizing with a lower tip may get a completely different canonical set.

I don't think min_confirmations is the bottleneck to move towards an UTXO-ancestor based trust categorization. I would argue that the shape of trust_predicate is the problem.

I agree that min_confirmations/Balance::confirmed is badly named. However, I think a similar concept is still useful as blocks in a bitcoin network aren't guaranteed to be permanent. Maybe "settled" would be a better terminology. I think the UTXO-ancestor based trust categorization should stop at "settled" (which is configurable) and not purely "confirmed" as different wallets have different trust assumptions. For some, 1 confirmation is not good enough.

I've made a prototype of the direction I think we should take in #2235. Let me know what you think.