wallet: don't read db every time that a new 'WalletBatch' is created #25383
Conversation
c5ef443 to
1f24a4f
Compare
|
Honestly this check doesn't even need to exist. I can't quite figure out why it was added in the first place as the code is actually traceable to the first commit in the project! I think it's related to something with using bdb as the transaction index back then. Nowadays, this record is only used to record the most recent client that has opened the wallet, and it is only relevant if the wallet was opened by 0.4 or 0.5 which had database encryption issues. As the version record is updated in |
|
Ha!, what an old gem hehe.
Yeah, that is cleaner 👌🏼. |
1f24a4f to
80400e9
Compare
|
Pushed. Ready to go. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
Better to perform the action only one time (during 'LoadWallet'). Where the value is being used.
80400e9 to
bda8ebe
Compare
|
Thanks for the feedback! Squashed #25383 (comment) inside bda8ebe. Now we will log both versions; the wallet version and the version of the last client who opened the db. |
The value was only being updated launching releases with higher version numbers and not if the user launched a previous release. Co-authored-by: MacroFake <falke.marco@gmail.com>
757592d to
c318211
Compare
|
ACK c318211 |
| @@ -909,7 +907,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) | |||
| if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000)) | |||
| return DBErrors::NEED_REWRITE; | |||
|
|
|||
| if (last_client < CLIENT_VERSION) // Update | |||
| if (!has_last_client || last_client != CLIENT_VERSION) // Update | |||
There was a problem hiding this comment.
Looks like this is changing the semantics of "last client". It used to be the newest version ever used, but now it is the most recent version used, newer or not. I'm not sure the latter is preferable - and even if so, maybe it should use a new key.
There was a problem hiding this comment.
Looks like this is changing the semantics of "last client". It used to be the newest version ever used, but now it is the most recent version used, newer or not. I'm not sure the latter is preferable
yeah, not sure if you have checked it already but we had the discussion above: #25383 (comment).
Comments there are very welcome, I'm fine either way.
I moved forward with what seemed to be the desired workflow for this variable (based on the code comments). At the end, as achow101 said, this variable is never actually used for anything important except for the 0.4 and 0.5 encryption fixes (on which, this will still be fine).
There was a problem hiding this comment.
@luke-jr I think the new behavior is actually an improvement and more correct than the previous. Arguably it's a bug fix.
Suppose a user creates a wallet in a version more recent than 0.5. They then downgrade to 0.4 and load that wallet and encrypt it there. They then open the wallet again in a newer version.
What we want to happen is that opening the wallet in the newer version will force a rewrite of the wallet because it was encrypted in 0.4 and has the bug where unencrypted keys are still in the database. However, because version is the most recent client that has opened the wallet, this does not happen and the wallet file contains the unencrypted keys perpetually. If 0.4 had the behavior implemented here, the version record would indicate that 0.4 had last opened the file and so the newer version would correctly do the database rewrite.
It is possible to test this by creating a wallet with 0.4, loading in a newer version, then loading it back into 0.4, encrypting it in 0.4, and loading it in a newer version. If you look at the data in the file, it will contain unencrypted keys in the slack space.
While this is a rather contrived scenario, if we do have a similar problem in the future, having the record be the last client would be more correct as to how it is being used.
…letBatch' is created c318211 walletdb: fix last client version update (furszy) bda8ebe wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on bitcoin#25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211 w0xlt: reACK bitcoin@c318211 Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
Found it while was working on #25297.
We are performing a db read operation every time that a new
WalletBatchis created, inside the constructor, just to check if the client version field is inside the db or not.As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached.