feat: introduce type-flexible NetInfoEntry to allow non-CService entries, use in MnNetInfo
#6629
Conversation
kwvg
changed the title
NetInfoEntry to allow non-CService entries, use in MnNetInfoNetInfoEntry to allow non-CService entries, use in MnNetInfo
|
This pull request has conflicts, please rebase. |
kwvg
force-pushed
the
netinfo_p2
branch
3 times, most recently
from
c495aee to
bfc833c
Compare
…`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
|
This pull request has conflicts, please rebase. |
WalkthroughThe changes introduce a new abstraction, ✨ 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: 12
🔭 Outside diff range comments (3)
src/evo/simplifiedmns.h (1)
38-60: 🛠️ Refactor suggestionAvoid comparing shared-ptr identity inside
operator==
netInfois now astd::shared_ptr<NetInfoInterface>, yet the equality operator still performs
netInfo == rhs.netInfo, i.e. compares pointer identity instead of value.
Two logically identical objects that were deserialized independently will hold different
shared_ptrinstances and will therefore evaluate as unequal even when the contained
entries are byte-for-byte identical. This breaks deterministic-MN-list diffing and can
cause spurious re-broadcasts.- netInfo == rhs.netInfo && + (!!netInfo && !!rhs.netInfo + ? *netInfo == *rhs.netInfo // deep-compare + : netInfo == rhs.netInfo) && // handles null vs null(Requires
operator==forNetInfoInterface, orAsVector()comparison.)src/evo/providertx.h (2)
40-48:⚠️ Potential issue
netInfoleft default-initialised tonullptr– introduces UB risk
CProRegTxandCProUpServTxdefault-construct withnetInfo==nullptr, yet their
IsTriviallyValid,ToString, and other routines now dereference it unconditionally.
Either
- initialise the member immediately:
- std::shared_ptr<NetInfoInterface> netInfo; + std::shared_ptr<NetInfoInterface> netInfo{MakeNetInfo()};
- or add explicit null-checks wherever the pointer is used (preferred together with an early-fail
consensus rule:"bad-protx-netinfo-null").Failing to do so allows a crafted tx to crash a node before validation.
66-72: 🛠️ Refactor suggestion
const_castinNetInfoSerWrapperhides const-correctness issuesThe need for
const_castmeansREADWRITEcannot handle aconst std::shared_ptr<...>&.
Instead of mutating through aconstreference, declare the wrapper to acceptauto& ptr
and passobj.netInfodirectly – this removes the cast and the clang-format warning flagged
by CI.
♻️ Duplicate comments (8)
src/coinjoin/client.cpp (4)
1109-1113: Repeat of null-check issue (netInfoaccess)This dereference has the same unchecked assumption as above. Applying the helper-method proposal once will fix all call-sites.
1181-1186: Repeat of null-check issue (netInfoaccess)
1220-1223: Repeat of null-check issue (netInfoaccess)
1823-1829: Repeat of null-check issue (netInfoaccess)src/evo/providertx.h (1)
132-138: Duplicateconst_castproblem inCProUpServTxSame remark as above – fix once in the wrapper template and both call-sites become clean.
src/evo/providertx.cpp (1)
135-138: SameToString()crash-risk inCProUpServTxsrc/evo/deterministicmns.cpp (2)
528-546: Same issue as above inupdateNetInfohelperThe update path still assumes
CServiceentries only; duplicate logic
should follow the relaxed rule proposed earlier.
610-621: Same issue inRemoveMNDeletion should skip entries that carry no
CService, rather than
aborting with an exception.
🧹 Nitpick comments (14)
src/evo/simplifiedmns.h (1)
79-81:const_castonshared_ptrleaks const-correctness
NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.netInfo))
forces away constness to satisfy the wrapper, but this lets serialization routines
mutatenetInfothrough aconstreference. Consider adding an overload of
NetInfoSerWrapperthat accepts aconst std::shared_ptr<…>&and only mutates
throughmutablemembers, or move the cast inside the wrapper.This prevents subtle UB and makes the intent clearer.
src/test/evo_simplifiedmns_tests.cpp (1)
22-28: Use routable test IPs to avoid future validation failures
"0.0.0.%d:%d"produces addresses in the 0.0.0.0/8 range which is reserved and
often rejected by stricter validators. A later hardening ofNetInfoEntry
could make this test fail. Prefer a well-known documentation subnet such as
192.0.2.0/24(RFC 5737) or198.51.100.0/24.- strprintf("%d.%d.%d.%d:%d", 0, 0, 0, i, i) + strprintf("192.0.2.%d:%d", i, 10000 + i) // 10000+ i keeps ports >1024src/rpc/masternode.cpp (2)
571-575: Useconst auto&for range-based loop to avoid copies
Iterating over entries by value can be expensive ifNetInfoEntrygrows. Change to:- for (const NetInfoEntry& entry : dmn.pdmnState->netInfo->GetEntries()) { + for (const auto& entry : dmn.pdmnState->netInfo->GetEntries()) { strAddress += entry.ToStringAddrPort() + " "; }
571-575: Expose all network entries in JSON mode
Currently, in JSON output you only return the primary address. Since masternodes can have multiple network entries (IPv4, Tor, I2P), clients may need full visibility. Consider adding:UniValue addrs(UniValue::VARR); for (const auto& entry : dmn.pdmnState->netInfo->GetEntries()) { addrs.push_back(entry.ToStringAddrPort()); } objMN.pushKV("addresses", addrs);This preserves the existing
"address"field for compatibility while surfacing the full list.src/test/evo_deterministicmns_tests.cpp (3)
108-111: Order of initialisation is correct – but capture failure status
AddEntrycan return non-Success. In tests we currently ignore the status (except theCHECK_EQUALjust below here). ConsiderBOOST_REQUIRE_EQUALto abort early on failure and aid debugging.
130-134: Same remark as above – tighten assertionUse
BOOST_REQUIRE_EQUALforproTx.netInfo->AddEntry(...)so that the test fails fast when the address unexpectedly becomes invalid.
714-717: Potential copy-paste maintenance burdenThe triple-line pattern (version,
MakeNetInfo,AddEntry) is duplicated many times across the test file. A tiny helper such asInitNetInfo(payload, "1.1.1.1:port")would make future signature changes trivial.src/test/evo_netinfo_tests.cpp (4)
18-37: Test vector covers edge-cases well – one missing caseConsider adding
"1.1.1.1:0"which should fail (BadPort) because port 0 is invalid and often pops up in fuzzing.
63-69: Single-entry limit test OK – could also assert status of second addYou already check size==1; an additional
BOOST_CHECK_EQUAL(..., NetInfoStatus::MaxLimit)would make intent explicit.
72-78:CheckIfSerSame– ensure endian safetyComparing SHA-256 hashes is portable, but serialising both objects with the same
CHashWriterversion 0 does not include network (little/big) meta-data. That’s fine for regression tests now; just document that this is wire serialisation, not disk serialisation, to avoid future confusion.
80-109: Robust compatibility test – consider negative round-tripAfter the positive
CheckIfSerSamecases, also test that different services yield different hashes to prevent false positives (e.g., compare"1.1.1.1:9999"vs"1.1.1.2:9999").src/evo/netinfo.cpp (2)
198-205: Dereference optional once, not twice
GetPrimary()callsservice->get()twice – first when retrieving the
optional, then again when returning the staticempty_service. While
this works, the intent is clearer (and avoids accidental copies) by
binding the reference once:- if (const auto& service{m_addr.GetAddrPort()}; service.has_value()) { - return *service; + if (const auto service{m_addr.GetAddrPort()}) { + return service->get(); }A small readability tweak, but helps avoid dangling-reference mistakes.
206-213: EmptyMnNetInforeported asMalformed
Validate()returnsMalformedwhenm_addr.IsTriviallyValid()is
false, which happens for an intentionally emptyMnNetInfo. Up-stream
call-sites (CheckProRegTx, etc.) already guard against empty objects,
but other callers (future code, RPC helpers, tests) will mis-interpret an
empty list as malformed.Consider treating emptiness as
Successto make the API symmetric with
IsEmpty():- if (!m_addr.IsTriviallyValid()) { - return NetInfoStatus::Malformed; + if (IsEmpty()) { + return NetInfoStatus::Success; + } + if (!m_addr.IsTriviallyValid()) { + return NetInfoStatus::Malformed; }src/evo/netinfo.h (1)
161-170:IsEmpty()comparison allocates temporary
bool IsEmpty() const override { return *this == MnNetInfo(); }This constructs a temporary every call. A lighter/faster alternative:
- bool IsEmpty() const override { return *this == MnNetInfo(); } + bool IsEmpty() const override { return m_addr == NetInfoEntry(); }Minor, but avoids needless instantiation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(4 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(4 hunks)src/evo/providertx.h(4 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(2 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(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)src/txmempool.h(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/test/block_reward_reallocation_tests.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(225-229)MakeNetInfo(225-225)
src/txmempool.cpp (1)
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/net.cpp (1)
test/functional/test_framework/messages.py (1)
CAddress(252-375)
src/rpc/evo.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(225-229)MakeNetInfo(225-225)
src/test/evo_simplifiedmns_tests.cpp (1)
src/evo/netinfo.h (2)
MakeNetInfo(225-229)MakeNetInfo(225-225)
src/evo/dmnstate.h (1)
src/evo/netinfo.h (7)
NetInfoSerWrapper(237-237)NetInfoSerWrapper(238-238)NetInfoSerWrapper(239-242)NetInfoSerWrapper(243-243)NetInfoSerWrapper(245-245)MakeNetInfo(225-229)MakeNetInfo(225-225)
src/evo/deterministicmns.h (1)
src/evo/netinfo.h (6)
NetInfoEntry(66-66)NetInfoEntry(67-72)NetInfoEntry(73-73)NetInfoEntry(75-75)NetInfoInterface(159-159)NetInfoInterface(159-159)
src/test/evo_netinfo_tests.cpp (2)
src/evo/netinfo.cpp (4)
rhs(40-51)rhs(40-40)rhs(53-73)rhs(53-53)src/evo/netinfo.h (11)
rhs(77-77)rhs(78-78)rhs(79-79)rhs(79-79)rhs(186-186)rhs(186-186)rhs(187-187)rhs(187-187)service(161-161)service(178-178)service(212-212)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/simplifiedmns.h
[error] 73-87: Clang format differences found. Code formatting does not match the expected style.
src/evo/dmnstate.h
[error] 98-111: Clang format differences found. Code formatting does not match the expected style.
src/evo/providertx.h
[error] 63-90: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h
[error] 70-110: Clang format differences found. Code formatting does not match the expected style.
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (41)
src/llmq/utils.cpp (2)
827-827: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.
876-876: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.src/masternode/utils.cpp (1)
81-81: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.src/qt/masternodelist.cpp (2)
224-224: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.
227-227: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.src/rpc/quorums.cpp (1)
206-206: Syntax update for pointer-based netInfo access.The code now correctly uses the arrow operator (
->) to access methods on thenetInfopointer rather than the dot operator (.). This change is consistent with the broader refactoring wherenetInfowas changed from a directMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.src/masternode/node.cpp (2)
149-149: Access updated to use pointer dereferenceThe change from direct member access to pointer dereference correctly aligns with the updated type of
netInfofrom a concreteMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.
204-204: Access updated to use pointer dereferenceThe change from direct member access to pointer dereference correctly aligns with the updated type of
netInfofrom a concreteMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.src/evo/simplifiedmns.cpp (2)
71-71: Access updated to use pointer dereferenceThe change from direct member access (
netInfo.ToString()) to pointer dereference (netInfo->ToString()) correctly reflects the type change from a concrete object to a shared pointer.
81-81: Access updated to use pointer dereferenceThe change from direct member access to pointer dereference correctly reflects the type change from a concrete object to a shared pointer, maintaining the same logical functionality while adapting to the new interface.
src/evo/dmnstate.cpp (2)
35-35: Access updated to use pointer dereferenceThe change from direct member access to pointer dereference correctly aligns with the updated type of
netInfofrom a concrete object to a shared pointer of an interface type.
76-76: Access updated to use pointer dereferenceThe change in the
CDeterministicMNStateDiff::ToJsonmethod correctly updates the access pattern to use pointer dereferencing for thenetInfomember, consistent with its type change to a shared pointer.src/evo/core_write.cpp (2)
70-70: Access updated to use pointer dereferenceThe change in
CProRegTx::ToJson()method correctly updates the access pattern for thenetInfomember to use pointer dereferencing, consistent with the refactoring of this member to a shared pointer type.
117-117: Access updated to use pointer dereferenceThe change in
CProUpServTx::ToJson()method correctly updates the access pattern for thenetInfomember to use pointer dereferencing, consistent with the refactoring of this member to a shared pointer type.src/test/block_reward_reallocation_tests.cpp (2)
119-119: InitializednetInfowith the new shared_ptr-based approach.The code correctly initializes the
netInfomember ofproTxusing theMakeNetInfofunction, which creates astd::shared_ptr<NetInfoInterface>. This aligns with the implementation of type-flexible network info entries.
121-121: Updated access syntax to use pointer dereference.The code has been properly updated to use pointer dereferencing (
->) instead of direct member access (.) to call theAddEntrymethod on thenetInfoshared pointer. This change matches the transition to pointer-based access in the netInfo refactoring.src/txmempool.h (2)
20-20: Added necessary header forNetInfoEntry.The include for
evo/netinfo.his correctly added to provide access to theNetInfoEntryclass and related types used in the mempool implementation.
529-529: Updated map key type fromCServicetoNetInfoEntry.The code now uses
NetInfoEntryinstead ofCServiceas the key type inmapProTxAddresses. This is a necessary change to support the new type-flexible network information entries.src/txmempool.cpp (8)
694-697: Updated iteration over network entries inaddUncheckedProTx.The code has been modified to use the new pointer-based
netInfo->GetEntries()access pattern and correctly iterates overNetInfoEntryobjects instead ofCServiceobjects.
707-709: Updated iteration over network entries inaddUncheckedProTxfor update service.The code correctly uses the pointer-based access for NetInfo entries in the provider update service transaction processing path.
798-800: Updated iteration over network entries inremoveUncheckedProTx.The removal of ProTx entries now correctly uses the pointer-based access pattern for the network info entries.
808-810: Updated iteration over network entries inremoveUncheckedProTxfor update service.The code properly handles the removal of service update transactions with the new pointer-based access pattern for network entries.
1037-1044: Updated iteration and conflict checking for network entries inremoveProTxConflicts.The code now iterates over
NetInfoEntryobjects using pointer-based access and handles conflict detection correctly with the new type.
1059-1066: Updated iteration and conflict checking for update service transactions.The code correctly handles conflict detection for service update transactions using the new
NetInfoEntrytype and pointer-based access pattern.
1397-1401: Updated network entry iteration inexistsProviderTxConflict.The code now properly iterates over
NetInfoEntryobjects when checking for provider transaction conflicts.
1422-1427: Updated network entry iteration inexistsProviderTxConflictfor update service.The conflict detection code for service update transactions has been correctly updated to use the new network info entry type.
src/evo/deterministicmns.h (1)
401-403: Added type exclusions forGetUniquePropertyHashtemplate method.The code adds necessary exclusions to prevent
GetUniquePropertyHashfrom being instantiated with the new network info types (NetInfoEntry,NetInfoInterface, andstd::shared_ptr<NetInfoInterface>). This prevents potential misuse of these types with the generic hash calculation method, which may not be appropriate for the network info classes.src/rpc/evo.cpp (1)
979-983: Possible state loss when rebuildingnetInfofor ProUpServTx
ptx.netInfo = MakeNetInfo(ptx);discards the current net-info entries stored
indmn->pdmnState->netInfo(e.g. Tor/I2P addr or multiple addresses).
If the update only intends to change the service address, the other previously
valid entries are silently dropped.Consider cloning the existing object instead:
ptx.netInfo = std::make_shared<MnNetInfo>(*dmn->pdmnState->netInfo);or provide a
Clone()inNetInfoInterface.src/rpc/masternode.cpp (1)
618-619: Guard against empty entries and clarify key naming
GetPrimary()assumes at least one entry exists; ifnetInfo->GetEntries()is empty this will crash. Consider returningstd::optional<NetInfoEntry>or asserting non-emptiness before calling. Also, renaming the JSON key to"primary_address"may improve clarity now that multiple addresses exist.src/net.cpp (7)
3677-3677: Access tonetInfoupdated to use pointer dereferencingThe code correctly updates the access pattern from direct member access (
netInfo.GetPrimary()) to pointer dereferencing (netInfo->GetPrimary()), which aligns with the refactoring of masternode network information wherenetInfochanged from a concreteMnNetInfoobject to astd::shared_ptr<NetInfoInterface>.
3743-3744: Updated conditional check using pointer dereferencingThe conditional check has been properly updated to use pointer dereferencing syntax for the
netInfomember, consistent with the architectural change tostd::shared_ptr<NetInfoInterface>.
3752-3753: Log message updated to use pointer-based accessorThe log formatting code correctly uses the new pointer dereferencing syntax to access the primary address from
netInfo.
3763-3763: Updated probing log messageThe log message for masternode probing has been properly updated to use pointer dereferencing syntax for accessing the primary address.
3779-3779: OpenMasternodeConnection call updatedThe call to
OpenMasternodeConnectionnow correctly uses the pointer dereferencing syntax to access the primary address from thenetInfoshared pointer.
3781-3781: ForNode call updated to use pointer dereferencingThe call to
ForNodehas been properly updated to use the pointer dereferencing syntax when accessing the primary network address.
3788-3788: Connection failure log message updatedThe log message for connection failures has been correctly updated to use pointer dereferencing for accessing the primary address from the
netInfoshared pointer.src/evo/dmnstate.h (2)
115-116: Good move – re-initialises after operator resetRe-creating
netInfoviaMakeNetInfoinstead of callingClear()on a concrete type is the correct abstraction-preserving approach.
249-252: Missing dependency flag fornetInfodiff field
pubKeyOperatorrequiresnVersion;netInfomay equally depend onnVersion(e.g. if validation rules vary between versions). If that is the case, emitField_nVersionwhenField_netInfois present, mirroring the logic above forpubKeyOperator. Otherwise upgrading rules in the future will silently mis-interpret old diffs.src/test/evo_deterministicmns_tests.cpp (1)
637-641: Good adaptation to new APIThe test correctly creates a fresh
MnNetInfofor the payload and validates the return code.src/test/evo_netinfo_tests.cpp (2)
14-15: Switch toBasicTestingSetupimproves realismGreat choice – this now exercises mainnet-like port rules and caught the earlier
BadPortbug.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
55-60: Good assertion change on malformed vs. successThe distinction between
Malformed/Successaligns with new status codes.
| if (!mixingMasternode) return; | ||
| if (mixingMasternode->pdmnState->netInfo.GetPrimary() != peer.addr) return; | ||
| if (mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) return; | ||
|
|
There was a problem hiding this comment.
Guard against nullptr before dereferencing netInfo
mixingMasternode->pdmnState->netInfo is assumed to be non-null, but there is no invariant guaranteeing this.
If a masternode state gets deserialised incorrectly or a future refactor forgets to initialise netInfo, this line will seg-fault long before any validation code notices.
- if (mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) return;
+ if (!mixingMasternode->pdmnState || // paranoia
+ !mixingMasternode->pdmnState->netInfo || // make sure pointer exists
+ mixingMasternode->pdmnState->netInfo->IsEmpty() ||
+ mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) {
+ return;
+ }The same pattern appears in multiple places below (see lines ~1109, ~1181, ~1220, ~1823).
Consider introducing a small helper, GetMnPrimaryService(const CDeterministicMNState* state), that returns an
std::optional<CService> and centralises the null/empty checks.
📝 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.
| if (!mixingMasternode) return; | |
| if (mixingMasternode->pdmnState->netInfo.GetPrimary() != peer.addr) return; | |
| if (mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) return; | |
| if (!mixingMasternode) return; | |
| - if (mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) return; | |
| + if (!mixingMasternode->pdmnState || // paranoia | |
| + !mixingMasternode->pdmnState->netInfo || // make sure pointer exists | |
| + mixingMasternode->pdmnState->netInfo->IsEmpty() || | |
| + mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) { | |
| + return; | |
| + } |
| EncodeDestination(PKHash(keyIDVoting)), payee, platformNodeID.ToString(), platformP2PPort, | ||
| platformHTTPPort, netInfo.ToString()); | ||
| platformHTTPPort, netInfo->ToString()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ToString() unsafe when netInfo is null
netInfo->ToString() will crash if the pointer is missing. Guard or fall back to "{}".
src/evo/providertx.cpp
Outdated
| for (const NetInfoEntry& entry : netInfo->GetEntries()) { | ||
| if (!entry.IsTriviallyValid()) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing null / empty test before iterating netInfo
netInfo may legally be nullptr (see header) or empty, yet the loop unconditionally
dereferences it. Add an early check akin to the CProUpServTx case:
- for (const NetInfoEntry& entry : netInfo->GetEntries()) {
+ if (!netInfo || netInfo->IsEmpty()) {
+ return state.Invalid(TxValidationResult::TX_BAD_SPECIAL,
+ "bad-protx-netinfo-empty");
+ }
+ for (const NetInfoEntry& entry : netInfo->GetEntries()) {Also consider harmonising error codes – CProUpServTx already uses
bad-protx-netinfo-empty.
📝 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.
| for (const NetInfoEntry& entry : netInfo->GetEntries()) { | |
| if (!entry.IsTriviallyValid()) { | |
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad"); | |
| } | |
| } | |
| if (!netInfo || netInfo->IsEmpty()) { | |
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, | |
| "bad-protx-netinfo-empty"); | |
| } | |
| for (const NetInfoEntry& entry : netInfo->GetEntries()) { | |
| if (!entry.IsTriviallyValid()) { | |
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad"); | |
| } | |
| } |
| BOOST_REQUIRE(dmn != nullptr && dmn->pdmnState->netInfo->GetPrimary().GetPort() == 100); | ||
| BOOST_REQUIRE(dmn != nullptr && !dmn->pdmnState->IsBanned()); |
There was a problem hiding this comment.
Repeat of previous null-ptr issue
Same protective check should be added here.
BOOST_REQUIRE(dmn != nullptr);
+BOOST_REQUIRE(dmn->pdmnState->netInfo);
BOOST_REQUIRE(dmn->pdmnState->netInfo->GetPrimary().GetPort() == 100);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/evo/dmnstate.h (1)
57-57: 🛠️ Refactor suggestionConsider initializing
netInfoto prevent null dereferenceThe
netInfoshared_ptr is initialized tonullptr, which could lead to dereference crashes if not properly checked. Since it's reset inResetOperatorFields()but might be accessed before that call, consider initializing it withMakeNetInfo(*this)here instead.- std::shared_ptr<NetInfoInterface> netInfo{nullptr}; + std::shared_ptr<NetInfoInterface> netInfo{MakeNetInfo(*this)};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(4 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(4 hunks)src/evo/providertx.h(4 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(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)src/txmempool.h(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/masternode/utils.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- src/llmq/utils.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/qt/masternodelist.cpp
- src/evo/core_write.cpp
- src/masternode/node.cpp
- src/evo/dmnstate.cpp
- src/txmempool.cpp
- src/evo/simplifiedmns.cpp
- src/evo/simplifiedmns.h
- src/evo/deterministicmns.h
- src/rpc/quorums.cpp
- src/txmempool.h
- src/rpc/masternode.cpp
- src/rpc/evo.cpp
- src/evo/providertx.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/providertx.h
- src/coinjoin/client.cpp
- src/net.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧠 Learnings (3)
src/test/evo_netinfo_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.173Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
src/evo/netinfo.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.567Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.cpp:186-196
Timestamp: 2025-05-10T00:54:10.965Z
Learning: `std::reference_wrapper<T>` can be implicitly constructed from lvalue references (T&) through its forwarding reference constructor template. Explicit use of std::ref/std::cref is not required when pushing references into containers like vectors of reference_wrappers.
src/evo/netinfo.h (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.868Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
🧬 Code Graph Analysis (1)
src/evo/dmnstate.h (1)
src/evo/netinfo.h (7)
NetInfoSerWrapper(241-241)NetInfoSerWrapper(242-242)NetInfoSerWrapper(243-246)NetInfoSerWrapper(247-247)NetInfoSerWrapper(249-249)MakeNetInfo(229-233)MakeNetInfo(229-229)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 98-111: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
[error] 183-210: Clang format differences found. Code formatting does not match the expected style.
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (11)
src/evo/dmnstate.h (1)
98-104:❓ Verification inconclusive
Clang format error detected
The clang-format check is failing for this section of code. Please format this block according to the project's style guidelines.
🏁 Script executed:
#!/bin/bash # Check which exact formatting issues are present git diff --check || trueLength of output: 18
Please verify clang-format compliance for the affected block
I ran
git diff --check(whitespace check) and didn’t find any issues, but that doesn’t guarantee full.clang-formatcompliance. Please double-check that this section insrc/evo/dmnstate.h(lines 98–104) is formatted according to the project’s style (for example, by running your local clang-format check or reviewing the CI logs).• File: src/evo/dmnstate.h
• Lines: 98–104🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 98-111: Clang format differences found. Code formatting does not match the expected style.
src/test/evo_netinfo_tests.cpp (4)
14-15: Updated test fixture to BasicTestingSetup aligns with PR objectivesChanging from
RegTestingSetuptoBasicTestingSetupproperly implements the PR objective to simulate mainnet conditions, providing stricter validation coverage as mentioned in the PR description.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
39-45: The validation triangle approach is appropriateUsing
IsTriviallyValid()inValidateGetEntries()maintains consistency with your approach of ensuring all three validation methods (AddEntry(),Validate(), andIsTriviallyValid()) work harmoniously together, as clarified in the previous review.
63-69: Good test for MaxLimit validationThis new test case correctly verifies that
MnNetInfoenforces the single-entry limit by rejecting additional entries with aMaxLimitstatus, consistent with the updatedAddEntrylogic.
72-78: Well-designed serialization compatibility checkThe
CheckIfSerSamehelper function is a clean approach to compare serialized representations, supporting the new compatibility checks betweenCServiceandMnNetInfo.src/evo/netinfo.cpp (2)
29-37: Effective input validation approachThe
MatchCharsFilterfunction provides an efficient way to quickly reject invalid inputs before more expensive operations like DNS lookups, aligning with the PR objective of introducing a fast-failing mechanism.
209-235: Comprehensive validation approachThe approach of checking
IsTriviallyValid()first before proceeding to detailed validation inValidate()creates a clear separation between basic structural validity and domain-specific validation rules, making the code more maintainable.src/evo/netinfo.h (4)
154-155: Efficient reference-based collectionUsing
std::reference_wrapperinNetInfoListdefinition is an excellent choice, preventing unnecessary copying while maintaining reference semantics and standard container compatibility.
227-233: Extensible factory patternThe
MakeNetInfofunction provides a clean extension point for potentially versioned implementations in the future while maintaining a consistent interface for users, good forward-thinking design.
71-90:❓ Verification inconclusive
Clang format error detected
The clang-format check is failing for this section of code. Please format this block according to the project's style guidelines.
🏁 Script executed:
#!/bin/bash # Check which exact formatting issues are present git diff HEAD^ HEAD | grep -A 10 -B 10 "src/evo/netinfo.h" | grep -A 50 -B 5 "^+.*{" || trueLength of output: 93
Clang-format check inconclusive
I wasn’t able to reproduce any formatting errors in the diff for
src/evo/netinfo.h. Please manually run clang-format against this file and confirm it adheres to the project’s style:
- Run
to see if any changes are needed.clang-format -style=file -i src/evo/netinfo.h git diff src/evo/netinfo.h🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
183-210:❓ Verification inconclusive
Clang format error detected
The clang-format check is failing for this section of code. Please format this block according to the project's style guidelines.
🏁 Script executed:
#!/bin/bash # Check which exact formatting issues are present git diff HEAD^ HEAD | grep -A 10 -B 10 "src/evo/netinfo.h" | grep -A 50 -B 5 "180-210" || trueLength of output: 93
🏁 Script executed:
#!/bin/bash set -e # Locate the header file FILE=$(fd --exclude node_modules --type f netinfo.h | head -n 1) if [[ -z "$FILE" ]]; then echo "netinfo.h not found" exit 1 fi echo "Inspecting $FILE (lines 180-210):" sed -n '180,210p' "$FILE" echo -e "\nRunning clang-format dry-run (style=file) on $FILE:" # Run clang-format in dry-run mode to detect style issues clang-format -style=file --dry-run --Werror "$FILE" 2>&1 || trueLength of output: 1401
Format
src/evo/netinfo.h(lines 180–210)
I couldn’t runclang-formatin this environment to pinpoint the exact style violations. Please run the formatter locally (e.g.clang-format -i --style=file src/evo/netinfo.h) or check your CI logs and commit the resulting changes to ensure this block conforms to the project’s style guidelines.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 183-210: Clang format differences found. Code formatting does not match the expected style.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/evo_netinfo_tests.cpp (1)
80-110: Test lacks verification of IPv6 rejection behavior described in PRThe PR objectives mention a specific behavior: "MnNetInfo::AddEntry() rejecting IPv6 addresses with a BadInput error due to delimiter filtering, while validating an already stored IPv6 address returns a BadType error." However, this test only verifies the BadInput error from AddEntry() but doesn't test the different error code (BadType) when validating an already stored IPv6 address.
Consider adding a test case that:
- Creates a NetInfoEntry directly with an IPv6 CService
- Verifies that Validate() returns BadType instead of BadInput
// Test IPv6 rejection behavior difference { CService ipv6 = LookupNumeric("[2606:4700:4700::1111]", 9999); MnNetInfo netInfo; // Direct construction bypasses delimiter filtering netInfo.m_addr = NetInfoEntry{ipv6}; // Should return BadType, not BadInput BOOST_CHECK_EQUAL(netInfo.Validate(), NetInfoStatus::BadType); }src/evo/netinfo.cpp (1)
29-37: Consider more descriptive naming for the filtering functionThe function checks if all characters in the input are contained in the filter set (i.e., it only allows characters from the filter).
A more descriptive name would be
ContainsOnlyAllowedCharsorHasOnlyAllowedCharsto better convey the purpose.-bool MatchCharsFilter(const std::string& input, const std::string_view& filter) +bool ContainsOnlyAllowedChars(const std::string& input, const std::string_view& filter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(4 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(4 hunks)src/evo/providertx.h(4 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(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/llmq/utils.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- src/masternode/utils.cpp
- src/masternode/node.cpp
- src/evo/dmnstate.cpp
- src/evo/deterministicmns.h
- src/test/block_reward_reallocation_tests.cpp
- src/evo/core_write.cpp
- src/rpc/masternode.cpp
- src/rpc/quorums.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/simplifiedmns.h
- src/txmempool.cpp
- src/qt/masternodelist.cpp
- src/evo/providertx.cpp
- src/evo/simplifiedmns.cpp
- src/evo/dmnstate.h
- src/rpc/evo.cpp
- src/coinjoin/client.cpp
- src/evo/providertx.h
- src/test/evo_deterministicmns_tests.cpp
- src/net.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧠 Learnings (3)
src/test/evo_netinfo_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.173Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
src/evo/netinfo.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.567Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.cpp:186-196
Timestamp: 2025-05-10T00:54:10.965Z
Learning: `std::reference_wrapper<T>` can be implicitly constructed from lvalue references (T&) through its forwarding reference constructor template. Explicit use of std::ref/std::cref is not required when pushing references into containers like vectors of reference_wrappers.
src/evo/netinfo.h (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.868Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
🧬 Code Graph Analysis (1)
src/evo/netinfo.h (1)
src/evo/netinfo.cpp (4)
rhs(40-51)rhs(40-40)rhs(53-73)rhs(53-53)
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
[error] 183-210: Clang format differences found. Code formatting does not match the expected style.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (25)
src/test/evo_netinfo_tests.cpp (6)
14-14: Test fixture changed to BasicTestingSetup for improved test coverageThe switch from RegTestingSetup to BasicTestingSetup aligns with the PR objectives to provide stricter validation coverage by simulating mainnet conditions rather than regtest.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
16-37: LGTM: Test vectors updated with more specific error codesThe test vectors now properly reflect the expanded NetInfoStatus enum with more specific error codes such as BadAddress, BadType, NotRoutable, etc. This provides better test coverage for the detailed validation behavior.
39-45: Appropriate use of IsTriviallyValid() in test validationThis function now correctly takes NetInfoList instead of CServiceList and validates each entry using IsTriviallyValid(). This approach properly aligns with the PR's validation strategy where all three validation methods (AddEntry(), Validate(), and IsTriviallyValid()) work together consistently.
54-56: Status code updated for clarityThe change from a generic error to NetInfoStatus::Malformed for empty MnNetInfo objects aligns with the improved error reporting in the PR.
63-69: Good test for single-entry limit enforcementThis test case properly verifies that MnNetInfo enforces its single-entry constraint by rejecting additional entries with a MaxLimit status.
72-78: Verify serialization consistency between CService and MnNetInfoThe helper function correctly compares serialized representations to ensure backward compatibility.
src/evo/netinfo.cpp (10)
16-19: LGTM: Added empty service constant and character filterThese additions support the new validation approach with a static empty service for fallback and a constant for fast validation of IPv4 inputs.
40-73: The comparison operators are well-implemented for variant typesThese operators correctly handle comparison of the variant types in NetInfoEntry, providing proper equality and ordering semantics necessary for container usage.
75-82: Safe accessor with appropriate debug assertionThe GetAddrPort method safely accesses the underlying CService with proper type checking and includes an assertion for debug builds to catch programmer errors early.
84-107: Clear implementation of IsTriviallyValid with good documentationThe implementation properly checks that the entry is correctly constructed with valid data, and the comment clearly explains its role in the validation hierarchy.
109-135: LGTM: String conversion methods handle all variant casesThese methods properly convert each variant type to an appropriate string representation, with fallback for invalid entries.
137-158: LGTM: Strong implementation of polymorphic equality comparisonThe IsEqual method handles all the edge cases for comparing shared pointers to interface objects: null checks, identity checks, and proper downcasting for deep comparison.
160-182: Improved service validation with specific error codesThe ValidateService method now returns more specific error codes like BadAddress, BadType, and NotRoutable instead of generic errors, aligning with the PR objectives.
184-207: Fast-failing input validation improves performanceThe AddEntry method now implements a fast character-based pre-filter before the more expensive lookup and validation, which aligns with the PR's optimization goals.
209-219: GetEntries now returns NetInfoList with proper reference handlingThe implementation correctly uses the implicit conversion from T& to std::reference_wrapper when pushing into the vector, as noted in the retrieved learning.
221-235: LGTM: GetPrimary and Validate methods handle empty/invalid casesThese methods correctly handle the case when there's no valid entry, returning an empty service or appropriate error code.
src/evo/netinfo.h (9)
17-29: LGTM: Expanded NetInfoStatus enum with more specific codesThe enum now provides more granular error reporting with categories for entry management and validation issues.
32-52: Error message strings updated for new status codesThe NISToString function properly handles all the new enum values with descriptive error messages.
54-65: Implementation limits initial type support despite PR titleWhile the PR title mentions allowing "non-CService entries", the NetInfoType enum only defines Service (0x01) and Invalid (0xff), without additional type codes for other entry types.
Does this implementation set the foundation for other entry types that will be added later, or is the PR title referring to a different aspect of flexibility? The current implementation only supports CService as a valid type.
66-153: LGTM: Well-designed NetInfoEntry class with proper variant handlingThe NetInfoEntry class correctly handles serialization, deserialization, construction, and validation with appropriate error handling and ADDRV2_FORMAT support as mentioned in the PR objectives.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
157-174: Clean interface design with polymorphic methodsThe NetInfoInterface provides a well-defined abstract interface for different network info implementations, supporting the extensibility goal mentioned in the PR objectives.
176-182: MnNetInfo updated to use NetInfoEntry and implement interfaceThe class now properly implements the NetInfoInterface and uses NetInfoEntry for storage.
193-214: LGTM: Serialization maintains backward compatibilityThe Serialize/Unserialize methods properly handle the transition between NetInfoEntry and CService, maintaining compatibility as mentioned in the PR objectives.
227-233: Factory method prepared for future versioningThe MakeNetInfo template function sets up infrastructure for version-based instantiation of appropriate implementations, though currently it always returns MnNetInfo.
235-273:❓ Verification inconclusive
Strong serialization wrapper with proper error handling
The NetInfoSerWrapper correctly handles serialization of shared pointers to interface objects, with proper type checking and error handling.
🏁 Script executed:
#!/bin/bash # Check for clang-format issues git diff --name-only | grep -E '\.cpp$|\.h$' | xargs clang-format -style=file --Werror --dry-runLength of output: 145
Approve serialization wrapper – formatting verification needed
The
NetInfoSerWrapperimplementation correctly handles serialization ofstd::shared_ptr<NetInfoInterface>with proper type checking and error handling. However, the automated clang-format check could not run in this environment (clang-format not found). Please run the following locally to ensure style compliance:git diff --name-only | grep -E '\.cpp$|\.h$' | xargs clang-format -style=file --Werror --dry-run
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/evo_netinfo_tests.cpp (1)
39-45: NetInfoList API properly validates entriesThe
ValidateGetEntriesfunction has been updated to work with the newNetInfoListtype (vector of references toNetInfoEntry) and validates that each entry passesIsTriviallyValid(). This approach correctly ensures consistency between the validation methods as highlighted in the previous feedback.src/evo/netinfo.h (1)
235-273: Serialization wrapper ensures type safetyThe
NetInfoSerWrapperclass correctly:
- Wraps shared pointers to
NetInfoInterface- Enforces type safety through dynamic casting
- Throws descriptive exceptions on malformed data
- Uses the specialized shared_ptr serialization in the codebase
This ensures proper serialization of the polymorphic types.
🧹 Nitpick comments (1)
src/evo/netinfo.h (1)
71-90: Fix clang-format issues in highlighted regionsThe pipeline has identified formatting issues in these line ranges. Please run clang-format on these sections to fix the formatting.
Also applies to: 183-210
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(4 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(4 hunks)src/evo/providertx.h(4 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(2 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- src/masternode/utils.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/llmq/utils.cpp
- src/evo/dmnstate.cpp
- src/qt/masternodelist.cpp
- src/txmempool.cpp
- src/rpc/quorums.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/deterministicmns.h
- src/evo/simplifiedmns.cpp
- src/evo/providertx.cpp
- src/rpc/evo.cpp
- src/evo/simplifiedmns.h
- src/evo/dmnstate.h
- src/rpc/masternode.cpp
- src/evo/core_write.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/masternode/node.cpp
- src/evo/providertx.h
- src/coinjoin/client.cpp
- src/net.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧠 Learnings (3)
src/test/evo_netinfo_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.173Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
src/evo/netinfo.h (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.868Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
src/evo/netinfo.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.567Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.cpp:186-196
Timestamp: 2025-05-10T00:54:10.965Z
Learning: `std::reference_wrapper<T>` can be implicitly constructed from lvalue references (T&) through its forwarding reference constructor template. Explicit use of std::ref/std::cref is not required when pushing references into containers like vectors of reference_wrappers.
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
[error] 183-210: Clang format differences found. Code formatting does not match the expected style.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (15)
src/test/evo_netinfo_tests.cpp (4)
14-15: Improved test setup to match production environmentChanging from
RegTestingSetuptoBasicTestingSetupensures tests run in conditions that more closely mirror mainnet, providing stricter validation coverage. This change aligns with the PR objective of preventing incorrect validation rules from being applied.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 14-14: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
16-37: Test vectors updated to reflect accurate validation rulesThe test vectors have been properly updated to test:
- Non-mainnet port validation on mainnet (line 22)
- Internal address rejection on mainnet (line 24)
- More specific error codes, like changing from
BadInputtoBadAddressfor invalid IPv4 (line 26)These changes properly validate the port enforcement rules mentioned in the PR objectives, ensuring that valid addr:port combinations on mainnet no longer fail incorrectly.
63-69: Verify single-entry constraint for MnNetInfoGood addition of a test case that verifies the single-entry constraint for
MnNetInfo, confirming that attempts to add a second entry are rejected withNetInfoStatus::MaxLimit.
72-110: Comprehensive serialization compatibility testsThe new
CheckIfSerSamehelper andcservice_compatibletest case properly verify serialization compatibility betweenCServiceandMnNetInfoacross multiple scenarios:
- Empty values
- Valid IPv4 addresses
- Invalid inputs (domains, IPv6)
These tests ensure backward compatibility with the serialization format, which is critical for a blockchain application.
src/evo/netinfo.cpp (8)
16-18: Static empty service and character filter for fast validationGood introduction of:
- A static empty service for consistent empty object returns
- A character filter for IPv4 to enable fast failing for invalid addresses
This aligns with the PR's goal of introducing a fast-failing mechanism based on permitted characters.
29-37: Helper for character validation enhances code maintainabilityThe
MatchCharsFilterfunction provides a clean way to validate input strings against character sets, which is used for pre-filtering address inputs before more expensive validation. This implementation supports the fast-failing mechanism mentioned in the PR objectives.
75-82: Robust GetAddrPort implementation returns std::optionalThe
GetAddrPortmethod properly usesstd::optionalto safely retrieve the underlyingCServicefrom the variant, with appropriate validation checks. This provides clean error handling for cases where the service is invalid.
87-107: IsTriviallyValid implementation provides fast validationThe
IsTriviallyValidmethod performs quick validation without expensive checks, properly examining both type and basic validity. The comment at lines 84-86 correctly documents the purpose of this method in relation to the validation responsibility hierarchy.
137-158: IsEqual implementation handles pointer comparison edge casesThe
NetInfoInterface::IsEqualstatic method correctly handles:
- Same object reference (or both null)
- Unequal initialization status
- Type checking through dynamic_pointer_cast
- Deep comparison when types match
This is important for correct equality testing in the polymorphic design.
186-207: AddEntry implements single-entry constraint and fast filteringThe
MnNetInfo::AddEntrymethod now:
- Checks for existing entries (MaxLimit)
- Parses host and port separately
- Fast-fails on invalid characters before expensive lookup
These changes improve input validation performance and enforce the single-entry constraint described in the PR.
221-227: GetPrimary safely handles invalid entriesThe
GetPrimarymethod properly checks if the underlying service exists before returning it, falling back to the static empty service when needed. This prevents null pointer dereferences and provides consistent behavior.
229-235: Two-stage validation improves performanceThe
Validatemethod performs a quick check withIsTriviallyValidbefore proceeding to the more expensiveValidateService, which is an efficient approach to validation.src/evo/netinfo.h (3)
16-52: Enhanced NetInfoStatus enum provides more specific error codesThe
NetInfoStatusenum has been expanded with new error codes:
MaxLimitfor entry constraintsBadAddress,BadType, andNotRoutablefor specific validation failuresMalformedfor internal structure issuesThis enhances error reporting as mentioned in the PR objectives.
54-156: Type-flexible NetInfoEntry uses variant for extensibilityThe
NetInfoEntryclass correctly implements:
- Type-safe variant storage with
std::variant- Proper serialization with ADDRV2_FORMAT flag
- Comparison operators for sorting and equality
- Utility methods for validation and string conversion
This design allows for future extension beyond
CServicetypes as mentioned in the PR objectives.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 71-90: Clang format differences found. Code formatting does not match the expected style.
157-174: NetInfoInterface enables polymorphic usageThe abstract
NetInfoInterfaceclass provides a clean interface with:
- Entry management methods (AddEntry, GetEntries)
- Accessor methods (GetPrimary, IsEmpty)
- Validation and conversion methods (Validate, ToString)
- A static IsEqual method for deep comparison
This polymorphic design supports the architecture described in the PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/evo/netinfo.h (1)
100-157: Fix formatting issuesThe pipeline failure indicates formatting issues in this section of the code. Please run clang-format to fix the formatting issues.
#!/bin/bash # Checking formatting issues in the file clang-format -style=file src/evo/netinfo.h | diff -u src/evo/netinfo.h -🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 100-157: Clang format differences found. The code formatting does not match the expected style. Please run clang-format to fix the formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/evo/deterministicmns.cpp(8 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(2 hunks)src/evo/providertx.cpp(2 hunks)src/rpc/masternode.cpp(1 hunks)src/test/evo_netinfo_tests.cpp(4 hunks)src/txmempool.cpp(8 hunks)src/txmempool.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/evo/deterministicmns.h
- src/rpc/masternode.cpp
- src/txmempool.h
- src/evo/providertx.cpp
- src/txmempool.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧠 Learnings (3)
src/test/evo_netinfo_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.184Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
src/evo/netinfo.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.cpp:186-196
Timestamp: 2025-05-10T00:54:10.998Z
Learning: `std::reference_wrapper<T>` can be implicitly constructed from lvalue references (T&) through its forwarding reference constructor template. Explicit use of std::ref/std::cref is not required when pushing references into containers like vectors of reference_wrappers.
src/evo/netinfo.h (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 100-157: Clang format differences found. The code formatting does not match the expected style. Please run clang-format to fix the formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: linux64_multiprocess-test / Test source
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (17)
src/test/evo_netinfo_tests.cpp (6)
43-45: Appropriate use of IsTriviallyValid() in ValidateGetEntriesThe check ensures all three validation methods (
AddEntry(),Validate(), andIsTriviallyValid()) work together consistently.AddEntry()andValidate()handle consensus rules checks, while this IsTriviallyValid() check confirms the basic validation steps.
55-60: Well-structured validation logic enhancementThe added validation logic properly ensures that a
MnNetInfoobject is considered malformed if addition failed, and valid if addition succeeded. This establishes a solid contract betweenAddEntry()andValidate()methods.
64-70: Good enforcement of single-entry constraintThe new test block correctly verifies that
MnNetInfoonly allows a single entry and rejects additional entries with the newMaxLimitstatus code. This is an important constraint for theMnNetInfoclass.
73-144: Comprehensive serialization/deserialization testsThe new test cases thoroughly verify various aspects of the
NetInfoEntryclass, including:
- Empty object serialization
- Invalid deserialization handling
- Invalid
CServicehandling- Unrecognized type handling
- Valid
CServicehandling- ADDRV2 address support
This ensures the new abstraction works correctly for all edge cases and maintains compatibility with existing code.
146-175: Robust equality and comparison testingThe
netinfo_retvalstest case thoroughly verifies that theNetInfoEntryclass handles all accessor methods, comparison operators, and string conversions correctly, maintaining compatibility with the underlyingCServicebehavior. The explicit testing of empty/invalid entries is particularly valuable.
177-209: Good serialization compatibility verificationThe
CheckIfSerSamehelper function and associated test cases ensure that serialization remains compatible betweenCServiceandMnNetInfo, which is critical for maintaining backward compatibility and data integrity across upgrades.src/evo/netinfo.cpp (7)
18-32: Well-designed input filtering mechanismThe addition of
SAFE_CHARS_IPV4and theMatchCharsFilterhelper function provides a fast-failing mechanism for IPv4 address validation, which is more efficient than performing full validation on obviously invalid inputs. This aligns well with the PR objective of implementing a character-based filtering mechanism.
35-68: Well-implemented comparison operators for variant handlingThe implementation of
operator==andoperator<correctly handles type comparison and delegates to the appropriate comparison for the underlying types when needed. The handling ofstd::monostateis particularly well done, ensuring consistent ordering between valid and invalid entries.
70-77: Safe optional reference handling for accessing CServiceThe
GetAddrPort()method provides a type-safe way to access the underlyingCServicewithout risking undefined behavior if theNetInfoEntrycontains an invalid state. The use ofstd::optional<std::reference_wrapper<const CService>>is an elegant solution.
93-115: Robust trivial validation implementation with static_assertThe
IsTriviallyValid()method is well-implemented with a static assertion to ensure only expected types are handled. The method correctly verifies type code truthfulness and underlying data validity, which is crucial for the overall validation logic.
159-230: Improved error reporting in validation logicThe updated
ValidateServiceandValidatemethods now return more specific error codes (BadAddress,BadType,NotRoutable) instead of the genericBadInput, which aligns with the PR objective of expanding error codes to be more specific.
180-203: Fast-failing character filtering in AddEntryThe implementation of character filtering in
AddEntryusingMatchCharsFilteraligns with the PR objective of introducing a fast-failing mechanism to quickly classify address types before more expensive validation.
205-230: Robust GetEntries and GetPrimary implementationThe
GetEntriesandGetPrimarymethods handle empty states correctly and provide appropriate assertions for debug builds. The use of a static empty service for the fallback case inGetPrimaryis a clean solution.src/evo/netinfo.h (4)
16-52: Good expansion of NetInfoStatus enum with specific error codesThe addition of new status codes (
MaxLimit,BadAddress,BadType,NotRoutable,Malformed) and corresponding string representations improves error reporting and aligns with the PR objective of expanding error codes to be more specific.
54-124: Well-designed NetInfoEntry class using variant-based approachThe
NetInfoEntryclass is well-designed with a clear separation between valid (CService) and invalid (std::monostate) states. The serialization/deserialization methods enforce theADDRV2_FORMATflag as mentioned in the PR objectives, which is crucial for storing Tor and I2P addresses.
142-163: Clean MnNetInfo serialization implementationThe serialization/deserialization methods for
MnNetInfocorrectly handle the transition to usingNetInfoEntryinternally while maintaining compatibility with the existing serialization format that expects aCService. This ensures backward compatibility while enabling the new abstraction.
165-174: Clean interface update for MnNetInfoThe updated method signatures for
MnNetInforeflect the change to usingNetInfoEntryinternally, while maintaining a consistent interface for callers. The addition ofGetPrimary()and the updatedClear()method complete the interface nicely.
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
PastaPastaPasta,
UdjinM6 and
knst
| } | ||
| } else { | ||
| throw std::runtime_error( | ||
| strprintf("%s: Can't add a masternode %s with invalid address", __func__, dmn->proTxHash.ToString())); |
There was a problem hiding this comment.
IMO, it should be validated much earlier; AddMN should return failure if and only if duplicated masternode (with same protx / address /etc are created); all other types of valiure (such as NetInfoEntry members) should be validated earlier.
Either, this code should be just removed (if it's already validated) or some extra validations for NetInfoEntry should be added. Maybe even just throw exception if you try to create NetInfoEntry with invalid object; but AddMN it's a bit too late to validate if MN is valid at all
Also, ProRegTx allows an empty netInfo, ProUpServTx does not, so we should check that ProUpServTx::netInfo isn't empty.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/evo/netinfo.h (2)
57-60: Consider using enum class for type safety.The
NetInfoTypeenum uses raw values that could potentially conflict with other enums in scope. Consider usingenum class NetInfoType : uint8_tfor better type safety.- enum NetInfoType : uint8_t { + enum class NetInfoType : uint8_t { Service = 0x01, Invalid = 0xff - }; + };Note: This would require updating references throughout the code to use
NetInfoType::Serviceinstead of justService.
100-157: Address clang-format issues.The pipeline indicates formatting issues in this range. Please run clang-format to fix the code style.
#!/bin/bash # Run clang-format on the affected file clang-format -i src/evo/netinfo.h🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 100-157: Clang format differences found. The code formatting does not match the expected style. Please run clang-format to fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/evo/deterministicmns.cpp(8 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/netinfo.cpp(3 hunks)src/evo/netinfo.h(2 hunks)src/evo/providertx.cpp(2 hunks)src/rpc/masternode.cpp(1 hunks)src/test/evo_netinfo_tests.cpp(4 hunks)src/txmempool.cpp(8 hunks)src/txmempool.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/rpc/masternode.cpp
- src/evo/deterministicmns.h
- src/txmempool.h
- src/txmempool.cpp
- src/evo/providertx.cpp
- src/evo/deterministicmns.cpp
- src/evo/netinfo.cpp
🧰 Additional context used
🧠 Learnings (2)
src/test/evo_netinfo_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.184Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
src/evo/netinfo.h (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
🧬 Code Graph Analysis (1)
src/evo/netinfo.h (1)
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/netinfo.h
[error] 100-157: Clang format differences found. The code formatting does not match the expected style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
🔇 Additional comments (16)
src/test/evo_netinfo_tests.cpp (8)
8-8: LGTM: Required include for version flag.The
clientversion.hinclude is needed for theCLIENT_VERSIONconstant used in the new serialization test cases.
25-27: LGTM: Updated error codes align with enhanced validation.The error status updates from generic codes to more specific ones (
NotRoutablefor internal addresses,BadAddressfor invalid IPv4) improve error reporting granularity as mentioned in the PR objectives.
40-46: LGTM: Function signature correctly updated for NetInfoEntry abstraction.The
ValidateGetEntriesfunction properly transitions fromCServiceListtoNetInfoListand validatesNetInfoEntryobjects usingIsTriviallyValid(). Based on retrieved learnings, this ensures all three validation methods work together consistently.
55-60: LGTM: Enhanced validation with specific error reporting.The updated validation checks correctly expect
Malformedstatus for emptyMnNetInfoobjects andSuccessfor valid entries, aligning with the new error code semantics.
64-71: LGTM: Comprehensive test for single-entry constraint.This test case effectively validates the
MaxLimitconstraint enforcement inMnNetInfo, ensuring only one entry can be stored and that attempts to add additional entries are properly rejected.
73-144: LGTM: Thorough serialization testing with edge cases.The
netinfo_sertest case comprehensively covers:
- Empty object serialization size validation
- Invalid byte handling during deserialization
- Invalid CService handling
- Unrecognized type handling
- Valid CService serialization/deserialization
- ADDRV2 address support for Tor addresses
This provides excellent coverage of the new serialization logic.
146-175: LGTM: Comprehensive validation of NetInfoEntry interface.The
netinfo_retvalstest case thoroughly validates:
- Constructor behavior and trivial validity checks
- Dispatch methods (GetPort, ToString variants, ToStringAddrPort)
- Comparison operators
- Empty/invalid entry error handling
- Correct ordering behavior for invalid entries
The test effectively validates the complete public interface of
NetInfoEntry.
177-215: LGTM: Excellent backward compatibility verification.The
CheckIfSerSamehelper andcservice_compatibletest case ensure thatMnNetInfomaintains serialization compatibility withCService. This is crucial for the backward compatibility goals mentioned in the PR objectives. The test covers:
- Empty values compatibility
- Valid IPv4 with explicit and default ports
- Failure case handling (domains, IPv6)
This provides strong assurance that existing code relying on
CServiceserialization will continue to work.src/evo/netinfo.h (8)
10-12: LGTM: Required includes for new functionality.The
streams.handvariantincludes are necessary for theOverrideStreamusage in serialization and thestd::variantused inNetInfoEntry.
17-29: LGTM: Enhanced error reporting with specific status codes.The extended
NetInfoStatusenum provides better granularity for error reporting as mentioned in the PR objectives. The new codes (MaxLimit,BadAddress,BadType,NotRoutable,Malformed) enable more precise feedback during validation.
31-52: LGTM: Complete string mapping for enhanced error codes.The
NISToStringfunction properly handles all enum values including the new ones, providing clear error messages for better user experience.
68-73: LGTM: Safe constructor with validation.The constructor properly validates the
CServicebefore storing it, ensuring only valid services are accepted and defaulting to invalid state otherwise.
82-91: LGTM: Robust serialization with ADDRV2 enforcement.The serialization logic correctly:
- Enforces ADDRV2 format for enhanced address support
- Validates both type and data before serializing
- Falls back to
Invalidtype for malformed entriesThis aligns with the PR objective of supporting extended addresses while maintaining robustness.
94-106: LGTM: Safe deserialization with comprehensive error handling.The deserialization logic properly:
- Enforces ADDRV2 format consistently with serialization
- Handles invalid types by clearing the entry
- Catches deserialization failures and marks entries invalid
- Validates the deserialized
CServicebefore accepting itThis provides excellent error resilience during network communication.
123-123: LGTM: Proper serialization trait specialization.The
is_serializable_enumspecialization forNetInfoTypeenables direct serialization of the enum type.
142-163: LGTM: Backward-compatible serialization strategy.The
MnNetInfoserialization maintains compatibility with existingCServiceserialization while internally usingNetInfoEntry. The fallback to emptyCServicefor invalid entries ensures consistent behavior.
|
@knst, regarding the concerns about So it's not really a validation check so much as it's trying to get the bare representation like the map expects and just includes an additional case for no valid underlying entry despite prior checks because the alternative is silently ignoring it or crashing the client, neither of which is desirable, especially since the overhead incurred isn't that different in either case. There is a potential optimization but that's currently outside the scope of this PR. |
…switching impls, add new ProTx version 3e65628 evo: introduce new ProTx version for extended addresses (`ExtNetInfo`) (Kittywhiskers Van Gogh) b4fa7a2 evo: add helper to get highest ProTx version based on deployment status (Kittywhiskers Van Gogh) c7948e6 evo: allow deciding `NetInfoInterface` impl based on object version (Kittywhiskers Van Gogh) 22d9e93 evo: use interface shared ptr for `netInfo` instead of implementation (Kittywhiskers Van Gogh) f212caa evo: introduce `NetInfoInterface`, make `MnNetInfo` an implementation (Kittywhiskers Van Gogh) 0b04978 evo: make `netInfo` a shared pointer (Kittywhiskers Van Gogh) 0a4e726 util: add deep comparison helpers for shared_ptr (Kittywhiskers Van Gogh) c4f1175 evo: cleanup CDeterministicMN{,List,ListDiff,StateDiff} ser logic (Kittywhiskers Van Gogh) Pull request description: ## Motivation To enable the switch to extended addresses, the current `CService` entry was first encapsulated in `MnNetInfo` and exposed through a public interface (see [dash#6627](#6627)) that restricted access to the underlying type, then the underlying type of the address was abstracted away through `NetInfoEntry` (see [dash#6629](#6629)), allowing us to store different types of addresses. This pull request finally abstracts away `MnNetInfo` as 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 #6669 * Depends on #6629 * Dependency for #6674 * Since [dash#6579](#6579) got rid of legacy `CDeterministicMNState` serialization, there is no more logic that can attempt legacy deserialization, making the version check ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/evo/deterministicmns.h#L75-L78)) 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 `OverrideStream` to ensure proper ADDRv2 serialization ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/addrman.cpp#L175)) 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 a `lhs && rhs ? *lhs == *rhs : lhs == rhs` would be enough), a separate equality function `NetInfoInterface::IsEqual()` has been implemented that attempts to downcast `lhs` and `rhs` to the same type and returns the implementation-specific equality result.~~ A solution that uses `typeid` to help define comparison operators has been implemented but `std::shared_ptr` does not deep compare, to aid with that, the deep comparison helpers `util::shared_ptr_{,not_}equal` have been implemented and can be used with any pair of `shared_ptr`s so long as their underlying types define the necessary operators. Care needs to be taken to ensure that all equality comparisons involving `netInfo` use those functions. * A new ProTx version has been introduced for the upcoming extended addresses implementation. It is only applicable to `CProRegTx` and `CProUpServTx` as no other special transaction stores `netInfo` and is therefore, unaffected by the switchover. No code has been implemented in this PR that uses this new version, that is reserved for the PR that introduces the extended implementation. * Additionally, as the transaction version is used to determine the underlying implementation, make sure that `nVersion` is set *before* calling `MakeNetInfo()` (also remember to call `MakeNetInfo()` as the default constructor will set `netInfo` to `nullptr` and a missing call will result in a `nullptr` dereference when `netInfo` is 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 `CProRegTx` and `CProUpServTx`. 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 in `CProRegTx` and `CProUpServTx`.~~ See [comment](#6665 (comment)). ## 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: UdjinM6: utACK 3e65628 Tree-SHA512: 2675712351c9bbcf8bff2c6f032df40a52503abc62f8cd74b4b4f70bbcca0b68a32c1f4e1310c8a674bad1e605a2949337a7ab862a2129c4753cf8f4dbfa8789
4 participants
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 anstd::variantthat provides dispatching for common endpoints withstd::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 aCService, to ensure a conformity between it andExtNetInfo(the upcoming extended implementation), its public functions will return aNetInfoEntrywhen applicable so that both can be abstracted by a common interface to aid with the transition.Additional Information
Depends on refactor: section off masternode network information to
MnNetInfo, lay some groundwork for managing multiple entries #6627Depends on fix: MN port validation on mainnet is broken after 6627 #6664
Dependency for refactor: model interface after
MnNetInfoand support switching impls, add new ProTx version #66652e9bde0 in dash#6627 incorrectly migrated validation conditions such that attempting to set a validaddr:portcombination on mainnet would result in aBadPorterror 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 wholeevo_netinfo_testssuite now usesBasicTestingSetup(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 likeSuperseded by dash#6664.evo_simplifiedmns_merkleroots's usage ofNetInfoInterface::*methods (source).Per replies to review comments (comment, comment) from dash#6627, reported error codes from
netInfointeractions have been expanded to be more specific.CServiceby default is serialized using ADDRV1 and utilizing ADDRV2 serialization is either explicitly opted into (source) or determined on-the-fly (source). 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 affectMnNetInfoasNetInfoEntryis only used for the sake of a consistent interface but internally still (de)serializes to an ADDRV1CService.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 inBadInputas the delimiter used in IPv6 addresses are not part of the permitted characters filter but validating aMnNetInfowith an IPv6 address already stored will return aBadType.Breaking Changes
None expected.
Checklist