bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
850 stars 307 forks source link

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

Open LLFourn opened 1 year ago

LLFourn commented 1 year ago

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 #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 commented 1 year ago

"the PR author"

notmandatory commented 11 months ago

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

LLFourn commented 10 months ago

FWIW I think this is still a good idea.

nondiremanuel commented 8 months ago

Can we postpone it to a 1.1 version @notmandatory?

evanlinjin commented 8 months ago

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.