bitcoindevkit / bdk

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

Remove `Anchor` trait and make anchors unique to `(Txid, BlockId)` #1507

Open evanlinjin opened 4 months ago

evanlinjin commented 4 months ago

The Problem

Why remove Anchor trait?

We don't need it. Confirmation block and anchor block will be the same block after #1489 is merged.

Anchor representation in TxGraph and tx_graph::ChangeSet is bad

This is how anchors are represented now.

pub struct TxGraph<A = ()> {
    anchors: BTreeSet<(A, Txid)>,
    // ... OTHER FIELDS
}

pub struct ChangeSet<A = ()> {
    pub anchors: BTreeSet<(A, Txid)>,
    // ... OTHER FIELDS
}

However, we can have multiple As that have the same anchor block BlockId for the same Txid. This is not ideal. Ideally, we want one anchor per (BlockId, Txid).

The Proposal

pub type Anchor = (Txid, BlockId);

pub struct TxGraph<AM> {
    anchors: BTreeMap<Anchor, AM>,
}

pub struct ChangeSet<AM> {
    anchors: BTreeMap<Anchor, AM>,
}

Where AM is "anchor metadata". I.e. You can store block time here (u32).

LagginTimes commented 4 months ago

I’d like to take this if nobody else has.

evanlinjin commented 3 months ago

I don't think it is good practice to change the anchor metadata (AM). However, we should allow it.

LLFourn commented 3 months ago

We don't need it. Confirmation block and anchor block will be the same block after https://github.com/bitcoindevkit/bdk/pull/1489 is merged.

I think that our interpretation of anchor has to remain the anchor block is not necessarily the block it is in. For example, lets say I have a tx A with a descendant B. I find out B is confirmed then I know the ancestor A is confirmed also but I don't know where. We should return A as confirmed and anchor it to B's anchor.

We can note that the confirmation is transitive so applications can decide what to do here but I think that this should just work. I had a look at the code and it doesn't look like we do transitive confirmations but somehow do transitive last_seen :P.

Another problem with removing the anchor trait: It's attractive for the TxGraph to know independently of any ChainOracle the "time" that a transaction was confirmed for the purpose of ordering them by "first_seen". It seem convenient to have anchors return a this value since they also return the block hash. I think we'd have to come up with a design here that encompasses how we'll communicate this. It could be through AM but this would then need a trait.

evanlinjin commented 3 months ago

We should return A as confirmed and anchor it to B's anchor.

Could we say A is transitively anchored to the block which B is anchored in? I think we can use the Anchor type I've suggested in the ticket description, and have another variant on ChainPosition for transitive anchoring.

We can add the idea of transitive anchors later on. To make this change backwards compatible, ChainPosition can be #[non_exhaustive].

It's attractive for the TxGraph to know independently of any ChainOracle the "time" that a transaction was confirmed for the purpose of ordering them by "first_seen".

Would you give an example where this would be useful? How would it look if a tx is anchored to multiple blocks?

I feel like we can add this functionality later on? I.e. have a method on TxGraph that is available if, and only if, AM impls a certain trait.

evanlinjin commented 3 months ago

@LagginTimes we have a thumbs up from @LLFourn. Please go ahead with the PR.

notmandatory commented 3 months ago

Per comment in #1515 moving this out of the alpha milestone.