backport: 0.25 batch 393 #6782
backport: 0.25 batch 393 #6782
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Warning Rate limit exceeded@PastaPastaPasta has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis set of changes introduces several refactorings, new tests, and minor improvements across the codebase. The wallet fuzz testing infrastructure is expanded by adding a new fuzz target for the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Complexity label: Moderate ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (3)
test/functional/test_framework/test_framework.py (1)
238-240: Logic is correct but could be clearer.The timeout factor handling logic works correctly:
- Line 238: Converts explicit 0 (disable timeouts) to 99999
- Line 240: Sets default values (4 for valgrind, 1 otherwise) only when timeout_factor is falsy (None)
However, the logic could be made more explicit to improve readability:
- if self.options.timeout_factor == 0: - self.options.timeout_factor = 99999 - self.options.timeout_factor = self.options.timeout_factor or (4 if self.options.valgrind else 1) + if self.options.timeout_factor == 0: + self.options.timeout_factor = 99999 + elif self.options.timeout_factor is None: + self.options.timeout_factor = 4 if self.options.valgrind else 1src/validation.h (2)
301-304: Consider making thetipparameter const.Since
CalculateLockPointsAtTipdoesn't modify the block index, the parameter should beconst CBlockIndex* tipfor better const-correctness.-std::optional<LockPoints> CalculateLockPointsAtTip( - CBlockIndex* tip, +std::optional<LockPoints> CalculateLockPointsAtTip( + const CBlockIndex* tip, const CCoinsView& coins_view, const CTransaction& tx);
312-316: Update outdated function reference in documentation.The documentation references "SequenceLocks()" which appears to be an old function name. Consider updating it to reflect the current implementation.
- * Simulates calling SequenceLocks() with data from the tip passed in. + * Evaluates sequence locks using the provided lock points against the given tip.Also, consider making the
tipparameter const:-bool CheckSequenceLocksAtTip(CBlockIndex* tip, +bool CheckSequenceLocksAtTip(const CBlockIndex* tip, const LockPoints& lock_points);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Makefile.test.include(1 hunks)src/rpc/client.cpp(4 hunks)src/rpc/client.h(2 hunks)src/test/miner_tests.cpp(1 hunks)src/test/rpc_tests.cpp(2 hunks)src/univalue/include/univalue.h(2 hunks)src/validation.cpp(5 hunks)src/validation.h(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/fuzz/coincontrol.cpp(1 hunks)src/wallet/wallet.cpp(4 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/rpc_tests.cppsrc/rpc/client.hsrc/wallet/spend.cppsrc/univalue/include/univalue.hsrc/test/miner_tests.cppsrc/wallet/wallet.cppsrc/wallet/test/fuzz/coincontrol.cppsrc/validation.hsrc/rpc/client.cppsrc/validation.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/rpc_tests.cppsrc/test/miner_tests.cppsrc/wallet/test/fuzz/coincontrol.cpp
src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Files:
src/univalue/include/univalue.h
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/test_framework.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
src/Makefile.test.include (6)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
src/test/rpc_tests.cpp (2)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/test/miner_tests.cpp (3)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #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.
src/wallet/wallet.cpp (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/wallet/test/fuzz/coincontrol.cpp (5)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: #6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
test/functional/test_framework/test_framework.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/validation.h (1)
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
src/rpc/client.cpp (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/validation.cpp (4)
Learnt from: kwvg
PR: #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.
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, IsVersionChangeValid() is only called by transaction types that update existing masternode entries (like ProUpServTx, ProUpRegTx, ProUpRevTx), not by ProRegTx which creates new entries. This means validation logic in IsVersionChangeValid() only applies to the subset of transaction types that actually call it, not all masternode transaction types.
🧬 Code Graph Analysis (4)
src/test/rpc_tests.cpp (1)
src/rpc/client.cpp (2)
ParseNonRFCJSONValue(293-298)ParseNonRFCJSONValue(293-293)
src/rpc/client.h (1)
src/rpc/client.cpp (2)
ParseNonRFCJSONValue(293-298)ParseNonRFCJSONValue(293-293)
src/test/miner_tests.cpp (1)
src/validation.cpp (4)
CalculateLockPointsAtTip(224-267)CalculateLockPointsAtTip(224-227)CheckSequenceLocksAtTip(269-285)CheckSequenceLocksAtTip(269-270)
src/validation.cpp (2)
src/consensus/tx_verify.cpp (4)
CalculateSequenceLocks(41-101)CalculateSequenceLocks(41-41)EvaluateSequenceLocks(103-111)EvaluateSequenceLocks(103-103)src/txmempool.cpp (2)
TestLockPointValidity(34-49)TestLockPointValidity(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (35)
src/Makefile.test.include (1)
224-226:wallet/test/fuzz/coincontrol.cppcorrectly added to the wallet fuzz suiteThe new harness is picked up by
FUZZ_WALLET_SRCalongside the existing wallet‐specific fuzzers, so it will be built wheneverENABLE_FUZZ_BINARYis on. No further changes required.src/wallet/spend.cpp (1)
779-783: Error message now usesstrprintf– looks goodSwitching to
error = strprintf(_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable %s."), "-fallbackfee");matches the style used elsewhere in the wallet codebase and keeps the string translatable.
No functional impact; compile-time types are unchanged becausestrprintfaccepts the translatable string and returns astd::string, which is implicitly convertible tobilingual_str.src/wallet/wallet.cpp (5)
2972-2981: Standardized error message for-fallbackfeelooks goodSwitching to
strprintf(_("Invalid amount for %s=<amount>: '%s'"), "-fallbackfee", …)aligns the message style with upstream and avoids the hard-coded argument name in the translation string. No further issues spotted.
2986-2995: Consistent pattern applied to-discardfeeThe new
strprintf-based construction mirrors the change above and removes the literal argument name from the translatable text. ✔️
3006-3011: Improved validation message for-paytxfeeUsing the parameterised format string keeps the message consistent with the other fee-related options. The min-relay-fee check remains intact. All good.
3020-3021: Warning string for high-maxtxfeeupdated correctlyThe
%splaceholder now receives the flag name, making translation cleaner. 👍
3023-3025: Error string for low-maxtxfeefollows the new schemeMessage is now fully parameterised and translated; logic unchanged. Looks fine.
test/functional/test_framework/test_framework.py (1)
224-224: Good refactoring: centralizing default value handling.Removing the default value from the argument parser allows for more explicit handling of the timeout_factor default logic in the post-processing code below.
src/wallet/test/fuzz/coincontrol.cpp (10)
1-11: LGTM! Clean includes and copyright header.The includes are appropriate for a fuzz test targeting
CCoinControlfunctionality.
12-16: LGTM! Proper namespace structure.The namespace usage and global setup pattern follows standard fuzz test conventions.
17-21: LGTM! Proper fuzz test initialization.The initialization correctly sets up the testing context without log files, which is appropriate for fuzzing.
23-28: LGTM! Standard fuzz target setup.The fuzz target initialization and data provider setup follows established patterns.
29-31: LGTM! Appropriate argument fuzzing.Setting
-avoidpartialspendsbased on fuzz data ensures testing both code paths that depend on this argument.
32-34: LGTM! Clean object initialization.The
CCoinControlandCOutPointinitialization is straightforward and appropriate.
35-37: LGTM! Appropriate fuzzing loop structure.The
LIMITED_WHILEwith 10,000 iterations provides good coverage while preventing infinite loops.
37-85: LGTM! Comprehensive API coverage with proper safety checks.The lambda functions provide excellent coverage of
CCoinControlmethods. The safety check in theGetInputWeightlambda (lines 82-84) properly avoids assertions by checkingHasInputWeightfirst.
63-64: LGTM! Proper CTxOut construction for fuzzing.Using
ConsumeMoneyandConsumeScriptto create realisticCTxOutobjects for theSelectExternaltest is appropriate.
88-90: LGTM! Proper namespace closure.The namespace closing follows standard formatting conventions.
src/test/miner_tests.cpp (1)
39-45: LGTM! Test correctly updated to use the new sequence lock API.The refactoring properly adapts the test to use the new two-phase sequence lock checking pattern, maintaining the same test semantics while following the updated validation API.
src/validation.h (1)
284-316: Well-designed refactoring of sequence lock handling.The separation of lock point calculation and verification improves modularity and error handling through the use of
std::optional. The documentation clearly explains the purpose and parameters of each function.src/validation.cpp (4)
269-285: Clean refactoring of CheckSequenceLocksAtTipThe function has been successfully simplified to focus solely on checking sequence locks with pre-calculated lock points, improving separation of concerns.
433-449: Improved lock point validation and caching logicThe refactored code properly handles lock point validation with efficient caching. It first checks validity of existing lock points and only recalculates when necessary, updating the mempool entry appropriately.
852-855: Correct integration of new lock points calculationThe code properly uses the new
CalculateLockPointsAtTipfunction and handles the optional return value appropriately before checking sequence locks.
883-884: Proper lock points value passing to mempool entryThe code correctly extracts the lock points value from the optional before passing it to the
CTxMemPoolEntryconstructor.src/rpc/client.h (2)
9-10: LGTM: Proper header additions for string_view support.The inclusion of both
<string>and<string_view>headers is appropriate for supporting the modernized string parameter handling.
23-23: Excellent modernization: String parameter optimization.The change from
const std::string&tostd::string_viewimproves performance by avoiding unnecessary string copying during JSON parsing while maintaining flexibility to accept various string types.src/test/rpc_tests.cpp (2)
299-302: Excellent test coverage for boolean parsing.The new assertions comprehensively test boolean literal parsing in various JSON contexts (standalone, arrays, objects) and correctly verify type discrimination between boolean literals and their string representations.
315-319: Robust JSON validation tests for object key requirements.The new tests properly validate that JSON object keys must be string names, covering various invalid types (numbers, booleans, arrays, objects) and ensuring appropriate error handling through
std::runtime_errorexceptions.src/univalue/include/univalue.h (2)
155-155: Technically sound implementation, but vendored dependency concern remains.The
string_viewoverload implementation is correct - properly forwarding to the existingread(const char*, size_t)method. However, this change is in a vendored dependency which should typically remain unmodified unless it's an intentional upstream backport.
13-13: Confirm Vendored UniValue Change
It looks like you’ve introduced<string_view>and astd::string_viewoverload in the vendored UniValue library. Per our guidelines, changes to vendored dependencies should only mirror upstream updates.Please confirm that this is an intentional backport of an official UniValue release or commit (and link to the upstream change). If not, consider reverting these edits or isolating them in a separate patch.
• src/univalue/include/univalue.h line 13: added
#include <string_view>
• src/univalue/include/univalue.h line 155: addedbool read(std::string_view raw)src/rpc/client.cpp (5)
8-8: Appropriate header additions for string_view modernization.The new includes (
tinyformat.h,<string>,<string_view>) properly support the updated string handling and error formatting functionality.Also applies to: 12-13
268-271: Well-implemented string_view optimization in ArgToUniValue methods.Both overloads correctly adopt
std::string_viewparameters while preserving existing logic. The conversion tostd::stringonly occurs when needed for the UniValue constructor, maximizing the efficiency benefits.Also applies to: 274-277
293-298: Excellent simplification of JSON parsing logic.The updated implementation eliminates the previous array-wrapping workaround and directly parses the input using
string_view. The error formatting is also modernized with tinyformat, resulting in cleaner and more efficient code.
305-306: Efficient string_view optimization in parameter conversion.Using
string_viewto reference the existing vector element avoids unnecessary copying when callingArgToUniValue, while maintaining the same functional behavior.
317-317: Smart optimization in named parameter parsing.The implementation efficiently uses
string_viewin the loop and for values, while appropriately converting the name tostring(required for map operations). This balances performance optimization with functional requirements.Also applies to: 324-325
| // tip->GetAncestor(max_input_height) should never return a nullptr | ||
| // because max_input_height is always less than the tip height. | ||
| // It would, however, be a bad bug to continue execution, since a | ||
| // LockPoints object with the maxInputBlock member set to nullptr | ||
| // signifies no relative lock time. | ||
| return LockPoints{min_height, min_time, Assert(tip->GetAncestor(max_input_height))}; | ||
| } |
There was a problem hiding this comment.
Replace Assert with proper error handling
Using Assert() at line 266 could cause a crash in production if GetAncestor() returns nullptr. While the comment suggests this shouldn't happen, defensive programming practices recommend proper error handling.
- // tip->GetAncestor(max_input_height) should never return a nullptr
- // because max_input_height is always less than the tip height.
- // It would, however, be a bad bug to continue execution, since a
- // LockPoints object with the maxInputBlock member set to nullptr
- // signifies no relative lock time.
- return LockPoints{min_height, min_time, Assert(tip->GetAncestor(max_input_height))};
+ // tip->GetAncestor(max_input_height) should never return a nullptr
+ // because max_input_height is always less than the tip height.
+ // It would, however, be a bad bug to continue execution, since a
+ // LockPoints object with the maxInputBlock member set to nullptr
+ // signifies no relative lock time.
+ CBlockIndex* max_input_block = tip->GetAncestor(max_input_height);
+ if (!max_input_block) {
+ LogPrintf("ERROR: %s: Failed to get ancestor at height %d for tip at height %d\n",
+ __func__, max_input_height, tip->nHeight);
+ return std::nullopt;
+ }
+ return LockPoints{min_height, min_time, max_input_block};🤖 Prompt for AI Agents
In src/validation.cpp around lines 261 to 267, replace the use of Assert() on
the result of tip->GetAncestor(max_input_height) with proper error handling to
avoid potential crashes in production. Check if GetAncestor returns nullptr, and
if so, handle the error gracefully by returning an error code, throwing an
exception, or logging an error and returning a safe default LockPoints object
instead of proceeding with a nullptr.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/rpc_blockchain.py (1)
251-257: Excellent addition for year 2106 compatibility testing.This test ensures proper blockchain functionality at the 32-bit unsigned integer timestamp boundary (year 2106). The implementation correctly:
- Sets mock time to the maximum 32-bit value (
2**32 - 1)- Generates sufficient blocks (14 total) to establish median time
- Verifies the median time matches the expected timestamp
Consider adding a comment explaining why 14 blocks are generated (8 + 6), as the median time calculation depends on having enough blocks for the median time past algorithm:
def _test_y2106(self): self.log.info("Check that block timestamps work until year 2106") + # Generate initial blocks to establish chain state self.generate(self.nodes[0], 8)[-1] time_2106 = 2**32 - 1 self.nodes[0].setmocktime(time_2106) + # Generate blocks at year 2106 timestamp - need 6 blocks for median time calculation last = self.generate(self.nodes[0], 6)[-1] assert_equal(self.nodes[0].getblockheader(last)["mediantime"], time_2106)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/mining_basic.py(1 hunks)test/functional/rpc_blockchain.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/mining_basic.pytest/functional/rpc_blockchain.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
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.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
test/functional/mining_basic.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🧬 Code Graph Analysis (1)
test/functional/rpc_blockchain.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/rpc/blockchain.cpp (2)
getblockheader(594-665)getblockheader(594-594)
🔇 Additional comments (2)
test/functional/mining_basic.py (1)
182-182: LGTM: Aligns timestamp testing with year 2106 boundary.The change from
2**31 - 1(max signed 32-bit) to2**32 - 1(max unsigned 32-bit) improves timestamp boundary testing consistency. This aligns with the new_test_y2106test inrpc_blockchain.pyand ensures proper coverage of timestamp handling near the year 2106 limit.test/functional/rpc_blockchain.py (1)
95-95: Test placement is appropriate.The
_test_y2106()call is correctly positioned after_test_getblock()and before the final chain verification, ensuring the test runs with proper blockchain state.
PastaPastaPasta
force-pushed
the
backport-0.25-batch-393
branch
from
5994678 to
47f268e
Compare
PastaPastaPasta
force-pushed
the
backport-0.25-batch-393
branch
from
47f268e to
582a014
Compare
|
df014ff is a duplicate (and empty) |
PastaPastaPasta
force-pushed
the
backport-0.25-batch-393
branch
from
582a014 to
016d3aa
Compare
|
Dropped empty commit |
…SequenceLocksAtTip()` Co-authored-by: glozow <[email protected]>
Co-authored-by: fanquake <[email protected]>
…names e43a547 refactor: wallet, do not translate init arguments names (furszy) Pull request description: Simple, and not interesting, refactor that someone has to do sooner or later. We are translating some init arguments names when those shouldn't be translated. ACKs for top commit: achow101: ACK e43a547 MarcoFalke: lgtm ACK e43a547 ryanofsky: Code review ACK e43a547. Just rebased since last review. Tree-SHA512: c6eca98fd66d54d5510de03ab4e63c00ba2838af4237d2bb135d01c47f8ad8ca9aa7ae1e45cf668afcfb9dd958b075a1756cc887b3beef2cb494933d4d83eab0
Co-authored-by: PastaBot <[email protected]> Co-authored-by: Andrew Chow <[email protected]>
…ock for problematic blocks Co-authored-by: Andrew Chow <[email protected]>
Co-authored-by: fanquake <[email protected]>
Co-authored-by: fanquake <[email protected]>
PastaPastaPasta
force-pushed
the
backport-0.25-batch-393
branch
from
016d3aa to
9ccf843
Compare
…GlobalMutex Co-authored-by: Andrew Chow <[email protected]> Co-authored-by: PastaPastaPasta <[email protected]>
| if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) { | ||
| // eventually allow a fallback fee | ||
| error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); | ||
| error = strprintf(_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable %s."), "-fallbackfee"); |
There was a problem hiding this comment.
25666 - consider follow-up to apply it for dash's strings;
I found several of them:
error = Untranslated("You should use upgradetohd RPC to upgrade non-HD wallet to HD");
error = strprintf(_("Cannot upgrade a non HD wallet from version %i to version %i which is non-HD wallet. Use upgradetohd RPC"), prev_version, version);
return InitError(Untranslated("-zapwallettxes has been removed. If you are attempting to remove a stuck transaction from your wallet, please use abandontransaction instead."));
return InitError(Untranslated("-sysperms is not allowed in combination with enabled wallet functionality"));
errorStr = strprintf(_("%s corrupt. Try using the wallet tool dash-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)));
error = _("No dump file provided. To use dump, -dumpfile=<filename> must be provided.");
error = _("No wallet file format provided. To use createfromdump, -format=<format> must be provided.");
InitWarning(_("Incorrect -rescan mode, falling back to default value"));
chain.initError(strprintf(_("Specified -walletdir \"%s\" does not exist"), fs::PathToString(wallet_dir)));
chain.initError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), fs::PathToString(wallet_dir)));
chain.initError(strprintf(_("Specified -walletdir \"%s\" is a relative path"), fs::PathToString(wallet_dir)));
chain.initWarning(strprintf(_("Ignoring duplicate -wallet %s."), wallet_file));
chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original)));
and one extra with forgotten dashification!
error = strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value);
| }, | ||
| [&] { | ||
| int64_t weight{fuzzed_data_provider.ConsumeIntegral<int64_t>()}; | ||
| (void)coin_control.SetInputWeight(out_point, weight); |
There was a problem hiding this comment.
27902: should skip this line IMO and next one, because it's segwit related. I will prepare fix to remove GetInputWeight and HasInputWeight from coin_control
There was a problem hiding this comment.
Why? What harm does keeping it do? it seems it just keeps our code closer to upstream w/o downside
There was a problem hiding this comment.
because we use everywhere a size of transaction instead of "weight".
Having leftover helpers with HasInputWeight and GetInpuptWeight can cause an error in future, if some new (especially backported) code will use it. If these helpers won't exist, freshly backported code will get compilation error -> a developer got a signal to replace "weight" to "size". Otherwise it maybe a silent bug
| // Note: the blocks specified here are different than the ones used in ConnectBlock because DisconnectBlock | ||
| // unwinds the blocks in reverse. As a result, the inconsistency is not discovered until the earlier | ||
| // blocks with the duplicate coinbase transactions are disconnected. | ||
| bool fEnforceBIP30 = !((pindex->nHeight==91722 && pindex->GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) || |
There was a problem hiding this comment.
nit: these hard-coded blocks should not be backported IMO
There was a problem hiding this comment.
@UdjinM6 said we may as well backport them
There was a problem hiding this comment.
if the reindex fails -> need to backport that specific blocks which fails. If re-index doesn't fail -> no need to backport any exception
5 participants
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.