Skip to content

[chain] Consider Merging IndexedTxGraph and TxGraph into one thing #130

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

Closed
Tracked by #86 ...
LLFourn opened this issue Oct 5, 2023 · 6 comments
Closed
Tracked by #86 ...

[chain] Consider Merging IndexedTxGraph and TxGraph into one thing #130

LLFourn opened this issue Oct 5, 2023 · 6 comments

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Oct 5, 2023

currently:

#[derive(Debug)]
pub struct IndexedTxGraph<A, I> {
    /// Transaction index.
    pub index: I,
    graph: TxGraph<A>,
}

But it could also just be:

#[derive(Clone, Debug, PartialEq)]
pub struct TxGraph<A = (), I = ()> {
    // all transactions that the graph is aware of in format: `(tx_node, tx_anchors, tx_last_seen)`
    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, u64)>,
    spends: BTreeMap<OutPoint, HashSet<Txid>>,
    anchors: BTreeSet<(A, Txid)>,

    // This atrocity exists so that `TxGraph::outspends()` can return a reference.
    // FIXME: This can be removed once `HashSet::new` is a const fn.
    empty_outspends: HashSet<Txid>,
    pub index: I
}

But we would have to give up the derive PartialEq on TxGraph because I don't think we want to force indexes to implement it.

The motivation is avoiding having to duplicate every mutating API on TxGraph to IndexedTxGraph. We seem to have some trouble doing this. For example, IndexedTxGraph and TxGraph both have insert_tx but they have different arguments atm but they should be the same. Also in bitcoindevkit/bdk#1041 I a method is being added to IndexedTxGraph but it should also be on TxGraph but the PR author forgot.

The downside is that TxGraph would not longer be a pure data structure but since we don't use it like that I don't think this matters.

@evanlinjin
Copy link
Member

"the PR author"

@notmandatory
Copy link
Member

I moved this to alpha.4 since we want to get any functional changes in before a beta release.

@LLFourn
Copy link
Contributor Author

LLFourn commented Nov 16, 2023

FWIW I think this is still a good idea.

@nondiremanuel
Copy link

Can we postpone it to a 1.1 version @notmandatory?

@nondiremanuel nondiremanuel moved this from Todo to Discussion in BDK Wallet Jan 25, 2024
@evanlinjin
Copy link
Member

I think it is okay to postpone this to after 1.0.0. I think it may simplify the API and make it cleaner to use with less foot-guns. However, it is also not critical to have this.

@evanlinjin
Copy link
Member

Closing. Refer to bitcoindevkit/bdk#1510 (comment)

@evanlinjin evanlinjin closed this as not planned Won't fix, can't repro, duplicate, stale Apr 12, 2025
@github-project-automation github-project-automation bot moved this from Discussion to Done in BDK Wallet Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants