Use single instance of CWalletDB in CSparkWallet #1239
Conversation
|
@psolstice Please rebase onto spark_ branch |
psolstice
force-pushed
the
spark_walletdb_refactor
branch
from
52643b3
to
a30481b
Compare
…er checks on all wallets
…h multiple wallets
WalkthroughThe recent updates focus on enhancing wallet management and database recovery within a cryptocurrency framework. Key changes include support for multiple wallets, streamlined wallet database operations, and improved error handling. The modifications span across initialization, shutdown processes, and wallet interaction mechanisms, aiming to refine the system's robustness and efficiency. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Actionable comments outside the diff hunks (9)
src/wallet/walletdb.cpp (1)
Line range hint
11-18: Consider encapsulating thenWalletDBUpdateCounterincrement logic in a separate function or using RAII to ensure consistency and reduce code duplication.+ void IncrementWalletDBUpdateCounter() { + nWalletDBUpdateCounter++; + }Then replace direct increments with calls to this function.
src/init.cpp (8)
Line range hint
10-10:
The handling of the-litemodeargument is correctly implemented.
Line range hint
10-10:
The handling of the-znodeargument and the associated logic to disable Dandelion for Znodes is correctly implemented.
Line range hint
10-10:
The initialization and validation of the Znode BLS private key are correctly implemented.
Line range hint
10-10:
The handling of the-torsetupargument and the configuration of network settings based on its value are correctly implemented.
Line range hint
10-10:
The initialization and configuration of networking components, including the connection manager and peer logic, are correctly implemented.
Line range hint
10-10:
The creation of the block import thread and the synchronization on the processing of the genesis block are correctly implemented.
Line range hint
10-10:
The initialization of ZMQ notifications and the scheduling of Dash-specific tasks are correctly implemented.
Line range hint
10-10:
The final steps of the initialization process, including wallet loading and data directory maintenance, are correctly implemented.
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (14)
- src/init.cpp (4 hunks)
- src/qt/bitcoin.cpp (1 hunks)
- src/spark/sparkwallet.cpp (14 hunks)
- src/spark/sparkwallet.h (5 hunks)
- src/util.cpp (1 hunks)
- src/wallet/db.cpp (3 hunks)
- src/wallet/db.h (3 hunks)
- src/wallet/rpcwallet.cpp (3 hunks)
- src/wallet/test/wallet_test_fixture.cpp (1 hunks)
- src/wallet/test/wallet_tests.cpp (7 hunks)
- src/wallet/wallet.cpp (8 hunks)
- src/wallet/wallet.h (4 hunks)
- src/wallet/walletdb.cpp (10 hunks)
- src/wallet/walletdb.h (5 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments not posted (41)
src/spark/sparkwallet.h (3)
24-24: LGTM! The change in parameter type is purely stylistic and does not affect functionality.
31-33: The removal ofCWalletDBparameters from functions and the introduction of aCWalletDB walletdbmember align with the PR's objectives to optimize database interactions.Also applies to: 48-48, 65-70, 90-90
139-139: Replacing thestrWalletFilemember with aCWalletDB walletdbinstance centralizes database interactions withinCSparkWallet, supporting the PR's goal of optimizing wallet performance.src/wallet/db.h (4)
59-59: The addition of therecoverFunc_typetypedef improves code readability and maintainability by providing a clear definition for the recovery function signature.
60-60: Modifying theVerifymethod to take therecoverFunc_typetypedef and an additionalout_backup_filenameparameter enhances its functionality.
94-142: The introduction of theCWalletDBWrapperclass with methods for database handling likely improves code organization and facilitates easier management of database interactions.
161-169: Additions to theCDBclass enhance its database management capabilities by providing methods for database recovery, periodic flushing, environment verification, and database file verification.src/wallet/walletdb.h (5)
189-208: The addition ofWriteICandEraseICprivate template functions encapsulates common patterns of writing/erasing and incrementing the update counter, enhancing code reuse and maintainability.
211-213: Modifying theCWalletDBconstructor to take aCWalletDBWrapperreference and initializebatchandm_dbwimproves the constructor's functionality by enhancing database operation management.
285-299: Updates to method signatures for functions likeFindWalletTx,ZapWalletTx,ZapSelectTx, and others align with the changes in database interaction patterns introduced by theCWalletDBWrapperand the new transaction control methods.
471-479: The addition of transaction control methodsTxnBegin,TxnCommit, andTxnAborttoCWalletDBenhances the robustness and flexibility of database operations.
481-482: The introduction ofbatchandm_dbwas private members inCWalletDBfacilitates more efficient and organized management of database operations and interactions with theCWalletDBWrapper.src/wallet/db.cpp (3)
143-149: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [146-159]
Consider adding a brief comment explaining the purpose and expected behavior of the
recoverFunc_typeparameter for future maintainability.
163-228: Ensure proper error handling and logging for file operations (rename, open) to handle potential failures gracefully.
435-438: LGTM!src/wallet/test/wallet_tests.cpp (2)
407-407: The replacement of::pwalletMain = &wallet;withvpwallets.insert(vpwallets.begin(), &wallet);aligns with the objective of supporting multiple wallets. Consider adding a comment explaining whyvpwallets.begin()is chosen for insertion to aid future maintainability.Also applies to: 477-477, 492-492, 505-505
429-437: The updates toBOOST_CHECK_EQUALcomparisons in test cases are correctly implemented to reflect the changes in wallet handling.src/qt/bitcoin.cpp (1)
483-486: Consider enhancing the GUI to fully support multiple wallets in the future. The current implementation only exposes the first wallet invpwalletsto the GUI.src/wallet/walletdb.cpp (5)
100-112: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [83-109]
Ensure that sensitive data, such as private keys, is securely handled and erased from memory after use to prevent potential security vulnerabilities.
1111-1136: Verify that the atomic operationfOneThread.exchange(true)and the subsequent checks provide the intended thread-safety and performance characteristics for database flushing.
1259-1268: Ensure that the database recovery process is thoroughly tested, especially the callback mechanism inRecoverto handle different types of data recovery scenarios.
1360-1363: Confirm that serialization and deserialization processes across the wallet database operations handle all edge cases and data types correctly to prevent data corruption.
100-112: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [83-109]
For functions dealing with critical wallet data, such as mnemonics and mint counts, ensure that error handling is robust and that any failures in these operations are appropriately managed to prevent data loss or corruption.
Also applies to: 132-143, 616-618
src/spark/sparkwallet.cpp (7)
16-16: The constructor initializes a thread pool without checking if the wallet database operations were successful. Ensure that resources are only allocated if initial checks pass.
65-65: Ensure that the explicit type casting toParallelOpThreadPool<void>*in the destructor is safe and necessary. Consider usingstatic_castfor clarity if the cast is indeed required.
65-65: Ensure that lockingcs_spark_walletin balance calculation methods likegetAvailableBalanceandgetUnconfirmedBalanceis sufficient for thread safety and consider potential performance implications of locking in high-frequency operations.
172-172: Verify that theupdatetDiversifierInDBmethod is called at the appropriate times when generating new addresses to ensure the diversifier is consistently updated in the database.
497-497: Ensure that fee calculation inCreateSparkSpendTransactionaccurately accounts for all transaction components and adheres to network requirements to prevent transaction rejection.
497-497: Review the coin selection logic inSelectSparkCoinsto ensure it efficiently selects coins, minimizing the change and the number of inputs used, which can impact transaction fees and privacy.
497-497: Ensure thatGetAvailableSparkCoinscorrectly filters coins based on spendability and coin control selections, avoiding the inclusion of coins that should not be spent.src/wallet/rpcwallet.cpp (2)
3471-3471: The removal ofwalletdbas a parameter inupdateMintcalls aligns with the PR's objectives and should simplify database interactions.Please verify the impact of this change on performance and ensure that it meets the expected improvements.
3502-3502: The consistency in removingwalletdbas a parameter inupdateMintcalls across different functions supports the PR's goal of optimizing database interactions.Please ensure that this change contributes positively to the performance as intended.
src/wallet/wallet.cpp (9)
63-63: Consider encapsulating the global variables into a singleton or a namespace to improve code organization and reduce global namespace pollution.
636-636: The update to plural form in the initialization message is appropriate given the support for multiple wallets.
638-665: Ensure that the error messages and conditions are clear and user-friendly, especially when dealing with file paths and wallet file verification. Consider abstracting some of this logic into helper functions for better readability and maintainability.
5994-5996: Verify thatpwalletdb->WriteAccountingEntryproperly handles the incrementednAccountingEntryNumberto ensure data consistency. Also, consider error handling for the push back operation tolaccentries.
7024-7024: Incrementing the update counter after scanning for wallet transactions and setting the best chain is a good practice for maintaining database consistency. Ensure thatIncrementUpdateCounteris thread-safe if concurrent access is possible.
7092-7097: The loop for creating wallets from files specified in-walletarguments is clear. Ensure thatCreateWalletFromFilehandles all possible error conditions and that the failure to create a wallet is communicated clearly to the user.
7119-7121: Setting a default wallet file and determining if multiwallet is enabled based on the number of-walletarguments is a straightforward approach. Ensure that the default behavior aligns with user expectations and documentation.
7130-7150: The parameter interaction logic correctly identifies conflicts and limitations when using certain flags with multiwallet. Ensure that these restrictions are documented and communicated to the user to avoid confusion.
7652-7652: Updating the mint state with spark coins is specific to the wallet's functionality. Ensure thatUpdateMintStateproperly handles all edge cases and that the coins are validated before being added to the wallet.
| // TODO: Some way to access secondary wallets | ||
| return vpwallets.empty() ? nullptr : vpwallets[0]; |
There was a problem hiding this comment.
Consider implementing a more robust solution for selecting wallets in GetWalletForJSONRPCRequest to support scenarios involving multiple wallets.
Would you like me to help design or implement this feature?
| mapMultiArgs[strArg].clear(); | ||
| mapMultiArgs[strArg].push_back(strValue); |
There was a problem hiding this comment.
Consider using mapMultiArgs[strArg] = {strValue}; for clarity and efficiency.
- mapMultiArgs[strArg].clear();
- mapMultiArgs[strArg].push_back(strValue);
+ mapMultiArgs[strArg] = {strValue};This refactoring simplifies the code by directly setting the vector to contain the single strValue, which is the intended outcome of clearing and then pushing back the value. It's more concise and potentially more efficient by avoiding the separate clear and push operations.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| mapMultiArgs[strArg].clear(); | |
| mapMultiArgs[strArg].push_back(strValue); | |
| mapMultiArgs[strArg] = {strValue}; |
| bool CWalletDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr) | ||
| { | ||
| return CDB::VerifyDatabaseFile(walletFile, dataDir, warningStr, errorStr, CWalletDB::Recover); | ||
| } |
There was a problem hiding this comment.
For the newly added BIP47 functionality, ensure that all new database operations are accompanied by comprehensive unit tests to verify correctness, especially given the complexity of BIP47 payment codes and privacy implications.
Would you like me to help generate unit tests for the BIP47 related database operations?
| bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr) | ||
| { | ||
| LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0)); | ||
| LogPrintf("Using wallet %s\n", walletFile); | ||
|
|
||
| // Wallet file must be a plain filename without a directory | ||
| if (walletFile != fs::basename(walletFile) + fs::extension(walletFile)) | ||
| { | ||
| errorStr = strprintf(_("Wallet %s resides outside data directory %s"), walletFile, dataDir.string()); | ||
| return false; | ||
| } | ||
|
|
||
| if (!bitdb.Open(dataDir)) | ||
| { | ||
| // try moving the database env out of the way | ||
| fs::path pathDatabase = dataDir / "database"; | ||
| fs::path pathDatabaseBak = dataDir / strprintf("database.%d.bak", GetTime()); | ||
| try { | ||
| fs::rename(pathDatabase, pathDatabaseBak); | ||
| LogPrintf("Moved old %s to %s. Retrying.\n", pathDatabase.string(), pathDatabaseBak.string()); | ||
| } catch (const fs::filesystem_error&) { | ||
| // failure is ok (well, not really, but it's not worse than what we started with) | ||
| } | ||
|
|
||
| // try again | ||
| if (!bitdb.Open(dataDir)) { | ||
| // if it still fails, it probably means we can't even create the database env | ||
| errorStr = strprintf(_("Error initializing wallet database environment %s!"), GetDataDir()); | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| bool CDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc) | ||
| { | ||
| if (fs::exists(dataDir / walletFile)) | ||
| { | ||
| std::string backup_filename; | ||
| CDBEnv::VerifyResult r = bitdb.Verify(walletFile, recoverFunc, backup_filename); | ||
| if (r == CDBEnv::RECOVER_OK) | ||
| { | ||
| warningStr = strprintf(_("Warning: Wallet file corrupt, data salvaged!" | ||
| " Original %s saved as %s in %s; if" | ||
| " your balance or transactions are incorrect you should" | ||
| " restore from a backup."), | ||
| walletFile, backup_filename, dataDir); | ||
| } | ||
| if (r == CDBEnv::RECOVER_FAIL) | ||
| { | ||
| errorStr = strprintf(_("%s corrupt, salvage failed"), walletFile); | ||
| return false; | ||
| } | ||
| } | ||
| // also return true if files does not exists | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Consider adding unit tests for CDB::VerifyEnvironment and CDB::VerifyDatabaseFile to ensure their functionality is correctly implemented and to catch any future regressions.
Would you like me to help with drafting these unit tests?
| @@ -18,6 +18,8 @@ | |||
| #include <boost/test/unit_test.hpp> | |||
| #include <univalue.h> | |||
|
|
|||
| extern CWallet* pwalletMain; | |||
There was a problem hiding this comment.
Remove the unnecessary extern CWallet* pwalletMain; declaration, as the code is moving away from using a global pwalletMain in favor of vpwallets.
| CWalletDB walletdb(strWalletFile); | ||
| this->strWalletFile = strWalletFile; | ||
|
|
||
| CSparkWallet::CSparkWallet(const std::string &strWalletFile) : walletdb(strWalletFile) { |
There was a problem hiding this comment.
Consider handling the case where walletdb initialization fails.
| walletdb.readDiversifier(lastDiversifier); | ||
| } | ||
|
|
||
| void CSparkWallet::updatetDiversifierInDB(CWalletDB& walletdb) { | ||
| void CSparkWallet::updatetDiversifierInDB() { |
There was a problem hiding this comment.
Fix the typo in the method name updatetDiversifierInDB to updateDiversifierInDB.
- void CSparkWallet::updatetDiversifierInDB() {
+ void CSparkWallet::updateDiversifierInDB() {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| void CSparkWallet::updatetDiversifierInDB() { | |
| void CSparkWallet::updateDiversifierInDB() { |
| @@ -502,7 +494,7 @@ CAmount CSparkWallet::getMySpendAmount(const std::vector<GroupElement>& lTags) c | |||
| return result; | |||
| } | |||
|
|
|||
| void CSparkWallet::UpdateMintState(const std::vector<spark::Coin>& coins, const uint256& txHash, CWalletDB& walletdb) { | |||
| void CSparkWallet::UpdateMintState(const std::vector<spark::Coin>& coins, const uint256& txHash) { | |||
There was a problem hiding this comment.
Consider refactoring CreateSparkSpendTransaction to improve readability and maintainability. The method is complex and handles multiple responsibilities, which could be separated into smaller, more focused functions.
| @@ -8,6 +8,8 @@ | |||
| #include "wallet/db.h" | |||
| #include "wallet/wallet.h" | |||
|
|
|||
| CWallet *pwalletMain; | |||
There was a problem hiding this comment.
Initialize the global variable pwalletMain to null to avoid potential undefined behavior.
- CWallet *pwalletMain;
+ CWallet *pwalletMain = nullptr;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| CWallet *pwalletMain; | |
| CWallet *pwalletMain = nullptr; |
PR intention
Performance optimization by using single instance of
CWalletDBCode changes brief
This is a first step in a refactor that strongly binds
CWallet/CSparkWalletandCWalletDB` togetherSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation