refactor: section off masternode network information to MnNetInfo, lay some groundwork for managing multiple entries
#6627
Conversation
…up index code, consolidate ProTx versioning and make it friendlier to upgrades 86bec48 refactor: rearrange checks in ProTx RPC wrapper to bail out faster (Kittywhiskers Van Gogh) 14b7cf3 refactor: rename CPro*Tx::GetVersion() to GetMaxVersion(), increase use (Kittywhiskers Van Gogh) 714fb3a refactor: consolidate ProTx versioning, make logic friendly to upgrades (Kittywhiskers Van Gogh) 2f9d65d refactor: cleanup index code, style updates, deduplication (Kittywhiskers Van Gogh) eaca912 refactor: defer resolving masternode service in CoinJoin code (Kittywhiskers Van Gogh) e722272 chore: use ProTx hash to identify masternodes in logs when possible (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6627 * As [dash#6627](#6627) aims to abstract away masternode network information (so far stored as a `CService`) behind an interface in preparation for supporting multiple addresses of different types, it still needs to be exposed to networking logic that actively relies on the "primary" address (i.e. the address used by masternodes to connect to each other). This is done through `MnNetInfo::GetPrimary()`. To reduce the `GetPrimary()` invocations needed to a minimum in [dash#6627](#6627), debug log messages have been changed to identify masternodes using the ProTx hash instead of the primary address and `CPendingDsaRequest` has been refactored to delay resolving the primary address, tracking the ProTx hash instead. * Currently, valid versions of `CPro*Tx` are expressed as `CPro*Tx::*_VERSION`. This isn't necessary as the `*_VERSION` (currently `{BASIC,LEGACY}_BLS_VERSION`) are the same for all `CPro*Tx`es, so they have been consolidated into a namespaced enum `ProTxVersion`. * Additionally, some checks in the codebase assume that are only two versions ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/evo/providertx.cpp#L20), [source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/evo/providertx.h#L150)), which isn't conducive to adding new versions. Those checks have been modified to allow higher versions. * `CPro*Tx::GetVersion()` has been renamed to `CPro*Tx::GetMaxVersion()` as its actually used to check what is the highest _allowed_ version given a set of consensus rules. `GetMaxVersion()` has not been consolidated like `ProTxVersion` as any changes to `addr` (as intended in [dash#6627](#6627)) would only affect `CProRegTx` and `CProUpServTx` (i.e. the remaining ProTx transactions would not be getting a version upgrade). * In later PRs, the change to a multiple-address capable implementation is hidden away using a `shared_ptr` to the interface class. To decide _what_ implementation to use, we need to set the version of `CPro*Tx`, as early as possible so that the version can be used to determine the applicable implementation. This is easy enough to do in serialization code but requires additional care when constructing a `CPro*Tx` in-place as it will be initialized with an empty `shared_ptr` and the applicable implementation needs to be explicitly set. To avoid problems in those later PRs, some instances of `CPro*Tx` construction have been reshuffled to set `nVersion` as early as possible. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 86bec48 knst: utACK 86bec48 Tree-SHA512: 3439ea4c51bed09b6147bb0bc26580cbb960ba1856855827d1db01e5303d6d9dce654e87b2982794aa6349a560e73b4b3fd99ca3bb7402e1b95f24a852657ffb
|
This pull request has conflicts, please rebase. |
…h `boost::hana` 0073e9a refactor: replace `CDeterministicMNStateDiff` macros with `boost::hana` (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6627 * Currently, we rely on a set of macros to use a different set of instructions for (de)serializing `pubKeyOperator` in `CDeterministicMNStateDiff`, one of those macros is the following https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/evo/dmnstate.h#L219-L226 If we pretend `DMN_STATE_DIFF_ALL_FIELDS` only pertains to `pubKeyOperator`, the macro would expand as ```c++ if (strcmp("pubKeyOperator", "pubKeyOperator") == 0 && (obj.fields & Field_pubKeyOperator)) { SER_READ(obj, read_pubkey = true); READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION)); } else if (obj.fields & Field_pubKeyOperator) READWRITE(obj.state.pubKeyOperator); ``` Even though `READWRITE(obj.state.pubKeyOperator)` is _logically_ unreachable, it is something the compiler still has to evaluate and it can because `READWRITE(obj.state.pubKeyOperator)` is still a valid expression. But if we need to carve out a similar different rule in a later PR for `newThing` where `newThing` is a `std::shared_ptr<Interface>` that is implemented by the serializable type `Implementation`, the unreachable but still evaluable expression `READWRITE(obj.state.newThing)` cannot be evaluated as you _cannot_ do anything with a pure virtual class, which `Interface` is even though the code right before it uses a wrapper to handle `newThing` correctly. To sidestep this issue, we need to be able to `constexpr` evaluate what field is being (de)serialized and decide the (de)serialization logic for it accordingly, which will _exclude_ all other logic that doesn't apply _at compile time_. The current macro-based solution doesn't allow for that. While `std::tuple` allows for storing a heterogenous collection of elements, iterating through it proves to be difficult. `std::apply` proves to be too restrictive for what we need to do ([source](https://stackoverflow.com/a/54053084)) and the capability needed to do this properly, "expansion statements" could be available in C++26 ([source](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2996r0.html)), which is a long time from now. So, our best option is to use a library that specializes in working with heterogenous collections and thankfully, such a library is already available in Boost called Hana ([source](https://www.boost.org/doc/libs/1_81_0/libs/hana/doc/html/index.html)) and it is headers-only ([source](https://www.boost.org/doc/libs/1_81_0/more/getting_started/unix-variants.html#header-only-libraries), list of all libraries that need building, Hana is not on the list) and is therefore, already available to us. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 0073e9a Tree-SHA512: c0b7e4214e6180f755e6d030bc24f7cb0d5ab0d5fb262a6394bc17d60bf4de0be9148cd0b07cd00f5e78c1b07c156d65c54c25c2ac6fa49bf7d2ae9a406650a8
…ble to object during construction, reduce `GetHash()` usage f292483 refactor: use empty() instead of comparing against empty string literal (Kittywhiskers Van Gogh) d30b9ae refactor: reduce ostringstream usage when constructing strings (Kittywhiskers Van Gogh) 7200505 refactor: stop using ostringstream to construct filter strings (Kittywhiskers Van Gogh) 1c4f441 util: implement string padding function (Kittywhiskers Van Gogh) 5bfc4bc refactor: use `UniValue::VOBJ` instead of `setObject()` when possible (Kittywhiskers Van Gogh) 8b09188 refactor: store `tx_hash` instead of calling `GetHash()` repeatedly (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6627 * We use `std::ostringstream` in `masternodelist` to pad values using `std::setw` ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/rpc/masternode.cpp#L574-L575)) for consistency, this was fine because we didn't plan on removing anything from the stream after it was added. But in [dash#6627](#6627), we intend to print multiple addresses and need to remove excess padding after the last entry, which can be trivially done with a `pop_back()` ([source](https://github.com/dashpay/dash/blob/bba9c6dd8e2edbbb83a26763a1d64209409b5b82/src/rpc/masternode.cpp#L569-L572)) but that cannot be done with a `std::ostringstream`. The best that can be done is rewinding the cursor but that doesn't change its contents and at most lets you overwrite it ([source](https://stackoverflow.com/a/26492431)), which is fine when you do have something to overwrite it with but this isn't always the case. To get around this, `PadString()` has been implemented and `std::ostringstream` usage has been replaced with constructing a string with `strprintf`. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK f292483 PastaPastaPasta: utACK f292483 Tree-SHA512: a3ca9dffc4d13bae80469f669af64d924a9bc6fd02d04832dc1bd07a7016b64973ce41c4dc54dc9c6f580ae92e00edb1cafeeb3b2ee29ae93f2f565ec7f18008
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
MnNetInfo, set some groundwork for managing multiple entries MnNetInfo, lay some groundwork for managing multiple entries
WalkthroughThis change set introduces a comprehensive migration from using a single network address ( All code that previously accessed or manipulated a masternode's address now uses Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
src/evo/simplifiedmns.h (1)
38-44: 💡 Verification agent🧩 Analysis chain
Guard against empty
MnNetInfo& maintain backward-compat fields
MnNetInfois now the single source of truth for the service address, but neither
CSimplifiedMNListEntry’s ctor nor its (de)serialisation ensure that at least one
entry is present. IfnetInfois default-constructed (e.g. when reading pre-fork
data or in fuzz tests) the equality operator and theREADWRITE(obj.netInfo, …)
statement will silently process an empty list, while later code assumes a primary
entry always exists (GetPrimary()willCHECK_LOCK-abort).
- Add an explicit run-time check (or an
assert) in the ctor and deserialiser
thatnetInfo.HasEntries()is true.- Consider serialising the legacy
CServiceas a fallback when
s.GetVersion() < NETINFO_PROTO_VERSIONso older peers don’t fail.+ if ((s.GetType() & SER_NETWORK) && !obj.netInfo.HasEntries()) { + throw std::ios_base::failure("MnNetInfo must contain at least one entry"); + }Also applies to: 52-63, 71-80
🏁 Script executed:
#!/bin/bash set -e # Locate the simplifiedmns.h file FILE=$(fd simplifiedmns.h | head -n1) echo "Inspecting file: $FILE" # Show the class definition and constructor region echo -e "\n--- Class and constructor snippet ---" sed -n '1,200p' "$FILE" | sed -n '1,200!d' # Search for serialization of netInfo echo -e "\n--- Serialization occurrences of netInfo ---" rg -n "netInfo" -C3 src/evoLength of output: 20380
🏁 Script executed:
#!/bin/bash set -e # Locate the MnNetInfo class definition and its methods echo "--- MnNetInfo class definition ---" rg -n "class MnNetInfo" -C10 src # Check for any HasEntries method in MnNetInfo echo -e "\n--- HasEntries or equivalent methods ---" rg -n -C2 "HasEntries" -g "*.h" src rg -n -C2 "IsEmpty" -g "*.h" src # Find the protocol version constant for netInfo serialization echo -e "\n--- NETINFO_PROTO_VERSION ---" rg -n "NETINFO_PROTO_VERSION" -C2 srcLength of output: 1178
🏁 Script executed:
#!/bin/bash set -e # Show full MnNetInfo class definition to locate methods for entry checks echo "--- MnNetInfo class definition ---" sed -n '/class MnNetInfo/,/};/p' src/evo/netinfo.h # Look for methods that expose entries or primary service echo -e "\n--- GetEntries and GetPrimary in MnNetInfo ---" rg -n "GetEntries" -C2 src/evo/netinfo.h rg -n "GetPrimary" -C2 src/evo/netinfo.hLength of output: 789
Add runtime guard for empty MnNetInfo using IsEmpty() and handle legacy serialization
The
MnNetInfoclass doesn’t exposeHasEntries()—it usesIsEmpty()to detect a default‐constructed (empty) state. We still need to prevent downstreamGetPrimary()calls from aborting when reading old or malformed data, and we should fall back to the legacyCServicefor pre-protocol data.• In
src/evo/simplifiedmns.h, inside theSerializationOpofCSimplifiedMNListEntry(around lines 75–80), add:+ // Guard against empty netInfo (default-constructed) on network deserialisation + if ((s.GetType() & SER_NETWORK) && obj.netInfo.IsEmpty()) { + throw std::ios_base::failure("MnNetInfo must contain at least one service entry"); + }• If you need to preserve compatibility with peers before the netInfo‐serialization bump, wrap the READWRITE of
obj.netInfoin a version check, e.g.:if (s.GetVersion() < MY_NETINFO_PROTO_VERSION) { // legacy fallback: serialise single CService then construct MnNetInfo CService legacyService; READWRITE(legacyService); obj.netInfo = MnNetInfo(legacyService); } else { READWRITE(obj.netInfo); }Define
MY_NETINFO_PROTO_VERSIONto match the release where netInfo switched from CService to MnNetInfo.src/txmempool.cpp (1)
694-710: 💡 Verification agent❓ Verification inconclusive
mapProTxAddresses.emplace()may silently drop duplicates
std::unordered_map::emplaceignores additional inserts with an existing key.
If two different pending ProTx transactions manage to claim the same address
before conflict detection runs, only the first hash is stored and the second
one will never be removed from the map, leaking state.Safer alternative:
- mapProTxAddresses.emplace(entry, tx_hash); + mapProTxAddresses.insert_or_assign(entry, tx_hash);Same applies to the
CProUpServTxbranch below.
Prevent silent drop of duplicate ProTx address mappings
The use of
mapProTxAddresses.emplace(entry, tx_hash)will ignore any insert where the key already exists, causing a later removal to miss stale entries and leak state. Replace it withinsert_or_assignto ensure the mapping is always updated.Locations to update in
src/txmempool.cpp:
- In the
TRANSACTION_PROVIDER_REGISTERbranch (around line 696)- In the
TRANSACTION_PROVIDER_UPDATE_SERVICEbranch (around line 706)Suggested diff:
- mapProTxAddresses.emplace(entry, tx_hash); + mapProTxAddresses.insert_or_assign(entry, tx_hash);Apply this change in both branches to guarantee consistent insertion and removal.
🧹 Nitpick comments (9)
src/evo/providertx.h (1)
63-63: Fix the Clang formatting issue.Clang format differences were found on this line. Please run clang-format to fix the style issues.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 63-63: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/dmnstate.h (1)
98-98: Fix the Clang formatting issues.Clang format differences were found on lines 98 and 181. Please run clang-format to fix the style issues.
Also applies to: 181-181
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 98-98: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/simplifiedmns.h (1)
71-79: clang-format violations break CIThe CI pipeline (
Clang Diff Format Check) flagged this block. Please run
clang-format -i src/evo/simplifiedmns.h(orgit clang-format) before pushing
the next revision to avoid rebasing only for style fixes.src/evo/dmnstate.cpp (1)
28-36: Minor: expensiveToString()call constructed but unused on old log levels
netInfo.ToString()allocates a temporarystd::string. In very verbose
debug builds that’s fine, but consider wrapping the wholestrprintfin
LogAcceptCategory(BCLog::MASTERNODE)or similar to avoid the cost when the
string is eventually dropped by the compiler.src/rpc/masternode.cpp (1)
574-600: Run clang-format to satisfy CICI reports formatting issues around these blocks. Re-running
clang-format
will realign the closing parentheses and spacing inside the lambdas.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 574-574: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 585-585: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 600-600: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/txmempool.cpp (1)
1397-1404: Loop can be replaced withany_offor clarityA tiny readability suggestion:
if (std::any_of(proTx.netInfo.Begin(), proTx.netInfo.End(), [&](const CService& s){ return mapProTxAddresses.count(s); })) return true;src/test/evo_netinfo_tests.cpp (1)
1-4: Copyright year should be current year.The copyright year is set to 2025, but should be the current year when the file was created.
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2024 The Dash Core developerssrc/evo/netinfo.cpp (2)
1-4: Copyright year should be current year.The copyright year is set to 2025, but should be the current year when the file was created.
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2024 The Dash Core developers
11-21: Consider initializing main params during startup rather than on-demand.The
MainParams()function initializesg_main_paramson the first call with a TODO comment about real args. Consider initializing this during application startup to avoid potential thread safety issues with lazy initialization.namespace { static std::unique_ptr<const CChainParams> g_main_params{nullptr}; bool IsNodeOnMainnet() { return Params().NetworkIDString() == CBaseChainParams::MAIN; } const CChainParams& MainParams() { - // TODO: use real args here - if (!g_main_params) g_main_params = CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN); + assert(g_main_params != nullptr); // Should be initialized during startup return *g_main_params; } } // anonymous namespace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(3 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(6 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(2 hunks)src/evo/providertx.h(5 hunks)src/evo/simplifiedmns.cpp(3 hunks)src/evo/simplifiedmns.h(4 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/txmempool.cpp(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/test/evo_simplifiedmns_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
src/evo/simplifiedmns.h (2)
src/evo/netinfo.h (4)
rhs(46-46)rhs(46-46)rhs(47-47)rhs(47-47)src/evo/simplifiedmns.cpp (2)
rhs(137-146)rhs(137-137)
src/test/evo_netinfo_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
src/evo/netinfo.cpp (4)
src/evo/netinfo.h (5)
ValidateService(59-59)service(40-40)service(54-54)m_addr(57-57)m_addr(62-62)src/evo/dmnstate.cpp (2)
ToString(16-36)ToString(16-16)src/evo/providertx.cpp (8)
ToString(82-98)ToString(82-82)ToString(112-125)ToString(112-112)ToString(148-158)ToString(148-148)ToString(174-178)ToString(174-174)src/evo/simplifiedmns.cpp (2)
ToString(53-73)ToString(53-53)
src/evo/simplifiedmns.cpp (2)
src/evo/dmnstate.h (1)
nVersion(111-119)src/evo/dmnstate.cpp (2)
obj(40-40)obj(71-71)
src/net.cpp (2)
test/functional/test_framework/messages.py (1)
CAddress(252-375)src/net.h (4)
ForNode(1310-1313)ForNode(1310-1310)ForNode(1316-1319)ForNode(1316-1316)
🪛 GitHub Actions: Clang Diff Format Check
src/masternode/utils.cpp
[error] 77-77: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/llmq/utils.cpp
[error] 823-823: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 872-872: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/providertx.cpp
[error] 91-91: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 120-120: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/test/evo_simplifiedmns_tests.cpp
[error] 34-34: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/providertx.h
[error] 63-63: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/dmnstate.h
[error] 98-98: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 181-181: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/rpc/masternode.cpp
[error] 574-574: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 585-585: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 600-600: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/simplifiedmns.h
[error] 51-51: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/test/evo_netinfo_tests.cpp
[error] 21-21: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/deterministicmns.h
[error] 394-394: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/netinfo.cpp
[error] 46-46: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 72-72: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/simplifiedmns.cpp
[error] 67-67: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/netinfo.h
[error] 10-10: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
[error] 46-46: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/deterministicmns.cpp
[error] 840-840: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
🪛 Cppcheck (2.10-2)
src/test/evo_simplifiedmns_tests.cpp
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
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: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64-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: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (64)
src/qt/masternodelist.cpp (1)
224-227: Properly updated to use the new MnNetInfo structure.The code correctly updates the way masternode network addresses are accessed, using the new
MnNetInfostructure'sGetPrimary()method instead of directly accessing the previousaddrfield.src/Makefile.test.include (1)
116-116: Added test for the new MnNetInfo functionality.Good addition of a specific test file for the new
MnNetInfofunctionality to ensure proper test coverage for the refactoring.src/test/block_reward_reallocation_tests.cpp (1)
120-120: Improved validation with the new MnNetInfo API.The test has been updated to use the new
MnNetInfoAPI, with added validation throughBOOST_CHECK_EQUALto verify that the address was successfully added. This is a good improvement over the previous direct assignment.src/rpc/quorums.cpp (1)
206-206: Updated to use the new MnNetInfo structure for service address.The code correctly accesses the masternode service address through the new
MnNetInfostructure'sGetPrimary()method, maintaining compatibility with the rest of the codebase's refactoring.src/Makefile.am (2)
194-194: Correctly added the new netinfo.h header file to the build system.The addition of the
evo/netinfo.htoBITCOIN_CORE_His necessary for making the newMnNetInfoclass available throughout the codebase. This change properly integrates the header into the build system.
778-778: Correctly added the new netinfo.cpp source file to the build system.The addition of
evo/netinfo.cpptolibbitcoin_common_a_SOURCESensures the implementation of theMnNetInfoclass is compiled into the common library. This is the appropriate location given the usage of this class across different components of the codebase.src/evo/core_write.cpp (3)
9-9: Properly included the new netinfo header.The inclusion of
<evo/netinfo.h>is correct and necessary to access theMnNetInfoclass definitions used in this file.
70-70: Updated JSON serialization to use the new network information structure.The change correctly adapts the JSON serialization to use
netInfo.GetPrimary().ToStringAddrPort()instead of directaddraccess, maintaining the same output format while supporting the new architecture for potentially multiple network addresses.
117-117: Updated JSON serialization for CProUpServTx class.Similar to the change for CProRegTx, this update correctly adapts the JSON serialization to use the new
netInfostructure, maintaining consistent handling across different transaction types.src/test/evo_simplifiedmns_tests.cpp (4)
13-13: Fixture upgrade is appropriateChanging from
BasicTestingSetuptoRegTestingSetupis appropriate since the PR aims to improve address validation. The regtest network environment provides proper context for validatingCServiceobjects.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
20-20: Improved address generationStarting the loop from 1 instead of 0 prevents the creation of invalid "0.0.0.0:0" addresses, which is in line with the PR objectives to improve address validation.
25-25: Verify IP address format is sufficientWhile you've changed to start indexes at 1 to avoid "0.0.0.0:0", the IP addresses still use zeros in the first three octets (e.g., "0.0.0.1:1").
Verify that these addresses are valid in your context. If you need actual valid IP addresses for proper testing, consider using something like:
-BOOST_CHECK_EQUAL(smle.netInfo.AddEntry(strprintf("%d.%d.%d.%d:%d", 0, 0, 0, i, i)), NetInfoStatus::Success); +BOOST_CHECK_EQUAL(smle.netInfo.AddEntry(strprintf("127.0.0.%d:%d", i, i)), NetInfoStatus::Success);
52-52: Updated hash values consistent with code changesThe updated hash values and merkle root are necessary consequences of the address changes and migration to
MnNetInfo. These changes ensure the tests pass with the new implementation.Also applies to: 65-65
src/net.cpp (7)
3677-3700: Correctly migrated from directaddraccess tonetInfo.GetPrimary()The code has been properly updated to use
netInfo.GetPrimary()instead of directly accessingaddr. This ensures consistency with the broader refactoring of masternode network information to use the newMnNetInfostructure.
3743-3746: Address retrieval properly updated to use newMnNetInfostructureThis condition has been correctly updated to check connection status using the primary address from the
netInfostructure rather than directly accessing theaddrfield.
3752-3754: Log message appropriately updated to usenetInfo.GetPrimary()Log message has been properly updated to retrieve the address from the new
MnNetInfostructure.
3763-3764: Probe log message correctly updated to use new address access patternThe log message for probing a masternode has been properly updated to use
netInfo.GetPrimary()for address retrieval.
3779-3780: Masternode connection function updated to use primary address fromnetInfoThe function call to
OpenMasternodeConnectionhas been correctly modified to use the primary address from the masternode'snetInfostructure.
3781-3787: ForNode method call updated to use the proper address retrievalThe call to
ForNodehas been properly updated to usenetInfo.GetPrimary()to check connection status after an attempt to connect.
3788-3792: Connection failure log updated to use new address access patternThe log message for connection failures has been properly updated to use the primary address from
netInfofor consistent logging.src/evo/providertx.h (5)
9-9: Header inclusion for the new MnNetInfo type added correctly.The new
#include <evo/netinfo.h>header inclusion is properly added to support the MnNetInfo type used in the refactoring.
44-44: CProRegTx field changed from addr to netInfo.The single
CService addrmember has been correctly replaced withMnNetInfo netInfoas part of the refactoring to support multiple network addresses.
70-70: Serialization updated for netInfo.Serialization of CProRegTx has been properly updated to use
obj.netInfoinstead of the previousobj.addr.
113-113: CProUpServTx field changed from addr to netInfo.The single
CService addrmember has been correctly replaced withMnNetInfo netInfoas part of the refactoring to support multiple network addresses.
136-136: Serialization updated for netInfo.Serialization of CProUpServTx has been properly updated to use
obj.netInfoinstead of the previousobj.addr.src/test/evo_deterministicmns_tests.cpp (7)
109-109: Network address properly added through netInfo.AddEntry()The test properly uses the new
netInfo.AddEntry()method instead of direct assignment toaddr. Good practice to verify the success of the addition withBOOST_CHECK_EQUAL.
131-131: Network address properly added through netInfo.AddEntry()The test properly uses the new
netInfo.AddEntry()method instead of direct assignment toaddrfor service transactions. The success verification is consistent with the approach used in other tests.
519-519: Address verification correctly updated to use netInfo.GetPrimary()The test now properly accesses the port through
netInfo.GetPrimary().GetPort()instead of directly fromaddr, maintaining backward compatibility while using the new structure.
577-577: Address verification correctly updated to use netInfo.GetPrimary()The test now properly accesses the port through
netInfo.GetPrimary().GetPort()instead of directly fromaddr, maintaining consistency with the overall refactoring approach.
637-638: Network address properly added through netInfo.AddEntry() in mempool testThe function test for mempool organization correctly uses the new
netInfo.AddEntry()method with success verification, consistent with the structure changes.
711-711: Network address properly added through netInfo.AddEntry() in dual proTx testThe function test for dual ProRegTx in mempool correctly uses the new
netInfo.AddEntry()method with success verification, aligned with the structure changes.
779-779: Network address properly added through netInfo.AddEntry() in verifyDB testThe function test for DB verification correctly uses the new
netInfo.AddEntry()method with success verification, maintaining consistency throughout the test suite.src/evo/dmnstate.h (6)
57-57: CDeterministicMNState field changed from addr to netInfo.The single
CService addrmember has been correctly replaced withMnNetInfo netInfoin the CDeterministicMNState class as part of the refactoring.
72-72: Constructor initialization updated for netInfo.The class constructor now properly initializes
netInfofromproTx.netInfo, maintaining the expected behavior during object construction.
103-103: Serialization updated for netInfo.Serialization has been properly updated to use
netInfoinstead of the previousaddrfield.
115-115: ResetOperatorFields updated for netInfo.The
ResetOperatorFieldsmethod now correctly usesnetInfo.Clear()instead ofaddr = CService()to reset the network information.
169-169: CDeterministicMNStateDiff enum field renamed to Field_netInfo.The enum value
Field_addrhas been renamed toField_netInfowhile maintaining the same bit value (0x0800), ensuring serialization compatibility with existing data.
201-201: Member reference updated in state diff for netInfo.The member reference in the state differencing code has been updated to use
netInfoinstead ofaddr.src/masternode/node.cpp (2)
149-149: Local address comparison updated to use netInfo.GetPrimary()The code correctly updates the comparison between the local service address and the masternode's address to use
netInfo.GetPrimary()instead of directly accessingaddr. This maintains the same functionality while using the new network info structure.
204-204: Address change detection updated to use netInfo.GetPrimary()The code that detects changes in a masternode's IP address now correctly compares
netInfo.GetPrimary()values instead of comparingaddrdirectly. This maintains consistency with the overall refactoring approach.src/test/evo_netinfo_tests.cpp (2)
14-20: LGTM: Helper validation function is well-structured.The
ValidateGetEntrieshelper function appropriately checks the size of entries returned by theGetEntries()method, which will be useful throughout the test cases.🧰 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)
21-53:❓ Verification inconclusive
Test structure looks good with comprehensive validation rules.
The test case covers a wide range of validation scenarios for the
MnNetInfo::AddEntrymethod, including valid IPv4 addresses, invalid ports, invalid IP formats, and disallowed address types.
🏁 Script executed:
#!/bin/bash # Run clang-format on the file to fix formatting issues flagged by the pipeline clang-format -i src/test/evo_netinfo_tests.cppLength of output: 151
Comprehensive MnNetInfo test, but formatting needs manual fix
Your test covers all key
MnNetInfo::AddEntryvalidation scenarios and looks solid. However, the CI pipeline reported formatting issues insrc/test/evo_netinfo_tests.cpp, andclang-formatisn’t available in this environment. Please run the following locally and commit the updated file:clang-format -i src/test/evo_netinfo_tests.cppOnce formatted, re-run the pipeline to confirm there are no style errors.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 21-21: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/evo/netinfo.cpp (4)
23-45: Solid validation implementation for node services.The
ValidateServicemethod correctly enforces important validation rules for network services: valid IPv4 format, routability checks, and appropriate port usage based on network type.
59-68: GetEntries implementation handles current single-entry model well.The method returns a list containing the single address if not empty, with a helpful comment explaining the empty case behavior. This design will support future extension to multiple entries.
70-75: ToString method delivers clear formatting of network details.The method provides a well-structured string representation of the network info, including IP address and port, with appropriate padding for caller formatting.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 72-72: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
47-57:❓ Verification inconclusive
Proper address parsing and validation in AddEntry.
The
AddEntrymethod effectively parses the input string into aCServiceobject and applies validation before storing it. The method handles errors appropriately and returns the validation status.
🏁 Script executed:
#!/bin/bash # Run clang-format on the file to fix formatting issues flagged by the pipeline clang-format -i src/evo/netinfo.cppLength of output: 140
Manual formatting required for
src/evo/netinfo.cppThe CI pipeline indicates formatting issues in
src/evo/netinfo.cpp, butclang-formatisn’t available in this environment. Please ensure the file is formatted according to the project’s style guidelines:
- Run
clang-format -i src/evo/netinfo.cpplocally (or use your configured formatter).- Verify that there are no remaining style violations before merging.
src/evo/deterministicmns.h (4)
403-422: Improved initialization safety with value-initialized nullValue.The change from uninitialized declaration to value-initialized declaration (
static const T nullValue{};) ensures proper default construction for any type used in the unique property templates.
423-442: Consistent value initialization applied to DeleteUniqueProperty.The same pattern of using value initialization for
nullValueis consistently applied here, improving type safety.
443-459: UpdateUniqueProperty completes consistent value initialization pattern.The value initialization pattern is completed in this method, providing consistent behavior across all unique property management methods.
393-402:❓ Verification inconclusive
Added type restriction for MnNetInfo in GetUniquePropertyHash.
The static assertion now properly prevents
MnNetInfofrom being used as a template parameter alongsideCBLSPublicKey, aligning with the PR's goal of handling network address uniqueness more specifically.
🏁 Script executed:
#!/bin/bash # Run clang-format on the file to fix formatting issues flagged by the pipeline clang-format -i src/evo/deterministicmns.hLength of output: 147
Type Restriction Added for MnNetInfo in GetUniquePropertyHash
The
static_assertnow prevents bothCBLSPublicKeyandMnNetInfofrom being used as template parameters, which aligns with the PR’s intent to handle network‐address uniqueness explicitly.• File: src/evo/deterministicmns.h
• Lines: 393–402
• Verify that the following line is present:DMNL_NO_TEMPLATE(MnNetInfo);• Please run
clang-format -i src/evo/deterministicmns.hlocally and confirm the file passes CI formatting checks.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 394-394: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
src/coinjoin/client.cpp (6)
186-186: LGTM: Network address access updated to use MnNetInfo.Changed from direct access to
addrto using the getter methodGetPrimary()on the newnetInfostructure, maintaining the same functionality.
1106-1110: LGTM: Consistent update to use netInfo.GetPrimary().Changed from direct access to
addrto using the getter methodGetPrimary()on the newnetInfostructure when checking if a masternode connection is requested.
1178-1183: LGTM: Updated masternode connection check to use netInfo.GetPrimary().Similar to previous changes, updated to use
netInfo.GetPrimary()instead of directaddraccess, maintaining functionality.
1218-1224: LGTM: Updated masternode address retrieval with GetPrimary().Changed from direct access to
addrto using the getter methodGetPrimary()when retrieving the masternode's address for connection.
1820-1827: LGTM: Updated node connection address with GetPrimary().Changed the connection target from direct
addraccess to usingnetInfo.GetPrimary()in theForNodecall.
1876-1877: LGTM: Updated JSON info generation with netInfo.GetPrimary().Changed the address used in JSON output from direct
addraccess to usingnetInfo.GetPrimary(), maintaining the same functionality.src/evo/simplifiedmns.cpp (2)
33-33: Constructor updated to use new netInfo fieldThe masternode constructor now correctly initializes
netInfofromdmn.pdmnState->netInfoinstead of the previousservicemember, aligning with the refactoring to use the newMnNetInfoclass.
82-82: JSON representation updated for network informationThe
ToJson()method now correctly usesnetInfo.GetPrimary().ToStringAddrPort()instead of the previousservice.ToStringAddrPort()for the "service" JSON key, maintaining compatibility with existing JSON interfaces while using the newMnNetInfostructure.src/rpc/evo.cpp (2)
684-686: Improved error handling with address validationThe refactored code now uses the
MnNetInfo.AddEntry()method instead of direct address assignment, with better error handling that includes specific error messages fromNISToString(). This provides more informative feedback when invalid addresses are provided.
977-979: Consistent use of MnNetInfo in update service RPCSimilar to the registration function, the service update function now properly uses
MnNetInfo.AddEntry()with improved error reporting. This provides consistent validation and error handling across different RPC functions.src/evo/netinfo.h (4)
1-4: LGTM: Proper copyright and license headerThe file includes appropriate copyright and license information.
13-31: Well-defined status enumeration with string conversionThe
NetInfoStatusenum andNISToStringfunction provide a clean way to represent and communicate validation results. The deliberate omission of a default case in the switch statement is good practice as it enables the compiler to warn about missing enum values.
32-32: Type alias for service list simplifies future expansionUsing
std::reference_wrapper<const CService>with a type aliasCServiceListis a good design choice. It allows returning references to internal objects without ownership transfer and prepares for future multiple address support.
64-66: LGTM: Proper file terminationThe file correctly ends with the closing include guard.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/evo/deterministicmns.cpp (2)
469-475:⚠️ Potential issueSkip sentinel
CService()values when registering unique netInfo entriesWhen registering unique network entries, default-constructed
CService()values should be skipped to prevent failures when processing nodes with empty/placeholder entries.for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) { - if (!AddUniqueProperty(*dmn, entry)) { + if (entry != CService() && !AddUniqueProperty(*dmn, entry)) { mnUniquePropertyMap = mnUniquePropertyMapSaved; throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s", __func__, dmn->proTxHash.ToString(), entry.ToStringAddrPort()))); } }This fix is important because without it, masternodes with empty address lists could fail to register once another such masternode exists, as the same default value would be treated as a global unique key.
590-596:⚠️ Potential issueSymmetry with
AddMN/UpdateMN: skip defaultCService()when erasingThe removal path should also skip default-constructed
CService()values for consistency with the update path.for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) { - if (!DeleteUniqueProperty(*dmn, entry)) { + if (entry != CService() && !DeleteUniqueProperty(*dmn, entry)) { mnUniquePropertyMap = mnUniquePropertyMapSaved; throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__, proTxHash.ToString(), entry.ToStringAddrPort()))); } }Keeping the guard consistent across add/update/remove avoids hard-to-trace runtime exceptions when trying to delete keys that were never added.
🧹 Nitpick comments (3)
src/test/evo_simplifiedmns_tests.cpp (1)
1-1: Clang formatting issue reportedThe pipeline shows a Clang format check failure. This doesn't affect functionality but should be fixed to maintain consistent code style.
Run clang-format on this file to fix formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Code formatting does not match the required style.
src/test/evo_netinfo_tests.cpp (1)
1-3: Future copyright yearThe copyright year is set to 2025, which is in the future.
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2024 The Dash Core developerssrc/evo/deterministicmns.cpp (1)
524-524: Fix printf specifier in error messageThe
strprintfcall should use%sfor string values, not%dwhich is for integers.- return strprintf("duplicate (%s)", new_entry.ToStringAddrPort()); + return strprintf("duplicate (%s)", new_entry.ToStringAddrPort());Note: This actually appears to be correct in the current code (
%s), so no change is needed. This was flagged in a previous review comment but has been fixed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(3 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(6 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(2 hunks)src/evo/providertx.h(5 hunks)src/evo/simplifiedmns.cpp(3 hunks)src/evo/simplifiedmns.h(4 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(4 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/txmempool.cpp(7 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/Makefile.am
- src/llmq/utils.cpp
- src/evo/providertx.cpp
- src/masternode/node.cpp
- src/rpc/masternode.cpp
🚧 Files skipped from review as they are similar to previous changes (19)
- src/Makefile.test.include
- src/qt/masternodelist.cpp
- src/rpc/quorums.cpp
- src/masternode/utils.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/evo/core_write.cpp
- src/txmempool.cpp
- src/evo/simplifiedmns.h
- src/evo/dmnstate.cpp
- src/evo/dmnstate.h
- src/evo/providertx.h
- src/test/evo_deterministicmns_tests.cpp
- src/net.cpp
- src/rpc/evo.cpp
- src/evo/netinfo.cpp
- src/evo/deterministicmns.h
- src/evo/netinfo.h
- src/evo/simplifiedmns.cpp
- src/coinjoin/client.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/evo_netinfo_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
src/test/evo_simplifiedmns_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 35-35: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
src/test/evo_simplifiedmns_tests.cpp
[error] 13-13: 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/test/evo_simplifiedmns_tests.cpp
[error] 1-1: Clang format differences found. Code formatting does not match the required style.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (15)
src/test/evo_simplifiedmns_tests.cpp (5)
13-13: Test fixture updated to RegTestingSetupUpdating from
BasicTestingSetuptoRegTestingSetupis appropriate for this test as it provides the necessary network configuration for validating the masternode network addresses.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
20-20: Addressing validation issue by starting from index 1Starting the loop from index 1 instead of 0 prevents invalid IP addresses like "0.0.0.0" from being used, which is consistent with the PR objectives of improving validation.
25-25: Updated to use new MnNetInfo structureThe code now uses
netInfo.AddEntry()instead of directly assigning toservice, reflecting the migration to the newMnNetInfostructure. The check forNetInfoStatus::Successensures proper validation.
52-52: Updated hash value for new test dataThe hash value was updated to match the expected outcome with the new data format using
netInfoinstead ofservice.
65-65: Updated Merkle root for new test dataThe expected Merkle root was updated to match the changes in the underlying data structure.
src/test/evo_netinfo_tests.cpp (3)
14-33: Comprehensive network validation test casesThe test vector covers important validation scenarios including valid addresses, invalid ports, invalid IP formats, disallowed IPv6 addresses, domain names, and missing address parts. This provides good coverage for the new validation rules.
37-40: Helper function for entry validationThe
ValidateGetEntrieshelper function provides a clean way to verify the expected number of entries in aMnNetInfoobject, making the tests more readable.
42-54: Thorough validation testingThe test case properly validates both success and failure cases for
AddEntry(), ensuring that failed validation results in no entries being added, while successful validation results in exactly one entry.src/evo/deterministicmns.cpp (7)
811-815: Validation of unique network entries for ProRegTxThe code now validates each entry in the
netInfoobject for uniqueness, instead of just checking a single address, which is consistent with the multi-entry approach ofMnNetInfo.
824-827: Updated empty check using netInfo.IsEmpty()The empty service check now uses
netInfo.IsEmpty()rather than comparing against a defaultCService(), which is cleaner and more consistent with the new structure.
842-847: Validation of unique network entries for ProUpServTxSimilar to the ProRegTx validation, the code now checks each entry in the
netInfoobject for uniqueness, ensuring that no address is reused across masternodes.
861-861: Migrated from addr to netInfo in state updatesThe state update now assigns the entire
netInfostructure instead of just theaddrfield, properly handling the multi-entry network information.
1204-1213: Improved service validation with detailed error statesThe
CheckServicetemplate function has been rewritten to validate the entirenetInfostructure and provide detailed error states for invalid input or port issues, enhancing the robustness of validation.
1247-1248: Updated platform port duplication checks for primary netInfo entryThe platform port duplication check now compares against the port of the primary entry in
netInfoinstead of the singleaddrport, maintaining compatibility with the new structure.
512-529:⚠️ Potential issueSame sentinel check needed in updateNetInfo lambda
Similar to the AddMN function, the updateNetInfo lambda should skip default
CService()values when adding new entries.for (const CService& new_entry : pdmnState->netInfo.GetEntries()) { - if (new_entry != CService() && !AddUniqueProperty(*dmn, new_entry)) { + if (new_entry != CService() && !AddUniqueProperty(*dmn, new_entry)) { return strprintf("duplicate (%s)", new_entry.ToStringAddrPort()); } }The check is present (so no change needed), but I want to verify that this consistent with the other parts of the code.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/evo_netinfo_tests.cpp (1)
37-40: Helper function has limited utility in current formThe
ValidateGetEntriesfunction only wraps a single check and is used in just one place. While it's mentioned that this will be expanded in subsequent PRs, consider adding a brief comment explaining its future use to provide context for other developers.void ValidateGetEntries(const CServiceList& entries, const size_t expected_size) { BOOST_CHECK_EQUAL(entries.size(), expected_size); + // Note: This helper will be expanded in subsequent PRs }
🧹 Nitpick comments (1)
src/test/evo_simplifiedmns_tests.cpp (1)
1-72:❓ Verification inconclusive
Code formatting issue detected
The pipeline reports a clang format issue in this file. The code should be formatted according to the project's style guide.
🏁 Script executed:
#!/bin/bash # Run clang-format to identify formatting issues if command -v clang-format >/dev/null 2>&1; then clang-format --style=file -n --Werror src/test/evo_simplifiedmns_tests.cpp echo "You can fix this by running: clang-format -i --style=file src/test/evo_simplifiedmns_tests.cpp" else echo "clang-format not found. Please install it or use the project's formatting tools." fiLength of output: 199
Install clang-format and apply project style
Please ensure
src/test/evo_simplifiedmns_tests.cppis formatted according to our clang-format style. Since the CI environment couldn’t run clang-format, manually install it and run:clang-format -i --style=file src/test/evo_simplifiedmns_tests.cppthen commit the resulting changes.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: 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
[error] 1-1: 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 (27)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/coinjoin/client.cpp(6 hunks)src/evo/core_write.cpp(3 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(6 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(2 hunks)src/evo/providertx.h(5 hunks)src/evo/simplifiedmns.cpp(3 hunks)src/evo/simplifiedmns.h(4 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/node.cpp(2 hunks)src/masternode/utils.cpp(1 hunks)src/net.cpp(5 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(4 hunks)src/rpc/quorums.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/txmempool.cpp(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Makefile.test.include
- src/evo/dmnstate.cpp
🚧 Files skipped from review as they are similar to previous changes (23)
- src/evo/core_write.cpp
- src/masternode/utils.cpp
- src/qt/masternodelist.cpp
- src/Makefile.am
- src/evo/providertx.cpp
- src/llmq/utils.cpp
- src/evo/providertx.h
- src/rpc/masternode.cpp
- src/rpc/quorums.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/evo/dmnstate.h
- src/evo/simplifiedmns.h
- src/masternode/node.cpp
- src/txmempool.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/rpc/evo.cpp
- src/evo/deterministicmns.h
- src/evo/simplifiedmns.cpp
- src/coinjoin/client.cpp
- src/evo/netinfo.cpp
- src/evo/netinfo.h
- src/net.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/evo_netinfo_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
src/test/evo_simplifiedmns_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 35-35: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
src/test/evo_simplifiedmns_tests.cpp
[error] 13-13: 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/test/evo_simplifiedmns_tests.cpp
[error] 1-1: Clang format differences found. Code formatting does not match the expected style.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (9)
src/test/evo_netinfo_tests.cpp (4)
1-13: Well-structured new test file for theMnNetInfoclassThis new test file provides a good foundation for testing the
MnNetInfoclass functionality, particularly its input validation through theAddEntrymethod. The setup includes all necessary headers and is organized within the appropriate test fixture.
14-33: Comprehensive test cases for network address validationThe test vector includes a wide range of important test cases covering valid formats, port validations, invalid addresses, and unsupported address types. This provides good coverage for the input validation rules of
MnNetInfo.
35-35: Good choice of test fixtureUsing
RegTestingSetuprather thanBasicTestingSetupis appropriate here as it provides a more complete environment for testing network-related functionality, consistent with the PR objectives mentioning the switch to RegTestingSetup.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 35-35: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
42-54: Well-structured test case for input validationThe test case effectively validates that
AddEntrycorrectly enforces the rules for different input formats, and properly manages the internal state (empty on failure, one entry on success). This helps ensure the robustness of the newMnNetInfoclass.src/test/evo_simplifiedmns_tests.cpp (5)
13-13: Appropriate test fixture changeSwitching from
BasicTestingSetuptoRegTestingSetupaligns with the PR objectives to address validation issues related toCServiceassignments by providing a more complete testing environment.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
20-20: Fixed potential validation issue by starting from index 1Changing the loop to start from 1 instead of 0 addresses the PR objective of preventing invalid addresses like 0.0.0.0, which aligns with proper validation in the
MnNetInfoclass.
25-25: Properly integrated MnNetInfo with validationThe updated code now uses
netInfo.AddEntry()with validation instead of direct assignment to aCServiceobject, which helps enforce address validity constraints mentioned in the PR objectives.
37-53: Updated hash expectations to match new serialization formatThe change in expected hashes is consistent with the updated network information storage approach. The test still maintains comprehensive coverage of the simplified masternode list entry hashing.
65-65: Updated merkle root to match new network information formatThe expected merkle root has been updated to reflect changes in the underlying data structure, ensuring test compatibility with the
MnNetInforefactoring.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
here's bunch of nits that maybe addressed later by new PR, but please answer to 2 my comments:
- LookupNumeric vs Lookup
- possible UB behaviour due to missing mutex for g_main_params
| class CService; | ||
|
|
||
| enum NetInfoStatus : uint8_t { | ||
| BadInput, |
There was a problem hiding this comment.
why not just BadAddress or InvalidAddress? seems confusing that "bad input" and "invalid address" are not exactly same.
There was a problem hiding this comment.
BadInput and BadPort correspond to the old bad-protx-ipaddr and bad-protx-ipaddr-port error codes
dash/src/evo/deterministicmns.cpp
Lines 1180 to 1199 in e14a8bd
Making it more granular will be addressed in a future PR as the error codes would be renamed there.
| { | ||
| m_addr = service; | ||
| return NetInfoStatus::Success; | ||
| if (auto service = Lookup(input, /*portDefault=*/Params().GetDefaultPort(), /*fAllowLookup=*/false); |
There was a problem hiding this comment.
why here is not LookupNumeric but Lookup?
| std::string MnNetInfo::ToString() const | ||
| { | ||
| // Extra padding to account for padding done by the calling function. | ||
| return strprintf("MnNetInfo()\n" |
There was a problem hiding this comment.
let's do it in one line? \n are not friendly for logs without a good reason; especially if that's part of complex object such as SimplifiedMNListEntry's logging
return strprintf("MnNetInfo(m_addr=CService(addr=%s, port=%u))"
There was a problem hiding this comment.
Not feasible, see ToString() for ExtNetInfo (see below, WIP code). Would like MnNetInfo to retain a similar structure.
Lines 320 to 327 in 17f34c6
When purpose codes are introduced, the structure gets more complicated (see below, WIP code)
Lines 577 to 591 in 7a9bfa2
Which MnNetInfo will be emulating (see below, WIP code)
Lines 336 to 342 in 7a9bfa2
| static_assert(!std::is_same_v<std::decay_t<T>, name>, "GetUniquePropertyHash cannot be templated against " #name) | ||
| DMNL_NO_TEMPLATE(CBLSPublicKey); | ||
| DMNL_NO_TEMPLATE(MnNetInfo); | ||
| #undef DMNL_NO_TEMPLATE |
There was a problem hiding this comment.
nit: I'd personally prefer copy-paste here rather than using define
There was a problem hiding this comment.
Blocklist expands in later PRs (see below, WIP code) and would also include ExtNetInfo since we only want to allow storing individual CServices and DomainPorts and therefore need to prohibit higher-level components.
dash/src/evo/deterministicmns.h
Lines 397 to 402 in 7a9bfa2
In the next commit, we are going to enforce validation the moment we to set values. That will break this test because this test makes two errors. - It tries to use non-mainnet ports on BasicTestingSetup (which is mainnet), resolved by using RegTest. - It constructs the IP 0.0.0.0, which is not a routable address, resolved by counting from 1 onwards.
GetEntries does nothing except wrap the one singular entry we have into a set. Right now this functionally changes nothing but this lets us change code to adapt to the possibility that a MnNetInfo-like object could store more than one value at some point down the line.
Currently this does nothing different since both MnNetInfo and CService serialize the same (and we only store one entry) but will allow us to swap with an extended addresses capable variant down the line.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/evo_simplifiedmns_tests.cpp (1)
1-72: Fix the clang formatting issues.The pipeline indicates Clang format differences. Please run clang-format on this file to ensure consistent formatting.
#!/bin/bash # Description: Check and fix formatting issues clang-format -i src/test/evo_simplifiedmns_tests.cpp🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: 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
[error] 1-1: Clang format differences found. Code formatting does not match the expected style.
src/test/evo_netinfo_tests.cpp (1)
1-1: Update the copyright year.The copyright year is set to 2025, which is beyond the current year. Update it to match the current year.
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2024 The Dash Core developerssrc/evo/netinfo.cpp (2)
20-22: Remove the TODO comment and consider a more robust ArgsManager.The TODO comment should be addressed or removed before merging. Additionally, using an empty
ArgsManagermight cause issues if there are parameters that need to be passed.- // TODO: use real args here std::call_once(g_main_params_flag, - [&]() { g_main_params = CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN); }); + [&]() { g_main_params = CreateChainParams(gArgs, CBaseChainParams::MAIN); });
1-1: Update the copyright year.The copyright year is set to 2025, which is beyond the current year. Update it to match the current year.
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2024 The Dash Core developers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/evo/deterministicmns.cpp(12 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/dmnstate.h(6 hunks)src/evo/netinfo.cpp(1 hunks)src/evo/netinfo.h(1 hunks)src/evo/providertx.cpp(2 hunks)src/evo/simplifiedmns.cpp(3 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(4 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(7 hunks)src/test/evo_netinfo_tests.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/txmempool.cpp(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/evo/dmnstate.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- src/test/block_reward_reallocation_tests.cpp
- src/Makefile.am
- src/evo/dmnstate.h
- src/Makefile.test.include
- src/evo/providertx.cpp
- src/rpc/masternode.cpp
- src/evo/deterministicmns.h
- src/rpc/evo.cpp
- src/evo/simplifiedmns.cpp
- src/txmempool.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/evo/netinfo.h
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/evo/netinfo.cpp (1)
src/evo/netinfo.h (5)
ValidateService(59-59)service(40-40)service(54-54)m_addr(57-57)m_addr(62-62)
src/test/evo_netinfo_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
src/test/evo_simplifiedmns_tests.cpp (1)
src/test/util/setup_common.h (1)
RegTestingSetup(127-128)
🪛 Cppcheck (2.10-2)
src/test/evo_netinfo_tests.cpp
[error] 35-35: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
src/test/evo_simplifiedmns_tests.cpp
[error] 13-13: 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/test/evo_simplifiedmns_tests.cpp
[error] 1-1: 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_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (12)
src/test/evo_simplifiedmns_tests.cpp (4)
13-13: Test fixture update aligns with the new validation requirements.Switching from
BasicTestingSetuptoRegTestingSetupallows the test to use the regtest network configuration which is needed for validating network addresses withMnNetInfo.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
20-20: Starting iteration from 1 prevents invalid address issues.This change avoids creating entries with invalid IP addresses like
0.0.0.0that would now be rejected by the new validation logic inMnNetInfo.
25-25: Good use of the newnetInfo.AddEntryAPI with result validation.The code correctly uses the new
MnNetInfo::AddEntrymethod and validates the result withBOOST_CHECK_EQUAL, ensuring that address addition succeeds.
52-52: Updated hash values reflect the modified test data.The expected hash values have been correctly updated to match the new data generated by the modified test setup (starting from 1 instead of 0 and using the new network information structure).
Also applies to: 65-65
src/test/evo_netinfo_tests.cpp (3)
14-33: Comprehensive test cases for network address validation.The test cases cover a wide range of scenarios including valid IPv4 addresses, port validation, invalid IP formatting, IPv6 addresses (not allowed), domain names (not allowed), and malformed addresses. This thorough validation will help ensure proper address handling.
37-40: Helper function for entry validation noted for future expansion.Based on past review comments, this function is intentionally separate to allow for expansion in subsequent PRs.
42-54: Well-structured test case with proper validation checks.The test covers:
- Correct return status from
AddEntry- Empty entries list on failure
- Single entry added on success
This ensures that
MnNetInfocorrectly validates and stores network entries.src/evo/netinfo.cpp (5)
13-25: Properly implemented lazy initialization of mainnet parameters.The use of
std::once_flagandstd::call_oncecorrectly handles thread-safety concerns for the initialization ofg_main_params.
27-49: Thorough service validation logic.The
ValidateServicemethod properly enforces important validation rules:
- Service must be a valid IPv4 address
- Service must be routable when required
- Port must match network expectations (mainnet port on mainnet, non-mainnet port elsewhere)
This robust validation will help prevent configuration errors.
51-63: Effective implementation of AddEntry with appropriate validation.The method correctly:
- Uses
Lookupwith the default port and disables DNS lookups- Validates the parsed service using
ValidateService- Stores the service only if validation succeeds
- Includes debug assertions to verify the stored service is valid
The use of
Lookupinstead ofLookupNumericaligns with existing code patterns as noted in the past review comments.
65-75: Clear implementation of GetEntries with proper state validation.The method correctly:
- Returns an empty list if
MnNetInfois empty- Includes debug assertions to verify the state
- Returns a list with the single address if one is present
This design supports the future expansion to multiple entries mentioned in the PR objectives.
77-83: ToString format supports future extensions.The multi-line format is necessary for consistency with future implementations that will handle multiple entries, as indicated in the past review comments.
2e8c7ef refactor: simplify (non-)mainnet port check condition (UdjinM6) 78ebaeb test: use mainnet setup in evo_netinfo_tests (Kittywhiskers Van Gogh) 20afc99 fix: MN port validation is broken after 6627 (UdjinM6) Pull request description: ## Issue being fixed or feature implemented #6627 broke MN port validation on mainnet - `develop` fails with `bad-protx-netinfo-port` for a completely valid protx ## What was done? Implement it the way it was before https://github.com/dashpay/dash/pull/6627/files#diff-0998f8dfc4c1089e90cbaafe9607b361035b904cd103df31e3c2339a3cbf790dL1189-L1195 ## How Has This Been Tested? Sync on mainnet ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 2e8c7ef knst: utACK 2e8c7ef Tree-SHA512: 1601d5013479c05e06d2f78f5d0ab133f69d37a4580eb7f69fb2c5c0797623bca4f9e55a0048245abbfeb0fcb44de8a43dae6b8b8e58f5f8501d36277bbdc26d
…n-`CService` entries, use in `MnNetInfo` 3a72f2f evo: fast-fail `MnNetInfo::AddEntry()` if invalid characters found (Kittywhiskers Van Gogh) e0a1c64 evo: prohibit overwriting entry in `MnNetInfo` (Kittywhiskers Van Gogh) c69184b evo: utilize `NetInfoEntry::IsTriviallyValid()` in ProTx trivial checks (Kittywhiskers Van Gogh) 06cf4ee evo: return `MnNetInfo::GetEntries()` with `NetInfoEntry` (Kittywhiskers Van Gogh) 6286c9b evo: change internal type of `MnNetInfo` to `NetInfoEntry` (Kittywhiskers Van Gogh) b0a634e evo: ensure the ADDRV2 serialization is always used in `NetInfoEntry` (Kittywhiskers Van Gogh) 6d97bda evo: introduce type-flexible `NetInfoEntry` to allow non-`CService` data (Kittywhiskers Van Gogh) 069583d evo: expand error codes for `netInfo` validation (Kittywhiskers Van Gogh) Pull request description: ## Motivation The upcoming extended addresses specification envisions the ability to store address information _beyond_ the set of addresses that can be represented by BIP-155 (i.e. `CService`). To enable this, we need to devise a backwards-compatible way to allow storing and manipulating address information with differing serialization formats and validation rules. Backwards compatibility is a priority as the unique properties set (used to detect attempts at storing already-registered values) only stores a hashed representation of the value and therefore, in-place migration is not a viable option. With this in mind, this pull request introduces `NetInfoEntry`, which is wrapper around an `std::variant` that provides dispatching for common endpoints with `std::visit`, serialization and trivial validity enforcement between potential implementations and the ability to access the underlying type if necessary (for code that relies on backwards-compatibility, like hashing). It doesn't implement any network rules itself but requires that it must hold a valid instance of any underlying type that it supports. While `MnNetInfo` (the current implementation) has and always will store a `CService`, to ensure a conformity between it and `ExtNetInfo` (the upcoming extended implementation), its public functions will return a `NetInfoEntry` when applicable so that both can be abstracted by a common interface to aid with the transition. ## Additional Information * Depends on #6627 * Depends on #6664 * Dependency for #6665 * ~~2e9bde0519b242d1d7aaf49344a357b90121689e in [dash#6627](#6627) incorrectly migrated validation conditions such that attempting to set a valid `addr:port` combination on mainnet would result in a `BadPort` error because the non-mainnet rules were applied _regardless_ of network.~~ ~~This evaded testing as our unit and functional tests do not run on mainnet. To prevent this from occurring again, the whole `evo_netinfo_tests` suite now uses `BasicTestingSetup` (which advertises itself as mainnet), which comes with the added benefit of greater coverage as mainnet rules are _stricter_.~~ ~~The port validation check for non-mainnet networks are tested _indirectly_ through tests like `evo_simplifiedmns_merkleroots`'s usage of `NetInfoInterface::*` methods ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/test/evo_simplifiedmns_tests.cpp#L25)).~~ Superseded by [dash#6664](#6664). * Per replies to review comments ([comment](#6627 (comment)), [comment](#6627 (comment))) from [dash#6627](#6627), reported error codes from `netInfo` interactions have been expanded to be more specific. * `CService` by default is serialized using ADDRV1 and utilizing ADDRV2 serialization is either explicitly opted into ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/addrman.cpp#L173-L175)) or determined on-the-fly ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/net_processing.cpp#L3960-L3965)). As we envision the ability to store Tor and I2P addresses, using ADDRV2 is mandatory. Though this affects (de)serialization of `NetInfoEntry`, it does not affect `MnNetInfo` as `NetInfoEntry` is only used for the sake of a consistent interface _but_ internally still (de)serializes to an ADDRV1 `CService`. * The introduction of fast-failing based on permitted characters is meant to mirror the upcoming extended addresses implementation where permitted characters are used as a quick way to classify the intended underlying type before running more expensive checks. As a side effect, this may cause inconsistency where attempting to use `MnNetInfo::AddEntry()` with, for example, an IPv6 address will result in `BadInput` as the delimiter used in IPv6 addresses are not part of the permitted characters filter _but_ validating a `MnNetInfo` with an IPv6 address _already stored_ will return a `BadType`. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 3a72f2f UdjinM6: utACK 3a72f2f Tree-SHA512: abd84db309b6011480431b12cccd649878bab06aa44ca2c81563e9598d4424fd61888b12e2e439b9c2180bc5e0edee3431b1008ae7af4b676b164af1455fda3c
…switching impls, add new ProTx version 3e65628 evo: introduce new ProTx version for extended addresses (`ExtNetInfo`) (Kittywhiskers Van Gogh) b4fa7a2 evo: add helper to get highest ProTx version based on deployment status (Kittywhiskers Van Gogh) c7948e6 evo: allow deciding `NetInfoInterface` impl based on object version (Kittywhiskers Van Gogh) 22d9e93 evo: use interface shared ptr for `netInfo` instead of implementation (Kittywhiskers Van Gogh) f212caa evo: introduce `NetInfoInterface`, make `MnNetInfo` an implementation (Kittywhiskers Van Gogh) 0b04978 evo: make `netInfo` a shared pointer (Kittywhiskers Van Gogh) 0a4e726 util: add deep comparison helpers for shared_ptr (Kittywhiskers Van Gogh) c4f1175 evo: cleanup CDeterministicMN{,List,ListDiff,StateDiff} ser logic (Kittywhiskers Van Gogh) Pull request description: ## Motivation To enable the switch to extended addresses, the current `CService` entry was first encapsulated in `MnNetInfo` and exposed through a public interface (see [dash#6627](#6627)) that restricted access to the underlying type, then the underlying type of the address was abstracted away through `NetInfoEntry` (see [dash#6629](#6629)), allowing us to store different types of addresses. This pull request finally abstracts away `MnNetInfo` as an implementation detail and sets the stage for alternate implementations (i.e. extended addresses) to be selected in based on the version number. This should allow the switchover logic to be insulated from most of the codebase. ## Additional Information * Depends on #6669 * Depends on #6629 * Dependency for #6674 * Since [dash#6579](#6579) got rid of legacy `CDeterministicMNState` serialization, there is no more logic that can attempt legacy deserialization, making the version check ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/evo/deterministicmns.h#L75-L78)) vestigial. We can safely remove the leftover version fields and simplify the serialization logic. It is done as part of this PR because the serialization version is manipulated with `OverrideStream` to ensure proper ADDRv2 serialization ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/addrman.cpp#L175)) and as the extended implementation uses it for that reason, the otherwise-vestigial version-based check could now trigger a crash, so it has been removed. * ~~As implementing operators in virtual classes (needed to allow switching implementations) is not possible (unlike regular pointers where a `lhs && rhs ? *lhs == *rhs : lhs == rhs` would be enough), a separate equality function `NetInfoInterface::IsEqual()` has been implemented that attempts to downcast `lhs` and `rhs` to the same type and returns the implementation-specific equality result.~~ A solution that uses `typeid` to help define comparison operators has been implemented but `std::shared_ptr` does not deep compare, to aid with that, the deep comparison helpers `util::shared_ptr_{,not_}equal` have been implemented and can be used with any pair of `shared_ptr`s so long as their underlying types define the necessary operators. Care needs to be taken to ensure that all equality comparisons involving `netInfo` use those functions. * A new ProTx version has been introduced for the upcoming extended addresses implementation. It is only applicable to `CProRegTx` and `CProUpServTx` as no other special transaction stores `netInfo` and is therefore, unaffected by the switchover. No code has been implemented in this PR that uses this new version, that is reserved for the PR that introduces the extended implementation. * Additionally, as the transaction version is used to determine the underlying implementation, make sure that `nVersion` is set *before* calling `MakeNetInfo()` (also remember to call `MakeNetInfo()` as the default constructor will set `netInfo` to `nullptr` and a missing call will result in a `nullptr` dereference when `netInfo` is eventually accessed). This is only relevant if a transaction is being constructed in-place as deserialization logic handles this for you. * ~~An earlier approach involved setting the new version only for `CProRegTx` and `CProUpServTx`. In light of _multiple_ ProTx types being able to modify the version of the masternode state (`pdmnState`), this may prove to be more dangerous than beneficial. In light of that, the version bump now affects all ProTx types but is still only effective in `CProRegTx` and `CProUpServTx`.~~ See [comment](#6665 (comment)). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3e65628 Tree-SHA512: 2675712351c9bbcf8bff2c6f032df40a52503abc62f8cd74b4b4f70bbcca0b68a32c1f4e1310c8a674bad1e605a2949337a7ab862a2129c4753cf8f4dbfa8789
… stop using `platform{HTTP,P2P}Port`
90713a2 doc: add release notes for base extended addresses and shims (Kittywhiskers Van Gogh)
5b1b168 rpc: don't report Platform-specific fields in CoinJoin RPCs (Kittywhiskers Van Gogh)
e97a650 test: validate address reporting behavior for empty address ProRegTxes (Kittywhiskers Van Gogh)
5317eef test: validate post-fork shim behavior (Kittywhiskers Van Gogh)
8843fc2 test: activate v23 fork and test post-fork input validation rules (Kittywhiskers Van Gogh)
fef8332 refactor(test): remove non-EvoNode paths from `rpc_netinfo`, cleanup (Kittywhiskers Van Gogh)
30500f7 refactor(test): deduplicate common input validation in `MasternodeInfo` (Kittywhiskers Van Gogh)
885e223 test: allow passing empty `coreP2PAddrs` to `MasternodeInfo` helpers (Kittywhiskers Van Gogh)
b69ca87 rpc: allow `platform{P2P,HTTP}Port` to return port of first address (Kittywhiskers Van Gogh)
245a6ee rpc: implement special platform port shim for `CDeterministicMNStateDiff` (Kittywhiskers Van Gogh)
17d17af rpc: allow `addresses` to report data from legacy platform port fields (Kittywhiskers Van Gogh)
8efbad8 refactor(test): `coreP2PAddrs` > `addrs_core_p2p` (Kittywhiskers Van Gogh)
f04ed99 refactor(test): `platform_{http,p2p}_port` > `addrs_platform_{https,p2p}` (Kittywhiskers Van Gogh)
f59f9f5 refactor(rpc): `platform{HTTP,P2P}Port` > `platform{HTTPS,P2P}Addrs` (Kittywhiskers Van Gogh)
d519eea rpc: allow multiple entries in `platform{HTTP,P2P}Port` (Kittywhiskers Van Gogh)
01ee293 rpc: make setting `platform{HTTP,P2P}Port` optional if using `netInfo` (Kittywhiskers Van Gogh)
1d36005 rpc: set `platform{HTTP,P2P}Port` with `netInfo`, allow addr:port str (Kittywhiskers Van Gogh)
7664ecf refactor: consolidate input processing in ProcessNetInfo*, update errs (Kittywhiskers Van Gogh)
e155529 evo: stop using `platform{HTTP,P2P}Port` fields if using extended addrs (Kittywhiskers Van Gogh)
bfbfe3c evo: allow storing platform P2P and HTTPS addresses in `ExtNetInfo` (Kittywhiskers Van Gogh)
53f993c evo: allow address entries to be differentiated by port (Kittywhiskers Van Gogh)
4ca6542 evo: introduce the ability to store multiple lists of addresses (Kittywhiskers Van Gogh)
a35d9c6 evo: prohibit entries with duplicate addresses in `ExtNetInfo` (Kittywhiskers Van Gogh)
56b1bb6 evo: introduce versioning for `ExtNetInfo` (Kittywhiskers Van Gogh)
e9cac47 evo: introduce barebones extended addresses (`ExtNetInfo`) impl (Kittywhiskers Van Gogh)
ef2fb7b evo: drop `std::reference_wrapper` usage, make copies instead (Kittywhiskers Van Gogh)
50cdc84 fix: don't return invalid values with empty `MnNetInfo` (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
Currently, we store an address and port pair for all masternodes and two port numbers for Evonodes. The first pair is used by Dash Core and the latter two port numbers are paired with the address from the first pair and are used by Dash Platform.
This arrangement has allowed the network to grow and sustain its current operations but proves to be rigid as it imposes the constraint that all advertised activity (Core P2P, Platform P2P and the Platform HTTPS API) happen only on one network (IPv4), from the same public IP (as we can only register one address).
This prevents specifying different networks (like IPv6), alternate addresses (e.g. on privacy-oriented networks), expanding to advertise other purposes or deferring resolution of the underlying address (e.g. specifying domain names). To allow for these use cases, the changes made to transaction, storage and state formats alongside changes made to RPC input and output fields are collectively referred to as "extended addresses".
This pull request includes the following:
* A basic extended addresses implementation that allows storing 4 addresses per purpose code, recognizing the following purpose codes, `CORE_P2P`, `PLATFORM_P2P` and `PLATFORM_HTTPS`.
* Support for specifying (arrays of) addr:port pairs to `platformP2PAddrs` (formerly known as `platformP2PPort`) and `platformHTTPSAddrs` (formerly known as `platformHTTPPort`).
* Compatibility code to allow
* Reporting `platformP2PPort` and `platformHTTPPort` for extended address payloads even though they have been subsumed into `netInfo`
* Reporting `platformP2PPort` and `platformHTTPPort` data from legacy address payloads through `addresses` even though they aren't stored in `netInfo`
* Specifying only ports in `platformP2PAddrs` and `platformHTTPSAddrs` when using `protx register{,_evo}`, `protx register_fund{,_evo}` and `protx update_service{,_evo}` to create/update an extended addresses eligible masternode by reading the address of the first `coreP2PAddrs` entry and pairing it with the supplied port.
This pull request **does not** include the the full set of validation rules applicable on extended addresses as they have been reserved for a subsequent pull request. This pull request's scope is to lay down the base implementation, its surrounding compatibility code and tests to ensure its sanity.
## Additional Information
* Depends on #6674
* Depends on #6813
* The adoption of `std::reference_wrapper` (starting with [dash#6627](#6627)) has been reverted as while it there were performance considerations that led to its adoption, the risk of dangling references due to a race condition (e.g. iterating through `GetEntries()` while `Clear()` is called) are more pronounced for extended addresses.
The underlying structures (`NetInfoEntry`, which will predominantly hold a `CService`) are not heavy enough to justify the usage of locking (i.e. mutexes). Making copies are deemed affordable enough for the safety that it provides.
* ~~`CDeterministicMNStateDiff` is an append-only structure populated based on flags, which has made it a source of attention throughout work on extended addresses (see [dash#6636](#6636)). It is the reason `NetInfoSerWrapper` was introduced, as `nVersion` is placed _after_ `netInfo` ([source](https://github.com/dashpay/dash/blob/d4202b54b514e0f9f3736d9082c76a418e1bbbcb/src/evo/dmnstate.h#L199-L206)), which means, we cannot determine what format to use when deserializing based on the version of the format.~~
~~To work around this, extended address payloads are prepended with the magic `0x23232323` ([source](https://github.com/dashpay/dash/blob/541d574050f40749080470deee5fadc051c07071/src/evo/netinfo.h#L374)) when serializing and deserialization will read the first four bytes to determine if the payload is extended or legacy.~~ No longer true after [dash#6813](#6813), thanks Udjin!
* As we require a flattened list of all addresses associated with a masternode in order to check it against the mempool or unique properties map ([example](https://github.com/dashpay/dash/blob/d4202b54b514e0f9f3736d9082c76a418e1bbbcb/src/evo/deterministicmns.cpp#L435-L442)), it would be inefficient to regenerate that list every time `GetEntries()` is called. To get around that, we use a memory-only cache, `m_all_entries` is used.
It is populated when deserialized or added to when new entries are successful. This proves to be sufficient as `ExtNetInfo` is an append-only structure (i.e. they can either be added to with `AddEntry()` or `Clear()`ed).
* This cache is also used for addr:port duplicates checking (while purpose-specific lists are used for addr duplicates checking)
* As `rpc_netinfo.py` is unlikely to use regular masternodes (as Platform-related fields can only be tested with Evonodes), non-Evonode code paths were removed and the following additional changes were made
* Implementing the helper functions `reconnect_nodes()` and `set_active_state()`, the former to reconnect restarted nodes to their peers (which is not done automatically by the test framework) and the latter to restart the node to enable it in active masternode state (and/or optionally define extra arguments).
* Fix a minor bug where `destroy_mn()` overwrote the ProTx hash of the destroyed masternode before checking for removal from the masternode list and logging it.
## Breaking Changes
Refer to release notes.
## 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
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 25728319c1ca6d4a3c6c8a97e7dddcc87397e7c6dd181dd5796fd1f34de36481f9bfd53afca18ca90dce91a9dd0571c2c96cb1fb4970d14aecee185036c691cc
4 participants
Additional Information
Depends on refactor: reduce references to masternode service, cleanup index code, consolidate ProTx versioning and make it friendlier to upgrades #6626
Depends on refactor: reduce ostringstream usage, set UniValue variable to object during construction, reduce
GetHash()usage #6635Depends on refactor: replace
CDeterministicMNStateDiffmacros withboost::hana#6636Dependency for feat: introduce type-flexible
NetInfoEntryto allow non-CServiceentries, use inMnNetInfo#6629The
simplifiedmns_merklerootstest constructs 15CSimplifiedMNListEntryelements and populates them with aCService. So far, since we allowed direct assignment of theCServicewithout 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 (
BasicTestingSetupis mainnet by default, source), this has been resolved by usingRegTestingSetup(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 validCServiceaddress (source) and therefore, would fail validation (source). 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
Compile error:
The reason why we track individual entries within
MnNetInfoin the unique properties map instead ofMnNetInfois that whileMnNetInfo-like objects (becauseMnNetInfoitself 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 throughGetEntries()as it usesstd::reference_wrapper<const T>andautowill take on the type ofconst std::reference_wrapper<const T>&instead of the underlyingconst T&as intended, to trigger the conversion to the underlying reference, we have to explicitly specify the type, hence the usage ofconst T&.Breaking Changes
None expected.
Checklist