Found it while was working on #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.
320 | - fReadOnly = false; 321 | - Write(std::string("version"), CLIENT_VERSION); 322 | - fReadOnly = fTmp; 323 | + 324 | + // At the first db open 325 | + if (database.client_version == -1) {
Concept ACK. But if we're going to do this only once (which is a great idea), why not do it immediately after opening the database, instead of checking it for every batch?
Mainly because of how the flow is currently structured. We are actually checking if the db is open on every new batch, and opening it if not (that is done few lines above, at line 312).
To be a bit more specific: I was going to add it inside the BerkeleyDatabase::Open() method but.. as BerkeleyBatch contains the functions to access the db (Read/Write/Erase etc), was going to end up duplicating the code there (the BerkeleyBatch::ReadKey functionality).
Not sure if I should include it here but I think that a good next step would be to decouple the db.open call from the BerkeleyBatch constructor, so it only receives an already opened database. And in that way, the client version read could be decoupled as well and not be part of the constructor.
Fair enough. If you have a good reason for it it's okay with me. I was just asking. It's "only" the legacy wallet so doing any large refactors is probably not worth it.
yeah, no problem. The question was great to be sync about following-up improvements. The SQLite flow has some similarities.. (the db constructor is, mostly, a no-op as the db is always opened etc..).
plus, I'm grateful for your quick feedback :).
Approach ACK
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 LoadWallet, I think it is safe to remove this code and just update LoadWallet to write the record if it doesn't already exist.
Ha!, what an old gem hehe.
As the version record is updated in LoadWallet, I think it is safe to remove this code and just update LoadWallet to write the record if it doesn't already exist.
Yeah, that is cleaner 👌🏼.
Pushed. Ready to go.
908 | @@ -909,7 +909,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) 909 | if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000)) 910 | return DBErrors::NEED_REWRITE; 911 | 912 | - if (last_client < CLIENT_VERSION) // Update 913 | + if (!has_last_client || last_client < CLIENT_VERSION) // Update
Any reason to keep this check? If yes, maybe rename last_client to highest_client?
Any reason to keep this check?
The purpose of this process is store (first condition) or update (second condition) the version of the last client who opened the wallet db. Plus, we don't want to write to db if the value is already there.
maybe rename last_client to highest_client?
hmm, I don't think that highest_client represents what this variable really is. I mean, the comment that is above the variable declaration is already saying "// Last client version to open this wallet, was previously the file version number".
Which doesn't necessarily need to be the "highest" version.
So, I think that the name is fine. Maybe could change it to last_client_version but.. not sure, with the existent comment we should be fine.
Yeah, the comment seems wrong as well. If you open with Bitcoin Core 23.0, it will write the 23.0 CLIENT_VERSION. If you then open with 22.0, it will not update because last_client < CLIENT_VERSION evaluates to false?
oh yeah, that is also wrong. Updating.. :)
Hmm, indeed, that is a misnomer. The comment and variable name should be changed to reflect what it actually does.
Hmm, I was going into the opposite direction actually: update the client version value if it's different than the new version. So we always know (and can log) which soft version opened the wallet db last (something like this https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e).
Which "should" be helpful to debug issues.
Looking through the history of the record, it's always been the highest version rather than the last client, although it also is never actually used for anything important except for the 0.4 and 0.5 encryption fixes. Changing this to actually be last client is probably fine, I don't feel strongly either way.
884 | @@ -885,7 +885,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) 885 | 886 | // Last client version to open this wallet, was previously the file version number 887 | int last_client = CLIENT_VERSION; 888 | - m_batch->Read(DBKeys::VERSION, last_client); 889 | + bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); 890 | 891 | int wallet_version = pwallet->GetVersion(); 892 | pwallet->WalletLogPrintf("Wallet File Version = %d\n", wallet_version > 0 ? wallet_version : last_client);
unrelated: Any reason for the > 0 check? If the minimum version needed to read the wallet is 0, it should probably be printed as-is?
yeah, that doesn't make sense. Even 0.4.0 wallet version was 40000.
I think the reason was that there was a belief that the version could be less than 0, particularly for super old wallets that did not have the minversion record yet. But looking through the old code, I don't think that can happen, and this is really just a log line so it doesn't really matter.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
Thanks for the feedback!
Squashed #25383 (review) 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>
reACK https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e
The change since my last ACK makes sense if the purpose of last_client and CLIENT_VERSION is to store the last client that opened the database and not the highest, as suggested by the original code.
ACK c318211ddd48d44dd81dded553afeee3bc41c89e
906 | @@ -909,7 +907,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) 907 | if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000)) 908 | return DBErrors::NEED_REWRITE; 909 | 910 | - if (last_client < CLIENT_VERSION) // Update 911 | + if (!has_last_client || last_client != CLIENT_VERSION) // Update
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.
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 (review).
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).
@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.