-
Notifications
You must be signed in to change notification settings - Fork 328
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
Introduce O(n)
canonicalization algorithm
#1670
Conversation
Per call today @evanlinjin will open a new PR for the 1.0.0-beta milestone that only makes expected breaking changes for |
5278b81
to
7610b65
Compare
On further discussion today at release planning call this PR was moved back into the 1.0 milestone if it can be completed and reviewed in time. If not it will have to wait for a 2.0 milestone because it required breaking changes to chain crate APIs that are exposed in the Alternatively we could remove the following /// Get a reference to the inner [`TxGraph`].
pub fn tx_graph(&self) -> &TxGraph<ConfirmationBlockTime> {
self.indexed_graph.graph()
}
/// Get a reference to the inner [`KeychainTxOutIndex`].
pub fn spk_index(&self) -> &KeychainTxOutIndex<KeychainKind> {
&self.indexed_graph.index
}
/// Get a reference to the inner [`LocalChain`].
pub fn local_chain(&self) -> &LocalChain {
&self.chain
} |
@notmandatory how does this solution compare with just giving wallet a major version bump? |
Removing the above functions and moving required chain error type to core should allow us to do breaking chain crate api changes without having to do a major wallet crate release. I also don't see why Wallet users need to access the inner chain types directly instead of using higher level functions like But all that said if it's less risky to just keep everything as is and do a 2.0 release in 6 mo or so I'd be fine with that too. |
78c9b0f
to
64733ca
Compare
Add `run_until_finished` methods for `TxAncestors` and `TxDescendants`. This is useful for traversing until the internal closure returns `None`. Signatures of `TxAncestors` and `TxDescendants` are changed to enforce generic bounds in the type definition.
This is an O(n) algorithm to determine the canonical set of txids. * Run 1: Iterate txs with anchors, starting from highest anchor height txs. * Run 2: Iterate txs with last-seen values, starting from highest last-seen values. * Run 3: Iterate txs that are remaining from run 1 which are not anchored in the best chain. Since all transitively-anchored txs are added to the `canonical` set in run 1, and anything that conflicts to anchored txs are already added to `not_canonial`, we can guarantee that run 2 will not traverse anything that directly or indirectly conflicts anything that is anchored. Run 3 is needed in case a tx does not have a last-seen value, but is seen in a conflicting chain. `TxGraph` is updated to include indexes `txids_by_anchor_height` and `txids_by_last_seen`. These are populated by the `insert_anchor` and `insert_seen_at` methods. Generic constaints needed to be tightened as these methods need to be aware of the anchor height to create `LastSeenIn`.
This is mostly taken from bitcoindevkit#1735 except we inline many of the functions and test `list_canonical_txs`, `filter_chain_unspents` and `filter_chain_txouts` on all scenarios. CI and README is updated to pin `csv`. Co-authored-by: valued mammal <[email protected]>
Also removed extra derives on `ObservedIn` and updated docs for `CanonicalTx`.
Tx anchored in orphaned block and not seen in the mempool should be canon.
In `Wallet::preselect_utxos()`, the code used to obtain chain position of the UTXO's transaction from the graph, however the chain position is already recorded within the UTXO's representation (`LocalOutput`). This patch reuses the existing chain position instead of obtaining a fresh one.
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 956d0a9
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 956d0a9
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 956d0a9
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 956d0a9
🔥
Thanks @evanlinjin and @ValuedMammal for all the work on getting this coded, documented, and benchmark tested, and to everyone who helped with review! |
Fixes #1665
Replaces #1659
Description
Previously, getting the canonical history of transactions/UTXOs required calling
TxGraph::get_chain_position
on each transaction. This was highly inefficient and resulted in anO(n^2)
algorithm. The situation is especially problematic when we have many unconfirmed conflicts.This PR introduces an
O(n)
algorithm to determine the canonical set of transactions inTxGraph
. The algorithm's premise is as follows:A
is determined to be canonical, all ofA
's ancestors must also be canonical.B
is determined to be NOT canonical, all ofB
's descendants must also be NOT canonical.We maintain two mutually-exclusive
txid
sets:canoncial
andnot_canonical
.Imagine a method
mark_canonical(A)
that is based on premise 1 and 2. This method will mark transactionA
and all of it's ancestors as canonical. For each transaction that is marked canonical, we can iterate all of it's conflicts and mark those asnon_canonical
. If a transaction already exists incanoncial
ornot_canonical
, we can break early, avoiding duplicate work.This algorithm iterates transactions in 3 runs.
mark_canonical
on it. We iterate in descending-height order to reduce the number of anchors we need to check against theChainOracle
(premise 1). The purpose of this run is to populatenon_canonical
with all transactions that directly conflict with anchored transactions and populatecanonical
with all anchored transactions and ancestors of anchors transactions (transitive anchors).mark_canonical
on all of these that do not already exist incanonical
ornot_canonical
.Benchmarks
Thank you to @ValuedMammal for working on this.
Benchmark results (this PR):
Benchmark results (master): https://github.com/evanlinjin/bdk/tree/fix/1665-master-bench
Notes to the reviewers
PLEASE MERGE feat(chain,wallet)!: Transitive
ChainPosition
#1733 BEFORE THIS PR! We had to change the signature ofChainPosition
to account for transitive anchors and unconfirmed transactions with nolast-seen
value.The canonicalization algorithm is contained in
/crates/chain/src/canonical_iter.rs
.Since the algorithm requires traversing transactions ordered by anchor height, and then last-seen values, we introduce two index fields in
TxGraph
;txs_by_anchor
andtxs_by_last_seen
. Methodsinsert_anchor
andinsert_seen_at
are changed to populate these index fields.An ADR is added:
docs/adr/0003_canonicalization_algorithm.md
. This is based on the work in Architectural Decision Records #1592.Changelog notice
O(n)
canonicalization algorithm. This logic is contained in/crates/chain/src/canonical_iter.rs
.TxGraph
;txs_by_anchor_height
andtxs_by_last_seen
. Pre-indexing allows us to construct the canonical history more efficiently.TxGraph
methods:try_get_chain_position
andget_chain_position
. This is superseded by the new canonicalization algorithm.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: