bitcoindevkit / bdk

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

Introduce `bdk_core` crate #1155

Closed evanlinjin closed 1 week ago

evanlinjin commented 11 months ago

Describe the enhancement

This will contain common types shared across bdk crates.

Use case

Better crate structure. Allows chain-source crates to be non-dependent on bdk_chain.

Additional context

danielabrozzoni commented 11 months ago

Concept ACK, the name bdk_types might be more explicative

LLFourn commented 11 months ago

I think this crate will include some logic and not just types. E.g. the CheckPoint I think is the main thing that's caused the desire to have this.

I did suggest this but fwiw I am not entirely convinced it's a good idea. In retrospect I like the idea of just not depending on any bdk crate and just using impl Iterator<Item=(u32, BlockHash)> instead of CheckPoint. I'd see if we cannot exhaust all other ways of doing this before adding this crate.

evanlinjin commented 11 months ago

Right now the RPC emitter must emit blocks contiguously (cannot skip blocks), because we are emitting (u32, Block) and we construct the chain update from the block header.

If the emitter emits (Checkpoint, Block) instead, we will have the flexibility to skip blocks.

I think the ability to skip blocks is useful for block-by-block chain sources (especially with CBF). Also, if we go along with #1079 and still want to ability to specify a start_height (instead of starting from genesis), we need this as well.

evanlinjin commented 11 months ago

I did suggest this but fwiw I am not entirely convinced it's a good idea.

@LLFourn can you please expand on why it's not a good idea? Thanks

LLFourn commented 11 months ago

I don't know that it's not a good idea I just am not convinced it is compared to other possible directions. Happy to leave this call to you if you are totally convinced that we cannot get a good API without it.

notmandatory commented 11 months ago

On the one hand I hate adding yet another crate to the project, but if these types will be more or less unchanging and will be needed for the CBF client then I support it. I also think it's easier to read and talk about code with specific types like (Checkpoint, Block) instead of dealing with all the places we have a u32 or u64 and having to think about is this a height or a timestamp or maybe some sort of index into transaction or block data, etc. Can we also put a type alias for u64 unix epoch timestamps here?

danielabrozzoni commented 11 months ago

On the one hand I hate adding yet another crate to the project

Important to note, the more crates we add, the slower the release process becomes. Which isn't a big deal but it's something to consider.

Edit: mhh or maybe if they are all in the same workspace it's just a matter of bumping the version in the Cargo.toml, but you can just do cargo publish once? So maybe it's not that bad?

notmandatory commented 11 months ago

Edit: mhh or maybe if they are all in the same workspace it's just a matter of bumping the version in the Cargo.toml, but you can just do cargo publish once? So maybe it's not that bad?

I agree it's not a big deal, especially if it's not a crate that changes very much. If changed it's only a couple more steps to bump the version and publish. But keep in mind we'll have to publish each updated crate on it's own so we can do it in the right order, ie. "bdk_types", "bdk_chain", then the rest. At least that's what I've been doing for the alpha releases since I don't think cargo publish for the whole workspace can figure it out.

notmandatory commented 10 months ago

Since this is a functional change I moved it to the alpha.4 release.

evanlinjin commented 1 week ago

Done by #1569