Replace trait `AnchorFromBlockPosition` with new struct #1594

jirijakes · 2024-09-08T08:20:31Z

Description

This change replaces a way of creating new generic anchor from block and its height. Previous way was using conversion trait, newly it is a struct and From.

Closes #1266.

Notes to the reviewers

Removed tx_pos: I did not see its relevance (Anchor does not give access to it). Was it a relic from the past or something I overlooked?
I put BlockPosition into tx_data_traits.rs because I believe it should be next to Anchor. But then it's not a module just for traits. What would be better place?

Checklists

All Submissions:

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

jirijakes · 2024-09-08T08:21:01Z

@evanlinjin @LLFourn, is this what you meant in #1266?

jirijakes · 2024-09-08T13:02:36Z

I am not sure about the failing test. Is there some flakiness? It ran alright in my fork and also my machine.

     Running tests/test_electrum.rs (target/debug/deps/test_electrum-5e966bdb43ca0ef0)

running 4 tests
test test_update_tx_graph_stop_gap ... FAILED
test test_update_tx_graph_without_keychain ... ok
test tx_can_become_unconfirmed_after_reorg ... ok
test test_sync ... ok

failures:

---- test_update_tx_graph_stop_gap stdout ----
Error: expected value at line 1 column 1


failures:
    test_update_tx_graph_stop_gap

error: test failed, to rerun pass `-p bdk_electrum --test test_electrum`
test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.73s

Error: Process completed with exit code 101.

crates/chain/src/indexed_tx_graph.rs

LLFourn

Concept ACK.

crates/chain/src/tx_data_traits.rs

LLFourn · 2024-09-10T00:26:27Z

crates/chain/src/tx_data_traits.rs

+/// Minimal set of parameters that are needed to construct a generic [`Anchor`].
+/// Typically used as an additional constraint on anchor:
+/// `A: Anchor + From<BlockPosition<'b>>`.
+pub struct BlockPosition<'b> {


I think this should be called BlockAndHeight and should be documented simply as that. I'm not sure it should be described as the "minimal" set of parameters. It's more like like the maximal amount of detail about the block that could be needed to make an anchor i.e. any anchor should be able to be made from a BlockPosition. We could even type constraint that Anchor: for<'a> From<BlockPosition<'a>>.

If we were to have a merkle-proof anchor (which I think we should have), don't we also need a tx position in block? I'm not too great with naming, but I'm thinking TxPosInBlock?

pub struct TxPosInBlock<'b> { block: &'b Block, height: u32, tx_pos: usize, }

Also, we are missing some derives. #[derive(Debug, Clone, Copy, PartialEq, Eq)] I think makes sense for this type.

Also, we are missing some derives. #[derive(Debug, Clone, Copy, PartialEq, Eq)] I think makes sense for this type.

Added, thanks.

~~All fields should be public as well, otherwise external modules/crates can't impl From<BlockPosition>.~~

Ohh I see u have a constructor and methods on it. I guess you did it this way instead of...

pub struct BlockPosition<'a> { pub block: &'a Block, pub height: u32, pub tx_pos: usize, }

So that the blockhash can be precalculated?

Hmm, it is more of my habit to start with hidden fields (in order to hide internals) and use getters. The blockhash was a bonus.

However, as BDK typically exposes publicly all fields, I guess it would be appropriate to follow also in this case.

We could even type constraint that Anchor: for<'a> From<BlockPosition<'a>>.

From<BlockPosition> is currently used only by two methods. Wouldn't that be too strict to require that of every future implementation of Anchor?

I'm not sure it should be described as the "minimal" set of parameters. It's more like like the maximal amount of detail about the block that could be needed to make an anchor

Mmm, yeah, I see how it can be unclear. My idea was “if you have at least these parameters at disposal, you can create any anchor.“ I will reword.

Yes @evanlinjin's design looks right to me.

From is currently used only by two methods. Wouldn't that be too strict to require that of every future implementation of Anchor?

I think if you can't get your anchor from the full block and position you may be using anchors in a way you probably shouldn't.

If we were to have a merkle-proof anchor (which I think we should have), don't we also need a tx position in block?

If we decide this new struct should include the position of the tx in block, then the original name BlockPosition would again makes sense to me but I won't belabor the point.

BlockPosition may be interpreted as position of block in the Blockchain though

crates/chain/src/indexed_tx_graph.rs

jirijakes · 2024-09-10T14:14:52Z

I reflected some of the comments but have not figured out the name yet. I will sleep on it and will see tomorrow if I come with a better one. Until then, I will keep it as draft.

jirijakes · 2024-09-11T12:21:02Z

Renamed to TxPosInBlock. Although this name still does not feel perfect, it is better than BlockPosition and I cannot think of other.
Reworded docomment.
I still kept TxPosInBlock's fields private, accessible via getters. It feels a bit better suitable for this purpose: with public fields instead of a constructor, one of the fields would be height and therefore block hash would have to be calculated on every access. Please let me know if public fields would be better regardless.

Thanks everybody for the comments!

oleonardolima

Concept ACK

I agree, the naming is hard. I wasn't able to come up with any other idea than TxBlockId (which I don't think really suits 😅)

evanlinjin · 2024-09-12T14:09:27Z

crates/chain/src/indexed_tx_graph.rs

+            graph.merge(self.graph.insert_anchor(
+                tx.compute_txid(),
+                TxPosInBlock::new(block, height, tx_pos).into(),
+            ));


This means we are constructing TxPosInBlock on every tx, thus re-computing the block hash for every tx... I have a couple of ideas to fix this.

Make all fields of TxPositionInBlock public and get rid of construction and getter methods.

Add a method to change the tx_pos. I.e. fn with_tx_pos(self, tx_pos: usize) -> Self.

I'm in favor of 1. because it is simpler and I don't see much value in having an API that enforces the consistency of the type. I don't see the caller constructing this type. The main usecase of this type (as I understand) is for implementing From<TxPositionInBlock> for anchors types that support it.

Make all fields of TxPositionInBlock public and get rid of construction and getter methods.

Made fields public as suggested.

Although that puts us back to the beginning (suggestion to pass only height, not BlockId), the repeated calculation of block hash should be avoided. Most likely why the creation of BlockId was in the original code in a first place.

jirijakes · 2024-09-13T09:42:35Z

Another round:

fields in TxPosInBlock public, one of them is block_id instead of height to avoid repetead recalculations of block hash
if we want to have all anchors implement From<TxPosInBlock>, as hinted by @LLFourn, I believe it could be done in a followup rather than here.

evanlinjin · 2024-09-13T15:55:03Z

I think it makes sense to include this change in v1.0 since this is a minor breaking change and the work is done.

The worst case scenario is that users that created custom anchors that used methods which required AnchorFromBlockPosition will need to change to impl From<TxPosInBlock>.

If we deem this breaking change as unacceptable, there is a way to make this non-breaking. We can keep AnchorFromBlockPosition and anything that impls it will impl From<TxPosInBlock>. We also mark AnchorFromBlockPosition as deprecated.

I'm in favor of just making this change breaking since it's so minor.

crates/chain/src/indexed_tx_graph.rs

ValuedMammal · 2024-09-14T18:02:18Z

How about calling it TxBlockPosition @jirijakes @evanlinjin

evanlinjin · 2024-09-19T09:34:10Z

How about calling it TxBlockPosition @jirijakes @evanlinjin

How about PositionInBlock? 😅

LagginTimes · 2024-09-19T09:35:27Z

How about calling it TxBlockPosition @jirijakes @evanlinjin

How about PositionInBlock? 😅

Maybe BlockTxPosition might be preferable? Or just TxPosition? 😅

evanlinjin

Looks good to me, just some doc changes and I'll happy to ACK.

There is a bit of bike-shedding going on with the naming.

I'm happy with how it is currently, or BlockTxPosition, or PositionInBlock, or TxBlockPosition. Whatever you choose, I will ACK.

crates/chain/src/tx_data_traits.rs

oleonardolima · 2024-09-19T20:10:55Z

Alongside Evan's comments, please do mind the commit message to follow https://www.conventionalcommits.org/en/v1.0.0/ too.

I'm fine with the name suggestions above, although I tend more toward either: TxPosInBlock (current) or TxBlockPosition (these seem clearer to me).

…ruct This change replaces a way of creating new generic anchor from block, its height and transaction position. Previous way was using conversion trait, newly it is a struct and `From`.

jirijakes · 2024-09-29T04:27:54Z

Addressed @evanlinjin's comments and changed commit message (hope it's right).

Apologies for the delay.

LagginTimes · 2024-09-29T16:16:12Z

Nit: I would probably reword the commit description to be something like:

This change revises how a generic anchor is created from a block, its
`BlockId`, and the transaction's position. The previous approach used a
conversion trait, while the new approach uses a struct and the `From`
trait.

LLFourn

ACK ab8068b

a trait is deleted I ack. Would like @evanlinjin and @LagginTimes to ACK too.

LagginTimes · 2024-09-30T07:11:09Z

ACK ab8068b

jirijakes marked this pull request as draft September 8, 2024 08:22

jirijakes force-pushed the issue/1266 branch from f2f74da to 26c5282 Compare September 8, 2024 08:25

jirijakes marked this pull request as ready for review September 8, 2024 08:31

ValuedMammal reviewed Sep 9, 2024

View reviewed changes

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

LLFourn reviewed Sep 10, 2024

View reviewed changes

jirijakes force-pushed the issue/1266 branch from 26c5282 to d1bd5d6 Compare September 10, 2024 14:00

jirijakes marked this pull request as draft September 10, 2024 14:00

jirijakes force-pushed the issue/1266 branch from d1bd5d6 to 7fa3043 Compare September 11, 2024 12:16

jirijakes marked this pull request as ready for review September 11, 2024 12:21

jirijakes force-pushed the issue/1266 branch from 7fa3043 to 4f46aeb Compare September 11, 2024 12:24

oleonardolima reviewed Sep 12, 2024

View reviewed changes

evanlinjin reviewed Sep 12, 2024

View reviewed changes

evanlinjin assigned jirijakes Sep 12, 2024

jirijakes force-pushed the issue/1266 branch 2 times, most recently from 07a8a27 to f96b705 Compare September 13, 2024 09:41

evanlinjin reviewed Sep 13, 2024

View reviewed changes

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

jirijakes force-pushed the issue/1266 branch from f96b705 to 424c05a Compare September 14, 2024 02:11

evanlinjin reviewed Sep 19, 2024

View reviewed changes

crates/chain/src/tx_data_traits.rs Outdated Show resolved Hide resolved

crates/chain/src/tx_data_traits.rs Show resolved Hide resolved

refactor(chain)!: Replace trait AnchorFromBlockPosition with new st…

ab8068b

…ruct This change replaces a way of creating new generic anchor from block, its height and transaction position. Previous way was using conversion trait, newly it is a struct and `From`.

jirijakes force-pushed the issue/1266 branch from 424c05a to ab8068b Compare September 29, 2024 04:25

ValuedMammal added this to the 1.0.0-beta milestone Sep 29, 2024

LLFourn approved these changes Sep 30, 2024

View reviewed changes

notmandatory merged commit 139d971 into bitcoindevkit:master Oct 1, 2024
21 checks passed

ValuedMammal mentioned this pull request Oct 2, 2024

Release 1.0.0-beta.5 #1638

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace trait `AnchorFromBlockPosition` with new struct #1594

Replace trait `AnchorFromBlockPosition` with new struct #1594

jirijakes commented Sep 8, 2024 •

edited

Loading

jirijakes commented Sep 8, 2024

jirijakes commented Sep 8, 2024

LLFourn left a comment

LLFourn Sep 10, 2024

evanlinjin Sep 10, 2024

evanlinjin Sep 10, 2024

jirijakes Sep 10, 2024

evanlinjin Sep 10, 2024 •

edited

Loading

jirijakes Sep 11, 2024

jirijakes Sep 11, 2024

LLFourn Sep 12, 2024

ValuedMammal Sep 13, 2024

evanlinjin Sep 13, 2024

jirijakes commented Sep 10, 2024

jirijakes commented Sep 11, 2024

oleonardolima left a comment

evanlinjin Sep 12, 2024

jirijakes Sep 13, 2024

jirijakes commented Sep 13, 2024

evanlinjin commented Sep 13, 2024

ValuedMammal commented Sep 14, 2024

evanlinjin commented Sep 19, 2024

LagginTimes commented Sep 19, 2024 •

edited

Loading

evanlinjin left a comment

oleonardolima commented Sep 19, 2024

jirijakes commented Sep 29, 2024

LagginTimes commented Sep 29, 2024

LLFourn left a comment

LagginTimes commented Sep 30, 2024

Replace trait AnchorFromBlockPosition with new struct #1594

Replace trait AnchorFromBlockPosition with new struct #1594

Conversation

jirijakes commented Sep 8, 2024 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

jirijakes commented Sep 8, 2024

jirijakes commented Sep 8, 2024

LLFourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlinjin Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirijakes commented Sep 10, 2024

jirijakes commented Sep 11, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirijakes commented Sep 13, 2024

evanlinjin commented Sep 13, 2024

ValuedMammal commented Sep 14, 2024

evanlinjin commented Sep 19, 2024

LagginTimes commented Sep 19, 2024 • edited Loading

evanlinjin left a comment

Choose a reason for hiding this comment

oleonardolima commented Sep 19, 2024

jirijakes commented Sep 29, 2024

LagginTimes commented Sep 29, 2024

LLFourn left a comment

Choose a reason for hiding this comment

LagginTimes commented Sep 30, 2024

Replace trait `AnchorFromBlockPosition` with new struct #1594

Replace trait `AnchorFromBlockPosition` with new struct #1594

jirijakes commented Sep 8, 2024 •

edited

Loading

evanlinjin Sep 10, 2024 •

edited

Loading

LagginTimes commented Sep 19, 2024 •

edited

Loading