Update CoinsView::NeedsUpgrade to add additional checks #35271

Additional two checks after cursor->Valid():

Seek() in LevelDB finds the first key ≥ the target. If no DB_COINS entry exists but there happens to be another key whose byte value is greater than DB_COINS, cursor->Valid() returns true — even though no legacy coins are present. The original code would incorrectly signal an upgrade is needed. The new code explicitly verifies the found key's prefix is DB_COINS.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35271.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Thank you for your contribution, but I don't think this needs fixing. This is logic that has been there for a long time and has served its purpose as such. If a change to the db format is ever repeated, it can be re-investigated then.

EDIT: If there is a motivation for this in the form of an actual bug report or down stream issue, I'll be happy to re-open this.

This check disappeared in https://github.com/bitcoin/bitcoin/pull/24236/changes#diff-cafbe1353eff6084b73fd3b6c3dee603e0827348fdd2fe12dfad1e01003a84edR52-R60

Can you please explain why there happens to be another key whose byte value is greater than DB_COINS can happen for a Bitcoin Core-produced chainstate? Did you encounter this on an actual datadir, or is this based on reasoning from LevelDB's Seek() semantics? Could you please check historically whether any Bitcoin Core version ever wrote another chainstate key that sorts after DB_COINS? @maflcko, do you remember if #24236 intentionally simplified this because cursor->Valid() implied the only possible key at that position was a legacy DB_COINS key?

If we do decide this defensive change makes sense for corrupt/foreign/past/future keys, I think it would be useful to add a first commit documenting the current behavior:

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
--- a/src/test/coins_tests.cpp	(revision 10ca73c02cbff59f2134c0c7da3b8d0a7e727475)
+++ b/src/test/coins_tests.cpp	(revision 786432696f5e702529bbc8becd3483c8b595c929)
@@ -5,6 +5,7 @@
#include <addresstype.h>
#include <clientversion.h>
#include <coins.h>
+#include <dbwrapper.h>
#include <streams.h>
#include <test/util/common.h>
#include <test/util/poolresourcetester.h>
@@ -16,8 +17,10 @@
#include <util/byte_units.h>
#include <util/strencodings.h>

+#include <cstdint>
#include <map>
#include <string>
+#include <utility>
#include <variant>
#include <vector>

@@ -305,6 +308,25 @@

BOOST_FIXTURE_TEST_SUITE(coins_tests, BasicTestingSetup)

+BOOST_AUTO_TEST_CASE(coins_view_db_needs_upgrade)
+{
+    static constexpr uint8_t LEGACY_COINS_KEY{'c'};
+    static constexpr uint8_t INVALID_KEY{'x'};
+
+    auto needs_upgrade_with_first_key{[&](const char* name, uint8_t key) {
+        const fs::path path{m_args.GetDataDirBase() / name};
+        {
+            CDBWrapper db{{.path = path, .cache_bytes = 1_MiB, .wipe_data = true, .obfuscate = false}};
+            db.Write(std::make_pair(key, uint256{}), uint256{});
+        }
+        CCoinsViewDB coins_view{{.path = path, .cache_bytes = 1_MiB, .obfuscate = false}, {}};
+        return coins_view.NeedsUpgrade();
+    }};
+
+    BOOST_CHECK(needs_upgrade_with_first_key("coins_view_db_needs_upgrade_legacy", LEGACY_COINS_KEY));
+    BOOST_CHECK(needs_upgrade_with_first_key("coins_view_db_needs_upgrade_invalid", INVALID_KEY));
+}
+
struct UpdateTest : BasicTestingSetup {
// Store of all necessary tx and undo data for next test
typedef std::map<COutPoint, std::tuple<CTransaction,CTxUndo,Coin>> UtxoData;

which could be flipped on the second commit with the fix:

  BOOST_CHECK(!needs_upgrade_with_first_key("coins_view_db_needs_upgrade_invalid", INVALID_KEY));

Also, if the intended check is only the key prefix, GetKey() does not need to deserialize the whole (prefix, uint256) pair:

  uint8_t key;
  return cursor->Valid() && cursor->GetKey(key) && key == DB_COINS;

(But if the goal is instead to recognize only well-formed legacy coins keys, then the current pair decode is more precise.)

Thank you for your contribution, but I don't think this needs fixing. This is logic that has been there for a long time and has served its purpose as such. If a change to the db format is ever repeated, it can be re-investigated then.

EDIT: If there is a motivation for this in the form of an actual bug report or down stream issue, I'll be happy to re-open this.

Motivation is that it is needed downstream in Elements (https://github.com/tomt1664/elements/commit/697f03a17e714ebca15f732bae83b0f2e4f0c05a) and thought it may be relevant in other contexts. But fine if it is not appropriate to address here.

Thanks for the reply and motivation @tomt1664. I'll re-open this then, given that it is a rather small change.

Was there a corresponding value in Core for https://github.com/tomt1664/elements/blob/60cf64e07053b6a1bac89d9d3fa25f354ad82933/src/txdb.cpp#L36? Regardless, if we add it here the test key could be adjusted to static constexpr uint8_t INVALID_KEY{'w'}; adding a comment to the corresponding repo.

nit: the linter fails for the trailing spaces in return cursor->Valid() && cursor->GetKey(key) && key.first == DB_COINS;

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/25745179925/job/75607168072} _{LLM reason (✨ experimental): CI failed due to the trailing_whitespace lint check detecting trailing whitespace in src/txdb.cpp (line 39–40).}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>