Make chain-sources non-dependent on a specific bdk_chain version.

[evanlinjin]

Describe the problem

We may want to be able to iterate fast on bdk_chain without breaking Wallet. This means Wallet may be of an older version of bdk_chain. Therefore, Wallet (currently) will have to use older versions of chain sources (since our chain sources depend on bdk_chain).

It will be a nice property if projects that use BDK can stay put on a specific bdk_chain version, but still use the latest chain-source crate version.

Additional context

Why would we need to break bdk_chain?

There are a couple a breaking changes that we simply do not have enough time to include in the v1.0-beta release.

1. refactor(chain)! removed IndexedTxGraph 🗑 #1510
   Get rid of IndexedTxGraph and just put the Indexer in TxGraph. This removes a bunch of overhead and is in general much cleaner.
2. refactor(chain)!: Remove Anchor trait and make anchors unique to (Txid, BlockId) #1515
   Removing the anchor trait, and representing the Anchor as a concrete type is much cleaner and a less error-prone API.

Both of these changes will break ChangeSet.

Proposed solution

Introduce bdk_core crate that moves structures and types from bdk_chain that will mostly stay constant.

• Types: BlockId, ConfirmationBlockTime.
• Updates: CheckPoint, introduce tx_graph::Update (instead of using TxGraph to update TxGraph).
• Spk-client types: FullScanRequest, FullScanResult, SyncRequest, SyncResult.

Chain sources will only depend on bdk_core. Note that we need to introduce tx_graph::Update though.

Potential downsides

bdk_core will still need to depend on bitcoin. Any major version updates to bitcoin will be a breaking change and we will need to introduce a new major version for bdk_core. To make these suggested changes useful, we need to lock down on specific major releases of bitcoin.

Acknowledgement

Thank you to @LLFourn for bringing up this idea.

[oleonardolima]

I'm not well versed on the bdk_chain, but it looks like a good approach.

Concept ACK, as long as we settle on the same rust-bitcoin version of our other crates and the ecosystem (0.32.x ?), in that way I think we "minimize" any further downsides.

I'll put more thought into it overnight, but we might've other structures/types that could be moved to bdk_core too 🤔.

[LagginTimes]

I'll work towards making a concept PR for this.

[notmandatory]

Rather than adding extra code/review work to make bdk_wallet and the chain source modules work on different versions of bdk_chain I think we should first focus on getting to the final bdk_wallet 1.0 API.

Once the final 1.0 is done we can start discussing what needs to go into the 2.0 milestone and even start working on some of the related redesigns in experimental PRs.

I may sound like a broken record on this, but I believe a final stable 1.0 is more important right now than a perfect, future proofed 1.0 API.

[LLFourn]

Rather than adding extra code/review work to make bdk_wallet and the chain source modules work on different versions of bdk_chain I think we should first focus on getting to the final bdk_wallet 1.0 API.

Not making bdk_core will necessarily require that bdk_wallet will have to stay stuck on old versions of the chain source modules since they currently depend on bdk_chain but bdk_wallet won't update its bdk_chain dependency of course. If you think that's acceptable we can close this but I wasn't sure that's what you were saying.

[evanlinjin]

Thinking ahead here. We are moving CheckPoint into bdk_core. In the future, we want to make CheckPoint generic so that we can embed more data into it (other than just the height and block hash).

pub struct CheckPoint<B = BlockHash> {
    // private fields...
}

We want to update CheckPoint by only introducing a minor version bump to bdk_core.

This can be done by:

1. Adding a defaulted type parameter.
2. Adding new public items.

This means we will NOT be able to remove/change our current method names for this minor version bump.

I'm wondering whether it will make sense to rename our CheckPoint methods now to more BlockId-specific names? (So that we have saner names for CheckPoint methods later on).

Method names to be changed:

• ::new -> ::from_block_id
• ::push -> ::push_block_id
• ::extend -> ::extend_with_block_ids

Also note that the CheckPoint::apply_changeset method should be turned into a standalone function in bdk_chain (this doesn't change the API since it's a private method anyway).

[LLFourn]

@evanlinjin err I don't understand .*_from_block_id. How can these methods make sense when you have to provide additional data. A BlockId wouldn't be sufficient to push then...

Also note that the CheckPoint::apply_changeset method should be turned into a standalone function in bdk_chain (this doesn't change the API since it's a private method anyway).

Yes and I don't think it should have anything to do with CheckPoint should just be on LocalChain somewhere.

[notmandatory]

I support the concept for this issue, I'm just not sure if we can get it coded, tested, reviewed, and merged this week. Otherwise we won't be able to include it in the beta.2 tag used for the 3rd party audit to start no later than Aug 26. And if not included in the review I wouldn't want to include it in the final 1.0 release. We should discuss on the dev call tomorrow.

[notmandatory]

I had a chat with @evanlinjin and @LLFourn and Evan is going to make a run at doing the refactoring and have it ready to review Thur/Friday, Lloyd will help us review with goal of having it ready for me to merge no later than Sunday.

Worst case if Evan finds the scope is too big to finish we we freeze 1.0 on it's own branch and this refactoring will have to be done as 2.0 changes