#35271 Update CoinsView::NeedsUpgrade to add additional checks
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35271 Update CoinsView::NeedsUpgrade to add additional checks | Author silent 173d | 0 | Neutral | 2 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Tightens CCoinsViewDB::NeedsUpgrade() to verify that the key found by cursor->Seek() matches the DB_COINS prefix. Because LevelDB's Seek finds the first key greater than or equal to the target, a database containing keys sorting after DB_COINS but lacking any DB_COINS entries would previously return true from cursor->Valid().
Problem: Downstream forks or nonstandard chainstate databases with key prefixes sorting after DB_COINS could falsely signal that a legacy UTXO database upgrade is required.
Reviewability
Stale · Author silent 173d
No author activity for 173 days; a review may wait a long time for a response.
Agreement
Neutral
objections settled, nobody has spoken for the PR
Thread notes: sedited initially closed the PR questioning its usefulness, but reopened it once the author explained the downstream need in Elements. Questions from l0rinc on Core applicability were answered and followed up with test suggestions, so they are recorded as questions rather than objections.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (contributor) | objection, usefulness | unnecessary modification to long-standing database upgrade logic without motivation from a bug report or downstream issue | accepted · v1, not yet re-checked | changed | yes | 2026-05-12: '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.' Status from: 2026-05-12: sedited: 'I'll re-open this then, given that it is a rather small change.' |
Participants:
- l0rinc (contributor, question, 2 statements 2026-05-12 to 2026-05-12): asked whether Bitcoin Core databases ever write keys sorting after DB_COINS and suggested tests
- sedited (contributor, objection, 2 statements 2026-05-12 to 2026-05-12): closed PR as unnecessary logic, then reopened after author explained downstream Elements motivation
- maflcko (member, neutral, 1 statements 2026-05-21 to 2026-05-21): asked if author is still working on the PR or if it should be closed
Files
0 lines under test/bench/ci.
- src/txdb.cpp +2/-1
Card
The short text the ranking pass compares across PRs.
This PR adds a key-prefix verification check to CCoinsViewDB::NeedsUpgrade() so that LevelDB cursor->Seek() does not mistake a subsequent key for a legacy DB_COINS key. The issue was observed downstream in Elements rather than in Bitcoin Core chainstate databases. Reviewers questioned whether Core is affected, and while sedited reopened it as a small cleanup, the author has been silent for four months with maflcko inquiring whether to close it.
Data
thread read through 2026-05-21 (6 statements), head 697ffa196e
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: CI failed · draft
Categories
Validation
P4 · bug fix
Tightens legacy database upgrade detection to prevent false positives when nonstandard keys follow DB_COINS. The problem has negligible impact on standard Bitcoin Core databases and primarily addresses downstream forks or nonstandard chainstates, making it marginal for scarce review attention.
Membership: Touches src/txdb.cpp, modifying chainstate database logic in CCoinsViewDB::NeedsUpgrade().
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0