Clean up wallet_store initialization and error handling #5033
Clean up wallet_store initialization and error handling #5033pwojcikdev merged 6 commits intonanocurrency:developfrom
Conversation
8fae4a2 to
baa0611
Compare
Test Results for Commit 42ec5d3Pull Request 5033: Results Test Case Results
Last updated: 2026-02-17 11:27:15 UTC |
baa0611 to
fac8181
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors wallet initialization and error propagation by replacing wallet_store “out-parameter” error reporting with exceptions, and by initializing new wallets using a KDF-derived key for the default password (empty string) instead of the previous zero-key bootstrap + immediate rekey flow.
Changes:
- Replace
wallet_store/walletconstructors’bool & errorparameters with exception-based error propagation. - Initialize newly created wallets by deriving the default password key via KDF and encrypting the wallet key with it.
- Update wallet loading/creation/import paths and unit tests to match the new initialization and error-handling model.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| nano/node/wallet.hpp | Updates public APIs for wallet/wallet_store construction and introduces default_password. |
| nano/node/wallet.cpp | Implements exception-based initialization/import flow and switches new wallet initialization to KDF-derived default password. |
| nano/core_test/wallet.cpp | Updates tests to use the new constructors and verifies default-password behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nano/node/wallet.cpp
Outdated
| bool missing = false; | ||
| missing |= mdb_get (env.tx (transaction_a), handle, &mdb_version_key2, &junk) != 0; | ||
| missing |= mdb_get (env.tx (transaction_a), handle, &mdb_wallet_key_key, &junk) != 0; | ||
| missing |= mdb_get (env.tx (transaction_a), handle, &mdb_salt_key, &junk) != 0; | ||
| missing |= mdb_get (env.tx (transaction_a), handle, &mdb_check_key, &junk) != 0; | ||
| nano::store::lmdb::db_val rep_key (representative_special); | ||
| auto mdb_rep_key = nano::store::lmdb::to_mdb_val (rep_key); | ||
| missing |= mdb_get (env.tx (transaction_a), handle, &mdb_rep_key, &junk) != 0; | ||
| if (missing) | ||
| { | ||
| throw std::runtime_error ("Wallet is missing required entries"); | ||
| } |
There was a problem hiding this comment.
The post-import validation treats any non-zero mdb_get status as “missing required entries”. This conflates MDB_NOTFOUND with real LMDB errors (corruption, bad DBI, etc.) and will throw a misleading exception. Consider checking explicitly for MDB_NOTFOUND vs other error codes and including the LMDB error string when it’s not a simple missing-key case.
…ctor The JSON-based wallet_store constructor imports all entries (including the representative) from the provided JSON, so the representative parameter was unused.
Include the database name and mdb_strerror output in the exception message to aid operational debugging when initialization fails.
Both constructors can throw after initialize() has opened/created the LMDB database. Wrap the post-initialize body in try/catch to destroy the database before rethrowing, avoiding orphaned databases.
fac8181 to
42ec5d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nano::raw_key key; | ||
| key = entry_get_raw (transaction_a, nano::wallet_store::wallet_key_special).key; | ||
| wallet_key_mem.value_set (key); | ||
| attempt_password (transaction_a, default_password); |
There was a problem hiding this comment.
The wallet_key_mem is set twice in this constructor: once at line 158 (when creating a new wallet) and again here at lines 173-174 (regardless of whether the wallet is new or existing). For existing wallets, this second assignment is necessary, but for new wallets, line 158 already sets the same value. Consider checking if the wallet was newly created to avoid the redundant operation at lines 173-174 for new wallets, or document why this double-assignment is intentional for new wallets.
Replace wallet_store initialization error output parameters with exceptions for cleaner error propagation. Initialize new wallets directly with a KDF-derived default password (empty string) instead of bootstrapping with a raw zero key and immediately rekeying in a separate transaction.