Skip to content
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

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

Open
Tracked by #86 ...
LLFourn opened this issue Oct 5, 2023 · 5 comments · May be fixed by #1510
Open
Tracked by #86 ...

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

LLFourn opened this issue Oct 5, 2023 · 5 comments · May be fixed by #1510
Labels
discussion There's still a discussion ongoing module-blockchain

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 #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 LLFourn added the discussion There's still a discussion ongoing label Oct 5, 2023
@evanlinjin
Copy link
Member

"the PR author"

@nondiremanuel nondiremanuel added this to the 1.0.0-beta.0 milestone Oct 10, 2023
@notmandatory notmandatory modified the milestones: 1.0.0-beta.0, 1.0.0-alpha.4 Nov 13, 2023
@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 nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@nondiremanuel
Copy link

Can we postpone it to a 1.1 version @notmandatory?

@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 evanlinjin modified the milestones: 1.0.0, 1.1.0 Feb 5, 2024
@notmandatory notmandatory removed this from the 1.1.0 milestone Mar 18, 2024
@LLFourn LLFourn linked a pull request Jul 11, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing module-blockchain
Projects
Status: Discussion
Development

Successfully merging a pull request may close this issue.

4 participants