refactor: model interface after MnNetInfo and support switching impls, add new ProTx version
#6665
Conversation
kwvg
changed the title
MnNetInfo and support switching impls, add new ProTx version, introduce DEPLOYMENT_EXTADDR paramsMnNetInfo and support switching impls, add new ProTx version, introduce DEPLOYMENT_EXTADDR params
… featureset 98749a7 feat: define BIP 9 fork `DEPLOYMENT_V23` for new version featureset (Kittywhiskers Van Gogh) Pull request description: ## Motivation Should handle the deployment needs for [dash#6662](#6662) and [dash#6665](#6665) as they're expected to be shipped in the same major version. ## 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 **(note: N/A)** - [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: UdjinM6: utACK 98749a7 PastaPastaPasta: utACK 98749a7 Tree-SHA512: b61302a0bcf236f06661a4a53f1d7b3ab0f65659c1c9ab2fcb782457a25eb1e6bdc942647dca6a4e22686e0c7cc83405e099e1fa790cf007d2bee3eedc456d5f
kwvg
force-pushed
the
netinfo_p3
branch
2 times, most recently
from
af9779d to
227b428
Compare
kwvg
changed the title
MnNetInfo and support switching impls, add new ProTx version, introduce DEPLOYMENT_EXTADDR paramsMnNetInfo and support switching impls, add new ProTx version
kwvg
force-pushed
the
netinfo_p3
branch
2 times, most recently
from
f559edc to
5cbddbb
Compare
…n-`CService` entries, use in `MnNetInfo` 3a72f2f evo: fast-fail `MnNetInfo::AddEntry()` if invalid characters found (Kittywhiskers Van Gogh) e0a1c64 evo: prohibit overwriting entry in `MnNetInfo` (Kittywhiskers Van Gogh) c69184b evo: utilize `NetInfoEntry::IsTriviallyValid()` in ProTx trivial checks (Kittywhiskers Van Gogh) 06cf4ee evo: return `MnNetInfo::GetEntries()` with `NetInfoEntry` (Kittywhiskers Van Gogh) 6286c9b evo: change internal type of `MnNetInfo` to `NetInfoEntry` (Kittywhiskers Van Gogh) b0a634e evo: ensure the ADDRV2 serialization is always used in `NetInfoEntry` (Kittywhiskers Van Gogh) 6d97bda evo: introduce type-flexible `NetInfoEntry` to allow non-`CService` data (Kittywhiskers Van Gogh) 069583d evo: expand error codes for `netInfo` validation (Kittywhiskers Van Gogh) Pull request description: ## Motivation The upcoming extended addresses specification envisions the ability to store address information _beyond_ the set of addresses that can be represented by BIP-155 (i.e. `CService`). To enable this, we need to devise a backwards-compatible way to allow storing and manipulating address information with differing serialization formats and validation rules. Backwards compatibility is a priority as the unique properties set (used to detect attempts at storing already-registered values) only stores a hashed representation of the value and therefore, in-place migration is not a viable option. With this in mind, this pull request introduces `NetInfoEntry`, which is wrapper around an `std::variant` that provides dispatching for common endpoints with `std::visit`, serialization and trivial validity enforcement between potential implementations and the ability to access the underlying type if necessary (for code that relies on backwards-compatibility, like hashing). It doesn't implement any network rules itself but requires that it must hold a valid instance of any underlying type that it supports. While `MnNetInfo` (the current implementation) has and always will store a `CService`, to ensure a conformity between it and `ExtNetInfo` (the upcoming extended implementation), its public functions will return a `NetInfoEntry` when applicable so that both can be abstracted by a common interface to aid with the transition. ## Additional Information * Depends on #6627 * Depends on #6664 * Dependency for #6665 * ~~2e9bde0519b242d1d7aaf49344a357b90121689e in [dash#6627](#6627) incorrectly migrated validation conditions such that attempting to set a valid `addr:port` combination on mainnet would result in a `BadPort` error because the non-mainnet rules were applied _regardless_ of network.~~ ~~This evaded testing as our unit and functional tests do not run on mainnet. To prevent this from occurring again, the whole `evo_netinfo_tests` suite now uses `BasicTestingSetup` (which advertises itself as mainnet), which comes with the added benefit of greater coverage as mainnet rules are _stricter_.~~ ~~The port validation check for non-mainnet networks are tested _indirectly_ through tests like `evo_simplifiedmns_merkleroots`'s usage of `NetInfoInterface::*` methods ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/test/evo_simplifiedmns_tests.cpp#L25)).~~ Superseded by [dash#6664](#6664). * Per replies to review comments ([comment](#6627 (comment)), [comment](#6627 (comment))) from [dash#6627](#6627), reported error codes from `netInfo` interactions have been expanded to be more specific. * `CService` by default is serialized using ADDRV1 and utilizing ADDRV2 serialization is either explicitly opted into ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/addrman.cpp#L173-L175)) or determined on-the-fly ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/net_processing.cpp#L3960-L3965)). As we envision the ability to store Tor and I2P addresses, using ADDRV2 is mandatory. Though this affects (de)serialization of `NetInfoEntry`, it does not affect `MnNetInfo` as `NetInfoEntry` is only used for the sake of a consistent interface _but_ internally still (de)serializes to an ADDRV1 `CService`. * The introduction of fast-failing based on permitted characters is meant to mirror the upcoming extended addresses implementation where permitted characters are used as a quick way to classify the intended underlying type before running more expensive checks. As a side effect, this may cause inconsistency where attempting to use `MnNetInfo::AddEntry()` with, for example, an IPv6 address will result in `BadInput` as the delimiter used in IPv6 addresses are not part of the permitted characters filter _but_ validating a `MnNetInfo` with an IPv6 address _already stored_ will return a `BadType`. ## 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: PastaPastaPasta: utACK 3a72f2f UdjinM6: utACK 3a72f2f Tree-SHA512: abd84db309b6011480431b12cccd649878bab06aa44ca2c81563e9598d4424fd61888b12e2e439b9c2180bc5e0edee3431b1008ae7af4b676b164af1455fda3c
|
This pull request has conflicts, please rebase. |
WalkthroughThis change refactors the handling of masternode network information throughout the codebase by replacing direct value-type usage of the ✨ Finishing Touches
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: 4
🔭 Outside diff range comments (4)
src/evo/simplifiedmns.h (1)
52-64:⚠️ Potential issueFix formatting issues in equality operator.
The equality operator logic is correct in using
NetInfoInterface::IsEqualfor type-safe comparison of polymorphic pointers. However, the pipeline indicates formatting issues that need to be addressed.Please fix the clang format issues in the equality operator as indicated by the pipeline failure. The logic is correct but the formatting needs to be consistent with project standards.
src/evo/providertx.h (2)
51-82:⚠️ Potential issueFix READWRITE macro formatting to comply with clang-format.
The pipeline indicates formatting inconsistencies in the READWRITE macros.
Apply clang-format to fix the formatting:
SERIALIZE_METHODS(CProRegTx, obj) { - READWRITE( - obj.nVersion - ); + READWRITE(obj.nVersion);
161-164:⚠️ Potential issueInconsistency between GetMaxVersion and IsTriviallyValid signatures.
The
GetMaxVersionmethod doesn't acceptis_extended_addrparameter, butIsTriviallyValiddoes. This seems inconsistent with the pattern established inCProRegTxandCProUpServTx.Consider updating
GetMaxVersionto match the pattern:-[[nodiscard]] static constexpr uint16_t GetMaxVersion(const bool is_basic_scheme_active) +[[nodiscard]] static constexpr uint16_t GetMaxVersion(const bool is_basic_scheme_active, [[maybe_unused]] const bool is_extended_addr)Also applies to: 203-203
src/evo/providertx.cpp (1)
143-147:⚠️ Potential issueUnused parameter
is_extended_addr.The
is_extended_addrparameter is not used becauseGetMaxVersiondoesn't accept it. This should be addressed along with the header file fix.
♻️ Duplicate comments (3)
src/evo/providertx.h (2)
117-147:⚠️ Potential issueApply consistent formatting to READWRITE macros.
Similar formatting issues as in CProRegTx.
211-214:⚠️ Potential issueInconsistency between GetMaxVersion and IsTriviallyValid signatures.
Same issue as in
CProUpRegTx.Also applies to: 256-256
src/evo/providertx.cpp (1)
176-180:⚠️ Potential issueUnused parameter
is_extended_addr.Same issue as in
CProUpRegTx::IsTriviallyValid.
🧹 Nitpick comments (6)
src/net.cpp (3)
3741-3746: Avoid duplicate calls tonetInfo->GetPrimary()
You invokenetInfo->GetPrimary()twice in the conditional. Consider caching the result in a local variable for readability and slight performance gain:- if (dmn && !connectedNodes.count(dmn->pdmnState->netInfo->GetPrimary()) && - !IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) { + const auto addr = dmn->pdmnState->netInfo->GetPrimary(); + if (dmn && !connectedNodes.count(addr) && + !IsMasternodeOrDisconnectRequested(addr)) { // ... }
3761-3765: Inconsistent use of function name for logging
Earlier logs in this scope use the_func_variable for the function name; here you use__func__. For consistency, switch to_func_:- LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- probing masternode %s, service=%s\n", - __func_, dmn->proTxHash.ToString(), addr2.ToStringAddrPort()); + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- probing masternode %s, service=%s\n", + _func_, dmn->proTxHash.ToString(), addr2.ToStringAddrPort());
3788-3792: Consistent function name macro in failure logging
Similarly, replace__func__with_func_in the connection failure log to match the surrounding usage:- LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- connection failed for masternode %s, service=%s\n", - __func__, connectToDmn->proTxHash.ToString(), addr2.ToStringAddrPort()); + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- connection failed for masternode %s, service=%s\n", + _func_, connectToDmn->proTxHash.ToString(), addr2.ToStringAddrPort());src/evo/netinfo.h (2)
75-76: Fix constructor formatting to match style guidelines.The pipeline failure indicates formatting issues with the multiline constructor template.
Apply this formatting fix:
- template <typename Stream> - NetInfoEntry(deserialize_type, Stream& s) { s >> *this; } + template <typename Stream> + NetInfoEntry(deserialize_type, Stream& s) + { + s >> *this; + }
159-160: Fix constructor formatting to match style guidelines.Similar formatting issue as with
NetInfoEntry.Apply this formatting fix:
- template <typename Stream> - MnNetInfo(deserialize_type, Stream& s) { s >> *this; } + template <typename Stream> + MnNetInfo(deserialize_type, Stream& s) + { + s >> *this; + }src/evo/deterministicmns.h (1)
66-66: Fix constructor formatting to match style guidelines.The pipeline failure indicates formatting issues with the constructor template.
Apply this formatting fix:
- CDeterministicMN(deserialize_type, Stream& s) { s >> *this; } + CDeterministicMN(deserialize_type, Stream& s) + { + s >> *this; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/common.h(1 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(15 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(7 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(5 hunks)src/evo/providertx.cpp(5 hunks)src/evo/providertx.h(10 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(3 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(4 hunks)src/txmempool.cpp(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/evo/netinfo.cpp (3)
src/evo/netinfo.h (9)
lhs(133-133)rhs(80-80)rhs(81-81)rhs(82-82)rhs(82-82)rhs(164-164)rhs(164-164)rhs(165-165)rhs(165-165)src/evo/simplifiedmns.h (5)
rhs(52-64)rhs(52-52)rhs(66-69)rhs(66-66)rhs(114-114)src/evo/simplifiedmns.cpp (2)
rhs(136-145)rhs(136-136)
src/txmempool.cpp (2)
src/txmempool.h (14)
entry(187-190)entry(187-187)entry(519-519)entry(535-535)entry(536-536)entry(608-608)entry(609-609)entry(611-611)entry(616-616)entry(705-705)entry(883-883)entry(899-899)entry(1031-1035)entry(1031-1031)src/rpc/misc.cpp (1)
entry(1309-1309)
src/test/evo_simplifiedmns_tests.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(203-207)MakeNetInfo(203-203)
src/evo/simplifiedmns.h (2)
src/evo/netinfo.cpp (6)
IsEqual(150-171)IsEqual(150-150)rhs(35-46)rhs(35-35)rhs(48-63)rhs(48-48)src/evo/netinfo.h (13)
rhs(80-80)rhs(81-81)rhs(82-82)rhs(82-82)rhs(164-164)rhs(164-164)rhs(165-165)rhs(165-165)NetInfoSerWrapper(216-216)NetInfoSerWrapper(217-217)NetInfoSerWrapper(218-224)NetInfoSerWrapper(226-226)NetInfoSerWrapper(228-228)
src/test/block_reward_reallocation_tests.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(203-207)MakeNetInfo(203-203)
src/test/evo_netinfo_tests.cpp (1)
src/evo/netinfo.cpp (2)
IsEqual(150-171)IsEqual(150-150)
src/test/evo_deterministicmns_tests.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(203-207)MakeNetInfo(203-203)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/common.h
[error] 10-18: Clang format differences found: inconsistent spacing around enum values; lines need reformatting.
src/evo/simplifiedmns.h
[error] 51-90: Clang format differences found: inconsistent line breaks and argument formatting in operator== and serialization macros.
src/evo/dmnstate.h
[error] 77-110: Clang format differences found: inconsistent brace placement and argument formatting in serialization macros.
src/evo/providertx.h
[error] 58-130: Clang format differences found: inconsistent argument formatting in READWRITE macros and multiline expressions.
src/evo/deterministicmns.h
[error] 63-75: Clang format differences found: multiline constructor and template formatting inconsistent with style guidelines.
src/evo/netinfo.h
[error] 73-80: Clang format differences found: multiline constructor formatting inconsistent with style guidelines.
🔇 Additional comments (66)
src/Makefile.am (1)
189-189: LGTM! Proper integration of new header.The addition of
evo/common.hto the core headers is correctly placed and integrates the new ProTx version constants into the build system appropriately.src/evo/core_write.cpp (2)
117-117: Consistent with the polymorphic netInfo refactor.The pointer access pattern matches the architectural changes and is consistent with the refactor across the codebase.
70-70:❓ Verification inconclusive
Verify null pointer safety for netInfo access.
The change from object access to pointer dereference is consistent with the polymorphic netInfo refactor. However, ensure that
netInfois always properly initialized before these methods are called to prevent null pointer dereferences.
🏁 Script executed:
#!/bin/bash # Description: Search for netInfo initialization patterns and null checks in provider transaction code. # Search for MakeNetInfo calls and netInfo assignments rg -A 3 -B 3 "netInfo.*=" src/evo/ # Search for any null checks on netInfo rg -A 2 -B 2 "netInfo.*null|!netInfo" src/evo/ # Search for netInfo pointer usage patterns rg -A 2 "netInfo->" src/evo/Length of output: 11950
Verify
netInfois always initialized before dereference in JSON RPC handlers.The switch from value to pointer access in
core_write.cppaligns with the polymorphicnetInforefactor, but there are no explicit null‐pointer checks onnetInfoin any of the RPC‐serialization paths (e.g. incore_write.cpp,providertx.cpp,simplifiedmns.cpp,dmnstate.cpp, etc.). TheNetInfoSerWrapperused in theREADWRITEblocks ofCProRegTxandCProUpServTxshould populate thenetInfopointer during deserialization, but please confirm:
- That
NetInfoSerWrapperindeed guaranteesnetInfois non‐null for bothCProRegTxandCProUpServTxbefore these JSON helpers are ever called.- If there exist any code paths where
netInfocould remainnullptr(for example, manual instantiation without going through the deserializer), add an explicit null check orassert(netInfo)aroundinret.pushKV("service", netInfo->GetPrimary().ToStringAddrPort());src/evo/core_write.cpp(and similarly in the other JSON routines).src/rpc/masternode.cpp (2)
568-568: Consistent pointer access pattern for netInfo refactor.The change from
netInfo.GetEntries()tonetInfo->GetEntries()is consistent with the polymorphic netInfo architecture. The refactor enables version-dependent network info implementations as described in the PR objectives.
615-615: Uniform application of netInfo pointer access.The pointer dereference pattern is consistently applied across all netInfo method calls, maintaining uniformity with the architectural changes.
src/masternode/utils.cpp (1)
81-81: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator, reflecting the refactor where
netInfohas been changed from a concrete object to astd::shared_ptr<NetInfoInterface>. The logic remains unchanged.src/llmq/utils.cpp (2)
827-827: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator in the logging statement, reflecting the refactor where
netInfois now a pointer type.
876-876: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator in the logging statement, reflecting the refactor where
netInfois now a pointer type.src/qt/masternodelist.cpp (2)
224-224: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator when getting the address key, reflecting the refactor where
netInfois now a pointer type.
227-227: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator when getting the address string for display, reflecting the refactor where
netInfois now a pointer type.src/masternode/node.cpp (2)
149-149: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator when comparing the local service address with the masternode's primary address, reflecting the refactor where
netInfois now a pointer type.
204-204: LGTM: Correct syntax update for pointer-based netInfo.This change correctly updates the member access from dot to arrow operator when comparing primary addresses between old and new masternode states, reflecting the refactor where
netInfois now a pointer type.src/evo/simplifiedmns.cpp (2)
71-71: LGTM: Correct pointer dereference for netInfo.The change from
netInfo.ToString()tonetInfo->ToString()correctly reflects the refactoring ofnetInfofrom a concrete object to astd::shared_ptr<NetInfoInterface>pointer.
81-81: LGTM: Correct pointer dereference for netInfo.The change from
netInfo.GetPrimary().ToStringAddrPort()tonetInfo->GetPrimary().ToStringAddrPort()correctly reflects the refactoring ofnetInfofrom a concrete object to astd::shared_ptr<NetInfoInterface>pointer.src/rpc/quorums.cpp (1)
206-206: LGTM: Correct pointer dereference for netInfo.The change from
dmn->pdmnState->netInfo.GetPrimary()todmn->pdmnState->netInfo->GetPrimary()correctly reflects the refactoring ofnetInfofrom a concrete object to astd::shared_ptr<NetInfoInterface>pointer.src/test/evo_simplifiedmns_tests.cpp (3)
22-22: LGTM: Correct version initialization for new interface.The version is properly set using
CProRegTx::GetMaxVersionwithis_extended_addr=false, which is appropriate for this test case that uses the legacy address scheme.
23-23: LGTM: Correct usage of MakeNetInfo factory function.The
MakeNetInfo(smle)call correctly creates astd::shared_ptr<NetInfoInterface>using the entry's version. SincenVersionwas set on the previous line, this should satisfy the factory function's assertions.
27-27: LGTM: Correct pointer dereference for netInfo.The change from
netInfo.AddEntry()tonetInfo->AddEntry()correctly reflects the refactoring ofnetInfofrom a concrete object to astd::shared_ptr<NetInfoInterface>pointer.src/net.cpp (1)
3676-3682: Consistent pointer-based access to NetInfo now in place
AccessingnetInfovia pointer is correct as part of the interface refactor; this aligns with the polymorphic design forNetInfoInterface.src/evo/netinfo.cpp (1)
150-171: Well-implemented polymorphic equality comparison.The
IsEqualstatic method correctly handles all necessary cases for comparingNetInfoInterfaceshared pointers:
- Pointer identity and null checks for optimization
- Type-safe dynamic casting to concrete implementations
- Deep comparison using the concrete type's
operator==- Proper fallback for differing derived types
This implementation aligns perfectly with the interface-based refactoring described in the PR objectives.
src/test/block_reward_reallocation_tests.cpp (1)
118-121: Correctly updated to use new netInfo interface pattern.The changes properly implement the refactored
netInfohandling:
- Version initialization includes the new
is_extended_addrparameterMakeNetInfo(proTx)correctly initializes the polymorphicnetInfopointer- Access pattern updated from direct member to pointer dereference
The sequence ensures
nVersionis set before callingMakeNetInfo(), preventing potential null pointer issues as mentioned in the PR objectives.src/evo/dmnstate.cpp (3)
35-35: Correctly updated netInfo access pattern.The change from
netInfo.ToString()tonetInfo->ToString()properly reflects the refactoring from concrete object to shared pointer.
42-42: Correctly updated netInfo access pattern.The change from
netInfo.GetPrimary()tonetInfo->GetPrimary()properly reflects the refactoring from concrete object to shared pointer.
76-76: Correctly updated netInfo access pattern.The change from
netInfo.GetPrimary()tonetInfo->GetPrimary()properly reflects the refactoring from concrete object to shared pointer.src/coinjoin/client.cpp (6)
189-189: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.
1109-1109: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.
1181-1181: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.
1221-1221: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.
1823-1823: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.
1879-1879: Correctly updated netInfo access pattern.The change to use pointer dereference (
netInfo->GetPrimary()) properly reflects the refactoring from concrete object to shared pointer.src/test/evo_netinfo_tests.cpp (1)
217-236: Excellent test coverage for the new interface equality logic.This test case thoroughly validates the
NetInfoInterface::IsEqualstatic method across all relevant scenarios:
- Uninitialized pointers (both null)
- Unequal initialization status (one null, one initialized)
- Equal initialization with same content (both empty)
- Equal initialization with different content
The test correctly uses
MnNetInfoas the concrete implementation and verifies that the equality detection properly handles both pointer comparison and deep content comparison.src/txmempool.cpp (8)
664-664: LGTM: Correct update for polymorphic netInfo access.The change from
proTx.netInfo.GetEntries()toproTx.netInfo->GetEntries()is consistent with the refactor to usestd::shared_ptr<NetInfoInterface>instead of a concreteMnNetInfoobject.
677-677: LGTM: Consistent pointer dereference for netInfo.Properly updated to use pointer dereference syntax for the polymorphic netInfo interface.
768-768: LGTM: Correct pointer access in removal logic.The change maintains the same logic while properly accessing the polymorphic netInfo through pointer dereference.
778-778: LGTM: Consistent with polymorphic netInfo design.Correct update to use pointer dereference for accessing netInfo entries.
1007-1007: LGTM: Proper polymorphic access in conflict detection.The change correctly updates the access pattern while maintaining the same conflict detection logic.
1029-1029: LGTM: Consistent pointer dereference pattern.Properly updated to use the new polymorphic netInfo interface.
1367-1367: LGTM: Correct update in provider transaction conflict check.The change maintains the same conflict detection logic while using the new pointer-based netInfo access.
1392-1392: LGTM: Final consistent update for polymorphic netInfo.All netInfo access patterns have been properly updated to use pointer dereference for the polymorphic interface.
src/test/evo_trivialvalidation.cpp (4)
20-22: Good addition of extended address parameter for future support.The addition of the
is_extended_addrparameter toTestTxHelperproperly prepares the test infrastructure for extended address validation while maintaining backward compatibility.
32-32: Correct parameter forwarding to validation method.The
is_extended_addrparameter is properly passed to theIsTriviallyValidmethod, enabling version-specific validation logic.
55-55: Appropriate placeholder for future extended address testing.The TODO comment and hardcoded
falsevalue is a reasonable approach for the current phase, allowing the infrastructure to be in place while extended address test vectors are developed.
72-91: Consistent parameter updates across all provider transaction types.All test cases for provider transactions (
CProRegTx,CProUpServTx,CProUpRegTx,CProUpRevTx) have been properly updated to pass theis_extended_addrparameter.src/evo/simplifiedmns.h (1)
38-38: Correct transition to polymorphic netInfo interface.The change from
MnNetInfo netInfotostd::shared_ptr<NetInfoInterface> netInfo{nullptr}properly implements the polymorphic design while ensuring safe initialization.src/rpc/evo.cpp (2)
656-662: LGTM! Correct implementation of the new netInfo interface.The changes properly implement the new polymorphic
NetInfoInterface:
- Correctly checks for V23 deployment to determine extended address support
- Properly initializes
netInfousing the factory function- Consistently uses pointer dereference syntax for member access
Also applies to: 688-688
980-980: LGTM! Consistent implementation with the register wrapper.The changes correctly apply the same pattern as in
protx_register_common_wrapper.Also applies to: 982-982
src/test/evo_deterministicmns_tests.cpp (1)
107-110: LGTM! Test updates correctly implement the new netInfo interface.All test functions have been consistently updated to:
- Use
MakeNetInfofactory for initialization- Pass
is_extended_addr=falseto maintain existing test behavior- Use pointer dereference syntax (
->) for member accessThe test coverage appropriately validates the refactored interface.
Also applies to: 130-133, 524-524, 582-582, 641-643, 717-719, 786-788
src/evo/dmnstate.h (2)
218-229: LGTM! Correct handling of polymorphic netInfo comparison.The use of
NetInfoInterface::IsEqualfor comparing shared pointers to polymorphic types is the right approach, as virtual operators are not feasible.
255-260: Clarify the TODO comment and verify the is_extended parameter.The TODO comment on line 258 states "Implement this", but there is already an implementation using
NetInfoSerWrapper. This is confusing and needs clarification.Additionally, the
is_extendedparameter is hardcoded tofalse. Please verify if this is intentional for backward compatibility or if it should be determined dynamically based on the version.src/evo/providertx.h (1)
29-33: LGTM! Version logic is properly implemented.The
GetMaxVersionmethod correctly returnsProTxVersion::ExtAddrwhen extended addresses are enabled, with proper fallback to BasicBLS or LegacyBLS versions.src/evo/providertx.cpp (2)
15-63: Proper implementation of extended address validation.The function correctly validates the ProTx version against the extended address flag and properly accesses
netInfovia pointer dereference.
108-126: Consistent implementation with pointer-based netInfo access.The function properly handles the extended address parameter and correctly uses pointer dereference for
netInfo.src/evo/deterministicmns.cpp (5)
460-473: Proper handling of netInfo entries in AddMN.The function correctly iterates through netInfo entries via pointer dereference and maintains proper error handling for invalid addresses.
510-544: Excellent implementation of netInfo update logic.The use of
NetInfoInterface::IsEqualfor comparison and the atomic update pattern with rollback capability ensures data consistency. The lambda properly handles the transition from old to new netInfo entries.
888-890: Good: Maintaining version consistency.Setting
nVersionfrom the ProTx version ensures the state properly reflects the transaction version, which is important for the polymorphic netInfo handling.
929-931: Correct netInfo reset after operator key change.The call to
MakeNetInfo(*newState)properly reconstructs the network info after updating the operator key, ensuring consistency with the new state.
1329-1335: Proper deployment-based extended address flag.The code correctly determines
is_extended_addrbased on V23 deployment status and passes it to validation, aligning with the PR's extended address support objectives.src/evo/netinfo.h (4)
8-8: New dependency added for common types.The inclusion of
evo/common.his appropriate for accessing theProTxVersionenum used in theMakeNetInfotemplate function.
130-147: Well-designed polymorphic interface for network info.The
NetInfoInterfaceprovides a clean abstraction with appropriate virtual methods. The staticIsEqualmethod is a good solution for comparing shared pointers since operator overloading in virtual classes is not feasible.
149-199: MnNetInfo correctly implements the interface.The refactoring of
MnNetInfoto inherit fromNetInfoInterfaceis well-implemented. All virtual methods are properly overridden with theoverridekeyword.
201-207:✅ Verification successful
Verify version range assertion logic.
The factory function asserts that
obj.nVersionis greater than 0 and less thanProTxVersion::ExtAddr. Ensure that this version checking logic aligns with the actual version values defined inevo/common.h.
🏁 Script executed:
#!/bin/bash # Description: Verify ProTxVersion enum values and version checking logic # Expected: ProTxVersion::ExtAddr should be defined and have appropriate value # Find the ProTxVersion enum definition ast-grep --pattern 'enum $_ ProTxVersion $_'Length of output: 46
Let’s locate the
ProTxVersionenum and confirm the value ofExtAddr:
🏁 Script executed:
#!/bin/bash # Find where ProTxVersion is defined and its members rg -n "enum .*ProTxVersion" -C3 # Confirm the numeric value (or surrounding context) of ExtAddr rg -n "ExtAddr" -C3Length of output: 4482
Assertion logic confirmed against enum definitions
ProTxVersion in
src/evo/common.his defined as:
- LegacyBLS = 1
- BasicBLS = 2
- ExtAddr = 3
The factory’s
assert(obj.nVersion > 0 && obj.nVersion < ProTxVersion::ExtAddr);therefore enforces
obj.nVersion ∈ {1,2}—exactly LegacyBLS and BasicBLS. No change is needed unless you plan to handleExtAddr(3) or higher here with a differentNetInfoInterfaceimplementation.src/evo/deterministicmns.h (5)
68-83: SERIALIZE_METHODS refactor looks correct.The migration to the
SERIALIZE_METHODSmacro simplifies the serialization code while maintaining backward compatibility with version checks for thenTypefield.
176-195: Improved serialization with structured bindings.The use of structured bindings (
const auto& [_, dmn]) and uniform initialization improves code readability. The newClear()method call before deserialization is a good practice.
197-202: Clear() method provides proper state reset.The
Clear()method properly resets all internal maps using default constructors, which is essential for safe deserialization.
393-394: Template static assertions updated for new interface types.The additions correctly exclude the new
NetInfoInterfaceandstd::shared_ptr<NetInfoInterface>types from theGetUniquePropertyHashtemplate method, maintaining type safety.
477-520: Serialization improvements with structured bindings.The changes improve code clarity with structured bindings and uniform initialization. The comment about CDeterministicMNStateDiff migration logic is helpful context.
src/evo/common.h
Outdated
| namespace ProTxVersion { | ||
| enum : uint16_t { | ||
| LegacyBLS = 1, | ||
| BasicBLS = 2, | ||
| ExtAddr = 3, | ||
| }; | ||
| } // namespace ProTxVersion |
There was a problem hiding this comment.
Fix formatting to address pipeline failure.
The clang format check failed due to inconsistent spacing around enum values. Apply consistent formatting to resolve the CI failure.
namespace ProTxVersion {
-enum : uint16_t {
- LegacyBLS = 1,
- BasicBLS = 2,
- ExtAddr = 3,
-};
+enum : uint16_t {
+ LegacyBLS = 1,
+ BasicBLS = 2,
+ ExtAddr = 3,
+};
} // namespace ProTxVersion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespace ProTxVersion { | |
| enum : uint16_t { | |
| LegacyBLS = 1, | |
| BasicBLS = 2, | |
| ExtAddr = 3, | |
| }; | |
| } // namespace ProTxVersion | |
| namespace ProTxVersion { | |
| enum : uint16_t { | |
| LegacyBLS = 1, | |
| BasicBLS = 2, | |
| ExtAddr = 3, | |
| }; | |
| } // namespace ProTxVersion |
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 10-18: Clang format differences found: inconsistent spacing around enum values; lines need reformatting.
🤖 Prompt for AI Agents
In src/evo/common.h around lines 10 to 16, the enum values have inconsistent
spacing which causes the clang format check to fail. Adjust the spacing around
the enum values so that the equals signs and values are aligned consistently,
ensuring uniform indentation and spacing to pass the formatting check.
| @@ -76,7 +76,7 @@ class CSimplifiedMNListEntry | |||
| READWRITE( | |||
| obj.proRegTxHash, | |||
| obj.confirmedHash, | |||
| obj.netInfo, | |||
| NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.netInfo), obj.nVersion >= ProTxVersion::ExtAddr), | |||
There was a problem hiding this comment.
Proper version-aware serialization with formatting concerns.
The use of NetInfoSerWrapper with obj.nVersion >= ProTxVersion::ExtAddr correctly implements version-aware serialization for extended address support. However, this line is also affected by the formatting issues flagged by the pipeline.
While the serialization logic is correct, please address the clang format differences identified by the pipeline for consistent code formatting.
🤖 Prompt for AI Agents
In src/evo/simplifiedmns.h at line 79, the serialization logic using
NetInfoSerWrapper with version check is correct, but the line formatting does
not comply with clang format standards. Reformat this line according to the
project's clang format rules to ensure consistent code style, such as proper
spacing, indentation, and line breaks if necessary.
src/evo/netinfo.h
Outdated
| class NetInfoSerWrapper | ||
| { | ||
| private: | ||
| std::shared_ptr<NetInfoInterface>& m_data; | ||
| const bool m_is_extended{false}; | ||
|
|
||
| public: | ||
| NetInfoSerWrapper() = delete; | ||
| NetInfoSerWrapper(const NetInfoSerWrapper&) = delete; | ||
| NetInfoSerWrapper(std::shared_ptr<NetInfoInterface>& data, const bool is_extended) : | ||
| m_data{data}, | ||
| m_is_extended{is_extended} | ||
| { | ||
| // TODO: Remove when extended addresses implementation is added in | ||
| assert(!m_is_extended); | ||
| } | ||
| template <typename Stream> | ||
| NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; } | ||
|
|
||
| ~NetInfoSerWrapper() = default; | ||
|
|
||
| void Clear() { m_addr.Clear(); } | ||
| template <typename Stream> | ||
| void Serialize(Stream& s) const | ||
| { | ||
| if (const auto ptr{std::dynamic_pointer_cast<MnNetInfo>(m_data)}) { | ||
| s << *ptr; | ||
| } else { | ||
| // MakeNetInfo() supplied an unexpected implementation or we didn't call it and | ||
| // are left with a nullptr. Neither should happen. | ||
| assert(false); | ||
| } | ||
| } | ||
|
|
||
| template <typename Stream> | ||
| void Unserialize(Stream& s) | ||
| { | ||
| m_data = std::make_shared<MnNetInfo>(deserialize, s); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NetInfoSerWrapper design needs refinement.
Several concerns with the current implementation:
- The constructor takes a mutable reference but the class has const and non-const methods that could cause confusion
- The assertion
assert(!m_is_extended)suggests incomplete implementation - The deserialization constructor template doesn't make sense in this context
Consider these improvements:
class NetInfoSerWrapper
{
private:
std::shared_ptr<NetInfoInterface>& m_data;
- const bool m_is_extended{false};
+ const bool m_is_extended;
public:
NetInfoSerWrapper() = delete;
NetInfoSerWrapper(const NetInfoSerWrapper&) = delete;
NetInfoSerWrapper(std::shared_ptr<NetInfoInterface>& data, const bool is_extended) :
m_data{data},
m_is_extended{is_extended}
{
- // TODO: Remove when extended addresses implementation is added in
- assert(!m_is_extended);
+ if (m_is_extended) {
+ throw std::runtime_error("Extended addresses not yet supported");
+ }
}
- template <typename Stream>
- NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class NetInfoSerWrapper | |
| { | |
| private: | |
| std::shared_ptr<NetInfoInterface>& m_data; | |
| const bool m_is_extended{false}; | |
| public: | |
| NetInfoSerWrapper() = delete; | |
| NetInfoSerWrapper(const NetInfoSerWrapper&) = delete; | |
| NetInfoSerWrapper(std::shared_ptr<NetInfoInterface>& data, const bool is_extended) : | |
| m_data{data}, | |
| m_is_extended{is_extended} | |
| { | |
| // TODO: Remove when extended addresses implementation is added in | |
| assert(!m_is_extended); | |
| } | |
| template <typename Stream> | |
| NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; } | |
| ~NetInfoSerWrapper() = default; | |
| void Clear() { m_addr.Clear(); } | |
| template <typename Stream> | |
| void Serialize(Stream& s) const | |
| { | |
| if (const auto ptr{std::dynamic_pointer_cast<MnNetInfo>(m_data)}) { | |
| s << *ptr; | |
| } else { | |
| // MakeNetInfo() supplied an unexpected implementation or we didn't call it and | |
| // are left with a nullptr. Neither should happen. | |
| assert(false); | |
| } | |
| } | |
| template <typename Stream> | |
| void Unserialize(Stream& s) | |
| { | |
| m_data = std::make_shared<MnNetInfo>(deserialize, s); | |
| } | |
| }; | |
| class NetInfoSerWrapper | |
| { | |
| private: | |
| std::shared_ptr<NetInfoInterface>& m_data; | |
| const bool m_is_extended; | |
| public: | |
| NetInfoSerWrapper() = delete; | |
| NetInfoSerWrapper(const NetInfoSerWrapper&) = delete; | |
| NetInfoSerWrapper(std::shared_ptr<NetInfoInterface>& data, const bool is_extended) : | |
| m_data{data}, | |
| m_is_extended{is_extended} | |
| { | |
| if (m_is_extended) { | |
| throw std::runtime_error("Extended addresses not yet supported"); | |
| } | |
| } | |
| ~NetInfoSerWrapper() = default; | |
| template <typename Stream> | |
| void Serialize(Stream& s) const | |
| { | |
| if (const auto ptr{std::dynamic_pointer_cast<MnNetInfo>(m_data)}) { | |
| s << *ptr; | |
| } else { | |
| // Unexpected type or null pointer | |
| assert(false); | |
| } | |
| } | |
| template <typename Stream> | |
| void Unserialize(Stream& s) | |
| { | |
| m_data = std::make_shared<MnNetInfo>(deserialize, s); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/evo/netinfo.h around lines 209 to 247, the NetInfoSerWrapper class has
design issues: it takes a mutable reference but has const and non-const methods
causing confusion, the assert(!m_is_extended) indicates incomplete features, and
the deserialization constructor template is unclear. To fix, change the
constructor to take a shared_ptr by value or const reference to avoid mutable
reference confusion, remove or properly implement the extended addresses feature
instead of asserting, and revise or remove the deserialization constructor
template to clearly separate construction and deserialization logic.
src/evo/netinfo.cpp
Outdated
| @@ -147,6 +147,29 @@ std::string NetInfoEntry::ToStringAddrPort() const | |||
| m_data); | |||
| } | |||
|
|
|||
| bool NetInfoInterface::IsEqual(const std::shared_ptr<NetInfoInterface>& lhs, const std::shared_ptr<NetInfoInterface>& rhs) | |||
There was a problem hiding this comment.
why make this static and not operator== inside the class?
There was a problem hiding this comment.
This is addressed in the PR description and is a product of how pure virtual classes work (source).
- As implementing operators in virtual classes (needed to allow switching implementations) is not possible (unlike regular pointers where a
lhs && rhs ? *lhs == *rhs : lhs == rhswould be enough), a separate equality functionNetInfoInterface::IsEqual()has been implemented that attempts to downcastlhsandrhsto the same type and returns the implementation-specific equality result.
There was a problem hiding this comment.
see: https://coliru.stacked-crooked.com/a/0e68596a6becd1f6 Something like that may be useful. In general, I would prefer we find ways to do this without all the dynamic runtime type checking stuff
There was a problem hiding this comment.
The proposed solution is quite complex for the simple one parent (interface), two children (legacy and extended implementations) inheritance structure. A simpler one involving the much cheaper typeid operator has been used instead.
A helper function is still needed as NetInfoInterface::IsEqual() handled both downcasting and deep comparison and the solution mentioned only deals with the former but that can be replaced with a more generic helper function (util::shared_ptr_{,not_}equal), which we have done here.
Resolved in latest push.
|
Added comments; also concerned about the performance impact of shared_ptr, and dynamic type deduction |
| @@ -885,6 +885,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no | |||
| } | |||
|
|
|||
| auto newState = std::make_shared<CDeterministicMNState>(*dmn->pdmnState); | |||
| newState->nVersion = opt_proTx->nVersion; | |||
There was a problem hiding this comment.
So far as I see, a newState here is an internal data structure; it should not have version.
Version should be used only for serialization / deserialization / send by network.
Could you justify nVersion usage here, please?
There was a problem hiding this comment.
Follows precedent set by pubKeyOperator for consistency's sake (setting nVersion when (re)setting any field that is version-dependent)
dash/src/evo/deterministicmns.cpp
Lines 940 to 943 in 5cbddbb
There was a problem hiding this comment.
PublicKey is very VERY heavy datastructure, it is not only hundreds of bytes data, but it's also very heavy serialozation / deserialisation and conversion between basic / legacy formats. Please, don't follow this precendent here, I wish public keys and signatures in memory, in network and disk be always basic, and just convert it to legacy / back when validating old blocks
| @@ -105,6 +105,7 @@ static CMutableTransaction CreateProRegTx(const CChain& active_chain, const CTxM | |||
|
|
|||
| CProRegTx proTx; | |||
| proTx.nVersion = CProRegTx::GetMaxVersion(!bls::bls_legacy_scheme); | |||
| proTx.netInfo = MakeNetInfo(proTx); | |||
There was a problem hiding this comment.
(some extra notes related to this line in the same commit)
newState->nVersion = opt_proTx->nVersion;
version of netInfo should depends on version of CProRegTx, CProUpServTx, etc.
If this object serialized / deserialized - it should not have any "internal version".it should be done only as part of protx / CDeterministicMNState object / etc. They control version of object.
netInfo should no have no any version inside; otherwise I am afraid it would be a bigger nightmare than legacy bls. :)
There was a problem hiding this comment.
While the specifics are outside the scope of this PR as it pertains to the extended addresses implementation, it is an important concern. The extended address implementation has a version number separate from the ProTx version (see below, WIP code) that governs its ruleset.
Lines 267 to 269 in dcddec3
The rationale is that the ProTx version informs us of two things, what is the format and what is the ruleset. Right now, if nVersion > ProTxVersion::LegacyBLS, it's a change in format (switching over to ExtNetInfo) and ruleset (storing Platform fields, multiple entries, etc.).
Say tomorrow we want to revise the ruleset (the format has been designed with extensibility in mind so hopefully we don't have to revise it), nVersion > ProTxVersion::ExtAddr is only a change in ruleset (say, we allow storing a new type of address), now ExtNetInfo needs to know about ProTx versioning, which goes against the idea of keeping it self-contained (MakeNetInfo() exists for this specific reason, to decide what to construct and how based on the version).
So we introduce ExtNetInfo::m_version, which is set based on the ProTx version and then validated by calling netInfo->GetVersion(). As we aren't doing this, such a function doesn't exist for NetInfoInterface or its implementations.
There was a problem hiding this comment.
If we need to change ruleset, it means that rules of consensus are changed. It is perfectly matched with protx version bump
There was a problem hiding this comment.
the format has been designed with extensibility in mind so hopefully we don't have to revise
Will "unkown" type of data be allowed to get mined in block? I think not, will it?
There was a problem hiding this comment.
Unknown data is not allowed, they have to meet the ruleset and yes, a ProTx version bump is required in lockstep with the ExtNetInfo version bump (the ProTx version sets the ExtNetInfo version which changes the ruleset)
src/evo/deterministicmns.cpp
Outdated
| @@ -1327,7 +1327,8 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not | |||
| return std::nullopt; | |||
| } | |||
| const bool is_basic_scheme_active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)}; | |||
| if (!opt_ptx->IsTriviallyValid(is_basic_scheme_active, state)) { | |||
| const bool is_extended_addr{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V23)}; | |||
There was a problem hiding this comment.
Consider implementing new helper which converts CBlockIndex* to ProTxVersion.
Something like
inline ProTxVersion ProtxVersionAt(const CBlockIndex& index, const Consensus::Params& params)
{
if (DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V23)) return ProTxVersion::Ext;
return (v19...) ? ProTxVersion::Basic : ProTxVersion::Legacy; }Passing 2 flags deep inside IsTriviallyValid and GetMaxVersion does look too good
There was a problem hiding this comment.
The manual flag option cannot be entirely gotten rid of without changing the way some tests are done (and it is still the faster way to compute nVersion as it is constexpr) but ProTxVersion::GetMaxFromDeployment() does what is described.
Resolved in latest push.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/evo/dmnstate.h (4)
80-80: Fix clang format issues in template constructor.The template deserialization constructor is correct but has formatting issues flagged by the pipeline.
99-99: Fix clang format issues in NetInfoSerWrapper usage.The use of
NetInfoSerWrapperwith version-dependent extended address flag is correct, but formatting needs to be fixed.
237-238: Fix clang format issues in template constructor.The template deserialization constructor is correctly implemented but has formatting issues.
255-260: Complete extended address implementation.The TODO comment indicates incomplete implementation for extended addresses. The hardcoded
is_extended=falseparameter suggests this feature is not yet ready.Do you want me to help track the remaining work for extended address support or create an issue to ensure this TODO is addressed?
Additionally, fix the clang format issues in this section.
src/evo/netinfo.h (3)
74-75: Fix clang format issues in template constructor.The template deserialization constructor is correctly implemented but has formatting issues flagged by the pipeline.
160-161: Fix clang format issues in template constructor.The template deserialization constructor is correctly implemented but has formatting issues.
203-241: 🛠️ Refactor suggestionAddress NetInfoSerWrapper design concerns.
The
NetInfoSerWrapperclass has several design issues that need attention:
- Incomplete extended address support: The assertion
assert(!m_is_extended)indicates incomplete implementation- Mutable reference confusion: Taking a mutable reference to
shared_ptrcan be confusing- Unnecessary template constructor: The deserialization template constructor seems redundant
Apply these improvements to address the design concerns:
class NetInfoSerWrapper { private: std::shared_ptr<NetInfoInterface>& m_data; - const bool m_is_extended{false}; + const bool m_is_extended; public: NetInfoSerWrapper() = delete; NetInfoSerWrapper(const NetInfoSerWrapper&) = delete; NetInfoSerWrapper(std::shared_ptr<NetInfoInterface>& data, const bool is_extended) : m_data{data}, m_is_extended{is_extended} { - // TODO: Remove when extended addresses implementation is added in - assert(!m_is_extended); + if (m_is_extended) { + throw std::runtime_error("Extended addresses not yet supported"); + } } - template <typename Stream> - NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; } ~NetInfoSerWrapper() = default;Also fix the clang format issues in the constructor.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 217-227: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
🧹 Nitpick comments (1)
src/evo/dmnstate.h (1)
218-229: Improve performance of NetInfoInterface::IsEqual usage.The special handling for
netInfofield usingNetInfoInterface::IsEqualis necessary due to pointer semantics, but the current implementation may have performance implications due to dynamic casting.Consider caching the comparison result if this code path is performance-critical:
if constexpr (BaseType::mask == Field_netInfo) { - if (!NetInfoInterface::IsEqual(member.get(a), member.get(b))) { + // Cache result to avoid repeated dynamic_pointer_cast in IsEqual + const bool are_equal = NetInfoInterface::IsEqual(member.get(a), member.get(b)); + if (!are_equal) { member.get(state) = member.get(b); fields |= member.mask; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/common.h(1 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(16 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(7 hunks)src/evo/netinfo.cpp(2 hunks)src/evo/netinfo.h(4 hunks)src/evo/providertx.cpp(5 hunks)src/evo/providertx.h(10 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(3 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(4 hunks)src/txmempool.cpp(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/masternode/utils.cpp
- src/llmq/utils.cpp
🚧 Files skipped from review as they are similar to previous changes (24)
- src/Makefile.am
- src/evo/core_write.cpp
- src/rpc/masternode.cpp
- src/evo/common.h
- src/masternode/node.cpp
- src/qt/masternodelist.cpp
- src/evo/simplifiedmns.cpp
- src/rpc/quorums.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/test/evo_netinfo_tests.cpp
- src/evo/netinfo.cpp
- src/evo/dmnstate.cpp
- src/evo/simplifiedmns.h
- src/txmempool.cpp
- src/test/evo_trivialvalidation.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/coinjoin/client.cpp
- src/rpc/evo.cpp
- src/evo/providertx.cpp
- src/evo/providertx.h
- src/evo/deterministicmns.h
- src/net.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/evo/netinfo.h (3)
src/evo/netinfo.cpp (4)
rhs(36-47)rhs(36-36)rhs(49-64)rhs(49-49)src/evo/simplifiedmns.h (5)
rhs(52-64)rhs(52-52)rhs(66-69)rhs(66-66)rhs(114-114)src/evo/dmnstate.h (1)
nVersion(107-115)
src/evo/dmnstate.h (2)
src/evo/netinfo.h (8)
s(178-181)s(178-178)NetInfoSerWrapper(210-210)NetInfoSerWrapper(211-211)NetInfoSerWrapper(212-218)NetInfoSerWrapper(220-220)NetInfoSerWrapper(222-222)nVersion(133-133)src/evo/netinfo.cpp (4)
MakeNetInfo(174-178)MakeNetInfo(174-174)IsEqual(151-172)IsEqual(151-151)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 72-82: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
[error] 158-168: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
[error] 217-227: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
src/evo/dmnstate.h
[error] 77-85: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
[error] 94-104: Clang format differences found: inconsistent indentation and line breaks in READWRITE macro arguments.
[error] 235-243: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
[error] 256-263: Clang format differences found: inconsistent line breaks and indentation in READWRITE macro arguments.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: check_merge
- GitHub Check: predict_conflicts
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (4)
src/evo/dmnstate.h (2)
57-57: LGTM! Correct transition to polymorphic interface.The change from concrete
MnNetInfotostd::shared_ptr<NetInfoInterface>properly enables polymorphic behavior for different network info implementations based on version.
111-111:✅ Verification successful
Verify NetInfoInterface::MakeNetInfo implementation.
The usage of
NetInfoInterface::MakeNetInfo(nVersion)is correct for factory-based creation. Ensure the factory method properly handles all supported versions.
🏁 Script executed:
#!/bin/bash # Description: Verify MakeNetInfo factory method implementation and usage # Expected: Factory method should handle all ProTx versions properly # Search for MakeNetInfo implementation ast-grep --pattern 'std::shared_ptr<NetInfoInterface> NetInfoInterface::MakeNetInfo($nVersion) { $$$ }' # Search for all MakeNetInfo usages to ensure consistent version handling rg -A 3 'MakeNetInfo'Length of output: 5182
NetInfoInterface::MakeNetInfo correctly implemented
The factory in src/evo/netinfo.cpp
– Asserts thatnVersion > 0 && nVersion < ProTxVersion::ExtAddr
– Returns aMnNetInfoinstance for all supported versionsNo further changes are required unless you add new NetInfo implementations for future ProTx versions—at that point, update this method with version‐specific branching.
src/evo/netinfo.h (2)
129-148: Well-designed interface for polymorphic network info.The
NetInfoInterfaceabstract class provides a clean separation between interface and implementation, enabling version-based polymorphism for different network info types.The static methods
IsEqualandMakeNetInfoare appropriately designed for comparing different implementations and factory creation.
150-201: Good implementation of NetInfoInterface.
MnNetInfoproperly inherits fromNetInfoInterfaceand implements all required virtual methods. The override declarations are explicit and the implementation maintains backward compatibility with existing serialization.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 158-168: Clang format differences found: multi-line constructor formatting inconsistent with style guidelines.
There was a problem hiding this comment.
utACK 7929c31;
with the understanding that:
- we try to investigate more about minimizing shared_ptr usage
- Investigate non-static operator==
- Investigate minimizing dynamic typing
- introduce ProtxVersionAt as suggested in future PR: https://github.com/dashpay/dash/pull/6665/files#r2120335836
Co-authored-by: Konstantin Akimov <[email protected]> There isn't any code left to report `format_version`, making the specializations vestigial, so we can remove it. Done to avoid potential conflict with OverrideStream usage.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(17 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(8 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(4 hunks)src/evo/providertx.cpp(5 hunks)src/evo/providertx.h(11 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(4 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(6 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/evo_netinfo_tests.cpp(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(4 hunks)src/txmempool.cpp(8 hunks)src/util/pointer.h(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/masternode/utils.cpp
- src/Makefile.am
- src/evo/netinfo.h
🚧 Files skipped from review as they are similar to previous changes (22)
- src/qt/masternodelist.cpp
- src/evo/simplifiedmns.cpp
- src/rpc/masternode.cpp
- src/rpc/quorums.cpp
- src/evo/dmnstate.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/core_write.cpp
- src/llmq/utils.cpp
- src/evo/netinfo.cpp
- src/coinjoin/client.cpp
- src/txmempool.cpp
- src/masternode/node.cpp
- src/evo/simplifiedmns.h
- src/test/block_reward_reallocation_tests.cpp
- src/test/evo_netinfo_tests.cpp
- src/test/evo_trivialvalidation.cpp
- src/net.cpp
- src/evo/dmnstate.h
- src/evo/deterministicmns.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/rpc/evo.cpp
- src/evo/providertx.h
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/evo/deterministicmns.h
[error] 63-70: Clang format differences found: code formatting does not match expected style.
src/evo/providertx.cpp
[error] 14-40: Clang format differences found: code formatting does not match expected style.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (18)
src/util/pointer.h (1)
13-16: LGTM! Well-designed deep equality helper.The logic correctly handles both pointer identity comparison and deep value comparison with proper null pointer safety.
src/evo/providertx.cpp (7)
16-24: LGTM! Improved deployment-based version validation.The centralized version determination logic based on deployment states is a good improvement over boolean flags.
50-52: LGTM! Good version consistency validation.The check ensures that the netInfo capabilities match the transaction version, which maintains consistency in the polymorphic interface design.
53-57: LGTM! Correct pointer dereference syntax.The update from direct object access to pointer dereference aligns with the new shared_ptr design.
116-116: LGTM! Consistent pointer dereference in ToString.The change maintains consistency with the new pointer-based netInfo design.
127-129: LGTM! Consistent validation logic across transaction types.The same netInfo version validation is appropriately applied to CProUpServTx.
130-137: LGTM! Consistent pointer access pattern.The netInfo validation follows the same pattern as in CProRegTx with proper pointer dereference.
26-29:❓ Verification inconclusive
Address clang-format issues.
The pipeline indicates formatting differences that need to be resolved.
Please run clang-format on this file to fix the formatting issues flagged by the pipeline:
🏁 Script executed:
#!/bin/bash # Description: Check clang-format differences in providertx.cpp # Show the formatting differences clang-format --dry-run --Werror src/evo/providertx.cppLength of output: 159
Run clang-format to resolve style issues
I attempted to run
clang-formatin this environment but it isn’t installed. Please ensureclang-formatis available and re-run the following locally to fix any formatting issues flagged by the CI:clang-format --dry-run --Werror src/evo/providertx.cppsrc/evo/deterministicmns.h (10)
66-66: LGTM! Modernized deserialization constructor.The simplified constructor using the deserialize_type is cleaner and more consistent.
178-181: LGTM! Improved iteration with structured bindings.Using structured bindings
[_, dmn]makes the code more readable and modern.
184-193: LGTM! Good addition of Clear() method.The Clear() method provides a clean way to reset state before deserialization, improving encapsulation.
190-192: LGTM! Improved loop with reverse counting.The reverse-counting loop pattern is more efficient and cleaner than the previous approach.
195-203: LGTM! Comprehensive state reset in Clear().The Clear() method properly resets all member variables to their default states.
394-396: LGTM! Good template constraints for type safety.The static assertions prevent incorrect template instantiation with NetInfoInterface types, which is important for the new polymorphic design.
484-487: LGTM! Consistent use of structured bindings.The structured binding pattern is consistently applied throughout the serialization methods.
490-492: LGTM! Clean iteration pattern.The consistent use of structured bindings and reverse-counting loops improves code readability.
501-503: LGTM! Consistent deserialization improvements.The reverse-counting loop pattern is consistently applied across all deserialization loops, improving code uniformity.
Also applies to: 505-510, 512-515
68-83:❓ Verification inconclusive
Address clang-format issues in serialization method.
The pipeline indicates formatting differences that need to be resolved in the SERIALIZE_METHODS block.
Please run clang-format on this file to fix the formatting issues:
🏁 Script executed:
#!/bin/bash # Description: Check clang-format differences in deterministicmns.h # Show the formatting differences clang-format --dry-run --Werror src/evo/deterministicmns.hLength of output: 163
Run clang-format on serialization method
I wasn’t able to verify formatting automatically (clang-format isn’t available in this environment). Please run the following locally to ensure the
SERIALIZE_METHODSblock insrc/evo/deterministicmns.hmeets project style guidelines, and commit any resulting changes:clang-format -i src/evo/deterministicmns.h git diff --exit-code src/evo/deterministicmns.h
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/evo/providertx.h (1)
27-150:⚠️ Potential issueFix clang-format issues in version handling code.
The pipeline indicates formatting issues with the new ProTx version logic and related code.
Run clang-format to fix the formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 27-150: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h (1)
72-80:⚠️ Potential issueFix clang-format issues in template constructor.
The pipeline indicates formatting issues in the template deserialization constructor.
Run clang-format to fix the formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 72-80: Clang format differences found. Code formatting does not match the expected style.
♻️ Duplicate comments (2)
src/evo/dmnstate.h (1)
78-110: Fix clang-format issues in serialization code.The pipeline indicates formatting issues with brace placement and argument formatting in the serialization macros. This is a duplicate of a previously flagged issue.
Run clang-format to fix the formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 78-110: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h (1)
224-230: 🛠️ Refactor suggestionAddress incomplete extended address implementation.
The assertion
assert(!m_is_extended)indicates that extended address functionality is not yet implemented, which could cause runtime failures when this feature is needed.Consider replacing the assertion with a proper error or exception:
-// TODO: Remove when extended addresses implementation is added in -assert(!m_is_extended); +if (m_is_extended) { + throw std::runtime_error("Extended addresses not yet implemented"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(17 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(8 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(4 hunks)src/evo/providertx.cpp(5 hunks)src/evo/providertx.h(11 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(4 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(6 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(9 hunks)src/test/evo_netinfo_tests.cpp(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(4 hunks)src/txmempool.cpp(8 hunks)src/util/pointer.h(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/qt/masternodelist.cpp
- src/evo/core_write.cpp
- src/rpc/quorums.cpp
- src/evo/simplifiedmns.cpp
🚧 Files skipped from review as they are similar to previous changes (20)
- src/Makefile.am
- src/masternode/utils.cpp
- src/masternode/node.cpp
- src/evo/netinfo.cpp
- src/txmempool.cpp
- src/test/evo_netinfo_tests.cpp
- src/util/pointer.h
- src/evo/dmnstate.cpp
- src/coinjoin/client.cpp
- src/evo/simplifiedmns.h
- src/test/evo_deterministicmns_tests.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/llmq/utils.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/test/evo_trivialvalidation.cpp
- src/evo/deterministicmns.cpp
- src/rpc/masternode.cpp
- src/rpc/evo.cpp
- src/net.cpp
- src/evo/providertx.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/evo/deterministicmns.h (3)
src/evo/netinfo.h (4)
s(180-183)s(180-180)NetInfoInterface(135-135)NetInfoInterface(135-135)src/evo/deterministicmns.cpp (3)
obj(48-48)AddMN(441-500)AddMN(441-441)src/evo/dmnstate.h (3)
CDeterministicMNStateDiff(215-215)CDeterministicMNStateDiff(216-237)CDeterministicMNStateDiff(239-239)
src/evo/netinfo.h (3)
src/evo/dmnstate.h (1)
nVersion(108-116)src/evo/deterministicmns.h (1)
service(311-311)src/evo/netinfo.cpp (4)
rhs(35-46)rhs(35-35)rhs(48-63)rhs(48-48)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 78-110: Clang format differences found. Code formatting does not match the expected style.
src/evo/providertx.h
[error] 27-150: Clang format differences found. Code formatting does not match the expected style.
src/evo/deterministicmns.h
[error] 63-70: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h
[error] 72-80: Clang format differences found. Code formatting does not match the expected style.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (8)
src/evo/dmnstate.h (2)
58-58: LGTM: Successful transition to polymorphic interface.The change from concrete
MnNetInfotostd::shared_ptr<NetInfoInterface>correctly implements the polymorphic design pattern, enabling version-dependent network info implementations.
219-230: LGTM: Correct handling of shared pointer comparison.The specialized comparison logic for
netInfousingutil::shared_ptr_not_equalis necessary because shared pointers require value-based comparison rather than pointer-based comparison for semantic equality.src/evo/providertx.h (1)
34-46: LGTM: Well-designed version determination logic.The
GetMaxfunction correctly implements hierarchical version requirements where extended addresses require both forks to be active. The logic properly handles the case where basic scheme might be disabled in RPC contexts.src/evo/deterministicmns.h (3)
68-83: LGTM: Excellent modernization of serialization patterns.The transition to
SERIALIZE_METHODSmacro and the template deserialization constructor follows modern C++ serialization best practices and improves code consistency.
195-203: LGTM: Comprehensive state reset in Clear() method.The
Clear()method properly resets all internal state members, which is essential for reliable deserialization and state management.
394-395: LGTM: Necessary static assertions for type safety.The static assertions correctly prevent template instantiation issues with the new
NetInfoInterfacetypes, ensuring type safety in the unique property system.src/evo/netinfo.h (2)
129-153: LGTM: Well-designed polymorphic interface.The
NetInfoInterfacedesign correctly uses pure virtual methods and implements equality semantics through a privateIsEqualmethod. The factory methodMakeNetInfoprovides clean version-dependent instantiation.
74-75: LGTM: Standard deserialization pattern.The template deserialization constructor for
NetInfoEntryfollows the established pattern used throughout the codebase.
| assert(_internalId != std::numeric_limits<uint64_t>::max()); | ||
| } | ||
|
|
||
| template <typename Stream> | ||
| CDeterministicMN(deserialize_type, Stream& s, const uint8_t format_version) | ||
| { | ||
| SerializationOp(s, CSerActionUnserialize(), format_version); | ||
| } | ||
| CDeterministicMN(deserialize_type, Stream& s) { s >> *this; } | ||
|
|
||
| template <typename Stream, typename Operation> | ||
| inline void SerializationOp(Stream& s, Operation ser_action, const uint8_t format_version) | ||
| SERIALIZE_METHODS(CDeterministicMN, obj) | ||
| { | ||
| // We no longer support EvoDB formats below MN_VERSION_FORMAT | ||
| if (format_version < MN_VERSION_FORMAT) { | ||
| throw std::ios_base::failure("EvoDb too old, run Dash Core with -reindex to rebuild"); | ||
| } | ||
| READWRITE(proTxHash); | ||
| READWRITE(VARINT(internalId)); | ||
| READWRITE(collateralOutpoint); | ||
| READWRITE(nOperatorReward); | ||
| READWRITE(pdmnState); | ||
| READWRITE(obj.proTxHash); |
There was a problem hiding this comment.
Fix clang-format issues in serialization macro.
The pipeline indicates formatting issues in the SERIALIZE_METHODS macro usage.
Run clang-format to fix the formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 63-70: Clang format differences found. Code formatting does not match the expected style.
🤖 Prompt for AI Agents
In src/evo/deterministicmns.h around lines 63 to 70, the usage of the
SERIALIZE_METHODS macro has clang-format style issues. Run clang-format on this
section to correct indentation, spacing, and line breaks according to the
project's style guidelines before merging.
| template <typename Stream> | ||
| NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary deserialization constructor template.
The template deserialization constructor NetInfoSerWrapper(deserialize_type, Stream& s) doesn't make sense in the context of a wrapper class that manages a reference to external data.
Remove this constructor as the wrapper should be constructed with the data reference and then used for serialization operations:
-template <typename Stream>
-NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename Stream> | |
| NetInfoSerWrapper(deserialize_type, Stream& s) { s >> *this; } |
🤖 Prompt for AI Agents
In src/evo/netinfo.h around lines 231 to 232, remove the template
deserialization constructor `NetInfoSerWrapper(deserialize_type, Stream& s)`
because it is unnecessary for this wrapper class that manages a reference to
external data. The wrapper should only be constructed with the data reference
and used for serialization, so delete this constructor entirely.
|
Notes:
|
kwvg
requested review from
knst,
PastaPastaPasta and
UdjinM6
and removed request for
UdjinM6
Motivation
To enable the switch to extended addresses, the current
CServiceentry was first encapsulated inMnNetInfoand exposed through a public interface (see dash#6627) that restricted access to the underlying type, then the underlying type of the address was abstracted away throughNetInfoEntry(see dash#6629), allowing us to store different types of addresses.This pull request finally abstracts away
MnNetInfoas an implementation detail and sets the stage for alternate implementations (i.e. extended addresses) to be selected in based on the version number. This should allow the switchover logic to be insulated from most of the codebase.Additional Information
Depends on feat: define BIP 9 fork
DEPLOYMENT_V23for new version featureset #6669Depends on feat: introduce type-flexible
NetInfoEntryto allow non-CServiceentries, use inMnNetInfo#6629Dependency for feat(rpc): allow reporting multiple addresses with new
addressesfield, deprecateservicefield, allow submitting multiple addresses #6674Since dash#6579 got rid of legacy
CDeterministicMNStateserialization, there is no more logic that can attempt legacy deserialization, making the version check (source) vestigial. We can safely remove the leftover version fields and simplify the serialization logic.It is done as part of this PR because the serialization version is manipulated with
OverrideStreamto ensure proper ADDRv2 serialization (source) and as the extended implementation uses it for that reason, the otherwise-vestigial version-based check could now trigger a crash, so it has been removed.As implementing operators in virtual classes (needed to allow switching implementations) is not possible (unlike regular pointers where alhs && rhs ? *lhs == *rhs : lhs == rhswould be enough), a separate equality functionNetInfoInterface::IsEqual()has been implemented that attempts to downcastlhsandrhsto the same type and returns the implementation-specific equality result.A solution that uses
typeidto help define comparison operators has been implemented butstd::shared_ptrdoes not deep compare, to aid with that, the deep comparison helpersutil::shared_ptr_{,not_}equalhave been implemented and can be used with any pair ofshared_ptrs so long as their underlying types define the necessary operators.Care needs to be taken to ensure that all equality comparisons involving
netInfouse those functions.A new ProTx version has been introduced for the upcoming extended addresses implementation.
It is only applicable toNo code has been implemented in this PR that uses this new version, that is reserved for the PR that introduces the extended implementation.CProRegTxandCProUpServTxas no other special transaction storesnetInfoand is therefore, unaffected by the switchover.Additionally, as the transaction version is used to determine the underlying implementation, make sure that
nVersionis set before callingMakeNetInfo()(also remember to callMakeNetInfo()as the default constructor will setnetInfotonullptrand a missing call will result in anullptrdereference whennetInfois eventually accessed). This is only relevant if a transaction is being constructed in-place as deserialization logic handles this for you.An earlier approach involved setting the new version only for
CProRegTxandCProUpServTx. In light of multiple ProTx types being able to modify the version of the masternode state (pdmnState), this may prove to be more dangerous than beneficial. In light of that, the version bump now affects all ProTx types but is still only effective inCProRegTxandCProUpServTx.Breaking Changes
None expected.
Checklist