-
Notifications
You must be signed in to change notification settings - Fork 405
CanonicalView
#2029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CanonicalView
#2029
Conversation
7eb1084
to
efb257f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cACK / utACK 809c153
It looks good to me so far, I left some nit and discussion comments above.
LGTM so far but needs a refactor now. |
- Add `CanonicalView` structure with canonical transaction methods - Move methods from `TxGraph` to `CanonicalView` (txs, filter_outpoints, balance, etc.) - Add canonical view methods to `IndexedTxGraph` - Update all tests and examples to use new API - Optimize examples to reuse canonical view instances 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Add min_confirmations parameter to control confirmation depth requirements: - min_confirmations = 0: Include all confirmed transactions (same as 1) - min_confirmations = 1: Standard behavior - require at least 1 confirmation - min_confirmations = 6: High security - require at least 6 confirmations Transactions with fewer than min_confirmations are treated as trusted/untrusted pending based on the trust_predicate. This restores the minimum confirmation functionality that was available in the old TxGraph::balance doctest but with a more intuitive API since CanonicalView has the tip internally. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Add test file `tests/test_canonical_view.rs` with three comprehensive test cases: 1. `test_min_confirmations_parameter`: Tests basic min_confirmations functionality - Verifies min_confirmations = 0 and 1 behave identically - Tests edge case where transaction has exactly required confirmations - Tests case where transaction has insufficient confirmations 2. `test_min_confirmations_with_untrusted_tx`: Tests trust predicate interaction - Verifies insufficient confirmations + untrusted predicate = untrusted_pending - Ensures trust predicate is respected when confirmations are insufficient 3. `test_min_confirmations_multiple_transactions`: Tests complex scenarios - Multiple transactions with different confirmation counts - Verifies correct categorization based on min_confirmations threshold - Tests both min_confirmations = 5 and min_confirmations = 10 scenarios These tests validate that the min_confirmations parameter correctly controls when transactions are treated as confirmed vs trusted/untrusted pending. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
BREAKING: Remove `CanonicalTx` as it's no longer needed.
Add comprehensive docs for CanonicalView module, structs, and methods. Fix all doctests to compile and pass with proper imports.
Also update submodule-level docs for `tx_graph`'s Canonicalization section.
809c153
to
3f9eec5
Compare
crates/chain/src/canonical_view.rs
Outdated
pub fn balance<'v, O: Clone + 'v>( | ||
&'v self, | ||
outpoints: impl IntoIterator<Item = (O, OutPoint)> + 'v, | ||
mut trust_predicate: impl FnMut(&O, ScriptBuf) -> bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think trust_predicate
is not expressive enough, for instance we're missing information about the txid. Using FullTxOut
as the operand, we have access to more things in addition to the script pubkey.
mut trust_predicate: impl FnMut(&O, ScriptBuf) -> bool, | |
mut trust_predicate: impl FnMut(&O, &FullTxOut<A>) -> bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Addressed in cb72f4a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it even more expressive, we can change the closure to output how many additional confirmations the UTXO needs to be considered "finalized". This will remove the additional_confirmations
parameter on balance
.
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing the connection between trust predicate and additional confirmations, and so I think it'd be best for them to remain separate. FTR I'm sure there's other ways for a user to filter unspents by a target number of confs, so I'm not exactly married to this approach, but it also seems convenient as most devs are already familiar with the balance
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK cb72f4a
BREAKING CHANGE: The trust_predicate parameter in CanonicalView::balance() now takes &FullTxOut<A> instead of ScriptBuf as its second argument. This provides more context to the predicate function. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
cb72f4a
to
1311a2e
Compare
I discussed with @evanlinjin and dropped the 7245631, which changed the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-ACK 1311a2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1311a2e
I'm wondering if we should already merge this to master, as we already have breaking changes on master. And with regards to the other standalone PRs that are non-breaking, if we should merge then in a specific new branch e.g. release/0.23.3. Let me know if you guys have another releasing approach in mind. |
I ran the benchmark on this PR, and so far there's no big regressions many_conflicting_unconfirmed::list_canonical_txs
time: [746.06 us 747.07 us 748.02 us]
change: [-0.6272% -0.4520% -0.2739%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
7 (7.00%) low mild
6 (6.00%) high mild
many_conflicting_unconfirmed::filter_chain_txouts
time: [842.09 us 844.42 us 846.63 us]
change: [-0.8799% +0.1490% +0.7945%] (p = 0.80 > 0.05)
No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) low mild
11 (11.00%) high mild
many_conflicting_unconfirmed::filter_chain_unspents
time: [841.98 us 844.33 us 846.80 us]
change: [+1.2639% +1.5485% +1.8660%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
many_chained_unconfirmed::list_canonical_txs
time: [5.7585 ms 5.7663 ms 5.7751 ms]
change: [+6.9882% +7.3202% +7.6471%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severe
many_chained_unconfirmed::filter_chain_txouts
time: [5.6686 ms 5.6742 ms 5.6803 ms]
change: [-4.3940% -3.7067% -3.1341%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
5 (5.00%) high mild
2 (2.00%) high severe
many_chained_unconfirmed::filter_chain_unspents
time: [5.6651 ms 5.6779 ms 5.6952 ms]
change: [-2.2329% -1.9763% -1.7036%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) low mild
3 (3.00%) high mild
4 (4.00%) high severe
nested_conflicts_unconfirmed::list_canonical_txs
time: [452.26 us 452.78 us 453.35 us]
change: [-0.3477% -0.1279% +0.0975%] (p = 0.26 > 0.05)
No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe
nested_conflicts_unconfirmed::filter_chain_txouts
time: [498.89 us 499.39 us 499.91 us]
change: [-2.0289% -1.5397% -1.1074%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severe
nested_conflicts_unconfirmed::filter_chain_unspents
time: [498.07 us 498.58 us 499.10 us]
change: [-1.5937% -1.1912% -0.8211%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
4 (4.00%) low mild
2 (2.00%) high mild
4 (4.00%) high severe |
@oleonardolima I don't think we need a point release for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1311a2e
Description
CanonicalView
allows us to canonicalize upfront, reducing our API surface and improving performance by reducing canonicalizations we need to do.This is also the first step to achieving many of our goals.
CanonicalUnspents
structure inbdk_tx
.Changelog notice
Checklists
All Submissions:
New Features: