backport: merge bitcoin#23065, #23498, #23557, #23736, #23855, #21464, #24403, #24968, #24933, #25358, #27823, #28253, #28612 (auxiliary backports: part 22) #6530
Conversation
… throw an error
… fix feature_init.py file perturbation
WalkthroughThe pull request introduces a comprehensive set of changes across multiple files in the project, focusing on several key areas:
These modifications aim to enhance error reporting, improve wallet functionality, and provide more granular control over transaction management. Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 2
🧹 Nitpick comments (6)
test/functional/wallet_basic.py (1)
139-142: Consider asserting that the lock was cleared after restartAfter restarting the node, the test aims to verify that non-persistent locks are cleared. Adding an assertion to confirm that
unspent_0is no longer locked before re-locking it would enhance the test's robustness.test/functional/mempool_limit.py (1)
79-81: LGTM! Consider adding more edge cases.Good addition of the minimum value validation test. Consider also testing:
- Edge case exactly at 5 MB (should pass)
- Non-integer values
- Very large values
src/test/orphanage_tests.cpp (2)
38-44: Consider enhancing key generation security.While this is test code, consider using
CKey::MakeNewKey()instead of manual key generation for better maintainability and consistency with production code.
96-118: Consider parameterizing the test values.The magic numbers (2777 inputs, 10 iterations) could be defined as constants at the top of the file with explanatory comments about why these specific values were chosen.
test/functional/feature_init.py (1)
49-49: Consider making block verification parameters configurable.The hardcoded values for
-checkblocks=200and-checklevel=4could be class constants or test parameters for better maintainability.src/wallet/rpcwallet.cpp (1)
2314-2314: Consider enhancing duplicate lock error handling.The current check prevents duplicate non-persistent locks, but consider also handling the case where a persistent lock exists. This would provide clearer error messages to users.
- if (!fUnlock && is_locked && !persistent) { + if (!fUnlock && is_locked) { + if (!persistent) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked"); + } else if (pwallet->IsLockedCoinPersistent(outpt.hash, outpt.n)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked persistently"); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
configure.ac(1 hunks)doc/release-notes-6530.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/bitcoind.cpp(2 hunks)src/fs.cpp(2 hunks)src/init.cpp(4 hunks)src/interfaces/wallet.h(1 hunks)src/node/blockstorage.cpp(1 hunks)src/node/chainstate.cpp(2 hunks)src/node/chainstate.h(2 hunks)src/qt/coincontroldialog.cpp(2 hunks)src/rpc/client.cpp(1 hunks)src/test/denialofservice_tests.cpp(0 hunks)src/test/orphanage_tests.cpp(1 hunks)src/test/util/setup_common.cpp(1 hunks)src/txmempool.cpp(4 hunks)src/txmempool.h(2 hunks)src/util/sock.cpp(2 hunks)src/util/syserror.cpp(1 hunks)src/util/syserror.h(1 hunks)src/util/system.cpp(2 hunks)src/validation.cpp(6 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(5 hunks)src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(2 hunks)src/wallet/walletdb.cpp(3 hunks)src/wallet/walletdb.h(2 hunks)test/functional/feature_assumevalid.py(0 hunks)test/functional/feature_bip68_sequence.py(0 hunks)test/functional/feature_block.py(0 hunks)test/functional/feature_csv_activation.py(0 hunks)test/functional/feature_dersig.py(0 hunks)test/functional/feature_init.py(4 hunks)test/functional/mempool_limit.py(1 hunks)test/functional/mempool_updatefromblock.py(1 hunks)test/functional/p2p_invalid_block.py(0 hunks)test/functional/rpc_blockchain.py(1 hunks)test/functional/test_framework/test_node.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_resendwallettransactions.py(0 hunks)test/lint/lint-locale-dependence.py(3 hunks)
💤 Files with no reviewable changes (8)
- test/functional/wallet_resendwallettransactions.py
- test/functional/feature_assumevalid.py
- test/functional/feature_csv_activation.py
- test/functional/feature_dersig.py
- test/functional/p2p_invalid_block.py
- test/functional/feature_block.py
- test/functional/feature_bip68_sequence.py
- src/test/denialofservice_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- configure.ac
🔇 Additional comments (67)
test/functional/wallet_basic.py (10)
132-134: Test case correctly checks error when unlocking an unlocked outputThe test ensures that attempting to unlock an output that is not locked raises the correct RPC error, verifying the proper error handling for unlocking unlocked UTXOs.
135-138: Test case correctly checks error when locking an already locked outputThis test confirms that attempting to lock an output that is already locked raises the appropriate RPC error, ensuring that duplicate locks are correctly handled.
143-148: Test correctly verifies that unloading and reloading the wallet clears non-persistent locksThe test ensures that after unloading and reloading the wallet, non-persistent locks are cleared as expected, validating the behavior of wallet persistence.
149-152: Test verifies that re-locking an output persistently after non-persistent lock is allowedThis test confirms that an output can be locked non-persistently and then re-locked persistently without errors, demonstrating the flexibility of the locking mechanism.
153-156: Test correctly verifies that persistent locks survive node restartThe test ensures that a persistent lock remains effective after restarting the node by checking that attempting to lock the output again results in an error, confirming lock persistence.
157-161: Test confirms that persistent locks survive wallet unload and reloadThis test verifies that persistent locks are maintained after unloading and reloading the wallet, ensuring the lock persists across wallet operations.
162-165: Test ensures locked outputs are not used for transactionsThe test confirms that locked outputs are excluded from available funds, even when they are the only funds available, by checking that sending funds fails with an "Insufficient funds" error.
166-170: Test verifies that unlocking removes the persistent lockAfter unlocking the output and restarting the node, the test confirms that the persistent lock has been removed by checking that
listlockunspentreturns an empty list.
171-174: Reconnecting node 2 after restartsThe test reconnects node 2 to nodes 0 and 1 after restarts to ensure proper network synchronization and that subsequent tests run in a connected environment.
Line range hint
175-178: Test covers error handling for invalid transaction IDs and output indicesThe test suite includes cases for invalid transaction IDs (incorrect length, non-hexadecimal strings) and invalid output indices, ensuring robust error handling and proper validation of input parameters for the
lockunspentRPC method.src/txmempool.h (2)
683-701: Documentation accurately reflects the updatedUpdateTransactionsFromBlockmethodThe method documentation has been updated to include the new parameters
ancestor_size_limitandancestor_count_limit, providing clear descriptions of their purpose and ensuring that the function's behavior is well-understood.
848-882: Documentation and method signature forUpdateForDescendantsupdated appropriatelyThe method
UpdateForDescendantsnow includes additional parameters:descendants_to_remove,ancestor_size_limit, andancestor_count_limit. The comments have been updated to reflect these changes, with detailed explanations of preconditions, postconditions, and parameter roles, enhancing code readability and maintainability.src/txmempool.cpp (2)
Line range hint
127-171: Correctly handles iterator invalidation when marking descendants for removalThe
UpdateForDescendantsmethod has been updated to avoid directly removing transactions while iterating, which could invalidate iterators and lead to undefined behavior. By collecting transaction IDs that exceed ancestor limits into thedescendants_to_removeset, the method ensures safe and effective transaction removal after processing.
Line range hint
177-228: Properly updatesUpdateTransactionsFromBlockto remove marked descendants safelyThe
UpdateTransactionsFromBlockmethod now processes thedescendants_to_removeset after updating transactions, removing any transactions that exceed ancestor limits. This approach prevents iterator invalidation and ensures mempool consistency by deferring removal until it is safe to do so.src/validation.cpp (1)
4437-4439: LGTM: Proper null checks addedThe null checks for
chainstate.m_chain.Tip()andchainstate.m_chain.Tip()->pprevprevent potential null pointer dereferences, ensuring the code is robust.src/util/syserror.h (1)
1-16: Header filesyserror.his well-structuredThe new header file correctly declares the
SysErrorStringfunction with proper documentation and include guards. This addition enhances error handling by providing a thread-safe alternative tostd::strerror.src/util/syserror.cpp (1)
14-34: Cross-platform implementation ofSysErrorStringfunctionThe function correctly handles thread-safe retrieval of system error strings across different platforms, accounting for Windows and POSIX variants of
strerror_r.src/fs.cpp (1)
6-6: LGTM! Good improvement in error handling consistency.The change to use
SysErrorString(errno)instead of directstrerror(errno)calls improves consistency in error message generation across the codebase.Also applies to: 45-45
src/test/orphanage_tests.cpp (2)
20-36: LGTM! Well-structured test helper class.The
TxOrphanageTestclass provides clean access to internal state for testing purposes.
48-54: LGTM! Good documentation of non-deterministic behavior fix.Excellent comment explaining the specific seed choice and its impact on test coverage.
test/functional/feature_init.py (2)
129-130: LGTM! Good practice using backup/restore mechanism.The use of
shutilfor directory backup and restore ensures clean test state between runs.Also applies to: 144-147
135-140: Consider bounds validation for file operations.The
seek()andwrite()operations with random values should validate that:
- The seek position doesn't exceed file size
- The write operation doesn't exceed file system limits
test/functional/mempool_updatefromblock.py (1)
20-20: LGTM! Verify the ancestor count limit.The addition of
-limitancestorcount=100is appropriate for this test case.Run this script to check if this limit is consistent with other test configurations:
✅ Verification successful
Different ancestor count limits are intentionally test-specific
The value of 100 in
mempool_updatefromblock.pyis appropriate as it tests block processing with generous limits. Other tests use different values (5 in packages tests, 6/12 in wallet tests) to verify behavior under various constraints, which is the expected testing strategy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of ancestor count limits in test files # to ensure consistency across test configurations rg -A 1 "limitancestorcount" "test/functional/"Length of output: 1074
Script:
#!/bin/bash # Find the definitions of MAX_ANCESTORS_CUSTOM and chainlimit echo "=== Searching in mempool_packages.py ===" rg "MAX_ANCESTORS_CUSTOM\s*=" test/functional/mempool_packages.py echo -e "\n=== Searching in wallet_basic.py ===" rg "chainlimit\s*=" test/functional/wallet_basic.pyLength of output: 349
src/node/chainstate.h (2)
74-74: LGTM! Improved comment clarity.The comment update makes the explanation more concise and clearer.
146-147: Verify the impact of changing parameter types from unsigned to signed.The type change from
unsigned inttointforcheck_blocksandcheck_levelparameters could potentially allow negative values, which might lead to unexpected behavior.Let's verify how these parameters are used:
✅ Verification successful
Type change from unsigned to signed is safe and beneficial
The parameters are already being handled as signed integers through GetArg() and get_int(). The existing checks, such as comparison with MIN_BLOCKS_TO_KEEP, prevent any issues with negative values. This change actually improves type consistency in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how check_blocks and check_level parameters are used # to ensure negative values are properly handled # Search for check_blocks and check_level usage rg -A 5 "check_blocks|check_level" "src/"Length of output: 4392
test/lint/lint-locale-dependence.py (2)
55-57: LGTM! Well-documented exceptions.The additions to KNOWN_VIOLATIONS are properly documented:
src/util/system.cpp:.*fprintfis added as a known violationsrc/wallet/bdb.cpp:.*DbEnv::strerroris marked as a false positivesrc/util/syserror.cpp:.*strerrorincludes a clear instruction to useSysErrorStringoutside this function
154-154: LGTM! Improved function detection.The changes enhance the linting tool's effectiveness:
- Adding "strerror" to tracked locale-dependent functions
- Improved regex pattern in git grep command for better function detection
Also applies to: 228-228
src/wallet/walletdb.h (2)
77-77: LGTM! Added database key for locked UTXOs.The addition of
DBKeys::LOCKED_UTXOconstant follows the existing pattern of database keys.
223-225: LGTM! Added methods for persistent UTXO locking.The new methods
WriteLockedUTXOandEraseLockedUTXOprovide the necessary functionality for persistent UTXO locking, following the established pattern of Write/Erase pairs in the codebase.Let's verify the implementation of these methods:
✅ Verification successful
Implementation verified: UTXO locking methods follow established patterns
The implementation of
WriteLockedUTXOandEraseLockedUTXOcorrectly follows the codebase's established patterns for database operations, using proper error handling through WriteIC/EraseIC and consistent key-value storage structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of the new UTXO locking methods # to ensure proper error handling and consistency rg "WriteLockedUTXO|EraseLockedUTXO" "src/wallet/"Length of output: 569
Script:
#!/bin/bash # Get implementation details and context rg -B 2 -A 10 "WriteLockedUTXO|EraseLockedUTXO" "src/wallet/walletdb.cpp" # Look for similar Write/Erase patterns ast-grep --pattern 'bool WalletBatch::Write$_($$$) { $$$ }'Length of output: 626
Script:
#!/bin/bash # Check WriteIC/EraseIC usage patterns rg "WriteIC|EraseIC" "src/wallet/walletdb.cpp" -B 1 -A 1Length of output: 4412
src/bitcoind.cpp (2)
22-22: LGTM: Header inclusion for improved error handling.The addition of
util/syserror.haligns with the standardization of error handling across the codebase.
216-216: LGTM: Improved error string handling.Replacing
strerror(errno)withSysErrorString(errno)standardizes error reporting and potentially provides more detailed error messages.src/util/sock.cpp (2)
10-10: LGTM: Header inclusion for error handling.The addition of
util/syserror.his consistent with the standardization of error handling.
363-364: LGTM: Simplified error string handling.Replacing platform-specific error string handling with
SysErrorStringimproves code maintainability and consistency.src/rpc/client.cpp (1)
157-157: LGTM: Added new parameter for persistent UTXO locking.The addition of the "persistent" parameter (position 2) to the
lockunspentRPC method follows the correct parameter convention and enhances the UTXO locking functionality.src/Makefile.test.include (1)
142-142: LGTM: Added orphanage tests.The addition of
test/orphanage_tests.cppto the test suite ensures proper coverage for orphan transaction handling.src/node/chainstate.cpp (1)
264-265: Verify the parameter type change implications.The change from
unsigned inttointforcheck_blocksandcheck_levelparameters allows negative values. While this enables using-1to check all blocks (as seen in the test file), we should ensure that other negative values are properly handled.Run the following script to verify the usage of these parameters across the codebase:
✅ Verification successful
Parameter type changes are safe and intentional
The change from
unsigned inttointis verified to be safe because:
- Negative values for
check_blocksare intentionally supported (-1 means check all blocks)- Both parameters have proper validation:
check_blocksis validated against MIN_BLOCKS_TO_KEEP for pruned nodescheck_levelhas a documented range of 0-4🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for all usages of check_blocks and check_level parameters # to ensure proper handling of negative values. # Search for check_blocks parameter usage echo "Searching for check_blocks parameter usage:" rg -A 2 "check_blocks" --type cpp # Search for check_level parameter usage echo "Searching for check_level parameter usage:" rg -A 2 "check_level" --type cppLength of output: 3572
Script:
#!/bin/bash # Search for default and minimum value definitions echo "Searching for DEFAULT_CHECKBLOCKS:" rg "DEFAULT_CHECKBLOCKS" --type cpp echo -e "\nSearching for DEFAULT_CHECKLEVEL:" rg "DEFAULT_CHECKLEVEL" --type cpp echo -e "\nSearching for MIN_BLOCKS_TO_KEEP:" rg "MIN_BLOCKS_TO_KEEP" --type cppLength of output: 3393
src/interfaces/wallet.h (1)
139-139: LGTM! Interface changes enhance error handling and persistence control.The changes to
lockCoinandunlockCoinmethods improve the API by:
- Adding return values to indicate success/failure
- Providing control over lock persistence through the
write_to_dbparameterAlso applies to: 142-142
test/functional/rpc_blockchain.py (1)
74-82: LGTM! Improved readability and enhanced block verification.The changes:
- Improve readability by using a multi-line format for
extra_args- Add
-checkblocks=-1to verify all blocks during restartsrc/wallet/interfaces.cpp (2)
241-246: LGTM! Clean implementation of persistent locking.The implementation properly handles database operations using RAII with
unique_ptrfor theWalletBatch, and correctly implements the conditional persistence based onwrite_to_db.
247-252: LGTM! Consistent implementation of unlock operation.The implementation follows the same pattern as
lockCoin, using RAII for database operations.src/Makefile.am (1)
375-375: LGTM: Addition of syserror.h to core headersThe addition of
util/syserror.htoBITCOIN_CORE_His appropriate for integrating system error handling capabilities across the codebase.src/test/util/setup_common.cpp (2)
282-310: LGTM: Improved error handling in LoadChainstateThe changes enhance error handling by using descriptive variable names and explicit error checking. The use of
maybe_load_errormakes the error state more clear and maintainable.
312-324: LGTM: Added chainstate verificationGood addition of explicit chainstate verification with proper error handling. The callback for BLS state logging adds useful debugging information.
src/qt/coincontroldialog.cpp (2)
210-210: LGTM: Added persistence for coin lock stateAdded
write_to_db=trueparameter to ensure coin lock states are persisted to the database in the toggle lock functionality.
303-303: LGTM: Added persistence for coin lock stateAdded
write_to_db=trueparameter to ensure coin lock states are persisted to the database in the context menu lock functionality.src/node/blockstorage.cpp (1)
292-298: LGTM: Added block index continuity validationGood addition of block index continuity checking. This ensures there are no gaps in block heights during index loading, enhancing blockchain integrity validation.
test/functional/test_framework/test_node.py (1)
262-268: Improved error reporting for node startup failures.The changes enhance error handling by including stderr output in the error message when dashd fails to start. This provides better context for debugging node initialization failures.
src/wallet/walletdb.cpp (4)
50-50: LGTM: Added database key for locked UTXOs.The new
LOCKED_UTXOconstant follows the naming convention of other database keys.
312-315: LGTM: Method to persist locked UTXO state.The
WriteLockedUTXOmethod correctly usesWriteICto store the locked state with a value of '1'.
317-320: LGTM: Method to remove locked UTXO state.The
EraseLockedUTXOmethod correctly usesEraseICto remove the locked state.
723-728: LGTM: Loading locked UTXOs during wallet initialization.The code correctly reads locked UTXOs from the database and calls
LockCointo restore the locked state.src/util/system.cpp (2)
24-24: LGTM: Added include for system error handling.Added include for
syserror.hwhich provides improved error message formatting.
1496-1496: LGTM: Enhanced error message formatting.Using
SysErrorStringinstead of raw error code provides more descriptive error messages.src/wallet/wallet.h (2)
1035-1035: LGTM: Added method to recalculate mixed credit.The new method allows recalculation of mixed credit for a specific transaction.
1038-1040: LGTM: Enhanced coin locking methods.The changes improve the coin locking mechanism by:
- Adding batch support to
LockCoinandUnlockCoinfor efficient database operations- Adding return value to
UnlockAllCoinsto indicate successsrc/init.cpp (4)
69-69: LGTM: Header inclusion for system error handlingThe addition of
<util/syserror.h>is appropriate for the improved error handling implementation.
169-169: LGTM: Improved error message handlingReplaced
std::strerrorwithSysErrorStringfor more consistent error handling across the codebase.
1837-1875: LGTM: Enhanced error handling for chain state loadingThe changes improve error handling robustness by:
- Adding proper exception handling
- Maintaining consistent error propagation
- Ensuring clean error state management
1934-1957: LGTM: Enhanced error handling for chain state verificationThe changes improve error handling robustness by:
- Adding proper exception handling
- Maintaining consistent error propagation
- Ensuring clean error state management
src/wallet/rpcwallet.cpp (2)
2207-2209: LGTM! Clear and well-documented parameter addition.The help text clearly explains the behavior of persistent vs non-persistent locks, the parameter is well-defined with appropriate defaults, and examples demonstrate proper usage.
Also applies to: 2223-2223, 2237-2238
2257-2263: Verify error handling for database operations.While the implementation looks correct, we should verify proper error handling for database operations, especially for:
- Database write failures during persistent lock/unlock
- Database read failures when checking existing locks
Also applies to: 2327-2331
✅ Verification successful
Error handling for database operations is properly implemented
The implementation includes:
- Return value checks for database operations
- Exception handling through JSONRPCError
- Proper cleanup of database resources
- Atomic operations with proper locking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns around database operations rg -A 5 "WalletBatch|GetDatabase"Length of output: 75457
src/wallet/wallet.cpp (5)
4452-4461: LGTM! Well-structured credit recalculation method.The new
RecalculateMixedCreditmethod properly handles cache invalidation and transaction marking. Good practice to hold the cs_wallet lock.
4463-4472: LGTM! Enhanced LockCoin with batch support.The method now properly handles database operations through the batch parameter and correctly updates mixed credit state.
4474-4482: LGTM! Improved UnlockCoin with proper return value.Good enhancement to return success status and handle database operations through batch parameter. The method properly updates mixed credit state.
4485-4494: LGTM! Enhanced UnlockAllCoins with proper error handling.Good improvement to return success status and use batch operations for database updates. The method properly clears the locked coins set.
628-638: LGTM! Enhanced AddToSpends with batch support.The method now properly handles coin unlocking with batch operations, creating a temporary batch when needed.
doc/release-notes-6530.md (1)
1-15: LGTM! Clear and well-structured release notes.The release notes effectively communicate the changes to both RPC and GUI functionality, explaining the benefits of persistent UTXO locking.
| const uint64_t ancestor_count_limit = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); | ||
| const uint64_t ancestor_size_limit = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000; | ||
| m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_size_limit, ancestor_count_limit); |
There was a problem hiding this comment.
Potential mismatch in argument order for UpdateTransactionsFromBlock
The parameters ancestor_size_limit and ancestor_count_limit may be passed in the wrong order to UpdateTransactionsFromBlock. The function might expect ancestor_count_limit as the second parameter.
Apply this diff to fix the argument order:
- m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_size_limit, ancestor_count_limit);
+ m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_count_limit, ancestor_size_limit);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This diff is incorrect, UpdateTransactionsFromBlock() looks like this
Lines 177 to 181 in c4fd6de
It takes ancestor_size_limit first and ancestor_count_limit second, the proposed diff, claiming to fix an inversion, causes it.
There was a problem hiding this comment.
@kwvg You are absolutely right, I apologize for the incorrect suggestion. The current code has the correct parameter order matching the function definition. Thank you for catching this error and providing the reference to the source code.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.796Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (pindexFailure) { | ||
| return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use state.Invalid instead of error for setting validation errors
Using return error(...) is not recommended for error handling in this context. Instead, use state.Invalid(...) to properly set the validation state and provide a clear error message.
Apply this diff to modify the code:
- return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions);
+ return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "verifydb-failed",
+ strprintf("*** coin database inconsistencies found (last %i blocks, %i good transactions before that)",
+ chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Egregious errors are returned using error as it very likely would require user intervention, see the following snippets below.
Lines 4491 to 4493 in c4fd6de
Lines 4533 to 4536 in c4fd6de
Furthermore, CVerifyDB::VerifyDB() doesn't take a BlockValidationState as an argument, so the error wouldn't bubble up regardless.
There was a problem hiding this comment.
@kwvg You're absolutely right - I apologize for the incorrect suggestion. The use of error() is appropriate here since it's used for critical failures requiring user intervention, and VerifyDB() doesn't have a validation state parameter anyway. Thank you for the correction and examples.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:4514-4516
Timestamp: 2025-01-14T08:40:00.675Z
Learning: In Dash Core, error() is used (instead of state.Invalid()) to report critical failures that require immediate user intervention, particularly for database/disk errors and data corruption scenarios. This is an established pattern seen in validation.cpp.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Additional Information
CWallet::(Un)lockCoinis spun-off intoCWallet::RecalculateMixedCredit()and will retain its previous behavior of being unconditionally called (i.e. even if persistent (un)locking fails).Breaking Changes
None expected.
Checklist