feat(chain)!: taint-aware CanonicalView::balance

[Dmenec]

Solves #2267

Description

Takes over #2235 (thanks @evanlinjin for the go-ahead). It reworks CanonicalView::balance to derive trust from an output's unconfirmed ancestry, and adds classify_outpoints, a per-output spend-eligibility classifier that balance becomes a thin fold over.

The old balance decided trust using a per-output trust_predicate, which cannot express transitive trust. As a result, owned outputs whose unconfirmed ancestry included foreign coins were counted as trusted. That is the root cause of the wallet trust-classification bugs (bitcoindevkit/bdk_wallet#16, bitcoindevkit/bdk_wallet#273).

The API now takes two separate predicates, one per concern:

• does_taint(&tx) - should this transaction be considered tainted? (e.g., because it spends a foreign unconfirmed output)
• is_settled(&pos) - do we consider this chain position settled / final? (generalizes min_confirmations)

For each unspent output, classify_outpoints reports its chain-level spend eligibility:

• Settled if considered settled according to is_settled
• Immature for a coinbase output that has not yet matured
• Unsettled(Trust) otherwise, where Trust is:
  • Trusted if the whole unconfirmed ancestry only spends owned coins
  • Untrusted if the output itself, or any unconfirmed ancestor, is tainted
  • Unknown if part of the ancestry is missing from the view, so trust can't be determined

balance then sums each output's value into the bucket corresponding to its Eligibility.

Notes to the reviewers

• Trust is resolved through a self-contained ancestry walk. The traversal is memoized: each visited transaction is cached so that already-classified transactions (and their ancestors) do not need to be walked again.
• Kept Balance::confirmed and did not rename it to settled. That rename is out of scope here. The folding logic is slated to move into bdk_wallet, and the current balance function in chain will eventually be deprecated.
• balance also drops the O generic and now takes plain OutPoints, since the taint predicate operates on transactions rather than per-outpoint associated data.
• does_taint is evaluated at most once per transaction.
• More sophisticated classification rules (e.g., for coin control or locked funds) can be built on top of classify_outpoints. Left for a follow-up.

Changelog notice

• Breaking: CanonicalView::balance now takes does_taint: impl FnMut(&CanonicalTx) -> bool and is_settled: impl Fn(&ChainPosition) -> bool instead of a per-output trust predicate and min_confirmations, and plain OutPoints instead of (identifier, outpoint) pairs (this drops the O generic). Trust is now derived from an output's unconfirmed ancestry.
• Breaking: added Balance::unknown_pending, where outputs whose trust can't be determined are counted instead of being lumped into untrusted_pending.
• Added CanonicalView::classify_outpoints and the Eligibility enum.

Checklists

All Submissions:

• I followed the contribution guidelines
• I ran just p before pushing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API

[codecov]

Codecov Report

❌ Patch coverage is 90.69767% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.84%. Comparing base (acc06e5) to head (e2dad37).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/chain/src/canonical.rs	90.69%	6 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2246      +/-   ##
==========================================
+ Coverage   78.71%   78.84%   +0.13%     
==========================================
  Files          31       31              
  Lines        5966     6060      +94     
  Branches      282      288       +6     
==========================================
+ Hits         4696     4778      +82     
- Misses       1194     1203       +9     
- Partials       76       79       +3     

Flag	Coverage Δ
rust	78.84% <90.69%> (+0.13%)	⬆️

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.

[Dmenec]

Thinking about some cases where settledness makes a transaction fall into a different Eligibility. With foreign inputs (e.g. a tx that needs 6 min_confirmations to be settled, while the current tip is only 1 block ahead of it) the output ends up as UntrustedPending, which doesn't really make sense: it isn't pending, just not settled. The same happens with owned inputs that falls into TrustedPending.

I think it might be clearer to call them TrustedUnsettled / UntrustedUnsettled rather than *Pending.

[evanlinjin]

@Dmenec Good point. What do you think about this?

pub enum Eligibility {
    Settled,
    Immature,
    Unsettled(Trust),
}

pub enum Trust {
    Trusted,
    Untrusted,
}

I think this increases clarity and makes it a bit easier for call sites that only care about whether it's unsettled or not.

[evanlinjin]

Thanks for pushing this forward - this is looking great.

I haven't looked too hard into the tests yet, follow-up reviews will come.

[evanlinjin]

The coinbase maturity check lives inside the is_settled check - this is wrong. Unsettled is not the same as unconfirmed.

Example: A confirmed transaction can be unsettled because the caller requires 3 confirmations to be classified as settled. With the current logic, this transaction will become "pending" instead of "immature".

[nymius]

So any unconfirmed coinbase tx with an owned script pubkey should be treated as Immature, not TrustedPending, even if it doesn't have a single confirmation?

[evanlinjin]

Hopefully there shouldn't be such a thing as an "unconfirmed coinbase" - the canonicalization algorithm should have considered those as non-canonical! If not, let's file a bug.

[nymius]

Not even if you assume it canonical?

[Dmenec]

Tried it with a coinbase without an anchor (which, as @evanlinjin said, shouldn't be possible on a real chain, but you can still construct it in a test), and if you assume it canonical it does end up in the set as unconfirmed.

bdk/crates/chain/src/canonical.rs

Lines 132 to 137 in 8897579

	let conf_height = match self.pos.confirmation_height_upper_bound() {
	Some(height) => height,
	None => {
	debug_assert!(false, "coinbase tx can never be unconfirmed");
	return false;
	}

As you can see above, it falls back to false there, so such an output is treated as immature anyway.

I agree with moving the maturity check out of is_settled, so it stays Immature regardless of settledness.

[Dmenec]

Moved the maturity check out of is_settled. Leaving this unresolved for now.

[nymius]

So any unconfirmed coinbase tx with an owned script pubkey should be treated as Immature, not TrustedPending, even if it doesn't have a single confirmation?

[Dmenec]

Thanks a lot @evanlinjin and @nymius for reviewing this :)
I think that covers everything now, but let me know if I missed something.

[nymius]

In commit ff71789, change BREAKING to BREAKING CHANGE to use the most common marker.

[Dmenec]

I'd like to add a locked balance category on top of this (bdk_wallet's balance needs to be aware of it), for outputs that are confirmed but not yet spendable because a descriptor timelock hasn't matured yet, as raised in bitcoindevkit/bdk_wallet#180.

I think that maybe, in a future PR, a separate frozen/reserved category could also be added for outputs the user manually locks. I'd leave it as a possible future addition, which can be folded over classify_outpoints without changing Balance in wallet.

Edit: still thinking about how to do it. We could do the fold directly in the wallet with a locked category and leave Balance untouched, but it might be worth modeling it in chain too. Since each Eligibility variant has a matching Balance field, adding a Locked variant would mean adding Balance.locked as well... (same on frozen/reserved) Some thoughts @evanlinjin @nymius @110CodingP?

[110CodingP]

LGTM 🚀 , just a nit.
I especially love the test coverage, great work @Dmenec !

[nymius]

Edit: still thinking about how to do it. We could do the fold directly in the wallet with a locked category and leave Balance untouched, but it might be worth modeling it in chain too. Since each Eligibility variant has a matching Balance field, adding a Locked variant would mean adding Balance.locked as well... (same on frozen/reserved)

This is part of the protocol, not user decided, so it belongs to chain. I would do both changes in Elegibility and Balance.
Using the same criteria, I'm not sure frozen belongs here, but Wallet. There you can pre-filter utxos to mark them as frozen before passing them to chain::balance. You could even create two separated list of outpoints, one set is frozen, the other is not, and compute balance for both separatedly. You would need some extra operations to compute a common output, but is doable.

[nymius]

ACK 3bbdf78

[noahjoeris]

ACK 3bbdf78

Great job ✨

[nymius]

ACK 2581fb2

[evanlinjin]

This is quality work.

I've found a final rough edge that should be hashed out before we merge.

I haven't finished reviewing the tests yet. However, we should probably rename some of them as the min_confirmations parameter no longer exists.

[evanlinjin]

ACK e2dad37

Solid work.

[nymius]

ACK e2dad37