Backwards compatibility of `bdk::ChangeSet` data structure

[evanlinjin]

Describe the enhancement

The bdk_wallet::ChangeSet is what the persistence interfaces with. As suggested by @tnull, this structure should be backwards compatible (in case we make changes to this structure).

An easy solution is to make bdk_wallet::ChangeSet a enum:

#[non_exhaustive]
pub enum ChangeSet {
    V0(ChangeSet0),
    V1(ChangeSet1), // in the future this may exist
}

// We always convert to the latest version
impl From<ChangeSet0> for ChangeSet1 { /* TODO */ }

[tnull]

Note that it would also be nice to consider whether even forwards compatibility could be guaranteed. It may make things a bit more complicated, but it's worth exploring, especially since these properties are transitive for any downstream users (e.g., LDK Node can only guarantee what BDK guarantees...).

An easy solution is to make bdk::ChangeSet a enum:

Mh, but then you'd need to define explicit migration functions to allow user migrating from V0 to V1? Another way would be to a) don't break compat for any existing fields and b) introduce any new fields as Options so that users upgrading could deal with 'old' objects in which the fields are still None.

[evanlinjin]

@tnull so you want an older version of BDK to understand changesets that a newer version of BDK outputs. Can you elaborate why this would be useful? Thank you

[tnull]

@tnull so you want an older version of BDK to understand changesets that a newer version of BDK outputs. Can you elaborate why this would be useful? Thank you

Forwards compatibility is really useful to allow downgrades for various reasons: say a user upgrades to a newer BDK version and then a bug is discovered. If the upgrade breaks serialization forwards compatibility they couldn't downgrade to get going again and would be stuck until BDK ships a fix. Similarly, it would be necessary to be a bit more forgiving when restoring from backups or when having multiple apps that operate on the same data set.

In LDK Node's case we for example plan to allow multi-device support synced via a VSS backend. Not having forwards compatibility of the serialized data would require our users to upgrade all apps and devices in lockstep as otherwise an outdated device would refuse to read any data written by the already upgraded devices.

As said above, I'm aware that it may complicate things, but it's at least worth considering, i.e., it should be a dedicated design choice not to provide these guarantees.

[LLFourn]

Thanks for bringing up this point. I think we should never change this for structures in bdk_chain once we have a proper release. Adding new functionality should be adding a new type. I think there are two remaining proposed changeset changes there:

1. LocalChain changes should be monotone bdk_wallet#148
2. Reintroduce descriptor data to bdk::Wallet persistence #1101

I'm not sure if we can apply the same policy to bdk::Wallet. If we do want to make changes there then we should indeed try and do it via optional fields at the end of the changeset.

I think forwards compatibility is possible but this depends on the application serializing each changeset in a particular way. Note in bdk_file_store we don't serialize changesets with a length so the decoder is expected to understand everything in a changeset. We could improve this by first bincode serializing Vec<u8> of the encoded entry which will prefix it with the length.

[tnull]

Note in bdk_file_store we don't serialize changesets with a length so the decoder is expected to understand everything in a changeset. We could improve this by first bincode serializing Vec<u8> of the encoded entry which will prefix it with the length.

Yeah, I think even independently from (forwards) compatibility guarantees, having some TLV-style encoding or at the very least adding the length sounds like a very good idea. Not having this really limits how you can make necessary changes to the data model in the future.

[notmandatory]

Do we really need forward and backwards compatibility baked into the data structures for persisted data? My understanding is that the data BDK persists should always able to be regenerated from the wallet/app descriptors and blockchain client. So in the case of a roll forward/back should be able to just do a fresh full scan.

Optionally some persistence modules like SQL based ones should be able to at least rolling forward new schema changes to work with new bdk::ChangeSet versions.

[notmandatory]

I added this to the alpha.4 milestone to decide at least if we're going to do this or not since it's a biggish functional change.

[tnull]

Do we really need forward and backwards compatibility baked into the data structures for persisted data? My understanding is that the data BDK persists should always able to be regenerated from the wallet/app descriptors and blockchain client. So in the case of a roll forward/back should be able to just do a fresh full scan.

I mean generally forwards compat. is a "nice-to-have", not a hard requirement. Still, ... nice to have it. ;)
And I agree that it doesn't have to be explicit in the data structures (in fact, none of the compat guarantees have to be), but if we want it, it should be clearly defined and documented, in the best case even (regularly) checked.

Optionally some persistence modules like SQL based ones should be able to at least rolling forward new schema changes to work with new bdk::ChangeSet versions.

Mh, but this is really about the data model itself, not the persistence layer. Of course the persistence layer could always provide migrations, but the idea compat. guarantees is exactly that the data model does not need an explicit migration step to go forwards/backwards.

[LLFourn]

Thanks for bringing up this point. I think we should never change this for structures in bdk_chain once we have a proper release. Adding new functionality should be adding a new type. I think there are two remaining proposed changeset changes there:

1. [LocalChain changes should be monotone bitcoindevkit/bdk_wallet#148](https://github.com/bitcoindevkit/bdk_wallet/issues/148)

2. [Reintroduce descriptor data to `bdk::Wallet` persistence bitcoindevkit/bdk#1101](https://github.com/bitcoindevkit/bdk/issues/1101)

I'm not sure if we can apply the same policy to bdk::Wallet. If we do want to make changes there then we should indeed try and do it via optional fields at the end of the changeset.

I just realized this suggestion doesn't make sense since we're using bincode. You indeed have to do what @evanlinjin suggested in the OP for backwards compat.