refactor(chain)!: drop the bound framing from confirmation APIs - #2299
evanlinjin wants to merge 2 commits into
Conversation
The `Anchor` trait doc claimed an anchored transaction "could also mean transaction A is confirmed in a parent block of B". No chain source in this repo produces such an anchor: electrum only builds one after `validate_merkle_proof` succeeds, esplora uses `TxStatus`'s confirming block, and `TxPosInBlock` carries the transaction's index within that block. The framing only leaves ambiguity for a case that does not occur. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2299 +/- ##
==========================================
- Coverage 78.75% 78.72% -0.03%
==========================================
Files 31 31
Lines 5974 5966 -8
Branches 284 284
==========================================
- Hits 4705 4697 -8
Misses 1193 1193
Partials 76 76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4ad7715 to
38f2078
Compare
An anchor names the block that confirmed the transaction, so its height is exact, not an upper bound. Rename `confirmation_height_upper_bound` to `confirmation_height` on both `Anchor` and `ChainPosition`. `ChainPosition::confirmations_lower_bound` was a lower bound only because it derives from that height: a higher height yields fewer confirmations, so an upper-bounded height yields a lower-bounded count. With the height exact the count is exact too, so it becomes `confirmations`. These are hard renames with no deprecated aliases. The next `bdk_chain` release is already breaking, so a downstream `Anchor` impl overriding an old name should fail to compile rather than be silently ignored. `ConfirmationBlockTime`'s override is removed as it was identical to the default. This drops the false-negative caveats from `CanonicalTxOut::is_mature` and `is_confirmed_and_spendable`, which only described the loose bound. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
38f2078 to
40cb3d1
Compare
confirmation_height_upper_bound to confirmation_height
nymius
left a comment
There was a problem hiding this comment.
cACK 40cb3d1
For now, if we have no use for the probabilistic case, I agree with removing it. If we need it in the future (e.g., Utreexo?), I would reintroduce it under a different name to avoid colliding with previous versions.
| /// | ||
| /// [`confirmation_height_upper_bound`]: Anchor::confirmation_height_upper_bound | ||
| /// A coinbase output is mature once [`COINBASE_MATURITY`] blocks (including the block that | ||
| /// confirmed it) have been mined up to and including `tip`. Non-coinbase outputs are always |
There was a problem hiding this comment.
I don't know the best way to word this, but a coin is mature if it's eligible to be included in the next block. The minimum such height is COINBASE_MATURITY - 1 , so this should say something like:
/// A coinbase output is mature once it can be included in the next block after
/// [`COINBASE_MATURITY`] blocks have been mined after its confirmation
|
@nymius Thank you for the review. How do you envision UTreexo potentially using this feature? |
|
for example like this #[test]
fn transitively_confirmed_tx_reports_descendant_anchor_height() {
let chain = local_chain![
(0, hash!("genesis")),
(1, hash!("parent_block")),
(5, hash!("child_block"))
];
let tip = chain.tip().block_id();
let parent_anchor = block_id!(1, "parent_block");
let child_anchor = block_id!(5, "child_block");
let templates = [
TxTemplate {
tx_name: "parent",
inputs: &[TxInTemplate::Bogus],
outputs: &[TxOutTemplate::new(100_000, Some(0))],
// Seen in the mempool, but its confirmation has not been fetched yet.
last_seen: Some(1),
..Default::default()
},
TxTemplate {
tx_name: "child",
inputs: &[TxInTemplate::PrevTx("parent", 0)],
outputs: &[TxOutTemplate::new(90_000, Some(0))],
anchors: &[child_anchor],
..Default::default()
},
];
let mut env = init_graph(&templates);
let parent_txid = env.txid_to_name["parent"];
let child_txid = env.txid_to_name["child"];
let parent_position = |tx_graph: &TxGraph<BlockId>| {
chain
.canonical_view(tx_graph, tip, Default::default())
.txs()
.find(|tx| tx.txid == parent_txid)
.expect("parent must be canonical")
.pos
};
// Without its own anchor, the parent borrows the child's anchor. It was really confirmed in
// block 1 (5 confirmations at tip), but the position can only report the child's block 5.
let pos = parent_position(&env.tx_graph);
assert_eq!(
pos,
ChainPosition::Confirmed {
anchor: child_anchor,
transitively: Some(child_txid),
}
);
assert_eq!(pos.confirmation_height(), Some(5));
assert_eq!(pos.confirmations(tip.height), 1);
// Once the parent's own anchor is known, the position becomes exact.
let _ = env.tx_graph.insert_anchor(parent_txid, parent_anchor);
let pos = parent_position(&env.tx_graph);
assert_eq!(
pos,
ChainPosition::Confirmed {
anchor: parent_anchor,
transitively: None,
}
);
assert_eq!(pos.confirmation_height(), Some(1));
assert_eq!(pos.confirmations(tip.height), 5);
} |
|
@noahjoeris That's a good point - I completely overlooked this. Yes, we should still support transitively anchored transactions found during canonicalization. The renames in |
I'm not envisioning anything yet, I was thinking along my comment to the referenced issue #2298 (comment), trying to come up with an answer to the open question. |
This has been discarded by @luisschwab in #2298 (comment) |
Description
Fixes #2298.
The
Anchortrait previously said an anchor block might be a descendant of the block that actually confirmed a transaction. Nothing in this repo ever produces that kind of anchor:TxStatusTxPosInBlockcarries the transaction’s index inside that blockSo the height is exact, not an upper bound. This PR removes that framing and renames:
confirmation_height_upper_bound→confirmation_height(on bothAnchorandChainPosition)confirmations_lower_bound→confirmationsThe second rename follows for the same reason: a higher height produces fewer confirmations, so an upper-bounded height used to yield a lower-bounded count. With an exact height the count is exact.
Notes to reviewers
No behaviour change. Every implementation already returned
anchor_block().height. The override onConfirmationBlockTimeis removed because it was identical to the trait default.Hard renames, no deprecated shims. The next
bdk_chainrelease is already breaking, so a downstream impl that still overrides the old name should fail to compile rather than be silently ignored.Changelog notice
bdk_chainAnchor::confirmation_height_upper_boundandChainPosition::confirmation_height_upper_boundtoconfirmation_height. An anchor names the confirming block, so the height is exact. No deprecated alias.ChainPosition::confirmations_lower_boundtoChainPosition::confirmations(the derived count is now exact).Anchortrait docs: they no longer claim an anchor block may be a descendant of the confirming block.Checklists
All Submissions:
Bugfixes: