wallet: Load database records in a particular order #24914

> explain query plan SELECT key, value FROM main WHERE key >= ? AND key < ?

Verified that this uses an index avoiding a table scan

SEARCH TABLE main USING INDEX sqlite_autoindex_main_1 (key>? AND key<?)`

👍 Like that approach

tiny nit: maybe GetNewCursor()?

Done. Also renamed GetPrefixCursor to GetNewPrefixCursor.

in 950c94f5:

Probably better an exception instead of an assertion (similar to what is being done in the SQLite method).

Done

Seems like the assertion is still there? Considering the documentation saying "The DB->cursor function may fail and return a non-zero error for errors specified for other Berkeley DB and C library or system functions." (http://web.mit.edu/ghudson/dev/nokrb/third/db/docs/api_c/db_cursor.html), I'd also agree that throwing an exception would be preferred.

In 040fa33f:

Bad copy here, error is DBErrors::CORRUPT.

The original error is incorrect, it's supposed to be TOO_NEW.

In commit "walletdb: Refactor wallet flags loading" (c825d7c85b0353bdfdf9968272bf5b6cc78c9f12)

The original error is incorrect, it's supposed to be TOO_NEW.

Would be good to note that this change is intentional in the commit message

Added a line in the commit message.

In a1d8d563:

Shouldn't this be a direct return DBErrors::CORRUPT as it was before? (same for the other one at line 807)

No, If you trace the code, it doesn't actually immediately return DBErrors::CORRPUT. Instead it does result = DBErrors::CORRUPT, continues with reading the rest of the records, and returns result towards the end.

num_keys isn't being increased.

Done

num_ckeys isn't being increased.

Done

tiny nit:

        LogPrintf("%s: Unable to execute cursor step: %s\n", __func__, sqlite3_errstr(res));

Done

Should log SQLiteCursor now, not SQLiteBatch.

        LogPrintf("%s: cursor closed but could not finalize cursor statement: %s\n", __func__, sqlite3_errstr(res));

Done

could delete this line, m_cursor will never be null.

Leaving it as belt-and-suspenders in case something happens to it in the future.

nit: what about making the cursor private and passing a sqlite3_stmt pointer to the constructor?

I tried doing that but kept running into issues with allocating the memory for it as the underlying object would have to survive the scope in which it was originally created. It was just easier to do it in the struct directly.

nit: this line can be removed.

Done

nit:

             "%s: Failed to setup cursor SQL statement: %s\n", __func__ sqlite3_errstr(res)));

Done

It's still there :grimacing:

nit: could default initialize m_first in the field declaration.

Done

In 72089765:

With this code, at the end of the range, Next returns one extra element pair that is not part of the range set. So, if the complete flag is not checked by the caller first and the ssValue tries to be unserialized, a runtime error will be thrown (different value type).

Would suggest to move this block above the ssKey and ssValue return values sets; so the function doesn't return elements that aren't in the filtered range, and directly return false:

Span<const std::byte> raw_key = {AsBytePtr(datKey.get_data()), datKey.get_size()};
if (!m_key_prefix.empty() && std::mismatch(raw_key.begin(), raw_key.end(), m_key_prefix.begin(), m_key_prefix.end()).second != m_key_prefix.end()) {
    complete = true;
    return false;
}

ssKey.SetType(SER_DISK);
ssKey.clear();
ssKey.write(raw_key);
// bla bla, all good..
return true;

Commit here: https://github.com/furszy/bitcoin/commit/e4b37e82ed19ff103f295ac5897e33e80e2f0679

Done

nit:

    ssKey.write(raw_key);

Done

this change shouldn't be needed.

Done

nit: unnecessary set. m_result member is default initialized to LOAD_OK.

Done

LoadRecords now is logging this:

            err = "Error reading wallet database: LegacyScriptPubKeyMan::LoadCScript failed\n";

Done here and elsewhere.

Why not return early here?:

if (key_res.m_result != DBErrors::LOAD_OK) {
    return key_res.m_result;
}

(Same for the crypted keys)

It is the retain the current behavior where we generally go through all the records in the wallet regardless of whether they are corrupt, and just report the worst result at the end. This allows us to check all of the records for corruption.

LoadRecords logs the corrupt errors.

                err = "Error reading wallet database: descriptor unencrypted key CPubKey corrupt\n";

(Same for the other ones below)

Same as above here:

spk_man->AddCryptedKey(pubkey.GetID(), pubkey, privkey);

Done

could remove this two loops, the map and vector need If the keys are added to the spkm inside each LoadRecords (check above, left a comment there).

Done

Missing corrupted_tx = true set here.

Done

        m_cursor = GetNewCursor();

I think this std::move on a temporary isn't needed? The compiler says (commit 94584601f12c51b3a781c84536f6b56be1ea8077)

In file included from ./wallet/walletdb.h:10:
./wallet/db.h:109:20: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
        m_cursor = std::move(GetNewCursor());
                   ^
./wallet/db.h:109:20: note: remove std::move call here
        m_cursor = std::move(GetNewCursor());
                   ^~~~~~~~~~              ~

nit in 10cf25c112ff4ac7bcece87a821996004cec2b24, less verbose

    auto cursor = std::make_unique<SQLiteCursor>();

Done

nit in e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be, less verbose

    auto cursor = std::make_unique<SQLiteCursor>(start_range, end_range);

Done

in 10cf25c112ff4ac7bcece87a821996004cec2b24: I know this hasn't been done in other RAII classes but wouldn't it be better to delete copy and copy assignment operators to avoid potential double frees?

Done in DatabaseCursor. AFAIK that should also delete in the child classes.

in e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be

    explicit SQLiteCursor(const std::vector<std::byte>& start_range, const std::vector<std::byte>& end_range)

Done

in c3181a344c7e80ca233be47e7b44a1074c382498 ssKey and strErr are now unused

Removed

in 5444f78824c8b44776acb3523c8a591d2abb570d

            auto [ins, inserted] = hd_chains.emplace(keyMeta.hd_seed_id, CHDChain());
            CHDChain& chain = ins->second;
            if (inserted) {

Done

in 5444f78824c8b44776acb3523c8a591d2abb570d

    if (!hd_chains.empty()) {

Done

in 5444f78824c8b44776acb3523c8a591d2abb570d

        for (const auto& [hd_seed_id, chain] : hd_chains) {
            if (hd_seed_id != pwallet->GetLegacyScriptPubKeyMan()->GetHDChain().seed_id) {
                pwallet->GetLegacyScriptPubKeyMan()->AddInactiveHDChain(chain);

Done

in 69f987e8ecbf8e8d3202c3ad118501aa1d6399e3: i had to do it

    return mkey_res.m_result;

Done

Considering prefix is a UTF-8 encoded string: https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/wallet/walletdb.cpp#L30-L61

is it useful to check for trailing 0xFF bytes? Simply incrementing the last byte could reduce code complexity.

    end_range.back() = std::byte(static_cast<unsigned char>(end_range.back()) + 1);

re: #24914 (review)

Considering prefix is a UTF-8 encoded string:

Just IMO, but I think it is better to have a little code complexity here and do prefix lookups correctly for all binary strings, than to make it subtly or unexpectedly fail for non-utf8 strings. There's a tradeoff but in this case it seems better to make the API implementation a little more complex if it can avoid making API usage more complex or error prone.

Alright, in this case I think a unit test with a prefix with a trailing 0xFF should be added.

Edit: this doesn't seem necessary considering #24914 (review).

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be)

The handling of this last condition doesn't seem right. For example if you are looking for keys with prefix X'FFFF', the right condition to check for is simply key >= X'FFFF'. It is not key >= X'FFFF' AND key < X'01FFFF'.

You could fix this by replacing this line with end_range.clear(). Then in the end_range.empty() case below, prepare the SQL statement WHERE key >= {begin_range} instead of WHERE key >= {begin_range} AND key < {end_range}.

This would fix the prefix scan for prefixes that consist of 0xff bytes. It would also fix the prefix scan for empty prefixes (by returning all rows), so you could drop the if (prefix.empty()) return nullptr; error above too.

Related to this, I previously implemented an sqlite prefix scan more simply using the sqlite instr function in https://github.com/bitcoin/bitcoin/commit/7a05b1dee2fa68b32bfb19e273fb55a5b3836a3e#diff-d43e46c8f7010dde905ec31057a1c5c2674df9be11683f98a21546377f2cd706 from #18608. But probably the approach in this PR is more efficient, unless the sql interpreter is doing some fancy rewriting.

Done as suggested.

In commit "wallet: Introduce DatabaseCursor RAII class for managing cursor" (10cf25c112ff4ac7bcece87a821996004cec2b24)

trailing whitespace

Fixed

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be)

Can prefix just be Span<const std::byte>? It seems like a prefix scan should only require an array of bytes to function, not a full CDataStream object, much less a nonconst one.

A CDataStream is needed to produce the correct prefix as all of the DBKeys are strings serialized with length prefixes. So CDataStreams need to be made for the prefixes anyways, and it's easier to just pass that straight in.

In commit "wallet: Have cursor users use DatabaseCursor directly" (5a8b0f16681bf8f755e84567e7ecee2feb28336c)

Code pre-exists this PR, and I guess it works, but this line should probably say if (complete || !ret). It's not clear that the Next function would or should failure if the end of the stream is hit.

Changing this also would make it clearer that this break is intended to handle two different conditions, and it would also make this code more similar to other code calling ReadAtCursor/Next

Done

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be)

Note to reviewers: this change seems safe because all of the cursor next callers except for one (in MigrateToSQLite) check the complete output variable before they use this return value.

I do think more ideally this function would return something like a util::Result<bool> or an enum { SUCCESS, FAILURE, DONE } so the failure and done cases don't overlap.

I think that's something that could be done in a followup.

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (e9b3bcb411b5742d6579d95b5e6aa2ad3f7592be)

I don't understand how datKey can be used to return the key data if it is now initialized to be a pointer to the m_key_prefix buffer instead of being a freestanding Dbt object that has the DB_DBT_MALLOC flag set and can allocate its own memory. If the keys are longer than the prefix how would they fit in datKey now?

Probably I am missing something if the code works, but I would expect it look more like:

if (!m_first && !m_key_prefix.empty()) {
    SafeDbt prefix{m_key_prefix.data(), m_key_prefix.size()};
    m_cursor->get(prefix, value, DB_SET_RANGE);
}
ret = m_cursor->get(datKey, datValue, first ? DB_CURRENT : DB_NEXT);
m_first = false;

In case I'm not missing something and there is a problem with this code, there was a BerkeleyBatch::ErasePrefix function in #18608 that did something very similar and could be an example.

AFAICT it just overwrites the datKey.data with a pointer to newly allocated memory. There's nothing that indicates that this doesn't work (i.e. no tests fail and wallets load succesfully).

AFAICT it just overwrites the datKey.data with a pointer to newly allocated memory. There's nothing that indicates that this doesn't work (i.e. no tests fail and wallets load succesfully).

I don't see how it could allocate memory if the DB_DBT_MALLOC flag is not set

I didn't get far enough in the PR to see if this has test coverage, but I guess you are pretty confident tests would fail if this was not working. It just seems like a mystery how it could work.

Considering that many prefixes are shorter than entire keys, if there was a problem here, I would expect deserialization of keys to be throwing exceptions, and that would cause any test that calls loadwallet to fail, which would result in a ton of test failures. Given that that doesn't happen, I don't think there's any issue here.

re: #24914 (review)

I don't see how it could allocate memory if the DB_DBT_MALLOC flag is not set

I went down a rabbithole trying to answer a question in another PR (https://github.com/bitcoin/bitcoin/pull/27224/files#r1150232826), and finally figured out how this could work...

It turns the DB_DBT_MALLOC flag is not a switch that controls whether BDB mallocs a new pointer for you or if it uses the pointer that you provide. BDB will malloc a new pointer whether or not you not pass DB_DBT_MALLOC. But you pass it, you are responsible for freeing the pointer, and if you don't pass it, BDB will manage the memory as part of the cursor object so you don't have to free it yourself. If you don't want BDB to malloc memory to return the key, you can pass it a different DB_DBT_USERMEM flag instead.

This does raises the question of whether it is ok to call memory_cleanse on memory that is owned by the cursor and should probably be read-only. But maybe it doesn't need to be read-only, or doesn't need to be read-only as long as you are done with the key and about to advance the cursor.

In 6106b02d:

No need to pass num_keys to this function. The variable is overwritten below with the key_res.m_records field (LoadRecords returns the read records number).

Done

In 6106b02d:

Same as above, no need to pass num_ckeys into this lambda.

Done

In 541cc8a7:

tiny nit:

            if (hd_seed_id != legacy_spkm->GetHDChain().seed_id) {
                legacy_spkm->AddInactiveHDChain(chain);

(could also save the active chain seed id into a variable so we don't call the getter on every iteration)

Done

In 6106b02:

This is a non-issue, just a conceptual talk: Shouldn't the cache be set after load the unencrypted/crypted keys?. Because, with it, we could validate that all the pubkeys inside the cache actually exist on the wallet at load time.

Perhaps, but it doesn't particularly matter. The cache is only used for lookup if available, it it's incorrect, it just means the things derivation needs are not available and so it can't derive.

Commit a0bc6bda272e7336deb55bdbf535534ed3a40306 currently doesn't compile, since the call to LoadHDChain(...) has too many arguments:

            if (!LoadHDChain(pwallet, ssValue)) return false;

Fixed.

nit in f91be084:

I'm not sure that this line make much sense. Return true without setting any key, value, nor the complete flag sounds like something that could cause a bug in the future. Might be better to just return false for now. (same for the DummyCursor class)

Done

small bug in b51b3fb4:

We just read the keymeta records, the result update has to be against keymeta_res.m_result, not script_res.

Fixed

In 327dfbfd:

Just loaded the locked utxo, this should be locked_utxo_res.m_result, not tx_res.m_result.

Fixed

CI is complaining because of this unused private field.

In c8c9b74f:

bug here, as the ret variable is initialized to true, need to set ret=false when the returned status is FAIL.

Done

Obviously it doesn't matter, since all tests still pass, but just for the sake of understanding the code better: is this change from returning true (compared to the earlier DummyBatch::ReadAtCursor method that is removed below) to false intended here? true would IMHO be a more logical choice for a cursor belonging to a database that never fails.

Check this #24914 (review). As we usually run this code inside a loop until it fails or return complete=true, if it never fails, then it can become a bug in the future.

    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<FailCursor>(); }

Otherwise, compiling the second commit (2c7b214cbc06ac07311add940925683c779e49b4) fails:

<details> <summary>Compiler output</summary>

  CXX      wallet/test/test_test_bitcoin-scriptpubkeyman_tests.o
In file included from wallet/test/wallet_tests.cpp:5:
In file included from ./wallet/wallet.h:10:
In file included from ./fs.h:8:
In file included from ./tinyformat.h:144:
In file included from /usr/include/c++/v1/algorithm:653:
In file included from /usr/include/c++/v1/functional:500:
In file included from /usr/include/c++/v1/__functional/function.h:20:
In file included from /usr/include/c++/v1/__memory/shared_ptr.h:25:
/usr/include/c++/v1/__memory/unique_ptr.h:728:32: error: no matching constructor for initialization of 'wallet::wallet_tests::FailCursor'
    return unique_ptr<_Tp>(new _Tp(_VSTD::forward<_Args>(__args)...));
                               ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
wallet/test/wallet_tests.cpp:891:75: note: in instantiation of function template specialization 'std::make_unique<wallet::wallet_tests::FailCursor, bool &>' requested here
    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<FailCursor>(m_pass); }
                                                                          ^
wallet/test/wallet_tests.cpp:870:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'bool' to 'const wallet::wallet_tests::FailCursor' for 1st argument
class FailCursor : public DatabaseCursor
      ^
wallet/test/wallet_tests.cpp:870:7: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided
1 error generated.

</details>

In 0e45584e: This emptiness check is done for sqlite but not for bdb. As this is a logical error, what if we throw an exception instead of returning a nullptr?

I've added the check to bdb. I've also changed the check to use Assume. This is basically equivalent with throwing an exception here since the only way to reach it is through programming errors.

In 0e45584e:

styling tiny nit (no need to do if you don't have to re-touch the area):

const char* stmt_text = end_range.empty() ? "SELECT key, value FROM main WHERE key >= ?" :
                            "SELECT key, value FROM main WHERE key >= ? AND key < ?";
res = sqlite3_prepare_v2(m_database.m_db, stmt_text, -1, &cursor->m_cursor_stmt, nullptr);

Done as suggested.

In 41445771:

This lock isn't needed. LoadKey, LoadCryptedKey, LoadEncryptionKey and LoadHDChain lock the wallet mutex internally.

Removed

In 9bd810a3:

Could return here, shouldn't continue iterating the db if a CORRUPT record was found.

No, this was intentional in order to avoid changing current behavior as little as possible. Right now, we will continue to read all records even if a corrupt one is found, so that all corrupt records can be found and reported to the user. So this doesn't return early in order to preserve that behavior.

Right now, we will continue to read all records even if a corrupt one is found, so that all corrupt records can be found and reported to the user

We print all corrupt records of one type, not all the corrupted records. Right now, some flows return due corruption right after finish reading all the records of one type, while others continue for a bit longer.

I'm just not so convinced that worth to log all corrupted records. I don't see much of a benefit for the user, might worth for debugging but in that case, might be better to have an RPC command or an external tool that scans the .dat file and dumps overall info about the wallet (including the corrupted info etc).

But no problem, all good if it's to preserve the current behavior. This PR is already doing a lot. Let's keep moving forward.

Side not, I just had a dejavú. Most likely, we had this convo earlier in the PR and I forgot it.. (my bad if that is the case).

re: #24914 (review)

In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)

We print all corrupt records of one type, not all the corrupted records

I guess I disagree a little, but I think it is good to log errors about everything we know is corrupt, even if we don't log errors about things we can't know are corrupt.

Regardless, if there is a change in behavior here I think it would be good mention what's changing in the commit message.

In 4f83673a:

tiny styling nit (no need to do it if you don't re-touch):

return LoadKey(pwallet, key, value, err) ? DBErrors::LOAD_OK : DBErrors::CORRUPT;

(same for the encrypted keys load)

Done

In cf4a765e:

This assertion is new to the code, could return a corruption error instead of abort the soft. (same applies to the similar assertions added to all the other descriptor records)

It wouldn't be corruption error, it would be a (major) bug in SQLite. We're requesting records by a prefix which includes the id, and then checking that that part of the record contains the same id. The only way this could go wrong is that the prefix retrieval is incorrect, so I think assert is appropriate here and for the other descriptor records.

yeah true, nvm then.

In 890143f2:

upgraded_txs is a return arg, needs to be passed by reference.

Fixed

In f76830e5:

This db cursor changes need to be in the first commit as it's currently not compiling.

Oops, fixed.

Note: If this is used in a read-write process, I'm quite sure that it will suffer from the same problematic tackled in #27556.

Done

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)

Would be good to log an error if this fails

The docs don't say what the return values may be. I think SQLite itself will also log an error using the ErrorLogCallback that we during initialization.

re: #24914 (review)

The docs don't say what the return values may be. I think SQLite itself will also log an error using the ErrorLogCallback that we during initialization.

Is it safe to assume from https://www.sqlite.org/rescode.html that this is supposed to return SQLITE_OK? I just think it's useful to at least log behavior we are not expecting, even if we don't know it is an error to help debugging. If you disagree though thats fine.

I think it's fine to leave it as is.

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)

C++ doesn't actually require that a byte is 8 bits, so it would be more correct to write this as if (*it == std::byte(std::numeric_limits<unsigned char>::max())) `

Done

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)

Using const references here forces vectors to be copied. Passing the argument by value would give the caller option to avoid copies:

    explicit SQLiteCursor(std::vector<std::byte> start_range, std::vector<std::byte> end_range)
        : m_prefix_range_start(std::move(start_range)),
        m_prefix_range_end(std::move(end_range))
    {}

Passing arguments by value instead of const reference is usually a good pattern for functions inserting the arguments into a data structure.

Done

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)

I think you can just do:

m_cursor_end = records.upper_bound(SerializeData(prefix.begin(), prefix.end()));

or

std::tie(m_cursor, m_cursor_end) = records.equal_range(SerializeData(prefix.begin(), prefix.end()));

And don't need the for loop.

I don't think upper_bound or equal_range will work. They will find the upper bound to be the first item that is greater than our prefix, but any record with the right prefix and is longer than the prefix will be greater than it, so we end up actually only getting the record(s) that exactly match the prefix.

re: #24914 (review)

I don't think upper_bound or equal_range will work. They will find the upper bound to be the first item that is greater than our prefix, but any record with the right prefix and is longer than the prefix will be greater than it, so we end up actually only getting the record(s) that exactly match the prefix.

Oops, you're right. But I think the following is an elegant way to make equal_range work (implementation based on https://stackoverflow.com/questions/44717939/elegant-way-to-find-keys-with-given-prefix-in-stdmap-or-elements-in-stdset):

diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index f14f451b52b8..0256d3fafbf6 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -61,25 +61,15 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type)
     return *Assert(w.GetNewDestination(output_type, ""));
 }
 
-MockableCursor::MockableCursor(const std::map<SerializeData, SerializeData>& records, bool pass, Span<std::byte> prefix)
+// BytePrefix object compares equal with other byte spans beginning with the same prefix.
+struct BytePrefix { Span<const std::byte> prefix; };
+bool operator<(BytePrefix a, Span<const std::byte> b) { return a.prefix < b.subspan(0, std::min(a.prefix.size(), b.size())); }
+bool operator<(Span<const std::byte> a, BytePrefix b) { return a.subspan(0, std::min(a.size(), b.prefix.size())) < b.prefix; }
+
+MockableCursor::MockableCursor(const MockableData& records, bool pass, Span<std::byte> prefix)
 {
     m_pass = pass;
-
-    // Start the cursor at the first value that is greater than or equal to the prefix
-    m_cursor = records.lower_bound(SerializeData(prefix.begin(), prefix.end()));
-
-    // The end cursor is the first item that is greater than or equal to the prefix + 1 (when interpreted as an integer)
-    SerializeData end_range(prefix.begin(), prefix.end());
-    auto it = end_range.rbegin();
-    for (; it != end_range.rend(); ++it) {
-        if (*it == std::byte(std::numeric_limits<unsigned char>::max())) {
-            *it = std::byte(0);
-            continue;
-        }
-        *it = std::byte(std::to_integer<unsigned char>(*it) + 1);
-        break;
-    }
-    m_cursor_end = records.lower_bound(end_range);
+    std::tie(m_cursor, m_cursor_end) = records.equal_range(BytePrefix{prefix});
 }
 
 DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value)
@@ -165,7 +155,7 @@ bool MockableBatch::ErasePrefix(Span<const std::byte> prefix)
     return true;
 }
 
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(std::map<SerializeData, SerializeData> records)
+std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records)
 {
     return std::make_unique<MockableDatabase>(records);
 }
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index 75f9b46c2a1d..3fdf40d13ccb 100644
--- a/src/wallet/test/util.h
+++ b/src/wallet/test/util.h
@@ -32,15 +32,17 @@ std::string getnewaddress(CWallet& w);
 /** Returns a new destination, of an specific type, from the wallet */
 CTxDestination getNewDestination(CWallet& w, OutputType output_type);
 
+using MockableData = std::map<SerializeData, SerializeData, std::less<>>;
+
 class MockableCursor: public DatabaseCursor
 {
 public:
-    std::map<SerializeData, SerializeData>::const_iterator m_cursor;
-    std::map<SerializeData, SerializeData>::const_iterator m_cursor_end;
+    MockableData::const_iterator m_cursor;
+    MockableData::const_iterator m_cursor_end;
     bool m_pass;
 
-    explicit MockableCursor(const std::map<SerializeData, SerializeData>& records, bool pass) : m_cursor(records.begin()), m_cursor_end(records.end()), m_pass(pass) {}
-    MockableCursor(const std::map<SerializeData, SerializeData>& records, bool pass, Span<std::byte> prefix);
+    explicit MockableCursor(const MockableData& records, bool pass) : m_cursor(records.begin()), m_cursor_end(records.end()), m_pass(pass) {}
+    MockableCursor(const MockableData& records, bool pass, Span<std::byte> prefix);
     ~MockableCursor() {}
 
     Status Next(DataStream& key, DataStream& value) override;
@@ -49,7 +51,7 @@ public:
 class MockableBatch : public DatabaseBatch
 {
 private:
-    std::map<SerializeData, SerializeData>& m_records;
+    MockableData& m_records;
     bool m_pass;
 
     bool ReadKey(DataStream&& key, DataStream& value) override;
@@ -59,7 +61,7 @@ private:
     bool ErasePrefix(Span<const std::byte> prefix) override;
 
 public:
-    explicit MockableBatch(std::map<SerializeData, SerializeData>& records, bool pass) : m_records(records), m_pass(pass) {}
+    explicit MockableBatch(MockableData& records, bool pass) : m_records(records), m_pass(pass) {}
     ~MockableBatch() {}
 
     void Flush() override {}
@@ -82,10 +84,10 @@ public:
 class MockableDatabase : public WalletDatabase
 {
 public:
-    std::map<SerializeData, SerializeData> m_records;
+    MockableData m_records;
     bool m_pass{true};
 
-    MockableDatabase(std::map<SerializeData, SerializeData> records = {}) : WalletDatabase(), m_records(records) {}
+    MockableDatabase(MockableData records = {}) : WalletDatabase(), m_records(records) {}
     ~MockableDatabase() {};
 
     void Open() override {}
@@ -105,7 +107,7 @@ public:
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<MockableBatch>(m_records, m_pass); }
 };
 
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(std::map<SerializeData, SerializeData> records = {});
+std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
 
 MockableDatabase& GetMockableDatabase(CWallet& wallet);
 } // namespace wallet
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
index 6823eafdfa7f..c1ff7baae117 100644
--- a/src/wallet/test/walletload_tests.cpp
+++ b/src/wallet/test/walletload_tests.cpp
@@ -83,7 +83,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup)
 {
     SerializeData ckey_record_key;
     SerializeData ckey_record_value;
-    std::map<SerializeData, SerializeData> records;
+    MockableData records;
 
     {
         // Context setup.

Adopted these suggestions.

In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)

I think you need to add a case like "\xff\xff" to test sqlite code without a key < end_range condition

Would also consider squashing this commit into earlier commit "wallet: Add GetPrefixCursor to DatabaseBatch" (57f35c90d2ebbc657a781ba3918398720b4319eb) because it's helpful to see test code calling a new function in the same commit that introduces the function.

Added the case and squashed.

In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)

Setting this flag seems unnecessary and unrelated to the test

Removed

In commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3)

Note: there is a slight change in behavior this commit which could be noted in the commit message.

If this deserialization fails, or if any error conditions are hit below, wss.nKeys++ will not be incremented anymore. This seems ok, but previously wss.nKeys++ included the total number of keys found, even ones that could not be loaded.

I've changed it to increment unconditionally as mentioned below. Ultimately, wss gets removed so I don't think the correctness of this counting particularly matters.

Additionally, the current behavior doesn't use the counts when there are failures, so having it be inaccurate in such cases shouldn't matter.

In commit "walletdb: Refactor crypted key loading to its own function" (59ac3bccffe3fda352dc068fa3a0b507ed9a73d6)

Similar to previous commit, now wss.nCKeys will not be incremented if this fails. Change in behavior could be noted in commit message.

Changed to increment unconditionally.

In commit "salvage: Remove use of ReadKeyValue in salvage" (c48aa9d0df7e34493a666c55e060304ef262d4b9)

No need to move KeyFilterFn here, can just drop it entirely:

diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 5d3ced0ad225..6ab29379fb26 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -461,22 +461,15 @@ bool LoadHDChain(CWallet* pwallet, CDataStream& ssValue)
     return true;
 }
 
-//! Callback for filtering key types to deserialize in ReadKeyValue
-using KeyFilterFn = std::function<bool(const std::string&)>;
-
 static bool
 ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
-             CWalletScanState &wss, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
+             CWalletScanState &wss, std::string& strType, std::string& strErr) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
 {
     try {
         // Unserialize
         // Taking advantage of the fact that pair serialization
         // is just the two items serialized one after the other
         ssKey >> strType;
-        // If we have a filter, check if this matches the filter
-        if (filter_fn && !filter_fn(strType)) {
-            return true;
-        }
         // Legacy entries in descriptor wallets are not allowed, abort immediately
         if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && DBKeys::LEGACY_TYPES.count(strType) > 0) {
             wss.unexpected_legacy_entry = true;

EDIT: I guess this is deleted later anyway, but it would be a little cleaner to remove it this commit.

Done

In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)

What's the reason for using (0, 0) flags here and other places instead of (SER_DISK, CLIENT_VERSION) flags used elsewhere? Having a code comment about this somewhere would be very helpful for understanding. It seems a little dodgy to get rid of SER_DISK flag if present or future code might rely on it.

For serializing the strings for the prefix, the stream type and client version don't matter so I just didn't use them.

You can just use DataStream for that, which comes with compile-time checks to enforce that both values are in fact never read, and thus can be omitted completely.

Also, in the same commit, is there a reason why you are changing DataStream ssKey to CDataStream ssKey? That seems like a step backward, no?

Good point.

I've changed the prefix to be made with a DataStream and for GetPrefixCursor to take Span<const std::byte> rather than a stream. Also updated to use DataStream key everywhere, I think that got lost in a rebase.

In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)

Would be helpful to have a code comment here saying enum number values are significant because if there are different error codes from reading different rows of the database, the error code with the highest number is the one that will be returned.

Added a comment.

In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)

It looks like in this commit, the side-effect in previous commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3) of changing the wss.nKeys value in case of key errors is partially reverted, because now the number of keys is incremented unconditionally, while the previous commit added more conditions.

Would suggest changing previous commit to just increment wss.nKeys++ unconditionally, so that behavior doesn't change here and this commit is simpler.

Same comment applies to wss.nCKeys and commit "walletdb: Refactor crypted key loading to its own function" (4940c1cb3dca97faa2b6a4ddfab889cd4b6f00f4)

The wss.nKeyMeta count also changes here if there is a deserialization exception. It would be good to note this in the commit message for this commit.

Changed to increment unconditionally, but these counters all get dropped in a later commit anyways.

In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (422a8436089c844934903a61fcec6b7b93995c07)

If prefix being non-empty is a requirement, it would be good to add this Assume to the MockableBatch cursor as well, so all of the implementations of this method consistently reject empty prefixes, and test cases don't appear to work with the mock database and then fail with real databases.

Alternately you could consider just moving the Assume checks out of the GetNewPrefixCursor methods into the into LoadRecords / LoadLegacyWalletRecords functions calling those methods. This would make the database code less fragile and basically catch same programming errors at a higher level in the wallet.

Moved the Assume to LoadRecords.

In commit "walletdb: Refactor key reading and loading to its own function" (82e025297ba8939660b15fad738be723d783165e)

Note for other reviewers: There appears to be a slight change of behavior because now wss.nKeys will be incremented if the public key is not valid. This doesn't actually change behavior because counts are not used if there are any failures:

https://github.com/bitcoin/bitcoin/blob/214f8f18b310e3af88eba6a005439ae423ccd76a/src/wallet/walletdb.cpp#L932-L933

Also wss variable is removed later anyway (https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200783492)

In commit "walletdb: Refactor hd chain loading to its own function" (f079b7580d4e7fa0d373decd1ffd7144abb83bab)

Seems ok, but is there a rationale for previous commits catching exceptions in the refactored functions and this commit not catching exceptions?

No, added the exception handling back.

In commit "salvage: Remove use of ReadKeyValue in salvage" (e9d974b7f4136c04f35091438f5e78e17d77299b)

I guess this will no longer catch std::exception, but that is ok?

Added it back.

In commit "walletdb: Refactor legacy wallet record loading into its own function" (b3425cc31feba14c5a0b0c4532437f12b5a901ff)

What its this change doing? It seems unrelated to the commit and maybe would be better as a separate commit with an explanation.

Don't remember, undone. Probably a bad rebase?

In commit "walletdb: refactor tx loading" (ad3ae8b6f4b2f3430b9d6c1b5bcf2627be15d961)

It's confusing that this commit leaves behind dead code implementing the same logic as the new code. Would suggest getting rid of it:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -300,11 +300,8 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output)
 
 class CWalletScanState {
 public:
-    bool fAnyUnordered{false};
-    std::vector<uint256> vWalletUpgrade;
     std::map<OutputType, uint256> m_active_external_spks;
     std::map<OutputType, uint256> m_active_internal_spks;
-    bool tx_corrupt{false};
 
     CWalletScanState() = default;
 };
@@ -1134,7 +1131,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 {
     CWalletScanState wss;
     bool fNoncriticalErrors = false;
-    bool rescan_required = false;
     DBErrors result = DBErrors::LOAD_OK;
     bool any_unordered = false;
     std::vector<uint256> upgraded_txs;
@@ -1205,17 +1201,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                 if (strType == DBKeys::MASTER_KEY ||
                     strType == DBKeys::DEFAULTKEY) {
                     result = DBErrors::CORRUPT;
-                } else if (wss.tx_corrupt) {
-                    pwallet->WalletLogPrintf("Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.\n");
-                    // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
-                    wss.tx_corrupt = false;
-                    result = DBErrors::CORRUPT;
                 } else {
                     // Leave other errors alone, if we try to fix them we might make things worse.
                     fNoncriticalErrors = true; // ... but do warn the user there is something wrong.
-                    if (strType == DBKeys::TX)
-                        // Rescan if there is a bad transaction record:
-                        rescan_required = true;
                 }
             }
             if (!strErr.empty())
@@ -1233,9 +1221,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
         pwallet->LoadActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, /*internal=*/true);
     }
 
-    if (rescan_required && result == DBErrors::LOAD_OK) {
-        result = DBErrors::NEED_RESCAN;
-    } else if (fNoncriticalErrors && result == DBErrors::LOAD_OK) {
+    if (fNoncriticalErrors && result == DBErrors::LOAD_OK) {
         result = DBErrors::NONCRITICAL_ERROR;
     }
 

Done as suggested.

In commit "walletdb: Refactor descriptor wallet records loading" (51518daa60952f31c34013c643feb8fe11c9777e)

This commit is leaving behind dead code, including another copy of this print statement. Would be better to get rid of it here:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -310,11 +310,7 @@ public:
     std::vector<uint256> vWalletUpgrade;
     std::map<OutputType, uint256> m_active_external_spks;
     std::map<OutputType, uint256> m_active_internal_spks;
-    std::map<uint256, DescriptorCache> m_descriptor_caches;
-    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;
     bool tx_corrupt{false};
-    bool descriptor_unknown{false};
 
     CWalletScanState() = default;
 };
@@ -1157,13 +1153,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
                     wss.tx_corrupt = false;
                     result = DBErrors::CORRUPT;
-                } else if (wss.descriptor_unknown) {
-                    strErr = strprintf("Error: Unrecognized descriptor found in wallet %s. ", pwallet->GetName());
-                    strErr += (last_client > CLIENT_VERSION) ? "The wallet might had been created on a newer version. " :
-                            "The database might be corrupted or the software version is not compatible with one of your wallet descriptors. ";
-                    strErr += "Please try running the latest software version";
-                    pwallet->WalletLogPrintf("%s\n", strErr);
-                    return DBErrors::UNKNOWN_DESCRIPTOR;
                 } else {
                     // Leave other errors alone, if we try to fix them we might make things worse.
                     fNoncriticalErrors = true; // ... but do warn the user there is something wrong.

Done as suggested.

In commit "walletdb: Refactor wallet flags loading" (ede0c95237e47de8a93af229f33f0cbedf5780fd)

It seems good that this commit is changing the error code from CORRUPT to TOO_NEW when there are recognized flags, but it is confusing that this still leaves behind more code below that attempts to do this same thing. It would be good to remove that code as part of this commit:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -880,9 +880,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                 // we assume the user can live with:
                 if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
                     result = DBErrors::CORRUPT;
-                } else if (strType == DBKeys::FLAGS) {
-                    // reading the wallet flags can only fail if unknown flags are present
-                    result = DBErrors::TOO_NEW;
                 } else if (wss.tx_corrupt) {
                     pwallet->WalletLogPrintf("Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.\n");
                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)

Done as suggested.

In commit "walletdb: Refactor legacy wallet record loading into its own function" (b3425cc31feba14c5a0b0c4532437f12b5a901ff)

There is still another LoadKey call above on line 544, so is this loading the keys twice?

https://github.com/bitcoin/bitcoin/blob/b3425cc31feba14c5a0b0c4532437f12b5a901ff/src/wallet/walletdb.cpp#L542-L544

It seems like the call on line 544 should be dropped at the same time this is added.

In general it would make this commit a lot clearer if it removed code that wasn't needed and didn't leave dead code behind. Would suggest:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -312,10 +312,8 @@ public:
     std::map<uint256, DescriptorCache> m_descriptor_caches;
     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 tx_corrupt{false};
     bool descriptor_unknown{false};
-    bool unexpected_legacy_entry{false};
 
     CWalletScanState() = default;
 };
@@ -470,10 +468,8 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
         // Taking advantage of the fact that pair serialization
         // is just the two items serialized one after the other
         ssKey >> strType;
-        // Legacy entries in descriptor wallets are not allowed, abort immediately
         if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && DBKeys::LEGACY_TYPES.count(strType) > 0) {
-            wss.unexpected_legacy_entry = true;
-            return false;
+            return true;
         }
         if (strType == DBKeys::NAME) {
             std::string strAddress;
@@ -541,7 +537,6 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
             wss.nWatchKeys++;
         } else if (strType == DBKeys::KEY) {
             wss.nKeys++;
-            if (!LoadKey(pwallet, ssKey, ssValue, strErr)) return false;
         } else if (strType == DBKeys::MASTER_KEY) {
             if (!LoadEncryptionKey(pwallet, ssKey, ssValue, strErr)) return false;
         } else if (strType == DBKeys::CRYPTED_KEY) {
@@ -1069,17 +1064,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
             std::string strType, strErr;
             if (!ReadKeyValue(pwallet, ssKey, ssValue, wss, strType, strErr))
             {
-                if (wss.unexpected_legacy_entry) {
-                    strErr = strprintf("Error: Unexpected legacy entry found in descriptor wallet %s. ", pwallet->GetName());
-                    strErr += "The wallet might have been tampered with or created with malicious intent.";
-                    pwallet->WalletLogPrintf("%s\n", strErr);
-                    return DBErrors::UNEXPECTED_LEGACY_ENTRY;
-                }
                 // losing keys is considered a catastrophic error, anything else
                 // we assume the user can live with:
-                if (strType == DBKeys::KEY ||
-                    strType == DBKeys::MASTER_KEY ||
-                    strType == DBKeys::CRYPTED_KEY||
+                if (strType == DBKeys::MASTER_KEY ||
                     strType == DBKeys::DEFAULTKEY) {
                     result = DBErrors::CORRUPT;
                 } else if (wss.tx_corrupt) {

Done as suggested.

In commit "salvage: Remove use of ReadKeyValue in salvage" (e9d974b7f4136c04f35091438f5e78e17d77299b)

It's unnecessarily complicated to call KeyFilter, then repeat the same filtering and raise an error if the filtering is different. Would suggest just getting rid of the KeyFilter function here:

--- a/src/wallet/salvage.cpp
+++ b/src/wallet/salvage.cpp
@@ -19,11 +19,6 @@ static const char *HEADER_END = "HEADER=END";
 static const char *DATA_END = "DATA=END";
 typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyValPair;
 
-static bool KeyFilter(const std::string& type)
-{
-    return WalletBatch::IsKeyType(type) || type == DBKeys::HDCHAIN;
-}
-
 class DummyCursor : public DatabaseCursor
 {
     Status Next(DataStream& key, DataStream& value) override { return Status::FAIL; }
@@ -192,9 +187,6 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
 
         // We only care about KEY, MASTER_KEY, CRYPTED_KEY, and HDCHAIN types
         ssKey >> strType;
-        if (!KeyFilter(strType)) {
-            continue;
-        }
         bool fReadOK = false;
         if (strType == DBKeys::KEY) {
             fReadOK = LoadKey(&dummyWallet, ssKey, ssValue, strErr);
@@ -205,8 +197,6 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
         } else if (strType == DBKeys::HDCHAIN) {
             fReadOK = LoadHDChain(&dummyWallet, ssValue);
         } else {
-            // This is a bug
-            CHECK_NONFATAL(false);
             continue;
         }
 
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -823,12 +823,6 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
     return true;
 }
 
-bool WalletBatch::IsKeyType(const std::string& strType)
-{
-    return (strType == DBKeys::KEY ||
-            strType == DBKeys::MASTER_KEY || strType == DBKeys::CRYPTED_KEY);
-}
-
 static DBErrors LoadMinVersion(CWallet* pwallet, DatabaseBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
 {
     AssertLockHeld(pwallet->cs_wallet);
@@ -916,7 +910,10 @@ 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 (strType == DBKeys::KEY ||
+                    strType == DBKeys::MASTER_KEY ||
+                    strType == DBKeys::CRYPTED_KEY||
+                    strType == DBKeys::DEFAULTKEY) {
                     result = DBErrors::CORRUPT;
                 } else if (wss.tx_corrupt) {
                     pwallet->WalletLogPrintf("Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.\n");
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -276,8 +276,6 @@ public:
     DBErrors LoadWallet(CWallet* pwallet);
     DBErrors FindWalletTxHashes(std::vector<uint256>& tx_hashes);
     DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut);
-    /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */
-    static bool IsKeyType(const std::string& strType);
 
     //! write the hdchain model (external chain child index counter)
     bool WriteHDChain(const CHDChain& chain);

Done as suggested.

In commit "walletdb: refactor active spkm loading" (8a4362f9bcf213164f43395faa164e50aab9363b)

There's still some dead code left behind this commit:

--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -300,8 +300,6 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output)
 
 class CWalletScanState {
 public:
-    std::map<OutputType, uint256> m_active_external_spks;
-    std::map<OutputType, uint256> m_active_internal_spks;
 
     CWalletScanState() = default;
 };

Done as suggested.

In 0506cc23: The key nor the value are used here but this should be (key, value) instead of (value, value).

oops, fixed

In c729f114: Since you removed the CHECK_NONFATAL call, this include is no longer needed.

Done