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.

Sign up for GitHub

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

Jump to bottom

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

Open
evanlinjin opened this issue Jul 9, 2024 · 6 comments · May be fixed by #1515
Open

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

evanlinjin opened this issue Jul 9, 2024 · 6 comments · May be fixed by #1515
Assignees
@LagginTimes
Labels
bug Something isn't working
Milestone
2.0.0

Comments

@evanlinjin
Copy link
Member

evanlinjin commented Jul 9, 2024 •
edited
Loading

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).
@evanlinjin evanlinjin added the bug Something isn't working label Jul 9, 2024
@evanlinjin evanlinjin changed the title Remove Anchor trait and make anchors unique to `(Txid, BlockId Remove Anchor trait and make anchors unique to (Txid, BlockId) Jul 9, 2024
@notmandatory notmandatory added this to BDK Jul 9, 2024
@notmandatory notmandatory moved this to Todo in BDK Jul 9, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jul 9, 2024
@LagginTimes
Copy link
Contributor

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

@evanlinjin evanlinjin moved this from Todo to In Progress in BDK Jul 11, 2024
@evanlinjin
Copy link
Member Author

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

@LLFourn
Copy link
Contributor

LLFourn commented Jul 12, 2024

We don't need it. Confirmation block and anchor block will be the same block after #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.

@LLFourn LLFourn mentioned this issue Jul 12, 2024
Open
@evanlinjin
Copy link
Member Author

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
Copy link
Member Author

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

@LagginTimes LagginTimes linked a pull request Jul 17, 2024 that will close this issue
Draft
3 tasks
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Jul 17, 2024
@notmandatory
Copy link
Member

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

@notmandatory notmandatory modified the milestones: 1.0.0-alpha, 1.0.0-beta Jul 19, 2024
@notmandatory notmandatory moved this from Needs Review to In Progress in BDK Jul 22, 2024
@notmandatory notmandatory modified the milestones: 1.0.0-beta, 2.0.0 Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LagginTimes LagginTimes

Labels
bug Something isn't working
Projects
BDK
Status: In Progress
Milestone
2.0.0
Development

Successfully merging a pull request may close this issue.

refactor(chain)!: Remove Anchor trait and make anchors unique to (Txid, BlockId) LagginTimes/bdk
4 participants
@LLFourn @notmandatory @evanlinjin @LagginTimes