bdk icon indicating copy to clipboard operation
bdk copied to clipboard

[chain] Consider Merging IndexedTxGraph and TxGraph into one thing

Open LLFourn opened this issue 1 year ago • 5 comments

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.

LLFourn avatar Oct 05 '23 05:10 LLFourn

"the PR author"

evanlinjin avatar Oct 07 '23 20:10 evanlinjin

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

notmandatory avatar Nov 13 '23 16:11 notmandatory

FWIW I think this is still a good idea.

LLFourn avatar Nov 16 '23 02:11 LLFourn

Can we postpone it to a 1.1 version @notmandatory?

nondiremanuel avatar Jan 25 '24 12:01 nondiremanuel

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 avatar Feb 05 '24 10:02 evanlinjin