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

refactor(chain)!: Remove Anchor trait and make anchors unique to (Txid, BlockId) #1515

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LagginTimes
Copy link
Contributor

Resolves #1507.

Description

The Anchor trait has been removed and anchors are now unique to (Txid, BlockId).

Notes to the reviewers

Documentation have been mostly untouched and need to be written/updated.

Changelog notice

  • Anchor trait has been removed.
  • Anchor type and BlockTime struct have been added to bdk_chain.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@LagginTimes LagginTimes marked this pull request as draft July 17, 2024 13:55
@LagginTimes LagginTimes force-pushed the anchor_restructure branch 2 times, most recently from c2f280f to afe5511 Compare July 17, 2024 14:02
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jul 17, 2024
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks like a nice simplification making Anchor a concrete type, and replacing ConfirmationBlockTime with BlockTime. I primarily focused on changes in tx_data_traits.rs as the rest of the changes are downstream.

Once docs are done this looks good to me.

@evanlinjin is it OK to merge this one before #1514 ?

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an initial look. Thanks for pushing so much work out in short notice.

crates/chain/src/chain_data.rs Outdated Show resolved Hide resolved
crates/bitcoind_rpc/tests/test_emitter.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
// all transactions that the graph is aware of in format: `(tx_node, tx_anchors)`
txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>)>,
txs: HashMap<Txid, (TxNodeInternal, BTreeMap<Anchor, AM>)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
txs: HashMap<Txid, (TxNodeInternal, BTreeMap<Anchor, AM>)>,
txs: HashMap<Txid, (TxNodeInternal, BTreeSet<BlockId>)>,

Anchor contains the txid. It does not make as part of the value of something keyed by txid.

Also we can lookup the A (anchor meta) from anchors.

/// Txid of the transaction.
pub txid: Txid,
/// A partial or full representation of the transaction.
pub tx: T,
/// The blocks that the transaction is "anchored" in.
pub anchors: &'a BTreeSet<A>,
pub anchors: &'a BTreeMap<Anchor, AM>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub anchors: &'a BTreeMap<Anchor, AM>,
pub anchors: Range<'a, Anchor, AM>,

I'm not sure if this will work? Getting a range from TxGraph::anchors.

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/common/mod.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph_conflicts.rs Show resolved Hide resolved
@evanlinjin
Copy link
Member

@notmandatory I prefer we rebase this on top of #1514 since that PR is more "final" than this one.

@evanlinjin
Copy link
Member

As discussed privately, it makes sense to rm TxGraph::anchors field.

pub struct TxGraph<AM = BlockTime> {
    // all transactions that the graph is aware of in format: `(tx_node, tx_anchors)`
    txs: HashMap<Txid, (TxNodeInternal, BTreeMap<BlockId, AM>)>,
    spends: BTreeMap<OutPoint, HashSet<Txid>>,
    last_seen: HashMap<Txid, u64>,


    // 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>,
}

See P.O.C: 63d9e6a

The `Anchor` trait has been removed and anchors are now unique to
(`Txid`, `BlockId`).
@evanlinjin
Copy link
Member

evanlinjin commented Jul 19, 2024

This is great work. However, we have concluded that we do not have enough time to get this into v1.0.beta (as discussed).

@notmandatory
Copy link
Member

notmandatory commented Jul 19, 2024

I'll move this out of the alpha milestone, thanks guys.

@notmandatory
Copy link
Member

notmandatory commented Jul 20, 2024

Technically wallet ChangeSet is part of the public wallet API I'll move this to 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Remove Anchor trait and make anchors unique to (Txid, BlockId)
3 participants