Merge bitcoin/bitcoin#36008: wallet: WalletBatch->WriteVersion respect argument
What the developer wrote
Authored by Ava Chow
Merge bitcoin/bitcoin#36008: wallet: WalletBatch->WriteVersion respect argument
ec5d19665b8935eabac36df4ec1ba2e19ee05c25 wallet: WalletBatch->WriteVersion respect argument. (David Gumberg)
Pull request description:
> Previously would use global `CLIENT_VERSION` no matter what, but this is one sense a refactor since all of the places where WriteVersion is called currently call it with `CLIENT_VERSION` anyways. The `client_version` argument is kept since future test code may want to write other versions.
> Addresses a review comment from [#32636](https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627):
This was originally pointed out in https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627, and the followup (#34490) was never merged. However I think it's confusing to have functions that take arguments but ignore them (and it's dead code), so I've cherry-picked the fix up from #34490.
ACKs for top commit:
achow101:
ACK ec5d19665b8935eabac36df4ec1ba2e19ee05c25
pablomartin4btc:
ACK ec5d19665b8935eabac36df4ec1ba2e19ee05c25
w0xlt:
ACK ec5d19665b8935eabac36df4ec1ba2e19ee05c25
Tree-SHA512: 3ad82d979493ac14704975bef504c791ac72fe7910c25097f0019bf61e3d384b0e1646f90c730d2f655733428b14ccd0196c333df2d6153ff5b55ab41918a6dd
What changed, and why it matters
This is a small code cleanup in Bitcoin Core's wallet database code. A function called WriteVersion was supposed to save a version number passed to it, but it was ignoring that input and always saving the current client version instead. The fix makes it actually use the passed-in value. All existing callers were already passing the current client version, so this does not change behavior today. It is described by the author as a refactor to remove confusing dead code and to allow future tests to write older versions safely.
No immediate action required. Treat as routine code-quality/refactor merge. If reviewing for backport, confirm all call sites pass CLIENT_VERSION (as stated by the author) and that no test or downstream code relies on the old ignored-argument behavior.
Security signals we found
Function argument was ignored (code-quality / latent bug)
No current caller passes a value other than CLIENT_VERSION
No validation, parsing, or trust-boundary logic changed
No advisory, CVE, or security discussion in commit or references
Evidence from the diff
In src/wallet/walletdb.h, WalletBatch::WriteVersion(int client_version) previously wrote CLIENT_VERSION to the database regardless of the client_version argument. The patch changes the implementation to write client_version. The PR description explicitly states this is effectively a refactor because every current call site passes CLIENT_VERSION, and the argument is retained for future testing. There is no evidence in the commit of a reachable vulnerability, incorrect version being written in production, or a security boundary being crossed.
Changed components
src/wallet/walletdb.hWalletBatch::WriteVersionInspect captured patch +8 / −2
### src/wallet/walletdb.h
@@ -271,8 +271,14 @@ class WalletBatch
DBErrors LoadWallet(CWallet* pwallet);
- //! Write the given client_version.
- bool WriteVersion(int client_version) { return m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); }
+ /**
+ * Write the given `client_version` to m_batch, indicating the last version
+ * of client software to load this wallet.
+ *
+ * @param[in] client_version `CLIENT_VERSION` outside of test code.
+ * @return A bool indicating whether or not the write succeeded.
+ */
+ bool WriteVersion(int client_version) { return m_batch->Write(DBKeys::VERSION, client_version); }
//! Delete records of the given types
bool EraseRecords(const std::unordered_set<std::string>& types);Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.