Fixes a regression from BDB wallets, where BDB generated a unique id for us.
This is needed for #19463, automatic backup reminders, and who knows what else in the future.
Ideally, we should probably also check if another copy of a wallet has already been loaded and warn the user, but that can be a follow-up PR.
287+ if (!batch->Exists("uuid")) {
288+ // UUID is missing (probably a new wallet)
289+ std::vector<unsigned char> new_uuid_buf(32);
290+ // We might not need strong random, but better safe than sorry
291+ // Can't make weak random stronger later
292+ GetStrongRandBytes(&new_uuid_buf[0], new_uuid_buf.size());
@achow101 Migration code just needs to be aware of how BDB stores the unique id. In any case, not related to this PR.
Disagree. The access to this ID should not be db specific. This id should not be dependent on the underlying database. Furthermore, migration via createfromdump (from #19137) won’t add the id as it is a strict record-for-record move.
I think the only db specific thing should be BDB adding the uuid record using the BDB generated id. Otherwise the wallet creation code should add an id if it doesn’t exist. This also makes it easy to add future db formats.
Weak concept NACK. When there’s a choice between (1) less code for greater usability and more use-cases and (2) more code for less usability and fewer use-cases, I try to choose (1)
I don’t see why it should be a problem to make two copies of a wallet and use it for different things. I don’t think we should warn when someone does that. I don’t think prune locks should use UUIDs. I think they should use wallet names so the locks are more understandable and easily managed by users. Wallet names are already part of the RPC interface, and they must be unique, and they are already a familiar concept to callers. No need to introduce a new concept that doesn’t have a good use-case and adds extra code, and then expose it externally through confusing warnings and pruning behavior.
I added the first use of BDB fileids in #11429 to avoid data corruption when multiple wallets with the same fileid are loaded into the same environment. There is no similar problem with sqlite and so “Fixes a regression” in the PR description is vague and misleading.
The access to this ID should not be db specific.
Debatable, but it currently is as a matter of fact. And again, unrelated to the issue addressed in this PR.
Furthermore, migration via createfromdump (from #19137) won’t add the id as it is a strict record-for-record move.
Sounds like a bug in createfromdump…
I don’t see why it should be a problem to make two copies of a wallet and use it for different things.
What did you have in mind? This is always broken behaviour, and has never been supported…
I don’t think prune locks should use UUIDs. I think they should use wallet names so the locks are more understandable and easily managed by users.
Wallet names are not unique and can be changed.
I think they should use wallet names so the locks are more understandable and easily managed by users.
Prune locks already have a human-readable description for this purpose. Regardless, it is just one example use case. We cannot reliably restore UUIDs later, but we can always ignore them later if they were to turn out not useful.
const char[5] instead of std::string
What did you have in mind?
Just basic file management. Maybe I want to make a bunch of wallets with the same password and import different keys into them. Maybe I want some wallets with the same keys. Maybe I want to set up a few wallets together on one machine and them copy them to different machines, and sometimes use them on the same machine again. Nothing impossible to work around, but it’s nice for wallets to behave like normal data files and be copyable, and I don’t think code should go out of its way to make wallet data more complicated or harder to manage.
This is always broken behaviour, and has never been supported…
It works fine, and I’m not saying go out of the way to add support. I’m saying don’t go out of the way to remove support.
Wallet names are not unique and can be changed.
It’s exhausting to discuss things like this with you Luke! We both understand they are unique at any point in time when they are loaded, and not unique at different points in time if they are unloaded and moved.
I think they should use wallet names so the locks are more understandable and easily managed by users.
Prune locks already have a human-readable description for this purpose. Regardless, it is just one example use case. We cannot reliably restore UUIDs later, but we can always ignore them later if they were to turn out not useful.
:+1:
If other people think this is a good idea, I’m not here to make an issue of it. Re-iterate weak nack.
It works fine, and I’m not saying go out of the way to add support. I’m saying don’t go out of the way to remove support.
This doesn’t remove support for anything at all.
It’s exhausting to discuss things like this with you Luke! We both understand they are unique at any point in time when they are loaded, and not unique at different points in time if they are unloaded and moved.
But it’s the latter case the UUID is needed to address.
Approach NACK
If we are going to have a wallet id, then support it properly. This means have it in CWallet, have a way to fetch it from CWallet, have the record in walletdb.cpp and writers for it in WalletBatch.
Furthermore, this PR is a massive layer violation. While I accept that we will have a layer violation here because of BDB (although I would prefer we don’t use the BDB id as a wallet id, @luke-jr insists that we must), the layer violation should be limited to just BDB. It should not be put somewhere that we plan on supporting for a long time. Put the layer violation in the module that’s slated for removal.
The wallet id should be at the wallet level, not the db level. The only thing that belongs in the db level is writing the BDB unique id as a wallet id record. Having SQLite write a newly generated record is not the correct way to implement a wallet id.
#20205 does this correctly.
It’s exhausting to discuss things like this with you Luke! We both understand they are unique at any point in time when they are loaded, and not unique at different points in time if they are unloaded and moved.
But it’s the latter case the UUID is needed to address.
Definitely not, at best they can be a partial solution and maybe help with a type of rename detection. There should be a GUI wallet rename feature and renamewallet API anyway to rename wallets explicitly and update external wallet references. Not just the ones in prune locks, but also references in the settings file, and references in memory if renaming is allowed without unloading and reloading. Any case it would be best to discuss this directly in the relevant issues.
