refactor: reduce references to masternode service, cleanup index code, consolidate ProTx versioning and make it friendlier to upgrades #6626
Conversation
WalkthroughThis set of changes primarily updates how masternodes are identified and referenced throughout the codebase, shifting from using network addresses ( Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (14)
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:
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
🔭 Outside diff range comments (1)
src/evo/simplifiedmns.h (1)
72-93: 💡 Verification agent❓ Verification inconclusive
Fix Clang format differences
The pipeline indicates formatting issues in this section. Please run clang-format to fix the style violations.
🏁 Script executed:
#!/bin/bash # Format the file according to project style clang-format -i src/evo/simplifiedmns.hLength of output: 144
Action Required: Install and run clang-format to fix style issues
The pipeline output indicates that the
clang-formatcommand is not available (error: "clang-format: command not found"). Please ensure thatclang-formatis installed in the CI environment, then run:clang-format -i src/evo/simplifiedmns.hThis should resolve the formatting discrepancies in the code block (lines 72-93 in
src/evo/simplifiedmns.h). Once installed, verify that the file adheres to the project’s style guidelines.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 72-80: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
🧹 Nitpick comments (8)
src/rpc/masternode.cpp (3)
266-266: Improved string emptiness check.Replacing
strFilter != ""with!strFilter.empty()is more idiomatic C++ and potentially more efficient.
275-275: Improved string emptiness check.Replacing
strFilter != ""with!strFilter.empty()is more idiomatic C++ and potentially more efficient.
567-570: Improved string emptiness checks throughout the filtering conditions.All instances of
strFilter != ""have been replaced with the more idiomatic and potentially more efficient!strFilter.empty()across various filtering conditions.Also applies to: 580-581, 589-590, 605-606, 628-628, 631-631, 634-635, 638-638, 641-641, 645-646, 649-649
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 567-650: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/coinjoin/server.cpp (1)
174-180: Code formatting issue detectedThe pipeline shows a Clang format check failure in this section. Please run clang-format on this code to ensure it matches the project's style guidelines.
#!/bin/bash # Fix the formatting issues clang-format -i src/coinjoin/server.cpp🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 174-180: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/rpc/evo.cpp (1)
971-974: Clang-format alignment issues
The pipeline reports formatting inconsistencies in lines [971–974]. Please run clang-format (or the repository’s formatting tool) to ensure these lines match the project’s style guidelines.- if (dmn->nType != mnType) { - throw std::runtime_error(strprintf("masternode with proTxHash %s is not a %s", ptx.proTxHash.ToString(), GetMnType(mnType).description)); - } + if (dmn->nType != mnType) { + throw std::runtime_error(strprintf( + "masternode with proTxHash %s is not a %s", + ptx.proTxHash.ToString(), + GetMnType(mnType).description)); + }🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 971-974: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/coinjoin/client.cpp (1)
109-112: AddedTrySubmitDenominatemethod
The method obtains a lock oncs_deqsessionsand scans the existing sessions for a matchingproTxHash. Returningtrueupon submission andfalseotherwise is consistent. Ensure proper logging or error-handling at the call-site if no session is found.Clang-format discrepancies
The pipeline indicates lines [108–112] need reformatting. Please run clang-format or the project’s style fixer to align them with established guidelines.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 108-112: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/evo/dmnstate.h (2)
97-100: Formatting mismatch
Pipeline logs show formatting issues for lines [97–100]. Align indentation and arguments to match the codebase’s style.- READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.pubKeyOperator), obj.nVersion == ProTxVersion::LegacyBLS)); + READWRITE(CBLSLazyPublicKeyVersionWrapper( + const_cast<CBLSLazyPublicKey&>(obj.pubKeyOperator), + obj.nVersion == ProTxVersion::LegacyBLS));🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 97-100: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
178-220: Formatting concerns
The pipeline indicates additional style inconsistencies within lines [178–220]. After finalizing logic, please run the project’s clang-format rules to ensure consistent spacing and indentation.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 178-220: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/coinjoin/client.cpp(9 hunks)src/coinjoin/client.h(2 hunks)src/coinjoin/server.cpp(1 hunks)src/evo/assetlocktx.h(1 hunks)src/evo/cbtx.h(1 hunks)src/evo/deterministicmns.cpp(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(6 hunks)src/evo/mnhftx.h(2 hunks)src/evo/providertx.cpp(6 hunks)src/evo/providertx.h(16 hunks)src/evo/simplifiedmns.cpp(3 hunks)src/evo/simplifiedmns.h(3 hunks)src/llmq/commitment.h(2 hunks)src/llmq/snapshot.cpp(2 hunks)src/rpc/evo.cpp(6 hunks)src/rpc/index_util.h(1 hunks)src/rpc/masternode.cpp(4 hunks)src/rpc/misc.cpp(3 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(6 hunks)src/test/util_tests.cpp(1 hunks)src/txdb.cpp(2 hunks)src/txmempool.cpp(10 hunks)src/txmempool.h(2 hunks)src/util/string.h(1 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)test/lint/lint-includes.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/util_tests.cpp (1)
src/util/string.h (1)
PadString(60-60)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/simplifiedmns.h
[error] 72-80: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/rpc/masternode.cpp
[error] 567-650: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/coinjoin/server.cpp
[error] 174-180: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/evo/dmnstate.h
[error] 97-100: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
[error] 178-220: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/coinjoin/client.cpp
[error] 108-112: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/rpc/evo.cpp
[error] 971-974: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/evo/providertx.h
[error] 22-80: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
🔇 Additional comments (78)
src/rpc/index_util.h (1)
30-30: Good signature update for consistency.The function declaration of
GetAddressIndexhas been modified to remove default values for thestartandendparameters, requiring explicit specification of these parameters at all call sites. This change aligns with the unified approach to calling this function insrc/rpc/misc.cpp.src/rpc/misc.cpp (3)
821-826: Well-structured range parameter handling.This change simplifies the control flow by using a boolean flag
range_definedto determine whether to use explicit range values or defaults. The refactoring improves code consistency by always passing explicit parameters toGetAddressIndexinstead of relying on conditional code paths.
892-893: Consistent approach for GetAddressIndex call.The change ensures consistency with other RPC commands by explicitly passing start and end parameters (0, 0) when calling
GetAddressIndex. This matches the updated function signature insrc/rpc/index_util.h.
974-977: Consistent pattern for parameter handling.This update follows the same pattern as the other RPC commands, using a boolean flag to determine whether to use range values or defaults. The unified approach to calling
GetAddressIndeximproves code maintainability and readability.src/util/string.h (1)
60-64: Well-designed utility function for string padding.The new
PadStringfunction provides a clean, reusable solution for string padding with appropriate parameter defaults. The implementation is concise yet readable, and the use of[[nodiscard]]ensures users are encouraged to use the return value.src/test/util_tests.cpp (1)
1831-1848: Good test coverage for the new utility function.The test case thoroughly verifies the new
PadStringfunction, covering both left and right padding, handling of strings already at or above the target length, and proper handling of empty strings.src/rpc/masternode.cpp (2)
584-588: Improved string formatting with the new PadString utility.Similar to the previous change, this also uses the new
PadStringutility withstrprintffor more consistent padding.
593-604: Improved string formatting with the new PadString utility.This change follows the same pattern of using
PadStringwithstrprintffor more consistent string formatting.src/txmempool.h (2)
522-525: Good refactoring of map declarationsThe changes simplify type declarations by removing typedefs and directly declaring map variables with their full types, which makes the code cleaner and more maintainable. This modern C++ approach is more explicit about types used throughout the code.
613-613:❓ Verification inconclusive
Function return type standardization looks good
Changing the return type of
removeAddressIndexandremoveSpentIndexfrombooltovoidis a sensible change if these methods no longer need to communicate success/failure. This makes the API cleaner and more consistent.Also applies to: 617-617
🏁 Script executed:
#!/bin/bash # Verify the implementation matches the header changes # Check implementation of removeAddressIndex in txmempool.cpp rg -n "void.*removeAddressIndex" src/txmempool.cpp # Check implementation of removeSpentIndex in txmempool.cpp rg -n "void.*removeSpentIndex" src/txmempool.cpp # Look for any places in the codebase that check the return value rg -n "if.*removeAddressIndex" --include="*.cpp" . rg -n "if.*removeSpentIndex" --include="*.cpp" .Length of output: 1213
Update Confirmation: Function Return Type Change Verified
The changes in both the header (
src/txmempool.hat lines 613 and 617) and the implementation (src/txmempool.cppat lines 622 and 673) correctly update the return type forremoveAddressIndexandremoveSpentIndexfrombooltovoid. This update simplifies the API by removing unused return values.Before final approval, please ensure that no call sites elsewhere in the codebase depend on a returned boolean value from either function. To assist with this, re-run the following commands to verify that there are no conditional checks on these functions:
#!/bin/bash # Check for conditional usage of removeAddressIndex in .cpp files find . -type f -name "*.cpp" -exec grep -Hn "if.*removeAddressIndex" {} \; # Check for conditional usage of removeSpentIndex in .cpp files find . -type f -name "*.cpp" -exec grep -Hn "if.*removeSpentIndex" {} \;Once you’ve confirmed that no such usage exists, the changes can be approved.
test/lint/lint-includes.py (1)
27-30: Adding Boost Hana headers to expected includesThe addition of Boost Hana headers (
boost/hana/for_each.hppandboost/hana/tuple.hpp) aligns with the PR objectives, which mention adopting Boost Hana for compile-time evaluation and selective serialization logic in place of macro-based serialization.src/evo/assetlocktx.h (1)
131-131: Improved UniValue initializationSimplifying the UniValue object initialization from a two-step process (
UniValue obj; obj.setObject();) to a direct constructor call (UniValue obj(UniValue::VOBJ);) is a good cleanup. This makes the code more concise and consistent with modern C++ initialization patterns.src/evo/cbtx.h (1)
70-70: Improved UniValue initializationSimplifying the UniValue object initialization from a two-step process to a direct constructor call is a good cleanup. This is consistent with similar changes made across other files and makes the codebase more uniform.
src/evo/dmnstate.cpp (2)
40-40: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
71-71: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
src/llmq/snapshot.cpp (2)
26-26: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
45-45: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
src/test/block_reward_reallocation_tests.cpp (1)
117-117:✅ Verification successful
Updated to use renamed method GetMaxVersion() instead of GetVersion().
This change aligns with the broader refactoring where
GetVersion()was renamed toGetMaxVersion()to better reflect its purpose. The method now more accurately indicates that it returns the maximum supported version rather than the current version.
🏁 Script executed:
#!/bin/bash # Check for consistent usage of GetMaxVersion across the codebase rg "GetMaxVersion" --type cppLength of output: 2612
Confirmed: Consistent Renaming of GetVersion to GetMaxVersion
The change in
src/test/block_reward_reallocation_tests.cppat line 117 now correctly usesGetMaxVersionas part of the broader refactoring. A search across the codebase confirms that this renamed method is consistently used in similar contexts (e.g., in other test files, RPC files, and provider transaction logic). No further modifications appear necessary.
- File:
src/test/block_reward_reallocation_tests.cpp
- Snippet:
proTx.nVersion = CProRegTx::GetMaxVersion(!bls::bls_legacy_scheme);- The usage aligns with similar updates in
src/test/evo_deterministicmns_tests.cpp,src/rpc/evo.cpp, andsrc/evo/providertx.h.src/evo/mnhftx.h (2)
52-52: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
89-89: Code simplification: Direct UniValue initialization.The change simplifies object initialization by directly constructing the UniValue with VOBJ type rather than using a two-step initialization process.
src/evo/deterministicmns.cpp (1)
46-46: Improved UniValue construction patternThe code now directly initializes the UniValue object with its type using the constructor parameter instead of a two-step initialization process, making the code more concise and clearer.
src/llmq/commitment.h (2)
124-124: Improved UniValue construction patternThe code now directly initializes the UniValue object with its type using the constructor parameter instead of a two-step initialization process (previously using default construction followed by
setObject()). This makes the code more concise and clearer.
170-170: Improved UniValue construction patternSame improvement as above, directly initializing the UniValue object with VOBJ type via constructor parameter.
src/test/evo_deterministicmns_tests.cpp (5)
106-106: Updated method name to GetMaxVersion()The code properly aligns with the renamed method in the production code, where
GetVersion()was renamed toGetMaxVersion()to better reflect its purpose.
128-128: Updated method name to GetMaxVersion()Consistently updated the method name from
GetVersion()toGetMaxVersion()forCProUpServTx.
148-148: Updated method name to GetMaxVersion()Consistently updated the method name from
GetVersion()toGetMaxVersion()forCProUpRegTx.
169-169: Updated method name to GetMaxVersion()Consistently updated the method name from
GetVersion()toGetMaxVersion()forCProUpRevTx.
777-777: Updated method name to GetMaxVersion()Consistently updated the method name from
GetVersion()toGetMaxVersion()in another part of the test file.src/coinjoin/server.cpp (2)
177-177: Enhanced identification using ProTxHash instead of IP addressChanged the log message to use the masternode's ProTxHash instead of its network address for identification. This is more consistent with the broader changes in the codebase to use deterministic identifiers instead of network addresses.
182-182: Enhanced identification using ProTxHash instead of IP addressChanged the log message to use the masternode's ProTxHash instead of its network address for identification, similar to the change above.
src/txdb.cpp (4)
293-302: Excellent modernization to range-based for loops with structured bindings!This change improves readability and maintainability while preserving the original logic. The structured binding syntax (
for (const auto& [key, value] : vect)) is much clearer than using explicit iterators and accessing members with arrow operators.
304-314: Good continuation of the modernization patternConsistent application of the range-based for loop with structured bindings pattern to
UpdateAddressUnspentIndex. The logic within the loop remains unchanged.
341-347: Clean refactoring to modern C++ idiomsThe
WriteAddressIndexmethod has been properly modernized to use range-based for loops with structured bindings, making the code more concise and readable.
349-355: Good use of underscore for unused valuesNice touch using the underscore (
_) for the unused value in the structured binding, which is a common convention to indicate intentionally unused parameters. This is consistent with modern C++ idioms.src/evo/simplifiedmns.h (4)
9-11: Appropriate addition of providertx.h includeThis include was added to access the centralized
ProTxVersionnamespace, supporting the refactoring effort to consolidate version constants.
45-46: Good centralization of version constantsReplaced hardcoded version with the namespaced constant
ProTxVersion::LegacyBLS, improving code maintainability and consistency.
79-80: Version check correctly updated to use namespaced constantThe BLS public key legacy check now uses the centralized
ProTxVersion::LegacyBLSconstant instead of a hardcoded value.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 72-80: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
86-92: Improved version handling for conditional serializationThe condition now properly uses
ProTxVersion::BasicBLSwith a greater-than-or-equal comparison, supporting the PR objective of handling versions beyond the current two.src/wallet/test/coinjoin_tests.cpp (2)
70-71: Updated to use ProTx hash instead of network addressChanged to check ProTx hash instead of address, aligning with the PR objective to use ProTx hash for identification rather than network addresses.
77-80: Properly updated test to use ProTx hash for identificationConstructor and getter now correctly use the ProTx hash (
uint256) instead of network address (CService), making this test consistent with the refactored implementation.src/evo/providertx.cpp (7)
16-19: Method name correctly updated to reflect its purposeChanged from
GetVersiontoGetMaxVersion, which better describes what the method does - it returns the maximum allowed version given the BLS scheme state.
20-22: Improved version checking with namespaced constantsThe condition now uses
ProTxVersion::BasicBLSinstead of a hardcoded constant, ensuring version checks are consistent throughout the codebase.
33-35: Consistent use of namespaced version constantsThe check for legacy BLS public key now uses
ProTxVersion::LegacyBLS, which maintains consistency with the centralized versioning approach.
100-106: Version validation standardized across transaction typesThe
CProUpServTx::IsTriviallyValidmethod now uses the same pattern for version checking as other transaction types, ensuring consistency in validation logic.
126-129: Consistent version validation in CProUpRegTxThe version check correctly uses
GetMaxVersioninstead ofGetVersion, aligning with the changes in other provider transaction types.
137-139: Standardized BLS public key legacy checkUsed the namespaced
ProTxVersion::LegacyBLSconstant for consistency in how the operator public key legacy status is validated.
159-162: Completed version validation standardizationThe
CProUpRevTx::IsTriviallyValidmethod now also uses the updatedGetMaxVersionmethod, ensuring all provider transaction types use the same validation pattern.src/coinjoin/client.h (5)
42-42: Good refactoring to use ProTx hashes instead of network addresses.This change from
CService addrtouint256 proTxHashaligns with best practices by using deterministic ProTx hashes for masternode identification rather than mutable network addresses.
49-51: Constructor updated correctly to match member variables.The constructor parameters have been properly updated to match the new member variable type, maintaining consistent initialization with move semantics.
56-56: Getter method updated to reflect new identifier type.The getter method has been appropriately renamed from
GetAddr()toGetProTxHash()with the correct return type, maintaining the API interface pattern.
62-62: Equality operator correctly updated to compare ProTx hashes.The equality comparison has been updated to compare ProTx hashes instead of network addresses, ensuring consistent identity comparison.
339-339:✅ Verification successful
Method signature updated to use ProTx hash parameter.
The
TrySubmitDenominatemethod now accepts a ProTx hash instead of a network address, completing the refactoring to use deterministic identifiers throughout the CoinJoin client code.Let's verify that all call sites to this method have been updated:
🏁 Script executed:
#!/bin/bash # Check if all call sites of TrySubmitDenominate have been updated to use ProTx hashes rg "TrySubmitDenominate\(" --type cppLength of output: 400
Verification successful: All call sites use the ProTx hash parameter consistently.
The refactoring to update theTrySubmitDenominatemethod signature has been validated. The grep output confirms that:
- The header in
src/coinjoin/client.hnow declares the method with aproTxHashparameter.- The call site in
src/coinjoin/client.cppcorrectly passesdmn->proTxHashto the method.- The method definition in
src/coinjoin/client.cppmatches the updated signature.No outstanding issues remain.
src/evo/simplifiedmns.cpp (3)
41-41: Improved version handling with abstracted version check.The version assignment has been refactored to use
CProRegTx::GetMaxVersion()with a boolean parameter derived from comparing the state version againstProTxVersion::BasicBLS. This approach:
- Centralizes version logic in the
GetMaxVersion()method- Makes the code more maintainable by abstracting version constants
- Better accommodates future version additions
This is consistent with the PR's goal of consolidating version constants into a namespaced enum.
75-75: Simplified UniValue object initialization.The code now directly constructs the UniValue object with
UniValue::VOBJtype rather than using a default constructor followed bysetObject(). This is a minor but worthwhile cleanup that improves code readability.
241-241: Consistent UniValue object initialization pattern.Similar to the earlier change, this simplifies UniValue object creation for better readability. The consistent application of this pattern throughout the codebase improves maintainability.
src/txmempool.cpp (8)
622-623: Simplified function signature by removing unused return value.The function
removeAddressIndexno longer returns a boolean value, as the return value wasn't being used. This is a good cleanup that simplifies the interface.
625-628: Modernized conditional and loop patterns.The code now uses the C++17 if-with-init statement and range-based for loop, which:
- Reduces variable scope for better code safety
- Makes the code more readable
- Eliminates potential iterator invalidation issues
This is a good modernization of the codebase.
673-674: Consistent signature update for removeSpentIndex.Similar to
removeAddressIndex, theremoveSpentIndexfunction no longer returns a boolean value. This maintains consistency in the API design.
676-681: Modernized iterator usage for map lookups and iteration.Similar to the previous modernization, this code now uses C++17 if-with-init statement and range-based for loop for cleaner, safer code.
688-688: Performance improvement through hash caching.The code now caches the transaction hash in a local variable
tx_hashinstead of repeatedly calling the potentially expensivetx.GetHash()method. This improves performance by avoiding redundant hash computations.Also applies to: 788-788
692-692: Consistent use of cached transaction hash.The cached
tx_hashvariable is now used consistently throughout the code instead of callingtx.GetHash()repeatedly. This improves both performance and readability.Also applies to: 693-693, 695-696, 697-700, 704-705, 708-709, 717-717, 725-725, 727-727
792-792: Consistent use of cached transaction hash in removal methods.The corresponding removal methods also use the cached
tx_hashvariable consistently, maintaining symmetry with the addition methods and ensuring consistent performance optimization throughout the codebase.Also applies to: 798-798, 801-801, 805-805, 809-809, 811-811
1020-1020: Consistent transaction hash caching in conflict detection.The cached
tx_hashis used consistently in the ProTx conflict detection logic, including error logging. This maintains the pattern of avoiding redundant hash computations throughout the mempool code.Also applies to: 1024-1024, 1031-1031, 1040-1040, 1045-1045, 1051-1051, 1058-1058, 1067-1067
src/rpc/evo.cpp (5)
659-659: Use ofGetMaxVersionfor new ProTx
The invocationptx.nVersion = CProRegTx::GetMaxVersion(/*is_basic_scheme_active=*/!use_legacy);correctly selects the newest version when!use_legacyis true. This ensures that any newly created ProTx payload aligns with the current best version logic.
694-694: Verify legacy BLS consistency
CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS));enforces that the operator’s BLS public key legacy flag must match the transaction’s legacy version. This check helps avoid conflicts between operator keys and transaction versions.
968-970: Defensive existence check
The retrieval of the masternode (dmn) and subsequent runtime error upon a missing MN is good defensive coding. Make sure the caller either handles or logs this error path in all usage contexts.
972-974: Type mismatch enforcement
Ensuringdmn->nType == mnTypebefore settingptx.nTypeavoids inconsistent updates. This early exit is correct for preventing invalid updates.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 971-974: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
977-977: Applying masternode’s version
Assigningptx.nVersion = dmn->pdmnState->nVersion;ensures the transaction remains consistent with the on-chain masternode’s version. This helps match the masternode’s actual state.src/evo/dmnstate.h (4)
39-39: Default version as LegacyBLS
InitializingnVersionwithProTxVersion::LegacyBLSensures older BLS-legacy usage by default. Confirm that subsequent code updates this value when a higher version is known.
100-100: Version-aware pubKeyOperator read
UsingCBLSLazyPublicKeyVersionWrapper(...)and checkingobj.nVersion == ProTxVersion::LegacyBLSis correct for ensuring we decode operator pubkeys in line with the chosen scheme.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 97-100: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
111-111: Reset to LegacyBLS
When resetting operator fields, settingnVersionback toProTxVersion::LegacyBLSensures fresh allocations revert to an initial state. This is a suitable approach for re-initializing state.
178-209: Boost.Hana-based member introspection
Replacing macro-heavy logic with aboost::hanatuple of field descriptors is a clean approach. This pattern clarifies the mapping of fields and bitmasks, allowing for a more maintainable and extensible system. Especially with critical invariants in operator fields, it’s good to see thenVersionfield is explicitly forced wheneverpubKeyOperatorchanges.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 178-220: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
src/evo/providertx.h (6)
34-37: Improved naming:GetVersion()→GetMaxVersion()The renaming from
GetVersion()toGetMaxVersion()better reflects the purpose of the method, which is to return the maximum supported version based on whether the basic BLS scheme is active. The naming change is consistent across all provider transaction classes.Also applies to: 127-130, 204-207, 266-269
39-39: Improved initialization with namespace constantsGood change using
ProTxVersion::LegacyBLSfor default initialization ofnVersionmembers instead of hardcoded values. This avoids magic numbers and ensures consistency throughout the codebase.Also applies to: 132-132, 209-209, 280-280
60-63: Consistent version range checkingThe version validation logic has been updated to use
GetMaxVersion()with a future-proof approach. This change ensures that the code gracefully handles both current and potential future versions by checking if the version is out of range rather than checking for specific values.Also applies to: 148-151, 223-226, 291-294
71-71: Consistent version-dependent BLS serializationThe BLS key and signature wrapper usage has been updated to consistently use the
ProTxVersion::LegacyBLSconstant instead of hardcoded values. This ensures that the code handles BLS serialization consistently based on the transaction version.Also applies to: 170-170, 230-230, 302-302
152-155: Future-proofing version-dependent serializationGood change using a version range check (
nVersion >= ProTxVersion::BasicBLS) instead of equality check. This makes the code more maintainable by allowing for future versions beyondBasicBLSto include thenTypefield without requiring additional code changes.
96-96: Simplified JSON object initializationThe JSON initialization has been simplified by directly constructing
UniValueobjects withUniValue::VOBJinstead of using a separate initialization and assignment. This is a minor but consistent style improvement across all ToJson() methods.Also applies to: 179-179, 246-246, 311-311
src/rpc/masternode.cpp
Outdated
| std::string strFull = strprintf("%s %s %s %s %s %s", | ||
| PadString(dmnToStatus(dmn), 18), | ||
| dmn.pdmnState->nPoSePenalty, | ||
| payeeStr, | ||
| PadString(ToString(dmnToLastPaidTime(dmn)), 10), | ||
| PadString(ToString(dmn.pdmnState->nLastPaidHeight), 6), | ||
| dmn.pdmnState->addr.ToStringAddrPort()); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Improved string formatting with the new PadString utility.
The code now uses the new PadString utility with strprintf instead of the previous approach (likely using std::ostringstream with std::setw). This change makes the code more consistent and facilitates removing excess padding after the last entry.
There's a CI pipeline failure indicating formatting issues in this region. Please run clang-format to fix these issues:
🏁 Script executed:
#!/bin/bash
# Check formatting issues with clang-format
clang-format -i src/rpc/masternode.cpp
git diff --name-only src/rpc/masternode.cppLength of output: 189
Action Required: Address CI Formatting Issues in src/rpc/masternode.cpp
The update to use the new PadString utility with strprintf is a clear improvement. However, there remains an outstanding CI issue regarding code formatting. The automated script attempted to run clang-format but failed because the command was not found in the environment. Please ensure that:
- clang-format is installed in your CI environment (or available locally) so that the formatting can be automatically applied.
- You re-run
clang-format -i src/rpc/masternode.cppand verify that all formatting issues are resolved. - Any formatting adjustments made by clang-format are committed to the branch.
After these changes, please verify that the CI formatting pipeline passes successfully.
| namespace ProTxVersion { | ||
| enum : uint16_t { | ||
| LegacyBLS = 1, | ||
| BasicBLS = 2, | ||
| }; | ||
| } // namespace ProTxVersion |
There was a problem hiding this comment.
Good refactoring: Centralized version constants
The introduction of the ProTxVersion namespace with enum constants is a good improvement that centralizes version management. This makes the code more maintainable by eliminating duplicated version constants across classes and makes it easier to add new versions in the future.
However, there's a clang format issue reported in the pipeline failures. Please run clang-format on this file to fix the formatting in lines 22-80.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 22-80: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
notes: - CPendingDsaRequest will now track the ProTx hash and will resolve the primary address just before sending the request. - Validation of the expected masternode address matching the peer has been deduplicated. Failure created a debug log entry for DSCOMPLETE (though not for DSSTATUSUPDATE or DSFINALTX) but now it will happen silently.
Co-authored-by: Konstantin Akimov <[email protected]>
Co-authored-by: Konstantin Akimov <[email protected]>
kwvg
changed the title
kwvg
changed the title
…`MnNetInfo`, lay some groundwork for managing multiple entries 1d52678 refactor: track every `MnNetInfo` entry in the unique property map (Kittywhiskers Van Gogh) bcb8a7d refactor: impl `GetEntries()`, adapt to support multiple addresses (Kittywhiskers Van Gogh) ecc9368 refactor: implement `MnNetInfo::ToString()` for printing internal state (Kittywhiskers Van Gogh) 2e9bde0 refactor: move service validation to `MnNetInfo`, run during setting (Kittywhiskers Van Gogh) 03ec604 fix: correct `simplifiedmns_merkleroots` unit test (Kittywhiskers Van Gogh) bac4a27 refactor: move address lookup to `MnNetInfo::AddEntry()` (Kittywhiskers Van Gogh) e1783cb refactor: remove direct access to `MnNetInfo::addr` (Kittywhiskers Van Gogh) e868aff refactor: use const-ref when accessing `MnNetInfo::addr` if read-only (Kittywhiskers Van Gogh) aaabc35 refactor: section off masternode service to `MnNetInfo` (Kittywhiskers Van Gogh) 2c6dd05 fix: avoid potential "no user-provided default constructor" error (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6626 * Depends on #6635 * Depends on #6636 * Dependency for #6629 * The `simplifiedmns_merkleroots` test constructs 15 `CSimplifiedMNListEntry` elements and populates them with a `CService`. So far, since we allowed direct assignment of the `CService` without validation, no checks were made to see if they would pass validation but if we start enforcing validation rules _when setting values_, two problems emerge. * Using non-default ports on mainnet (`BasicTestingSetup` is mainnet by default, [source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/test/util/setup_common.h#L100)), this has been resolved by using `RegTestingSetup` (which is based on regtest) instead. * As the index is used to generate the address, starting from 0, the first address is `0.0.0.0`, which is not a valid `CService` address ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/test/net_tests.cpp#L140)) and therefore, would fail validation ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/evo/deterministicmns.cpp#L1219)). This has been resolved by changing the index to start from 1. * To avoid a potential "no user-provided default constructor" compile-time error, we now explicitly request the default constructor <details> <summary>Compile error:</summary> ``` In file included from evo/deterministicmns.cpp:5: ./evo/deterministicmns.h:404:24: error: default initialization of an object of const type 'const ExampleType' without a user-provided default constructor 404 | static const T nullValue; | ^ | {} evo/deterministicmns.cpp:479:18: note: in instantiation of function template specialization 'CDeterministicMNList::AddUniqueProperty<ExampleType>' requested here 479 | if (!AddUniqueProperty(*dmn, domain)) { | ^ ``` </details> * The reason why we track individual entries _within_ `MnNetInfo` in the unique properties map instead of `MnNetInfo` is that while `MnNetInfo`-like objects (because `MnNetInfo` itself only stores one value) could check _internally_ for duplicates, the uniqueness requirement for addresses is _across_ ProTx'es and therefore, we are concerned not so much as _how_ the addresses are bundled (`MnNetInfo`) but if the individual addresses (`CService`) are unique _across_ all known addresses. * We cannot use `const auto&` when iterating through `GetEntries()` as it uses `std::reference_wrapper<const T>` and `auto` will take on the type of `const std::reference_wrapper<const T>&` instead of the underlying `const T&` as intended, to trigger the conversion to the underlying reference, we have to explicitly specify the type, hence the usage of `const T&`. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 1d52678 UdjinM6: utACK 1d52678 Tree-SHA512: 0f13f51fff6c279e8c4e3f88ea4db925f4137e25b030d28afd48b5db9c073421d5bb3a3dc3e067ee4f559887bec9e8a981d2e6ae5f2d0a042e5817a3d59ea0bf
4 participants
Additional Information
Dependency for refactor: section off masternode network information to
MnNetInfo, lay some groundwork for managing multiple entries #6627As dash#6627 aims to abstract away masternode network information (so far stored as a
CService) behind an interface in preparation for supporting multiple addresses of different types, it still needs to be exposed to networking logic that actively relies on the "primary" address (i.e. the address used by masternodes to connect to each other). This is done throughMnNetInfo::GetPrimary().To reduce the
GetPrimary()invocations needed to a minimum in dash#6627, debug log messages have been changed to identify masternodes using the ProTx hash instead of the primary address andCPendingDsaRequesthas been refactored to delay resolving the primary address, tracking the ProTx hash instead.Currently, valid versions of
CPro*Txare expressed asCPro*Tx::*_VERSION. This isn't necessary as the*_VERSION(currently{BASIC,LEGACY}_BLS_VERSION) are the same for allCPro*Txes, so they have been consolidated into a namespaced enumProTxVersion.Additionally, some checks in the codebase assume that are only two versions (source, source), which isn't conducive to adding new versions. Those checks have been modified to allow higher versions.
CPro*Tx::GetVersion()has been renamed toCPro*Tx::GetMaxVersion()as its actually used to check what is the highest allowed version given a set of consensus rules.GetMaxVersion()has not been consolidated likeProTxVersionas any changes toaddr(as intended in dash#6627) would only affectCProRegTxandCProUpServTx(i.e. the remaining ProTx transactions would not be getting a version upgrade).In later PRs, the change to a multiple-address capable implementation is hidden away using a
shared_ptrto the interface class. To decide what implementation to use, we need to set the version ofCPro*Tx, as early as possible so that the version can be used to determine the applicable implementation. This is easy enough to do in serialization code but requires additional care when constructing aCPro*Txin-place as it will be initialized with an emptyshared_ptrand the applicable implementation needs to be explicitly set.To avoid problems in those later PRs, some instances of
CPro*Txconstruction have been reshuffled to setnVersionas early as possible.Breaking Changes
None expected.
Checklist