wallet: Prepare for future upgrades by recording versions of last client to open and decrypt #32895
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32895. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
I wonder if this can be uncoupled from
|
|
Concept ACK. It shouldn't hurt to add more metadata to help detect when there might be a mix of old data in a newer format and new data in an older format in the same wallet. Conceptually, I think we could always detect these cases by just scanning the entire wallet for data that needs to be upgraded on loading. But having this extra metadata is nice because it can avoid slowing down wallet loading by avoiding unnecessary upgrade rescans. On the approach in e94ef51, I think it is ok, but would suggest some ways it might be improved:
|
Definitely, however I'd like to be able to reuse the existing "version" record which mostly has the desired behavior since v24.0. The detection only works because of older versions overwriting the field with their CLIENT_VERSION. Introducing a new field now means that we cannot detect when an upgraded wallet was opened in any version older than v30.0 (assuming this is merged for the next release). The timeline would be too soon for any newly introduced features in say v31.0 or v32.0 (and I've got a few things in mind already that would utilize this). I'm already not particularly thrilled by the fact that the current implementation can only detect down to 24.0 as I expect that users doing upgrade-downgrade-upgrade with even older versions is commonplace.
I don't think that is actually viable as it would largely end up performing the upgrade on all loads regardless of whether the upgrade has already been done. |
|
Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed. That is, old wallets will (obviously) continue to write their |
Sorry, I was responding to @ryanofsky's comment at the same time since they were very similar. I think that may be workable. |
e94ef51 to
f747394
Compare
|
I've updated the PR to do a combination of @maflcko's and @ryanofsky's suggestions. The new approach decouples I believe this preserves the behavior of detecting downgrades down to v24.0, while also giving us the flexibility of a new field and feature flags. |
f747394 to
5d367db
Compare
5d367db to
3662960
Compare
I believe that's only because the tasks were canceled by a new push. |
b0470f5 to
93d4e89
Compare
Instead of tying the last client record with the node version, introduce a separate wallet client version that will be increased as necessary when new automatic upgrades are introduced.
…ecord LastClientFeatures are feature flags indicating what automatic upgrade features supported by the last client that opened the wallet. The LAST_OPENED_FEATURES record stores these flags and must be set to match the exact features supported by the client that opens a wallet.
Set these flags to avoid the automatic upgrade after migrating.
93d4e89 to
bd59a25
Compare
When a wallet is automatically upgraded, we always do so in a way that allows the user to load their wallet in an older version. When the user loads their wallet into an upgraded version again, the wallet may not perform the automatic upgrade and end up with upgraded and non-upgraded material in the wallet.
If we write some information about the last client to open the wallet, we would be able to detect these upgrade-downgrade-upgrade situations and perform another upgrade if so. However, in order for this to work, we need to have implemented writing such information into versions prior to the ones which introduce new automatic upgrades.
We are already writing the CLIENT_VERSION into all wallets in a "version" record, although this record is not updated in the same way across all previous versions. Since v24.0, we are always updating the record when the client version differs, but prior to that, the record would only be updated when the client version is newer. Even so, we can use this record to store the last client version.
The first commit decouples the written client version from the node version. This aids in development of new features. The version is entirely decoupled from previous versions by requiring all versions to have bit 30 set, and the initial version being
0 | (1 << 30). The versions just need to be at least 299900, and forcing bit 30 to be set was an easy way to achieve this.However, using version numbers is not as flexible as feature flags, so the second commit introduces the concept of Wallet Client Features. These flags are distinct from the existing Wallet Feature Flags since they are conceptually different. These are written in a separate record, and the wallet client version is incremented to
(1 << 0) | (1 << 30).Lastly, since some upgrades may need private keys and can only occur after the wallet is decrypted, the third commit also adds a last decrypted features flags record to record the features of the last client to decrypt the wallet.
The result is that we will now be able to detect a downgrade down to v24.0 and be able to implement in parallel multiple features that require automatic upgrades.