wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

Any reason to ask translators to waste effort when these were previously untranslated and the new translations are unused?

Since these are utility functions, we might add them to the gui in the future. Or maybe add a gui to the wallettool to make it more user friendly. I wanted to make sure that we could use these in the gui if we wanted to.

I'd prefer to err on the side of not adding a translation message if it's not currently used. This can always be done when necessary.

I've changed these to use Untranslated.

style nit in commit "wallettool: ..."

A range based for loop does not need to be guarded by a size check, so you can remove the if (warnings.size() > 0)

Fixed.

In commit "Return false instead of asserting when a loaded tx isn't new" (21349ec568abd5b25b6ca722aa6a9df3037fa4e8)

This definitely needs a comment. I don't understand when this condition happens. Does it indicate corruption or is it a thing expected to happen in certain cases? Comment doesn't need to be involved but should give a clue about what is happening here, since by itself this is enigmatic.

The assert has been added in https://github.com/bitcoin/bitcoin/commit/65b9d8f8ddb5a838454efc8bdd6576f0deb65f6d?w=1 , assuming that adding it does not change behavior. Making it a return false would restore the previous behavior instead of crashing.

This condition can happen with corruption. It means that the db has the same tx in multiple records which shouldn't happen. However this isn't something we should be asserting for because it isn't a critical error where money can be lost. It should be recoverable with rescan, else zapwallettxs will fix it.

I've added a comment indicating this.

This shouldn't be corruption, or at least it seems to happen in normal operation. All that the test in #19078 does is call getnewaddress and sendmany

The fact that #19078 is hitting this is probably a result of the aggressive salvage which, afaik, sometimes creates corruption when there isn't corruption. It's probably from BDB shuffling data around (as part of the normal btree operations) and leaving behind records that are marked as deleted but not actually deleted. So the aggressive salvage still finds these "deleted" records.

In commit "Return false instead of asserting when a loaded tx isn't new" (fbe8816f1a95e266b59b39327da4374a0442e279)

The fact that #19078 is hitting this is probably a result of the aggressive salvage

Maybe this change is ultimately the right one, but until we clearly understand what is happening I think it would be better to either drop this commit and follow up in a separate PR, or update the commit to ensure the error is not ignored like:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -256,6 +256,7 @@ public:
     std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys;
     std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys;
     std::map<uint160, CHDChain> m_hd_chains;
+    bool corrupt = false;
 
     CWalletScanState() {
     }
@@ -289,6 +290,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                 if (!new_tx) {
                     // There's probably some corruption here since the tx we just tried to load was already in the wallet
                     // This error is recoverable with zapwallettxs and is not a major failure
+                    wss.corrupt = true;
                     return false;
                 }
                 ssValue >> wtx;
@@ -730,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
             {
                 // losing keys is considered a catastrophic error, anything else
                 // we assume the user can live with:
-                if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
+                if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY || wss.corrupt) {
                     result = DBErrors::CORRUPT;
                 } else if (strType == DBKeys::FLAGS) {
                     // reading the wallet flags can only fail if unknown flags are present

in commit b27b5f7307:

why is open_error ignored and overwritten?

Dunno. I've restored the previous behavior.

Are there any followups queued for this file, or did you want to fix this now?

wallet/wallettool.cpp:133:33: warning: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Wrange-loop-analysis]
                for (const auto warning : warnings) {
                                ^
wallet/wallettool.cpp:133:22: note: use reference type 'const bilingual_str &' to prevent copying
                for (const auto warning : warnings) {
                     ^~~~~~~~~~~~~~~~~~~~
1 warning generated.