-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: introduce type-flexible NetInfoEntry
to allow non-CService
entries, use in MnNetInfo
#6629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
NetInfoEntry
to allow non-CService
entries, use in MnNetInfo
NetInfoEntry
to allow non-CService
entries, use in MnNetInfo
This pull request has conflicts, please rebase. |
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
src/evo/simplifiedmns.h (1)
38-60
: 🛠️ Refactor suggestionAvoid comparing shared-ptr identity inside
operator==
netInfo
is 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_ptr
instances 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
netInfo
left default-initialised tonullptr
– introduces UB risk
CProRegTx
andCProUpServTx
default-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_cast
inNetInfoSerWrapper
hides const-correctness issuesThe need for
const_cast
meansREADWRITE
cannot handle aconst std::shared_ptr<...>&
.
Instead of mutating through aconst
reference, declare the wrapper to acceptauto& ptr
and passobj.netInfo
directly – 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 (netInfo
access)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 (netInfo
access)
1220-1223
: Repeat of null-check issue (netInfo
access)
1823-1829
: Repeat of null-check issue (netInfo
access)src/evo/providertx.h (1)
132-138
: Duplicateconst_cast
problem inCProUpServTx
Same 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 inCProUpServTx
src/evo/deterministicmns.cpp (2)
528-546
: Same issue as above inupdateNetInfo
helperThe update path still assumes
CService
entries only; duplicate logic
should follow the relaxed rule proposed earlier.
610-621
: Same issue inRemoveMN
Deletion should skip entries that carry no
CService
, rather than
aborting with an exception.
🧹 Nitpick comments (14)
src/evo/simplifiedmns.h (1)
79-81
:const_cast
onshared_ptr
leaks const-correctness
NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.netInfo))
forces away constness to satisfy the wrapper, but this lets serialization routines
mutatenetInfo
through aconst
reference. Consider adding an overload of
NetInfoSerWrapper
that accepts aconst std::shared_ptr<…>&
and only mutates
throughmutable
members, 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 ifNetInfoEntry
grows. 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
AddEntry
can return non-Success
. In tests we currently ignore the status (except theCHECK_EQUAL
just below here). ConsiderBOOST_REQUIRE_EQUAL
to abort early on failure and aid debugging.
130-134
: Same remark as above – tighten assertionUse
BOOST_REQUIRE_EQUAL
forproTx.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
CHashWriter
version 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
CheckIfSerSame
cases, 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
: EmptyMnNetInfo
reported asMalformed
Validate()
returnsMalformed
whenm_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
Success
to 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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 thenetInfo
pointer rather than the dot operator (.
). This change is consistent with the broader refactoring wherenetInfo
was changed from a directMnNetInfo
object 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
netInfo
from a concreteMnNetInfo
object 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
netInfo
from a concreteMnNetInfo
object 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
netInfo
from a concrete object to a shared pointer of an interface type.
76-76
: Access updated to use pointer dereferenceThe change in the
CDeterministicMNStateDiff::ToJson
method correctly updates the access pattern to use pointer dereferencing for thenetInfo
member, 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 thenetInfo
member 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 thenetInfo
member 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
: InitializednetInfo
with the new shared_ptr-based approach.The code correctly initializes the
netInfo
member ofproTx
using theMakeNetInfo
function, 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 theAddEntry
method on thenetInfo
shared 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.h
is correctly added to provide access to theNetInfoEntry
class and related types used in the mempool implementation.
529-529
: Updated map key type fromCService
toNetInfoEntry
.The code now uses
NetInfoEntry
instead ofCService
as 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 overNetInfoEntry
objects instead ofCService
objects.
707-709
: Updated iteration over network entries inaddUncheckedProTx
for 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 inremoveUncheckedProTx
for 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
NetInfoEntry
objects 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
NetInfoEntry
type and pointer-based access pattern.
1397-1401
: Updated network entry iteration inexistsProviderTxConflict
.The code now properly iterates over
NetInfoEntry
objects when checking for provider transaction conflicts.
1422-1427
: Updated network entry iteration inexistsProviderTxConflict
for 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 forGetUniquePropertyHash
template method.The code adds necessary exclusions to prevent
GetUniquePropertyHash
from 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 rebuildingnetInfo
for 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 tonetInfo
updated 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 wherenetInfo
changed from a concreteMnNetInfo
object 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
netInfo
member, 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
OpenMasternodeConnection
now correctly uses the pointer dereferencing syntax to access the primary address from thenetInfo
shared pointer.
3781-3781
: ForNode call updated to use pointer dereferencingThe call to
ForNode
has 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
netInfo
shared pointer.src/evo/dmnstate.h (2)
115-116
: Good move – re-initialises after operator resetRe-creating
netInfo
viaMakeNetInfo
instead of callingClear()
on a concrete type is the correct abstraction-preserving approach.
249-252
: Missing dependency flag fornetInfo
diff field
pubKeyOperator
requiresnVersion
;netInfo
may equally depend onnVersion
(e.g. if validation rules vary between versions). If that is the case, emitField_nVersion
whenField_netInfo
is 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
MnNetInfo
for the payload and validates the return code.src/test/evo_netinfo_tests.cpp (2)
14-15
: Switch toBasicTestingSetup
improves realismGreat choice – this now exercises mainnet-like port rules and caught the earlier
BadPort
bug.🧰 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
/Success
aligns with new status codes.
src/coinjoin/client.cpp
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |
+ } |
src/evo/providertx.cpp
Outdated
EncodeDestination(PKHash(keyIDVoting)), payee, platformNodeID.ToString(), platformP2PPort, | ||
platformHTTPPort, netInfo.ToString()); | ||
platformHTTPPort, netInfo->ToString()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/evo/dmnstate.h (1)
57-57
: 🛠️ Refactor suggestionConsider initializing
netInfo
to prevent null dereferenceThe
netInfo
shared_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-format
compliance. 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
RegTestingSetup
toBasicTestingSetup
properly 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
MnNetInfo
enforces the single-entry limit by rejecting additional entries with aMaxLimit
status, consistent with the updatedAddEntry
logic.
72-78
: Well-designed serialization compatibility checkThe
CheckIfSerSame
helper function is a clean approach to compare serialized representations, supporting the new compatibility checks betweenCService
andMnNetInfo
.src/evo/netinfo.cpp (2)
29-37
: Effective input validation approachThe
MatchCharsFilter
function 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_wrapper
inNetInfoList
definition is an excellent choice, preventing unnecessary copying while maintaining reference semantics and standard container compatibility.
227-233
: Extensible factory patternThe
MakeNetInfo
function 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-format
in 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ContainsOnlyAllowedChars
orHasOnlyAllowedChars
to 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
NetInfoSerWrapper
implementation 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/evo_netinfo_tests.cpp (1)
39-45
: NetInfoList API properly validates entriesThe
ValidateGetEntries
function has been updated to work with the newNetInfoList
type (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
NetInfoSerWrapper
class 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
RegTestingSetup
toBasicTestingSetup
ensures 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
BadInput
toBadAddress
for 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
CheckIfSerSame
helper andcservice_compatible
test case properly verify serialization compatibility betweenCService
andMnNetInfo
across 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
MatchCharsFilter
function 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
GetAddrPort
method properly usesstd::optional
to safely retrieve the underlyingCService
from 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
IsTriviallyValid
method 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::IsEqual
static 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::AddEntry
method 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
GetPrimary
method 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
Validate
method performs a quick check withIsTriviallyValid
before 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
NetInfoStatus
enum has been expanded with new error codes:
MaxLimit
for entry constraintsBadAddress
,BadType
, andNotRoutable
for specific validation failuresMalformed
for internal structure issuesThis enhances error reporting as mentioned in the PR objectives.
54-156
: Type-flexible NetInfoEntry uses variant for extensibilityThe
NetInfoEntry
class 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
CService
types 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
NetInfoInterface
class 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
MnNetInfo
object 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
MnNetInfo
only allows a single entry and rejects additional entries with the newMaxLimit
status code. This is an important constraint for theMnNetInfo
class.
73-144
: Comprehensive serialization/deserialization testsThe new test cases thoroughly verify various aspects of the
NetInfoEntry
class, including:
- Empty object serialization
- Invalid deserialization handling
- Invalid
CService
handling- Unrecognized type handling
- Valid
CService
handling- 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_retvals
test case thoroughly verifies that theNetInfoEntry
class handles all accessor methods, comparison operators, and string conversions correctly, maintaining compatibility with the underlyingCService
behavior. The explicit testing of empty/invalid entries is particularly valuable.
177-209
: Good serialization compatibility verificationThe
CheckIfSerSame
helper function and associated test cases ensure that serialization remains compatible betweenCService
andMnNetInfo
, 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_IPV4
and theMatchCharsFilter
helper 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::monostate
is 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 underlyingCService
without risking undefined behavior if theNetInfoEntry
contains 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
ValidateService
andValidate
methods 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
AddEntry
usingMatchCharsFilter
aligns 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
GetEntries
andGetPrimary
methods handle empty states correctly and provide appropriate assertions for debug builds. The use of a static empty service for the fallback case inGetPrimary
is 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
NetInfoEntry
class is well-designed with a clear separation between valid (CService
) and invalid (std::monostate
) states. The serialization/deserialization methods enforce theADDRV2_FORMAT
flag 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
MnNetInfo
correctly handle the transition to usingNetInfoEntry
internally 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
MnNetInfo
reflect the change to usingNetInfoEntry
internally, while maintaining a consistent interface for callers. The addition ofGetPrimary()
and the updatedClear()
method complete the interface nicely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's resolve 3c7210c
Otherwise it looks good to me; other comments are nits
} | ||
} 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/evo/netinfo.h (2)
57-60
: Consider using enum class for type safety.The
NetInfoType
enum uses raw values that could potentially conflict with other enums in scope. Consider usingenum class NetInfoType : uint8_t
for 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::Service
instead 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.h
include is needed for theCLIENT_VERSION
constant 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 (
NotRoutable
for internal addresses,BadAddress
for invalid IPv4) improve error reporting granularity as mentioned in the PR objectives.
40-46
: LGTM: Function signature correctly updated for NetInfoEntry abstraction.The
ValidateGetEntries
function properly transitions fromCServiceList
toNetInfoList
and validatesNetInfoEntry
objects 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
Malformed
status for emptyMnNetInfo
objects andSuccess
for valid entries, aligning with the new error code semantics.
64-71
: LGTM: Comprehensive test for single-entry constraint.This test case effectively validates the
MaxLimit
constraint 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_ser
test 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_retvals
test 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
CheckIfSerSame
helper andcservice_compatible
test case ensure thatMnNetInfo
maintains 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
CService
serialization will continue to work.src/evo/netinfo.h (8)
10-12
: LGTM: Required includes for new functionality.The
streams.h
andvariant
includes are necessary for theOverrideStream
usage in serialization and thestd::variant
used inNetInfoEntry
.
17-29
: LGTM: Enhanced error reporting with specific status codes.The extended
NetInfoStatus
enum 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
NISToString
function 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
CService
before 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
Invalid
type 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
CService
before accepting itThis provides excellent error resilience during network communication.
123-123
: LGTM: Proper serialization trait specialization.The
is_serializable_enum
specialization forNetInfoType
enables direct serialization of the enum type.
142-163
: LGTM: Backward-compatible serialization strategy.The
MnNetInfo
serialization maintains compatibility with existingCService
serialization while internally usingNetInfoEntry
. The fallback to emptyCService
for 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 3a72f2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 3a72f2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 3a72f2f
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::variant
that 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 aNetInfoEntry
when 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
MnNetInfo
and support switching impls, add new ProTx version #66652e9bde0 in dash#6627 incorrectly migrated validation conditions such that attempting to set a validaddr:port
combination on mainnet would result in aBadPort
error because the non-mainnet rules were applied regardless of network.This evaded testing as our unit and functional tests do not run on mainnet. To prevent this from occurring again, the wholeevo_netinfo_tests
suite 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
netInfo
interactions have been expanded to be more specific.CService
by default is serialized using ADDRV1 and utilizing ADDRV2 serialization is either explicitly opted into (source) 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 affectMnNetInfo
asNetInfoEntry
is 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 inBadInput
as the delimiter used in IPv6 addresses are not part of the permitted characters filter but validating aMnNetInfo
with an IPv6 address already stored will return aBadType
.Breaking Changes
None expected.
Checklist