feat(chain): Add extract_subgraph() and roots() methods to CanonicalView#2041
feat(chain): Add extract_subgraph() and roots() methods to CanonicalView#2041evanlinjin wants to merge 2 commits into
Conversation
|
@evanlinjin This one could use a rebase, as #2029 landed on master. |
These methods are intended to help implement RBF logic. `CanonicalView::roots` is also added.
evanlinjin
force-pushed
the
canonical-view-extract-rbf
branch
from
f081300 to
af536db
Compare
Test extraction of transaction subgraphs with descendants, verifying that spends and txs fields maintain consistency. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
evanlinjin
force-pushed
the
canonical-view-extract-rbf
branch
from
af536db to
9b44f75
Compare
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
utACK abdf572
It looks good, I left a few nits that might be helpful, and a few comments that could be addressed in a follow-up. I'm going through the tests righ now.
| pub struct CanonicalView<A> { | ||
| /// Ordered list of transaction IDs in in topological-spending order. | ||
| order: Vec<Txid>, | ||
| order: Vec<Txid>, // TODO: Not ordered - waiting on #2038 |
There was a problem hiding this comment.
nit: we can probably update the documentation for now.
| // TODO: Use this with `TopologicalIter` once #2027 gets merged to return a view that has | ||
| // topologically-ordered transactions. |
| while let Some(txid) = to_visit.pop() { | ||
| let tx_entry = match txs.entry(txid) { | ||
| Entry::Occupied(_) => continue, // Already visited. | ||
| Entry::Vacant(entry) => entry, | ||
| }; | ||
|
|
||
| let (tx, pos) = match self.txs.remove(&txid) { | ||
| Some(tx_and_pos) => tx_and_pos, | ||
| None => continue, // Doesn't exist in graph. | ||
| }; | ||
|
|
||
| tx_entry.insert((tx.clone(), pos.clone())); | ||
|
|
||
| for op in (0_u32..tx.output.len() as u32).map(|vout| OutPoint::new(txid, vout)) { | ||
| let spent_by = match self.spends.remove(&op) { | ||
| Some(spent_by) => spent_by, | ||
| None => continue, | ||
| }; | ||
| spends.insert(op, spent_by); | ||
| to_visit.push(spent_by); | ||
| } | ||
| } |
There was a problem hiding this comment.
discussion: I've been thinking about this, it does the job. However, I wonder if it'd be a good idea to have TxDescendants, TxAncestors, or at least it's methods for the CanonicalView too (eg. CanonicalView::walk_descendants or CanonicalView::list_descendants).
I mean that would allow to get all the ancestors for given txids, and then remove/extract in a single batch, maybe it's only bikeshedding though 😂
| /// Extracts a subgraph containing the specified transactions and all their descendants. | ||
| /// | ||
| /// Takes transaction IDs and returns a new `CanonicalView` containing those transactions | ||
| /// plus any transactions that spend their outputs (recursively). The extracted transactions | ||
| /// are removed from this view. |
There was a problem hiding this comment.
It might be a good idea to mention that it does not errors or asserts that the txids exists in the CanonicalView, it's up to the user to make sure of that.
| let _roots = txs | ||
| .iter() | ||
| .filter(|(_, (tx, _))| { | ||
| tx.is_coinbase() | ||
| || tx | ||
| .input | ||
| .iter() | ||
| .all(|txin| !spends.contains_key(&txin.previous_output)) | ||
| }) | ||
| .map(|(&txid, _)| txid); |
There was a problem hiding this comment.
fix: As the _roots are not being used it can be removed, also if it's to be used we should probably think of a method that reuses the CanonicalView::roots() with an optional param, or something like that.
| /// Returns transactions without parent transactions in this view. | ||
| /// | ||
| /// Root transactions are either coinbase transactions or transactions whose inputs | ||
| /// reference outputs not present in this canonical view. | ||
| pub fn roots(&self) -> impl Iterator<Item = CanonicalTx<A>> + '_ { |
There was a problem hiding this comment.
nit: it's a good idea to also have a test for this one, at least asserting the behavior for coinbase and txs whose inputs are not in the TxGraph.
|
@evanlinjin needs a rebase/update |
2 participants
Description
This PR adds two new methods to
CanonicalViewto support RBF (Replace-By-Fee) transaction workflows:extract_subgraph(): Extracts a transaction and all its descendants from the canonical viewroots(): Returns transactions that have no parent transactions in the viewThese methods are essential for RBF support as they allow extracting specific transaction chains that can be replaced.
Note: This PR depends on #2029 which introduces the
CanonicalViewstruct.Notes to the reviewers
The
extract_subgraph()method removes the extracted transactions from the original view and returns them in a newCanonicalView. This is useful for isolating transaction chains that need to be replaced in RBF scenarios.Changelog notice
Added
CanonicalView::extract_subgraph()method to extract transactions with all their descendantsCanonicalView::roots()method to find root transactions without parents in the viewChecklists
All Submissions:
New Features:
🤖 Generated with Claude Code