feat: add support for internet domains, CJDNS, I2P and Tor to extended addresses, permit registering internet domains for Platform HTTPS API #6809
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Tip For best results, initiate chat on the files or code changes. @kwvg, I'm here to help! How can I assist you with this PR? Would you like me to:
Just let me know what you need! 🐰 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds DomainPort support across masternode netinfo handling. NetInfoEntry now holds CService or DomainPort; DomainPort includes validation, normalization, and serialization. Introduces ExtNetInfo with domain-aware parsing, validation, duplicate detection, JSON/string output, and processing logic. Deterministic MN operations (add/update/remove), unique-property hashing, and netinfo duplicate checks were extended to include domain:port alongside addr:port. Tests and functional RPC tests were expanded and refactored to cover domain, privacy-network, and extended-address behaviors. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-05-10T00:54:15.597ZApplied to files:
🧬 Code graph analysis (4)src/evo/netinfo.h (1)
src/test/evo_netinfo_tests.cpp (2)
test/functional/rpc_netinfo.py (1)
src/evo/netinfo.cpp (3)
🪛 GitHub Actions: Clang Diff Format Checksrc/evo/netinfo.h[error] 1-1: Clang format differences found in src/evo/netinfo.h. Run the provided clang-format-diff script or clang-format to fix formatting. src/test/evo_netinfo_tests.cpp[error] 1-1: Clang format differences found in src/test/evo_netinfo_tests.cpp. Run the provided clang-format-diff script or clang-format to fix formatting. src/evo/netinfo.cpp[error] 1-1: Clang format differences found in src/evo/netinfo.cpp. Run the provided clang-format-diff script or clang-format to fix formatting. ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (18)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (8)
src/txmempool.cpp (1)
1382-1387: Avoid false-positive conflicts for ProUpServ when the same MN already advertises the addressWhen checking conflicts for TRANSACTION_PROVIDER_UPDATE_SERVICE, you compare the stored value against
proTxHash. SincemapProTxAddressesstores a txid (either ProRegTx or ProUpServ tx), this correctly allows reuse if the mapping came from the MN’s ProRegTx (equal toproTxHash) but falsely flags when the mapping came from the MN’s prior ProUpServ tx (txid !=proTxHash). This erroneously turns a “same MN, existing update in mempool” into an address conflict.Minimally, also allow the case where the stored txid equals the current tx’s
tx_hash, preventing self-conflicts and bringing consistency withremoveProTxConflicts.- for (const auto& entry : opt_proTx->netInfo->GetEntries()) { - auto it = mapProTxAddresses.find(entry); - if (it != mapProTxAddresses.end() && it->second != opt_proTx->proTxHash) { - return true; - } - } + for (const auto& entry : opt_proTx->netInfo->GetEntries()) { + auto it = mapProTxAddresses.find(entry); + // Conflict only if the entry belongs to a different tx and a different MN + if (it != mapProTxAddresses.end() && it->second != tx_hash && it->second != opt_proTx->proTxHash) { + return true; + } + }Alternative (more explicit): resolve whether
it->secondbelongs to the same MN usingmapProTxRefs.equal_range(opt_proTx->proTxHash)and skip conflicts if it matches one of those txids.contrib/seeds/makeseeds.py (1)
167-174: Handle domains and None parsing results to avoid crashes; keep legacy fallbackWith extended addresses,
core_p2pcan be a domain.parseip()returns None for domains, and the current list comprehension will produce a list with None entries, causing later subscripts likeip['net']to throw. Also, retain legacy compatibility (state.service) for pre-extended-address payloads.- # Extract IPs - ips = [parseip(mn['state']['addresses']['core_p2p'][0]) for mn in mns] + # Extract IPs (skip domains) and keep legacy fallback + ips = [] + for mn in mns: + addr = None + try: + # Extended addresses format + addrs = mn['state'].get('addresses', {}) + core = addrs.get('core_p2p') or [] + if core: + addr = core[0] + except Exception: + addr = None + # Legacy fallback + if not addr: + addr = mn['state'].get('service') + if not addr: + continue + parsed = parseip(addr) + if parsed is not None: + ips.append(parsed) for onion in onions: parsed = parseip(onion) if parsed is not None: ips.append(parsed)Optional: add domain resolution support (A/AAAA) behind a flag; for now, skipping non-IP
core_p2pkeeps the script robust.src/evo/simplifiedmns.h (1)
65-95: Reformatsrc/evo/simplifiedmns.hwith clang-formatCI reports formatting differences in the region around lines 65–95. Please run the project’s clang-format tooling to restore compliance and unblock CI:
- File:
src/evo/simplifiedmns.h(around lines 65–95)- From the repo root, for example:
git clang-format HEAD~1 -- src/evo/simplifiedmns.h- or
./contrib/devtools/clang-format.sh src/evo/simplifiedmns.hAfter formatting, commit the changes so that the CI formatting check passes.
src/evo/providertx.h (1)
87-130: Run clang-format on src/evo/providertx.h to satisfy CICI is failing due to formatting diffs in the serialization block (lines 87–130). Please apply the project’s clang-format rules:
- Execute:
clang-format -i src/evo/providertx.h- Verify that indentation, line breaks, and parentheses align with the surrounding code style.
src/evo/dmnstate.h (1)
95-111: Run clang-format onsrc/evo/dmnstate.hto fix formattingWe’ve confirmed that the use of
NetInfoSerWrapper<decltype(obj)>matches other locations in the codebase (e.g. insimplifiedmns.h,providertx.h), so the serialization logic is consistent. The only outstanding issue is code style:• File: src/evo/dmnstate.h
• Lines: 95–111 (insideREADWRITEblock)Please run
clang-format(or your project’s formatting script) over this file to align with the project’s style rules.src/rpc/masternode.cpp (1)
6-30: Fix code formatting in the includes section.The Clang format check indicates formatting issues in the includes section. Please run clang-format to ensure consistent code style.
src/evo/deterministicmns.cpp (1)
923-965: CheckPlatformFields: avoid constructing fresh main params; use current Params() and correct default-port comparisonsTwo issues:
- Creating mainnet params via CreateChainParams per call is unnecessary and expensive; use Params() for the active network.
- The “not equal to default core P2P port” checks currently compare against mainnet’s default P2P port regardless of active network. Compare to the current network’s default core port instead.
Apply this refactor:
- // TODO: use real args here - const auto main_params{CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN)}; - if (Params().NetworkIDString() == CBaseChainParams::MAIN) { - if (proTx.platformP2PPort != main_params->GetDefaultPlatformP2PPort()) { + if (Params().NetworkIDString() == CBaseChainParams::MAIN) { + if (proTx.platformP2PPort != Params().GetDefaultPlatformP2PPort()) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-p2p-port"); } - if (proTx.platformHTTPPort != main_params->GetDefaultPlatformHTTPPort()) { + if (proTx.platformHTTPPort != Params().GetDefaultPlatformHTTPPort()) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-http-port"); } } - if (proTx.platformP2PPort == main_params->GetDefaultPort()) { + if (proTx.platformP2PPort == Params().GetDefaultPort()) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-p2p-port"); } - if (proTx.platformHTTPPort == main_params->GetDefaultPort()) { + if (proTx.platformHTTPPort == Params().GetDefaultPort()) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-http-port"); }src/evo/netinfo.h (1)
1-20: Header missing required includes for used typesThis header uses many STL and project symbols without including the corresponding headers, which can break standalone compilation and slow builds due to transitive dependencies. Add the minimal set explicitly.
#ifndef BITCOIN_EVO_NETINFO_H #define BITCOIN_EVO_NETINFO_H #include <netaddress.h> #include <serialize.h> #include <streams.h> +#include <span.h> -#include <variant> +#include <array> +#include <cstdint> +#include <map> +#include <memory> +#include <optional> +#include <set> +#include <string> +#include <string_view> +#include <typeinfo> +#include <unordered_set> +#include <utility> +#include <variant> +#include <vector> +#include <algorithm> +#include <ranges>
🧹 Nitpick comments (37)
doc/release-notes-6666.md (3)
32-38: Wording: replace “prior to” with “before”Use a leaner construction per style guidance.
- * On payloads prior to extended addresses, if a masternode update affects `platformP2PPort` and/or `platformHTTPPort` + * On payloads before extended addresses, if a masternode update affects `platformP2PPort` and/or `platformHTTPPort`
41-41: Typo/clarity: “any three” → “any of the three”The current phrasing implies exactly three rather than any subset.
- * Attempting to populate any three address fields will make populating all fields mandatory. + * Attempting to populate any of the three address fields will make populating all fields mandatory.
64-66: Grammar: missing verb (“that are not serialized”)Fix minor grammar for clarity.
- To avoid the perception of changes to fields that not serialized by extended address payloads, data from `netInfo` will + To avoid the perception of changes to fields that are not serialized by extended address payloads, data from `netInfo` willsrc/rpc/quorums.cpp (1)
209-213: Switch to ShimNetInfoPlatform for addresses is correct; verify JSON shape consistency across RPCsGood move away from direct netInfo->ToJson() to the platform-aware shim. Please verify that:
- ShimNetInfoPlatform returns an object with keys consistent across RPCs (e.g., core_p2p, platform_p2p, platform_https) and matches help/man pages.
- Non-Evo nodes return only relevant keys, and legacy nodes behave as expected during pre-/post-fork transitions.
If needed, add a small unit/functional test covering the quorum info “addresses” shape.
src/test/block_reward_reallocation_tests.cpp (1)
117-123: Adopted extended NetInfo API correctlyUsing Purpose::CORE_P2P in AddEntry is correct for the extended addresses API.
Minor: avoid relying on transitive includes. Explicitly include evo/netinfo.h where Purpose is defined to prevent future breakage.
Apply this diff near other includes:
#include <evo/deterministicmns.h> #include <evo/providertx.h> #include <evo/specialtx.h> +#include <evo/netinfo.h> #include <masternode/payments.h>src/evo/simplifiedmns.cpp (1)
59-68: Only print platform fields for Evo nodes to reduce noiseCurrently platformNodeID (and optionally platformHTTPPort) are printed for all nodes. For Regular MNs these fields are irrelevant and default to zeros, which is noisy.
Suggest gating platform fields by nType == MnType::Evo and simplifying the format construction.
Apply this refactor:
- return strprintf("CSimplifiedMNListEntry(nVersion=%d, nType=%d, proRegTxHash=%s, confirmedHash=%s, " - "pubKeyOperator=%s, votingAddress=%s, isValid=%d, payoutAddress=%s, operatorPayoutAddress=%s, " - "platformNodeID=%s%s)\n" - " %s", - nVersion, ToUnderlying(nType), proRegTxHash.ToString(), confirmedHash.ToString(), - pubKeyOperator.ToString(), EncodeDestination(PKHash(keyIDVoting)), isValid, payoutAddress, - operatorPayoutAddress, platformNodeID.ToString(), - (nVersion >= ProTxVersion::ExtAddr ? "" : strprintf(", platformHTTPPort=%d", platformHTTPPort)), - netInfo->ToString()); + std::string platform_str; + if (nType == MnType::Evo) { + platform_str = strprintf(", platformNodeID=%s", platformNodeID.ToString()); + if (nVersion < ProTxVersion::ExtAddr) { + platform_str += strprintf(", platformHTTPPort=%d", platformHTTPPort); + } + } + return strprintf( + "CSimplifiedMNListEntry(nVersion=%d, nType=%d, proRegTxHash=%s, confirmedHash=%s, " + "pubKeyOperator=%s, votingAddress=%s, isValid=%d, payoutAddress=%s, operatorPayoutAddress=%s%s)\n" + " %s", + nVersion, ToUnderlying(nType), proRegTxHash.ToString(), confirmedHash.ToString(), + pubKeyOperator.ToString(), EncodeDestination(PKHash(keyIDVoting)), isValid, payoutAddress, + operatorPayoutAddress, platform_str, netInfo->ToString());src/rpc/coinjoin.cpp (1)
410-532: Clang-format failure in this regionCI reports formatting differences in this block. Please run clang-format or apply the project’s format to resolve stylistic issues.
src/coinjoin/client.cpp (1)
17-17: Adopt shim for extended addresses; also guard deprecatedserviceoutput
- Good switch to ShimNetInfoPlatform for the new extended-address schema. Include is correct.
- Minor nit: avoid emitting a bogus "service" when no CORE_P2P is set. Empty netInfo results in port 0; only print if port != 0.
Proposed guarded output:
- if (m_wallet->chain().rpcEnableDeprecated("service")) { - obj.pushKV("service", mixingMasternode->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); - } + if (m_wallet->chain().rpcEnableDeprecated("service")) { + const auto svc = mixingMasternode->pdmnState->netInfo->GetPrimary(); + if (svc.GetPort() != 0) { + obj.pushKV("service", svc.ToStringAddrPort()); + } + }Also applies to: 1881-1885
src/test/evo_deterministicmns_tests.cpp (1)
111-112: Tests updated for purpose-aware AddEntry look correct; consider adding extended-address coverage
- Passing Purpose::CORE_P2P is consistent with the new API and existing assertions.
- Suggest adding tests that exercise PLATFORM_P2P and PLATFORM_HTTPS purposes and at least one ExtAddr-version ProTx path (domain/tor/i2p/cjdns), to cover the new functionality end-to-end.
I can draft additional unit tests (e.g., in evo_netinfo_tests.cpp) that add platform entries, verify GetPrimary selection, and JSON shape for ExtAddr. Want me to open a follow-up?
Also applies to: 134-135, 644-645, 720-721, 789-790, 853-854
src/evo/core_write.cpp (1)
16-16: JSON now uses shims for addresses/ports; add small robustness for deprecated “service”
- Good switch to ShimNetInfoPlatform and ShimPlatformPort to normalize legacy vs ExtAddr outputs.
- Minor nit: only emit “service” if GetPrimary().GetPort() != 0 to avoid printing “0.0.0.0:0”.
Suggested guarded emission (apply to all three “service” sites shown):
- if (IsServiceDeprecatedRPCEnabled()) { - ret.pushKV("service", netInfo->GetPrimary().ToStringAddrPort()); - } + if (IsServiceDeprecatedRPCEnabled()) { + const auto svc = netInfo->GetPrimary(); + if (svc.GetPort() != 0) { + ret.pushKV("service", svc.ToStringAddrPort()); + } + }Also applies to: 76-79, 127-129, 165-169
src/rpc/evo.cpp (8)
186-193: Help text mismatches accepted types (string vs array) for platformP2PAddrsRPC parsing accepts either a single string or an array, but the help describes only an array and mentions “empty string” which conflicts with Type::ARR. Clarify that input may be a string or an array; and when empty is allowed (only on registration, optional=true).
- "Array of addresses in the form \"ADDR:PORT\" used by Platform for peer-to-peer connection.\n" - "Must be unique on the network. Can be set to an empty string, which will require a ProUpServTx afterwards.", + "String or array of addresses in the form \"ADDR:PORT\" used by Platform for peer-to-peer connection.\n" + "Must be unique on the network. On registration, this may be set to an empty string or an empty array\n" + "to defer configuration (a ProUpServTx will be required afterwards).",
194-201: Help text for platformP2PAddrs_update should not reference empty stringUpdate RPC requires values (optional=false). Drop “empty string” language and note string-or-array is accepted.
- "Array of addresses in the form \"ADDR:PORT\" used by Platform for peer-to-peer connection.\n" - "Must be unique on the network.", + "String or array of addresses in the form \"ADDR:PORT\" used by Platform for peer-to-peer connection.\n" + "Must be unique on the network.",
202-209: Help text mismatches accepted types (string vs array) for platformHTTPSAddrsSame issue as P2P addrs. Document string-or-array and registration-only allowance for empty values.
- "Array of addresses in the form \"ADDR:PORT\" used by Platform for their HTTPS API.\n" - "Must be unique on the network. Can be set to an empty string, which will require a ProUpServTx afterwards.", + "String or array of addresses in the form \"ADDR:PORT\" used by Platform for their HTTPS API.\n" + "Must be unique on the network. On registration, this may be set to an empty string or an empty array\n" + "to defer configuration (a ProUpServTx will be required afterwards).",
210-217: Help text for platformHTTPSAddrs_update should not suggest emptinessUpdate RPC requires values; keep string-or-array wording, no emptiness mention.
- "Array of addresses in the form \"ADDR:PORT\" used by Platform for their HTTPS API.\n" - "Must be unique on the network.", + "String or array of addresses in the form \"ADDR:PORT\" used by Platform for their HTTPS API.\n" + "Must be unique on the network.",
592-593: Update examples: still show numeric platform ports instead of addr:port stringsExamples should reflect the new parameters. Propose a minimal IPv4 example using arrays (also valid as single strings).
- HelpExampleCli("protx", "register_fund_evo \"<addr>\" \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" 22821 22822") + HelpExampleCli("protx", "register_fund_evo \"<addr>\" \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" [\"1.2.3.4:22821\"] [\"1.2.3.4:22822\"]")Would you like me to update all CLI/JSON examples consistently (register_evo, register_prepare_evo, update_service_evo) to use addr:port arrays and include a domain example (e.g., "example.com:22822")?
631-633: Update examples for register_evoSame modernization as register_fund_evo; reflect addr:port arrays.
- HelpExampleCli("protx", "register_evo \"<collateralHash>\" 0 \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" 22821 22822") + HelpExampleCli("protx", "register_evo \"<collateralHash>\" 0 \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" [\"1.2.3.4:22821\"] [\"1.2.3.4:22822\"]")
668-669: Update examples for register_prepare_evoUse addr:port arrays for platform endpoints.
- HelpExampleCli("protx", "register_prepare_evo \"<collateralHash>\" 0 \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" 22821 22822") + HelpExampleCli("protx", "register_prepare_evo \"<collateralHash>\" 0 \"1.2.3.4:1234\" \"<owner>\" \"<opBLS>\" \"<vote>\" 0 \"<payout>\" \"<nodeID>\" [\"1.2.3.4:22821\"] [\"1.2.3.4:22822\"]")
985-986: Update examples for update_service_evoReflect new parameters with addr:port arrays.
- HelpExampleCli("protx", "update_service_evo \"<proTxHash>\" \"1.2.3.4:1234\" \"<operatorKey>\" \"<nodeID>\" 22821 22822") + HelpExampleCli("protx", "update_service_evo \"<proTxHash>\" \"1.2.3.4:1234\" \"<operatorKey>\" \"<nodeID>\" [\"1.2.3.4:22821\"] [\"1.2.3.4:22822\"]")src/rpc/evo_util.cpp (2)
71-169: Robust platform address parsing, but tweak numeric handling and error messagesParsing is solid: supports string/array/port with version gating. Two refinements:
- Arrays: error text says “must be string” but you also reject purely numeric strings. Clarify to “must be non-numeric string addr:port”.
- When CanStorePlatform() and numeric port is provided, you require coreP2PAddrs first. Consider referencing the missing purpose by name in the message for clarity.
Suggested message tweaks:
- strprintf("Invalid param for %s[%d], must be string", field_name, idx) + strprintf("Invalid param for %s[%d], must be non-numeric string in form \"ADDR:PORT\"", field_name, idx)- "Must specify coreP2PAddrs in order to set %s" + "Must specify coreP2PAddrs before setting %s with a bare port (needed to bind the port to a primary address)"
142-161: Potential inheritance slice; prefer using the primary address directlyCurrent construction is fine, but more direct and intent-revealing:
- const CService service{CNetAddr{ptx.netInfo->GetPrimary()}, static_cast<uint16_t>(port)}; + const CService primary = ptx.netInfo->GetPrimary(); + const CService service(primary, static_cast<uint16_t>(port));src/evo/dmnstate.cpp (1)
134-166: State diff JSON: good fallback when netInfo unchanged, but consider a neutral placeholderUsing 255.255.255.255:port as an “unknown address” placeholder is pragmatic but could be misinterpreted downstream. Consider using a clearer sentinel like [":"] or documenting this behavior in release notes.
src/rpc/evo_util.h (2)
26-45: ShimNetInfoPlatform: solid, prefer clearer key stringsFunction is correct. Optional tweak: avoid .data() on PurposeToString return if it’s already a std::string; UniValue will accept std::string directly.
- ret.pushKV(PurposeToString(Purpose::PLATFORM_HTTPS, /*lower=*/true).data(), + ret.pushKV(PurposeToString(Purpose::PLATFORM_HTTPS, /*lower=*/true), ArrFromService(CService(addr, obj.platformHTTPPort)));(and similarly for PLATFORM_P2P)
47-81: ShimPlatformPort: semantics good; tiny typo and edge-case note
- Typo in static_assert message (“plaform”).
- Consider documenting that return -1 means “no value present” to callers, and that multiple entries will pick the first.
- static_assert(!std::is_same_v<std::decay_t<T1>, CSimplifiedMNListEntry>, "CSimplifiedMNListEntry doesn't have plaformP2PPort"); + static_assert(!std::is_same_v<std::decay_t<T1>, CSimplifiedMNListEntry>, "CSimplifiedMNListEntry doesn't have platformP2PPort");src/evo/deterministicmns.cpp (1)
491-525: Update path correctly removes then re-adds unique properties per-entryThe two-phase delete/add with rollback on failure is sound. Error strings are terse; consider adding the conflicting proTxHash where available for diagnostics.
src/test/evo_netinfo_tests.cpp (1)
172-176: Use correct format specifier for size_t in strprintfidx is size_t; passing it to "%d" is UB on some platforms. Cast to unsigned and use "%u" (or use "%zu" if strprintf supports it in your util).
- BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::CORE_P2P, strprintf("1.1.1.%d:9998", idx)), NetInfoStatus::Success); + BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::CORE_P2P, strprintf("1.1.1.%u:9998", static_cast<unsigned>(idx))), NetInfoStatus::Success);test/functional/rpc_netinfo.py (8)
176-181: Remove unnecessary .keys() when checking dict membershipUse
key in dictinstead ofkey in dict.keys()for readability and speed.- for arr in rpc_output['updatedMNs']: - if proregtx_hash in arr.keys(): + for arr in rpc_output['updatedMNs']: + if proregtx_hash in arr: return arr[proregtx_hash]
444-449: Use direct membership checks on dicts (no .keys())[sim118] Ruff warning. Same pattern applies to the following assertions.
- assert "addresses" in proregtx_rpc['proRegTx'].keys() - assert "addresses" in masternode_status['dmnState'].keys() - assert "addresses" in proupservtx_rpc['proUpServTx'].keys() - assert "addresses" in protx_diff_rpc['mnList'][0].keys() - assert "addresses" in protx_listdiff_rpc.keys() + assert "addresses" in proregtx_rpc['proRegTx'] + assert "addresses" in masternode_status['dmnState'] + assert "addresses" in proupservtx_rpc['proUpServTx'] + assert "addresses" in protx_diff_rpc['mnList'][0] + assert "addresses" in protx_listdiff_rpc
459-462: Usenot in dict(no .keys())- assert "core_p2p" not in protx_listdiff_rpc_pl['addresses'].keys() + assert "core_p2p" not in protx_listdiff_rpc_pl['addresses']
463-469: Use direct membership checks for 'service' key (no .keys())- assert "service" not in proregtx_rpc['proRegTx'].keys() - assert "service" not in masternode_status['dmnState'].keys() - assert "service" not in proupservtx_rpc['proUpServTx'].keys() - assert "service" not in protx_diff_rpc['mnList'][0].keys() - assert "service" not in protx_listdiff_rpc.keys() + assert "service" not in proregtx_rpc['proRegTx'] + assert "service" not in masternode_status['dmnState'] + assert "service" not in proupservtx_rpc['proUpServTx'] + assert "service" not in protx_diff_rpc['mnList'][0] + assert "service" not in protx_listdiff_rpc
471-473: Use direct membership check for 'service'- assert "service" in masternode_status.keys() + assert "service" in masternode_status
487-493: Use direct membership checks for 'service' with deprecated RPC- assert "service" in proregtx_rpc['proRegTx'].keys() - assert "service" in masternode_status['dmnState'].keys() - assert "service" in proupservtx_rpc['proUpServTx'].keys() - assert "service" in protx_diff_rpc['mnList'][0].keys() - assert "service" in protx_listdiff_rpc.keys() + assert "service" in proregtx_rpc['proRegTx'] + assert "service" in masternode_status['dmnState'] + assert "service" in proupservtx_rpc['proUpServTx'] + assert "service" in protx_diff_rpc['mnList'][0] + assert "service" in protx_listdiff_rpc
575-576: Use direct membership checks for 'platform*Port' in listdiff- assert "platformP2PPort" not in protx_listdiff_rpc.keys() - assert "platformHTTPPort" not in protx_listdiff_rpc.keys() + assert "platformP2PPort" not in protx_listdiff_rpc + assert "platformHTTPPort" not in protx_listdiff_rpc
603-613: Avoid overshadowing outer self in nested functionInner function parameter named
selfshadows the outerself, which is confusing. Rename the parameter.- def update_node_two(self, duplicate_addr = None, duplicate_tor = None, duplicate_i2p = None, duplicate_domain = None): + def update_node_two(ctx, duplicate_addr = None, duplicate_tor = None, duplicate_i2p = None, duplicate_domain = None): args = [ - self, True, - [duplicate_addr or f"127.0.0.2:{self.node_two.mn.nodePort}", duplicate_tor or f"{DOMAINS_TOR[1]}:{self.node_two.mn.nodePort}"], - [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_P2P}", duplicate_i2p or f"{DOMAINS_I2P[1]}:0"], - [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_HTTP}", duplicate_domain or f"{DOMAINS_CLR[1]}:{DEFAULT_PORT_PLATFORM_HTTP}"] + ctx, True, + [duplicate_addr or f"127.0.0.2:{ctx.node_two.mn.nodePort}", duplicate_tor or f"{DOMAINS_TOR[1]}:{ctx.node_two.mn.nodePort}"], + [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_P2P}", duplicate_i2p or f"{DOMAINS_I2P[1]}:0"], + [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_HTTP}", duplicate_domain or f"{DOMAINS_CLR[1]}:{DEFAULT_PORT_PLATFORM_HTTP}"] ] if duplicate_addr or duplicate_tor or duplicate_i2p or duplicate_domain: args += [-1, "bad-protx-dup-netinfo-entry"] - self.node_two.update_mn(*args) + ctx.node_two.update_mn(*args)src/evo/netinfo.cpp (2)
21-37: Missing standard headers for used symbolsThis TU uses std::array, std::all_of, std::equal. Add required includes explicitly to avoid relying on transitive includes.
#include <span.h> #include <util/check.h> #include <util/string.h> #include <util/system.h> #include <univalue.h> +#include <array> +#include <algorithm>
64-73: Explicitly document allowed platform HTTPS ports rationaleIsAllowedPlatformHTTPPort only allows 80 and 443. Consider adding a comment referencing the rule source (policy or spec) and update unit tests if you plan to allow more standard ports in the future.
src/evo/netinfo.h (1)
451-529: Prefer a simpler equality check to avoid dependency in the headerUsing std::ranges::equal in a widely included header pulls in . Consider comparing with a for-loop or std::equal (already included via ) to reduce compile-time and dependencies.
- if (std::ranges::equal(bytes, EXTADDR_MAGIC)) { + if (std::equal(bytes.begin(), bytes.end(), EXTADDR_MAGIC.begin(), EXTADDR_MAGIC.end())) {test/functional/test_framework/test_framework.py (1)
603-613: Avoid shadowing outer self in nested helperThe nested function parameter
selfshadows the outer method’sself. Rename to improve readability.- def update_node_two(self, duplicate_addr = None, duplicate_tor = None, duplicate_i2p = None, duplicate_domain = None): + def update_node_two(ctx, duplicate_addr = None, duplicate_tor = None, duplicate_i2p = None, duplicate_domain = None): args = [ - self, True, - [duplicate_addr or f"127.0.0.2:{self.node_two.mn.nodePort}", duplicate_tor or f"{DOMAINS_TOR[1]}:{self.node_two.mn.nodePort}"], - [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_P2P}", duplicate_i2p or f"{DOMAINS_I2P[1]}:0"], - [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_HTTP}", duplicate_domain or f"{DOMAINS_CLR[1]}:{DEFAULT_PORT_PLATFORM_HTTP}"] + ctx, True, + [duplicate_addr or f"127.0.0.2:{ctx.node_two.mn.nodePort}", duplicate_tor or f"{DOMAINS_TOR[1]}:{ctx.node_two.mn.nodePort}"], + [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_P2P}", duplicate_i2p or f"{DOMAINS_I2P[1]}:0"], + [f"127.0.0.2:{DEFAULT_PORT_PLATFORM_HTTP}", duplicate_domain or f"{DOMAINS_CLR[1]}:{DEFAULT_PORT_PLATFORM_HTTP}"] ] if duplicate_addr or duplicate_tor or duplicate_i2p or duplicate_domain: args += [-1, "bad-protx-dup-netinfo-entry"] - self.node_two.update_mn(*args) + ctx.node_two.update_mn(*args)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
contrib/seeds/makeseeds.py(1 hunks)doc/release-notes-6666.md(1 hunks)src/coinjoin/client.cpp(2 hunks)src/evo/core_write.cpp(5 hunks)src/evo/deterministicmns.cpp(8 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/dmnstate.cpp(4 hunks)src/evo/dmnstate.h(2 hunks)src/evo/netinfo.cpp(14 hunks)src/evo/netinfo.h(14 hunks)src/evo/providertx.cpp(5 hunks)src/evo/providertx.h(3 hunks)src/evo/simplifiedmns.cpp(1 hunks)src/evo/simplifiedmns.h(1 hunks)src/evo/specialtxman.cpp(3 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(7 hunks)src/rpc/evo_util.cpp(4 hunks)src/rpc/evo_util.h(1 hunks)src/rpc/masternode.cpp(3 hunks)src/rpc/quorums.cpp(2 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(6 hunks)src/test/evo_netinfo_tests.cpp(4 hunks)src/test/evo_simplifiedmns_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)test/functional/feature_dip3_deterministicmns.py(1 hunks)test/functional/rpc_netinfo.py(8 hunks)test/functional/rpc_quorum.py(1 hunks)test/functional/test_framework/test_framework.py(12 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/block_reward_reallocation_tests.cppsrc/test/evo_simplifiedmns_tests.cppsrc/rpc/quorums.cppsrc/evo/simplifiedmns.cppsrc/evo/core_write.cppsrc/evo/simplifiedmns.hsrc/evo/dmnstate.hsrc/rpc/coinjoin.cppsrc/evo/providertx.hsrc/rpc/masternode.cppsrc/coinjoin/client.cppsrc/test/evo_deterministicmns_tests.cppsrc/evo/deterministicmns.hsrc/evo/providertx.cppsrc/txmempool.cppsrc/rpc/evo_util.cppsrc/evo/specialtxman.cppsrc/evo/dmnstate.cppsrc/rpc/evo_util.hsrc/evo/deterministicmns.cppsrc/rpc/evo.cppsrc/test/evo_netinfo_tests.cppsrc/evo/netinfo.cppsrc/evo/netinfo.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/block_reward_reallocation_tests.cppsrc/test/evo_simplifiedmns_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_netinfo_tests.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_quorum.pytest/functional/feature_dip3_deterministicmns.pytest/functional/rpc_netinfo.pytest/functional/test_framework/test_framework.py
contrib/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/seeds/makeseeds.py
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/simplifiedmns.cppsrc/evo/core_write.cppsrc/evo/simplifiedmns.hsrc/evo/dmnstate.hsrc/evo/providertx.hsrc/evo/deterministicmns.hsrc/evo/providertx.cppsrc/evo/specialtxman.cppsrc/evo/dmnstate.cppsrc/evo/deterministicmns.cppsrc/evo/netinfo.cppsrc/evo/netinfo.h
doc/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6666.md
🧠 Learnings (9)
📚 Learning: 2025-06-16T17:59:55.669Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Applied to files:
src/test/block_reward_reallocation_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/rpc/evo_util.cppsrc/evo/specialtxman.cppsrc/evo/deterministicmns.cppsrc/rpc/evo.cpptest/functional/rpc_netinfo.pytest/functional/test_framework/test_framework.py
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/providertx.hsrc/test/evo_deterministicmns_tests.cppsrc/evo/providertx.cppsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/providertx.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/providertx.cppsrc/evo/specialtxman.cppsrc/evo/deterministicmns.cppsrc/test/evo_netinfo_tests.cppsrc/evo/netinfo.cppsrc/evo/netinfo.h
📚 Learning: 2025-05-10T00:55:16.184Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/test/evo_netinfo_tests.cpp:39-45
Timestamp: 2025-05-10T00:55:16.184Z
Learning: In Dash's validation testing for network entries, checking `IsTriviallyValid()` is specifically used to ensure all three validation methods (`AddEntry()`, `Validate()`, and `IsTriviallyValid()`) consistently work together rather than replacing one check with another. `AddEntry()` and `Validate()` handle the consensus rule checks, while `IsTriviallyValid()` confirms the basic validation steps.
Applied to files:
src/evo/providertx.cpp
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/test_framework/test_framework.py
🧬 Code Graph Analysis (6)
test/functional/rpc_quorum.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/evo/simplifiedmns.cpp (3)
src/evo/simplifiedmns.h (3)
nType(37-100)platformNodeID(33-33)platformHTTPPort(32-32)src/evo/dmnstate.h (2)
platformNodeID(62-62)platformHTTPPort(64-64)src/evo/providertx.h (4)
platformNodeID(68-68)platformNodeID(137-137)platformHTTPPort(70-70)platformHTTPPort(139-139)
src/evo/core_write.cpp (2)
src/evo/simplifiedmns.h (2)
nType(37-100)platformNodeID(33-33)src/evo/dmnstate.h (1)
platformNodeID(62-62)
test/functional/feature_dip3_deterministicmns.py (2)
test/functional/test_framework/test_framework.py (2)
update_service(1413-1458)generate(803-806)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/evo/dmnstate.cpp (3)
src/evo/netinfo.cpp (3)
obj(77-77)ArrFromService(75-80)ArrFromService(75-75)src/evo/simplifiedmns.h (1)
nType(37-100)src/evo/netinfo.h (1)
CService(282-292)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/test_node.py (2)
TestNode(56-793)generate(345-347)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/simplifiedmns.h
[error] 65-95: Clang format differences found. Code formatting does not match the expected style.
src/evo/dmnstate.h
[error] 95-111: Clang format differences found. Code formatting does not match the expected style.
src/rpc/coinjoin.cpp
[error] 410-532: Clang format differences found. Code formatting does not match the expected style.
src/evo/providertx.h
[error] 87-130: Clang format differences found. Code formatting does not match the expected style.
src/rpc/masternode.cpp
[error] 6-30: Clang format differences found. Code formatting does not match the expected style.
src/rpc/evo_util.cpp
[error] 88-174: Clang format differences found. Code formatting does not match the expected style.
src/rpc/evo_util.h
[error] 57-62: Clang format differences found. Code formatting does not match the expected style.
src/evo/deterministicmns.cpp
[error] 1217-1218: Clang format differences found. Code formatting does not match the expected style.
src/rpc/evo.cpp
[error] 64-700: Clang format differences found. Code formatting does not match the expected style.
src/test/evo_netinfo_tests.cpp
[error] 40-230: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.cpp
[error] 21-70: Clang format differences found. Code formatting does not match the expected style.
src/evo/netinfo.h
[error] 149-167: Clang format differences found. Code formatting does not match the expected style.
🪛 LanguageTool
doc/release-notes-6666.md
[style] ~32-~32: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ..._https']` respectively. * On payloads prior to extended addresses, if a masternode upd...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 Ruff (0.12.2)
test/functional/rpc_netinfo.py
178-178: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
444-444: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
445-445: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
446-446: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
447-447: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
448-448: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
459-459: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
464-464: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
465-465: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
466-466: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
467-467: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
468-468: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
471-471: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
488-488: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
489-489: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
490-490: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
491-491: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
492-492: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
575-575: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
576-576: Use key not in dict instead of key not in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (33)
test/functional/rpc_quorum.py (1)
31-31: LGTM: assertion updated to new structureAsserting against member['addresses']['core_p2p'][0] aligns with the extended address schema.
src/txmempool.cpp (1)
654-656: LGTM: use ofconst auto&when iterating entriesThe shift to
const auto&fornetInfo->GetEntries()improves resilience to API/type evolution and keeps semantics unchanged.Also applies to: 667-669, 758-760, 768-770, 997-1004, 1019-1026, 1357-1361
src/test/evo_simplifiedmns_tests.cpp (1)
27-28: LGTM: purpose specified for AddEntryPassing
Purpose::CORE_P2Preflects the updated API and keeps the test deterministic.src/rpc/quorums.cpp (1)
10-10: Include addition aligns with new shim usageIncluding rpc/evo_util.h is appropriate here since ShimNetInfoPlatform is used below.
test/functional/feature_dip3_deterministicmns.py (1)
275-282: Functional test updated to new extended-addresses layout
- Passing addrs_core_p2p aligns with the updated TestFramework signature.
- Assertions check both protx info and masternode list under addresses.core_p2p[0]; good coverage.
No further changes needed here.
src/evo/simplifiedmns.h (1)
65-95: Version-gated SML entry serialization aligns with ExtAddr design
- NetInfoSerWrapper with explicit decltype(obj) is fine.
- Gating platformHTTPPort to pre-ExtAddr is correct; ExtAddr moves platform endpoints into netInfo.
Please ensure corresponding deserialization paths/tests exist for both legacy and ExtAddr versions (round-trip SML serialization under SMNLE_VERSIONED_PROTO_VERSION and DMN_TYPE_PROTO_VERSION).
src/evo/providertx.h (2)
79-115: CProRegTx serialization: conditional platform ports is correct; matches team guidance
- Keeping the “unknown version” check using GetMax(/true/, /true/) complies with the serialization rule-of-thumb (handle up to max known version irrespective of deployments).
- Serializing platformNodeID always for MnType::Evo, and platform ports only pre-ExtAddr, is the right compatibility split.
144-179: CProUpServTx serialization mirrors ProRegTx correctly
- Same positive notes: explicit NetInfoSerWrapper type, conditional platform ports, and version cap.
src/evo/dmnstate.h (1)
263-264: Diff serialization correctly fixesis_extended=falsefor backward compatibility.We’ve verified that in the main
CDeterministicMNStateserialization (around line 100 ofdmnstate.h) theis_extendedflag is passed asobj.nVersion >= ProTxVersion::ExtAddr, whereas in the diff serialization (around line 263) it’s hard-coded tofalse. This difference is intentional to ensure diffs remain in the legacy format for backward compatibility—no changes are needed.src/rpc/masternode.cpp (2)
570-570: Good use of auto for type deduction.The change to
auto&improves code maintainability by allowing the compiler to deduce the type, making the code more resilient to future type changes.
620-627: Shim functions correctly cover all masternode types and formats
Both ShimNetInfoPlatform and ShimPlatformPort handle:
- Non-Evo vs Evo nodes (early return for non-Evo)
- Legacy vs extended netInfo storage (uses obj.platform{P2P,HTTP}Port when legacy, falls back to netInfo entries otherwise)
- CSimplifiedMNListEntry exception for P2P port
No gaps found in Evo, core RPC, quorum, or CoinJoin code paths. Approved.src/evo/deterministicmns.h (1)
443-454: Approve CService ADDRv2 serialization in GetUniquePropertyHashVerified that
IsAddrV1Compatible()andADDRV2_FORMATare used consistently across the codebase (e.g. innet_processing.cpp,addrman.cpp,netaddress.h,net.cpp), and this implementation correctly applies ADDRv2 formatting for extended addresses without impacting legacy behavior. No further changes required.src/evo/providertx.cpp (3)
35-55: Excellent centralization of netInfo validation logic.The new
IsNetInfoTriviallyValidtemplate function properly centralizes the validation rules for network info across different provider transaction types. The validation correctly enforces:
- Mandatory CORE_P2P entries for all nodes
- No platform entries for regular nodes
- Required platform entries for Evo nodes when supported
This reduces code duplication and ensures consistent validation.
84-87: Clean integration of centralized validation.The integration of
IsNetInfoTriviallyValidinCProRegTx::IsTriviallyValidmaintains the validation flow while adding the centralized checks. The early return pattern is correct.
145-154: Appropriate version-dependent string formatting.The conditional inclusion of platform ports in the string representation correctly reflects the version-dependent serialization. For ExtAddr versions, these fields are part of the netInfo structure.
src/evo/specialtxman.cpp (2)
335-338: Correct version-dependent platform port handling.The conditional assignment of platform ports only for versions prior to
ExtAddrensures proper backward compatibility while preventing legacy fields from interfering with the extended address format.
260-272: NetInfo entry validation correctly handles both IP and domain addressesVerified that
NetInfoEntryimplements bothGetAddrPort()andGetDomainPort(), rejects entries missing both, and applies duplicate checks consistently. Error codes match those inprovidertx.cppanddeterministicmns.cpp, ensuring uniform handling of invalid and duplicate netInfo entries. No further changes needed.src/rpc/evo.cpp (2)
764-767: Platform addresses optionality at registration vs update: confirm intended consensus behaviorRegistration uses optional=true (allows empty), update uses optional=false (requires values). Given prior rules mandated non-zero platform endpoints at ProRegTx, please confirm the intended consensus shift with ExtAddr: are blank platform addresses allowed at registration? If yes, ensure tests cover ProRegTx with empty platform addrs and subsequent ProUpServTx to populate them.
Also applies to: 1034-1037
64-700: Fix clang-format issues flagged by CIClang-format diff check failed for this file. Please run the repository’s clang-format configuration over the modified ranges.
src/rpc/evo_util.cpp (2)
30-35: Explicit purpose passed to AddEntry is correctPassing Purpose::{CORE_P2P,...} into AddEntry aligns with ExtAddr support and purpose-aware storage. Good change.
Also applies to: 56-59
88-174: Fix clang-format issues (CI failure)Clang-format diff check failed for this file. Please format the modified hunks.
src/evo/dmnstate.cpp (2)
9-11: Includes for netinfo and shims are appropriateHeaders are correctly included to support new serialization paths.
43-57: Use shimmed addresses and ports in JSON is correct and backward-compatible
- addresses: ShimNetInfoPlatform emits native JSON if ExtAddr-capable; falls back to legacy when needed.
- platformP2PPort/platformHTTPPort: ShimPlatformPort handles legacy/modern uniformly.
Looks good.src/rpc/evo_util.h (1)
57-62: Fix clang-format issues (CI failure)Please run clang-format on the modified ranges in this header.
src/evo/deterministicmns.cpp (5)
435-453: Uniqueness checks now support domain entries; error handling is correctProperly handles GetAddrPort vs GetDomainPort and rejects invalid entries. Good atomic rollback behavior on failure.
592-610: Removal path mirrors add/update; solid and consistentDomain handling and invalid-entry checks are handled correctly.
915-919: CheckService: include Duplicate in unreachable casesIncluding NetInfoStatus::Duplicate in the assert(false) group is correct for self-checks.
1122-1140: Duplication checks extended to domains: good coverageBoth ProRegTx and ProUpServTx now check uniqueness for addr:port and domain:port entries against other MNs. This is correct and consistent.
Also applies to: 1213-1226
1217-1218: Fix clang-format issues (CI failure)Clang-format diff check flagged this file. Please format the modified hunks.
src/test/evo_netinfo_tests.cpp (1)
40-230: Fix formatting errors with clang-format
- File needing attention:
src/test/evo_netinfo_tests.cppPlease run the Dash Core clang-format style locally and commit the updated file:
clang-format -i --style=Dash src/test/evo_netinfo_tests.cpp git add src/test/evo_netinfo_tests.cppThen push the changes so that CI’s format check passes.
src/evo/netinfo.cpp (2)
452-457: I2P_SAM31_PORT constant verifiedI2P_SAM31_PORT is defined in src/netaddress.h (line 118) as a
static constexpr uint16_twith value 0, matching the ADDRv2/BIP155 requirement. No changes needed.
21-70: Formatting Check Needed: Run clang-format LocallyCI is reporting formatting mismatches in
src/evo/netinfo.cpp, butclang-formatisn’t available in this environment. Please installclang-format(matching the project’s version), then run:clang-format -i src/evo/netinfo.cpp git diff -- src/evo/netinfo.cppand commit any resulting changes to ensure the file passes the CI format check.
src/evo/netinfo.h (1)
149-167: Clang-format check unavailable – please verify manuallyThe automated clang-format check couldn’t run (clang-format not found). Please run the following locally and ensure this region is properly formatted:
- File: src/evo/netinfo.h
Lines: 149–167clang-format -i src/evo/netinfo.h git diff -- src/evo/netinfo.h
| bool ExtNetInfo::HasAddrPortDuplicates() const | ||
| { | ||
| std::set<NetInfoEntry> known{}; | ||
| for (const auto& entry : m_all_entries) { | ||
| if (auto [_, inserted] = known.insert(entry); !inserted) { | ||
| return true; | ||
| } | ||
| } | ||
| ASSERT_IF_DEBUG(known.size() == m_all_entries.size()); | ||
| return false; | ||
| } | ||
|
|
||
| bool ExtNetInfo::IsAddrPortDuplicate(const NetInfoEntry& candidate) const | ||
| { | ||
| return std::any_of(m_all_entries.begin(), m_all_entries.end(), | ||
| [&candidate](const auto& entry) { return candidate == entry; }); | ||
| } | ||
|
|
||
| bool ExtNetInfo::HasAddrDuplicates(const NetInfoList& entries) const | ||
| { | ||
| std::unordered_set<std::string> known{}; | ||
| for (const auto& entry : entries) { | ||
| if (auto [_, inserted] = known.insert(entry.ToStringAddr()); !inserted) { | ||
| return true; | ||
| } | ||
| } | ||
| ASSERT_IF_DEBUG(known.size() == entries.size()); | ||
| return false; | ||
| } | ||
|
|
||
| bool ExtNetInfo::IsAddrDuplicate(const NetInfoEntry& candidate, const NetInfoList& entries) const | ||
| { | ||
| const std::string& candidate_str{candidate.ToStringAddr()}; | ||
| return std::any_of(entries.begin(), entries.end(), | ||
| [&candidate_str](const auto& entry) { return candidate_str == entry.ToStringAddr(); }); | ||
| } | ||
|
|
||
| NetInfoStatus ExtNetInfo::ProcessCandidate(const uint8_t purpose, const NetInfoEntry& candidate) | ||
| { | ||
| assert(candidate.IsTriviallyValid()); | ||
|
|
||
| if (IsAddrPortDuplicate(candidate)) { | ||
| return NetInfoStatus::Duplicate; | ||
| } | ||
| if (candidate.GetDomainPort().has_value() && purpose != Purpose::PLATFORM_HTTPS) { | ||
| // Domains only allowed for Platform HTTPS API | ||
| return NetInfoStatus::BadInput; | ||
| } | ||
| if (auto it{m_data.find(purpose)}; it != m_data.end()) { | ||
| // Existing entries list found, check limit | ||
| auto& [_, entries] = *it; | ||
| if (entries.size() >= MAX_ENTRIES_EXTNETINFO) { | ||
| return NetInfoStatus::MaxLimit; | ||
| } | ||
| if (IsAddrDuplicate(candidate, entries)) { | ||
| return NetInfoStatus::Duplicate; | ||
| } | ||
| entries.push_back(candidate); | ||
| } else { | ||
| // First entry for purpose code, create new entries list | ||
| auto [_, status] = m_data.try_emplace(purpose, std::vector<NetInfoEntry>({candidate})); | ||
| assert(status); // We did just check to see if our value already existed, try_emplace shouldn't fail | ||
| } | ||
|
|
||
| // Candidate succesfully added, update cache | ||
| m_all_entries.push_back(candidate); | ||
| return NetInfoStatus::Success; | ||
| } |
There was a problem hiding this comment.
Implement per-purpose partial-duplicate rule
To satisfy tests and intended behavior, allow partial duplicates (same addr, different port) within PLATFORM_P2P while keeping them disallowed within CORE_P2P. See failing/expected case in src/test/evo_netinfo_tests.cpp Lines 196-200.
Proposed change:
- if (IsAddrDuplicate(candidate, entries)) {
- return NetInfoStatus::Duplicate;
- }
+ // For CORE_P2P, prohibit duplicates by address (even if port differs).
+ // For PLATFORM_* purposes, only exact addr:port duplicates are disallowed.
+ if (purpose == Purpose::CORE_P2P && IsAddrDuplicate(candidate, entries)) {
+ return NetInfoStatus::Duplicate;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool ExtNetInfo::HasAddrPortDuplicates() const | |
| { | |
| std::set<NetInfoEntry> known{}; | |
| for (const auto& entry : m_all_entries) { | |
| if (auto [_, inserted] = known.insert(entry); !inserted) { | |
| return true; | |
| } | |
| } | |
| ASSERT_IF_DEBUG(known.size() == m_all_entries.size()); | |
| return false; | |
| } | |
| bool ExtNetInfo::IsAddrPortDuplicate(const NetInfoEntry& candidate) const | |
| { | |
| return std::any_of(m_all_entries.begin(), m_all_entries.end(), | |
| [&candidate](const auto& entry) { return candidate == entry; }); | |
| } | |
| bool ExtNetInfo::HasAddrDuplicates(const NetInfoList& entries) const | |
| { | |
| std::unordered_set<std::string> known{}; | |
| for (const auto& entry : entries) { | |
| if (auto [_, inserted] = known.insert(entry.ToStringAddr()); !inserted) { | |
| return true; | |
| } | |
| } | |
| ASSERT_IF_DEBUG(known.size() == entries.size()); | |
| return false; | |
| } | |
| bool ExtNetInfo::IsAddrDuplicate(const NetInfoEntry& candidate, const NetInfoList& entries) const | |
| { | |
| const std::string& candidate_str{candidate.ToStringAddr()}; | |
| return std::any_of(entries.begin(), entries.end(), | |
| [&candidate_str](const auto& entry) { return candidate_str == entry.ToStringAddr(); }); | |
| } | |
| NetInfoStatus ExtNetInfo::ProcessCandidate(const uint8_t purpose, const NetInfoEntry& candidate) | |
| { | |
| assert(candidate.IsTriviallyValid()); | |
| if (IsAddrPortDuplicate(candidate)) { | |
| return NetInfoStatus::Duplicate; | |
| } | |
| if (candidate.GetDomainPort().has_value() && purpose != Purpose::PLATFORM_HTTPS) { | |
| // Domains only allowed for Platform HTTPS API | |
| return NetInfoStatus::BadInput; | |
| } | |
| if (auto it{m_data.find(purpose)}; it != m_data.end()) { | |
| // Existing entries list found, check limit | |
| auto& [_, entries] = *it; | |
| if (entries.size() >= MAX_ENTRIES_EXTNETINFO) { | |
| return NetInfoStatus::MaxLimit; | |
| } | |
| if (IsAddrDuplicate(candidate, entries)) { | |
| return NetInfoStatus::Duplicate; | |
| } | |
| entries.push_back(candidate); | |
| } else { | |
| // First entry for purpose code, create new entries list | |
| auto [_, status] = m_data.try_emplace(purpose, std::vector<NetInfoEntry>({candidate})); | |
| assert(status); // We did just check to see if our value already existed, try_emplace shouldn't fail | |
| } | |
| // Candidate succesfully added, update cache | |
| m_all_entries.push_back(candidate); | |
| return NetInfoStatus::Success; | |
| } | |
| NetInfoStatus ExtNetInfo::ProcessCandidate(const uint8_t purpose, const NetInfoEntry& candidate) | |
| { | |
| assert(candidate.IsTriviallyValid()); | |
| if (IsAddrPortDuplicate(candidate)) { | |
| return NetInfoStatus::Duplicate; | |
| } | |
| if (candidate.GetDomainPort().has_value() && purpose != Purpose::PLATFORM_HTTPS) { | |
| // Domains only allowed for Platform HTTPS API | |
| return NetInfoStatus::BadInput; | |
| } | |
| if (auto it{m_data.find(purpose)}; it != m_data.end()) { | |
| // Existing entries list found, check limit | |
| auto& [_, entries] = *it; | |
| if (entries.size() >= MAX_ENTRIES_EXTNETINFO) { | |
| return NetInfoStatus::MaxLimit; | |
| } | |
| // For CORE_P2P, prohibit duplicates by address (even if port differs). | |
| // For PLATFORM_* purposes, only exact addr:port duplicates are disallowed. | |
| if (purpose == Purpose::CORE_P2P && IsAddrDuplicate(candidate, entries)) { | |
| return NetInfoStatus::Duplicate; | |
| } | |
| entries.push_back(candidate); | |
| } else { | |
| // First entry for purpose code, create new entries list | |
| auto [_, status] = m_data.try_emplace(purpose, std::vector<NetInfoEntry>({candidate})); | |
| assert(status); // try_emplace shouldn't fail here | |
| } | |
| // Candidate successfully added, update cache | |
| m_all_entries.push_back(candidate); | |
| return NetInfoStatus::Success; | |
| } |
🤖 Prompt for AI Agents
In src/evo/netinfo.cpp between lines 372 and 439, the current duplicate checks
do not differentiate by purpose, causing partial duplicates (same address,
different port) to be disallowed universally. Modify the HasAddrPortDuplicates
and IsAddrPortDuplicate methods or the ProcessCandidate method to allow partial
duplicates within the PLATFORM_P2P purpose but disallow them within CORE_P2P.
Specifically, adjust the duplicate detection logic in ProcessCandidate to check
the purpose and apply the stricter duplicate rule only for CORE_P2P, while
permitting partial duplicates for PLATFORM_P2P. Ensure the rest of the logic
remains consistent with this new rule.
src/rpc/coinjoin.cpp
Outdated
| {RPCResult::Type::ARR, "addresses", "Network addresses of the masternode", | ||
| { | ||
| { | ||
| {RPCResult::Type::STR, "address", ""}, | ||
| } | ||
| {RPCResult::Type::ARR, "core_p2p", /*optional=*/true, "Addresses used for protocol P2P", | ||
| { | ||
| {RPCResult::Type::STR, "address", ""}, | ||
| } | ||
| }, | ||
| {RPCResult::Type::ARR, "platform_p2p", /*optional=*/true, "Addresses used for Platform P2P", | ||
| { | ||
| {RPCResult::Type::STR, "address", ""}, | ||
| } | ||
| }, | ||
| {RPCResult::Type::ARR, "platform_https", /*optional=*/true, "Addresses used for Platform HTTPS API", | ||
| { | ||
| {RPCResult::Type::STR, "address", ""}, | ||
| } | ||
| }, | ||
| }}, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix RPC result schema: “addresses” should be an object, not an array
The help currently declares addresses as an array and then adds named arrays inside, which implies an object. Make addresses an object with optional arrays core_p2p, platform_p2p, platform_https to match the new layout and to be consistent with other RPCs.
Apply this diff:
- {RPCResult::Type::ARR, "addresses", "Network addresses of the masternode",
+ {RPCResult::Type::OBJ, "addresses", "Network addresses of the masternode",
{
- {RPCResult::Type::ARR, "core_p2p", /*optional=*/true, "Addresses used for protocol P2P",
+ {RPCResult::Type::ARR, "core_p2p", /*optional=*/true, "Addresses used for protocol P2P",
{
{RPCResult::Type::STR, "address", ""},
}
},
- {RPCResult::Type::ARR, "platform_p2p", /*optional=*/true, "Addresses used for Platform P2P",
+ {RPCResult::Type::ARR, "platform_p2p", /*optional=*/true, "Addresses used for Platform P2P",
{
{RPCResult::Type::STR, "address", ""},
}
},
- {RPCResult::Type::ARR, "platform_https", /*optional=*/true, "Addresses used for Platform HTTPS API",
+ {RPCResult::Type::ARR, "platform_https", /*optional=*/true, "Addresses used for Platform HTTPS API",
{
{RPCResult::Type::STR, "address", ""},
}
},
}},Also ensure the actual JSON-producing code matches this schema; if not, update it accordingly.
🏁 Script executed:
#!/bin/bash
# Verify how “addresses” is constructed in the JSON result
rg -n "UniValue addresses" src/rpc/coinjoin.cpp
rg -n "addresses.pushKV" src/rpc/coinjoin.cppLength of output: 95
🏁 Script executed:
#!/bin/bash
# Locate coinjoin.cpp and display lines around the RPC definition
file=$(fd --full-path coinjoin.cpp)
echo "Inspecting file: $file"
sed -n '400,500p' "$file"Length of output: 500
🏁 Script executed:
#!/bin/bash
# Show the RPC help snippet around where “addresses” is defined
echo "::DOCS SNIPPET (lines 430–470)::"
sed -n '430,470p' src/rpc/coinjoin.cpp
# Find the getcoinjoininfo RPC implementation
echo "::FIND getcoinjoininfo FUNCTION::"
rg -n 'UniValue getcoinjoininfo' -A5 src/rpc/coinjoin.cpp || echo "No direct UniValue signature found"
rg -n 'getcoinjoininfo' -A5 src/rpc/coinjoin.cpp
# Search for JSON construction of core_p2p, platform_p2p, platform_https
echo "::JSON CONSTRUCTION for core_p2p::"
rg -n 'core_p2p' -A5 src/rpc/coinjoin.cpp || echo "core_p2p not found"
echo "::JSON CONSTRUCTION for platform_p2p::"
rg -n 'platform_p2p' -A5 src/rpc/coinjoin.cpp || echo "platform_p2p not found"
echo "::JSON CONSTRUCTION for platform_https::"
rg -n 'platform_https' -A5 src/rpc/coinjoin.cpp || echo "platform_https not found"
# Check whether the JSON builder pushes an 'addresses' object or array
echo "::CHECK pushKV for \"addresses\"::"
rg -n 'pushKV.*"addresses"' -A2 src/rpc/coinjoin.cpp || echo "No pushKV of \"addresses\" found"Length of output: 6772
🏁 Script executed:
#!/bin/bash
# Extract the JSON construction lambda for getcoinjoininfo to inspect how "addresses" is built
sed -n '475,580p' src/rpc/coinjoin.cppLength of output: 3107
🏁 Script executed:
#!/bin/bash
# Locate all definitions of GetJsonInfo to inspect how "addresses" is serialized
echo "Searching for GetJsonInfo definitions..."
rg -n "GetJsonInfo" -A5 -tcpp src
# List the files containing GetJsonInfo
echo
echo "Files with GetJsonInfo:"
rg -l "GetJsonInfo" -tcpp srcLength of output: 4354
🏁 Script executed:
#!/bin/bash
# Inspect how "addresses" is serialized in the masternode server JSON
echo "::SEARCHING FOR addresses IN CCoinJoinServer::GetJsonInfo()::"
rg -n '"addresses"' -A10 src/coinjoin/server.cpp || echo "No occurrences of \"addresses\" found in server.cpp"
# Verify core_p2p serialization in server.cpp
echo "::SEARCHING FOR core_p2p IN server.cpp::"
rg -n 'core_p2p' -A10 src/coinjoin/server.cpp || echo "No occurrences of core_p2p found in server.cpp"Length of output: 532
Fix RPC result schema for “addresses” and implement JSON output
The RPC help declares addresses as an array but nests named arrays inside, which semantically makes it an object. Additionally, I didn’t find any pushKV("addresses", …) in the server’s JSON builder, so this field isn’t actually emitted.
• In src/rpc/coinjoin.cpp (lines 436–453), change the result type for addresses from ARR to OBJ:
- {RPCResult::Type::ARR, "addresses", "Network addresses of the masternode",
+ {RPCResult::Type::OBJ, "addresses", "Network addresses of the masternode",
{
{RPCResult::Type::ARR, "core_p2p", /*optional=*/true, "Addresses used for protocol P2P", { {RPCResult::Type::STR, "address", ""} }},
{RPCResult::Type::ARR, "platform_p2p",/*optional=*/true, "Addresses used for Platform P2P",{ {RPCResult::Type::STR, "address", ""} }},
{RPCResult::Type::ARR, "platform_https",/*optional=*/true,"Addresses used for Platform HTTPS API",{ {RPCResult::Type::STR, "address", ""} }},
}},• In src/coinjoin/server.cpp inside CCoinJoinServer::GetJsonInfo(UniValue& obj), emit an addresses object matching the help schema. For example:
UniValue addrObj(UniValue::VOBJ);
// Build core_p2p array
UniValue coreArr(UniValue::VARR);
for (const auto& addr : GetCoreP2PAddresses()) coreArr.push_back(addr.ToString());
addrObj.pushKV("core_p2p", coreArr);
// Build platform_p2p array
UniValue platP2P(UniValue::VARR);
for (const auto& addr : GetPlatformP2PAddresses()) platP2P.push_back(addr.ToString());
addrObj.pushKV("platform_p2p", platP2P);
// Build platform_https array
UniValue platHTTPS(UniValue::VARR);
for (const auto& addr : GetPlatformHTTPSAddresses()) platHTTPS.push_back(addr.ToString());
addrObj.pushKV("platform_https", platHTTPS);
// Finally push the addresses object
obj.pushKV("addresses", addrObj);Ensure these helper methods return the correct endpoint lists.
🤖 Prompt for AI Agents
In src/rpc/coinjoin.cpp lines 436 to 453, change the RPCResult type of
"addresses" from ARR to OBJ to correctly represent it as an object with named
arrays inside. Then, in src/coinjoin/server.cpp within the
CCoinJoinServer::GetJsonInfo(UniValue& obj) method, add code to build a UniValue
object for "addresses" containing arrays for "core_p2p", "platform_p2p", and
"platform_https" by iterating over their respective address lists and pushing
their string representations. Finally, push this "addresses" object into the
main JSON object "obj" using pushKV. Make sure the helper functions
GetCoreP2PAddresses(), GetPlatformP2PAddresses(), and
GetPlatformHTTPSAddresses() return the correct address lists to populate these
arrays.
| #include <netaddress.h> | ||
| #include <util/check.h> | ||
|
|
||
| #include <evo/dmn_types.h> | ||
| #include <evo/netinfo.h> | ||
|
|
||
| #include <cstdint> | ||
|
|
||
| #include <univalue.h> | ||
|
|
There was a problem hiding this comment.
Missing <type_traits> include (required by std::decay_t and std::is_same_v)
Header uses std::decay_t and std::is_same_v. Add the include to avoid relying on transitive headers.
#include <cstdint>
+#include <type_traits>
#include <univalue.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <netaddress.h> | |
| #include <util/check.h> | |
| #include <evo/dmn_types.h> | |
| #include <evo/netinfo.h> | |
| #include <cstdint> | |
| #include <univalue.h> | |
| #include <netaddress.h> | |
| #include <util/check.h> | |
| #include <evo/dmn_types.h> | |
| #include <evo/netinfo.h> | |
| #include <cstdint> | |
| #include <type_traits> | |
| #include <univalue.h> |
🤖 Prompt for AI Agents
In src/rpc/evo_util.h around lines 8 to 17, the header uses std::decay_t and
std::is_same_v but does not include <type_traits>, which is required for these
templates. Add #include <type_traits> to the include section to ensure these
types are properly recognized without relying on transitive includes.
| { | ||
| // ExtNetInfo has restrictions on duplicates | ||
| ExtNetInfo netInfo; | ||
| BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::CORE_P2P, "1.1.1.1:9998"), NetInfoStatus::Success); | ||
|
|
||
| // Exact (i.e. addr:port) duplicates are prohibited *within* a list | ||
| BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::CORE_P2P, "1.1.1.1:9998"), NetInfoStatus::Duplicate); | ||
| // Partial (i.e. different port) duplicates are prohibited *within* a list | ||
| BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::CORE_P2P, "1.1.1.1:9997"), NetInfoStatus::Duplicate); | ||
|
|
||
| // Exact (i.e. addr:port) duplicates are prohibited *across* lists | ||
| BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::PLATFORM_P2P, "1.1.1.1:9998"), NetInfoStatus::Duplicate); | ||
| // Partial (i.e. different port) duplicates are allowed *within* a list | ||
| BOOST_CHECK_EQUAL(netInfo.AddEntry(Purpose::PLATFORM_P2P, "1.1.1.1:9997"), NetInfoStatus::Success); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mismatch with implementation: partial-duplicate rule for PLATFORM_P2P is tested as allowed but code forbids it
Tests expect partial duplicates (same addr, different port) to be allowed within PLATFORM_P2P (Line 199 expects Success), but ExtNetInfo::ProcessCandidate unconditionally rejects same-addr duplicates within a purpose. Adjust the implementation to allow partial duplicates for PLATFORM_P2P (and keep prohibiting them for CORE_P2P), or update the test expectation.
Proposed change in ExtNetInfo::ProcessCandidate (src/evo/netinfo.cpp):
- if (IsAddrDuplicate(candidate, entries)) {
- return NetInfoStatus::Duplicate;
- }
+ // Disallow partial duplicates (same addr, different port) only for CORE_P2P
+ if (purpose == Purpose::CORE_P2P && IsAddrDuplicate(candidate, entries)) {
+ return NetInfoStatus::Duplicate;
+ }🤖 Prompt for AI Agents
In src/test/evo_netinfo_tests.cpp around lines 186 to 200, the test expects
partial duplicates (same address, different port) to be allowed within the
PLATFORM_P2P purpose, but the current ExtNetInfo::ProcessCandidate
implementation forbids any same-address duplicates within a purpose. To fix
this, modify ExtNetInfo::ProcessCandidate in src/evo/netinfo.cpp to allow
partial duplicates for PLATFORM_P2P while continuing to prohibit them for
CORE_P2P, or alternatively update the test to expect duplicates to be rejected
for PLATFORM_P2P as well.
kwvg
force-pushed
the
netinfo_p5
branch
3 times, most recently
from
1b19183 to
946b0c1
Compare
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/evo/netinfo.cpp (1)
1-1: Address clang-format issues.The pipeline indicates formatting differences in this file. Please run clang-format to fix the formatting issues before merging.
Based on pipeline failures.
src/evo/netinfo.h (1)
1-1: Clang format check failed for this file.The pipeline indicates clang-format differences. Please run clang-format to fix formatting issues.
src/evo/deterministicmns.cpp (1)
1-1: Clang format check failed for this file.The pipeline indicates clang-format differences. Please run clang-format to fix formatting issues.
🧹 Nitpick comments (1)
src/evo/netinfo.cpp (1)
154-158: Confirm lexicographic fallback ordering for mixed CService/DomainPort
- The
operator<fallback usesToStringAddrPort(), which for CService wraps IPv6 addresses in brackets ([2001:db8::1]:8333) but for DomainPort printsdomain:portwithout brackets. As a result, entries sort by the first character ('['(91) precedes letters and digits), which may not align with expected ordering instd::set<NetInfoEntry>.- Verify that this ASCII-based ordering is acceptable for your use cases. If not, consider normalizing both formats (e.g., always omit brackets or compare raw address/port tuples) or introducing a custom comparator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/evo/deterministicmns.cpp(6 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/netinfo.cpp(13 hunks)src/evo/netinfo.h(5 hunks)src/evo/specialtxman.cpp(2 hunks)src/test/evo_netinfo_tests.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/deterministicmns.hsrc/evo/netinfo.hsrc/evo/deterministicmns.cppsrc/evo/netinfo.cppsrc/test/evo_netinfo_tests.cppsrc/evo/specialtxman.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/netinfo.hsrc/evo/deterministicmns.cppsrc/evo/netinfo.cppsrc/evo/specialtxman.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/evo_netinfo_tests.cpp
🧠 Learnings (2)
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/netinfo.hsrc/evo/deterministicmns.cppsrc/evo/netinfo.cppsrc/test/evo_netinfo_tests.cppsrc/evo/specialtxman.cpp
📚 Learning: 2025-06-16T17:59:55.669Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Applied to files:
src/evo/deterministicmns.cpp
🧬 Code graph analysis (4)
src/evo/deterministicmns.h (1)
src/evo/netinfo.h (1)
CService(285-295)
src/evo/netinfo.h (1)
src/evo/netinfo.cpp (24)
ValidateDomain(80-104)ValidateDomain(80-80)rhs(131-142)rhs(131-131)rhs(144-163)rhs(144-144)Validate(120-129)Validate(120-120)Validate(341-347)Validate(341-341)Validate(576-621)Validate(576-576)Set(106-118)Set(106-106)GetPort(183-194)GetPort(183-183)ToStringAddr(241-252)ToStringAddr(241-241)ToStringAddrPort(254-265)ToStringAddrPort(254-254)GetDomainPort(174-181)GetDomainPort(174-174)ValidateDomainPort(468-486)ValidateDomainPort(468-468)
src/evo/netinfo.cpp (3)
src/rpc/evo_util.h (1)
addr(39-50)src/evo/netinfo.h (5)
Validate(347-362)Validate(436-456)GetPort(262-300)service(240-244)domain(246-250)src/netaddress.h (1)
I2P_SAM31_PORT(118-118)
src/test/evo_netinfo_tests.cpp (2)
src/evo/netinfo.h (2)
service(240-244)domain(246-250)src/evo/netinfo.cpp (4)
rhs(131-142)rhs(131-131)rhs(144-163)rhs(144-144)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 1-1: Clang format differences found in src/evo/netinfo.h. Run clang-format to fix formatting.
src/evo/deterministicmns.cpp
[error] 1-1: Clang format differences found in src/evo/deterministicmns.cpp. Run clang-format to fix formatting.
src/evo/netinfo.cpp
[error] 1-1: Clang format differences found in src/evo/netinfo.cpp. Run clang-format to fix formatting.
src/test/evo_netinfo_tests.cpp
[error] 1-1: Clang format differences found in src/test/evo_netinfo_tests.cpp. Run clang-format to fix formatting.
🔇 Additional comments (33)
src/evo/netinfo.cpp (15)
21-63: LGTM with minor observation.The character filters, TLD lists, and port validation are well-defined. The privacy TLD list correctly includes
.i2pand.onion, and the bad TLD list appropriately excludes reserved and internal-use domains.One minor note:
IsAllowedPlatformHTTPPortcurrently only allows port 443. Consider whether port 80 should also be permitted for development/testing environments, or document why 443 is the exclusive choice.
80-104: LGTM!
DomainPort::ValidateDomaincorrectly implements RFC 1035 domain validation with appropriate length checks, character restrictions, dot positioning, and label validation. The status codes provide clear feedback for different failure modes.
106-118: LGTM!
DomainPort::Setcorrectly validates the port (rejecting zero), callsValidateDomain, and normalizes the domain to lowercase for case-insensitive duplicate detection.
120-129: LGTM!
DomainPort::Validateappropriately checks for malformed state (empty or non-lowercase address, zero port) before delegating toValidateDomain.
174-181: LGTM!The
GetDomainPortaccessor correctly mirrors the pattern ofGetAddrPort, returning an optional DomainPort only when the type is Domain and the data is valid.
183-194: LGTM!
GetPortcorrectly handles both CService and DomainPort types using std::visit, returning 0 for invalid/empty entries.
205-224: LGTM!
IsTriviallyValidcorrectly validates that the type code matches the underlying data type and that the data passes surface-level validity checks. The static_assert ensures only expected types are handled.
233-235: LGTM!
ToStringfor NetInfoEntry correctly handles the DomainPort case.
246-248: LGTM!
ToStringAddrcorrectly delegates to both CService and DomainPort.
259-261: LGTM!
ToStringAddrPortcorrectly delegates to both CService and DomainPort.
417-420: LGTM!
ProcessCandidatecorrectly rejects DomainPort entries for non-PLATFORM_HTTPS purposes, enforcing the constraint that domains are only allowed for Platform HTTPS API.
447-466: LGTM!
ValidateServicecorrectly extends support to CJDNS, I2P, IPv4, IPv6, and Tor addresses. The I2P port constraint (must beI2P_SAM31_PORT, which is 0) correctly enforces the SAM 3.1 limitation mentioned in the PR description.
468-486: LGTM!
ValidateDomainPortcorrectly validates domain entries:
- Checks domain validity
- Ensures port is non-zero and not a bad port (with exception for allowed Platform HTTP ports like 443)
- Rejects bad TLDs and privacy TLDs
- Requires the final label to be alphabetic (valid TLD)
605-613: LGTM!The
Validatemethod correctly enforces that DomainPort entries are only valid for PLATFORM_HTTPS purpose and delegates validation toValidateDomainPort.
500-539: Verify CJDNS support in netinfo, deterministic MNs, and serialization
- Confirm
MaybeFlipIPv6toCJDNSis applied on every CJDNS‐service flow in evo/netinfo.cpp.- Ensure
deterministicmns.h’s unique-property hash logic correctly includes NET_CJDNS.- Validate duplicate detection for CJDNS across ADDRv1 (all‐zeros fallback) and ADDRv2 (BIP155 CJDNS).
src/evo/deterministicmns.h (1)
443-453: LGTM! Critical fix for ADDRv2 address hashing.This change correctly addresses the hash contamination issue mentioned in the PR description. By using
ADDRV2_FORMATserialization for addresses that are not ADDRv1-compatible (Tor, I2P, CJDNS), the code ensures:
- The full address is hashed (not truncated)
- The unique properties map remains consistent
- Duplicate detection works correctly for extended addresses
The conditional application (only for non-ADDRv1-compatible addresses) maintains backward compatibility with existing hash map entries.
src/evo/specialtxman.cpp (2)
265-272: LGTM!The ProRegTx processing correctly extends uniqueness checks to DomainPort entries, mirroring the existing CService logic. The error handling appropriately rejects:
- Duplicate DomainPort entries
- Entries that are neither CService nor DomainPort
Based on learnings.
305-313: LGTM!The ProUpServTx processing correctly extends uniqueness checks to DomainPort entries. The logic appropriately allows the same DomainPort to be reused by the same ProTxHash (owner updating their own service) while rejecting duplicates from different owners.
Based on learnings.
src/test/evo_netinfo_tests.cpp (5)
43-52: Test coverage for domain handling looks correct.The test expectations correctly reflect the domain validation rules:
- Privacy network domains (.onion, .i2p, etc.) are allowed in ExtNetInfo across purposes
- Internet domains (example.com) are only allowed for PLATFORM_HTTPS
- MnNetInfo rejects all non-CORE_P2P purposes
148-164: Privacy network test vectors are well-structured.The test cases correctly validate:
- I2P addresses require port 0 (SAM 3.1 limitation as noted in PR description)
- Tor addresses require non-zero ports
- CJDNS addresses allow non-zero ports
212-227: Domain validation test cases cover edge cases well.The tests correctly validate:
- Port 443 is allowed for PLATFORM_HTTPS despite being privileged
- Port 80 is rejected (standard privileged port restriction)
- TLDs must be alphabetic (ICANN guideline)
- .local is prohibited
- Privacy network names without proper TLD are rejected
230-252: Privacy network type validation is thorough.The test correctly verifies that privacy network addresses are stored as
CServiceinternally and validates the correct network type (CJDNS, I2P, Tor) is registered.
436-497: Comprehensive DomainPort validation test coverage.The test cases thoroughly validate:
- Domain label length constraints (1-63 characters per label)
- Total domain length constraints (4-253 characters)
- Character restrictions (no spaces, apostrophes)
- Hyphen positioning rules (no leading/trailing hyphens in labels)
- Empty label detection (trailing/leading/extra delimiters)
- Dotless domain rejection
- Port 0 rejection
- Case normalization to prevent duplicates
Excellent coverage of edge cases and RFC compliance.
src/evo/netinfo.h (4)
109-181: DomainPort class design is well-structured.Key design highlights:
- Detailed validation status enum provides clear error reporting
- Domain stored in lowercase to prevent case-sensitivity duplicates
- Port serialized in BigEndian format (network byte order)
- Immutable after construction (no public setters beyond Set())
- Comparison operators enable proper uniqueness checks
The implementation follows RAII principles and provides strong validation guarantees.
194-203: NetInfoEntry extension follows existing patterns correctly.The variant extension to include
DomainPortmirrors the existingCServicehandling:
- Invalid entries remain typed as
Invalid- Constructor pattern is consistent
- Type safety maintained through variant
226-229: Serialization implementation is consistent and safe.The DomainPort serialization follows the established CService pattern:
- Type code 0x02 distinguishes Domain entries
- Invalid deserialization results in
Clear()(Invalid type)- Exception handling prevents crashes on malformed data
- Validation after deserialization ensures integrity
Also applies to: 245-250
387-387: ValidateDomainPort method declaration mirrors ValidateService pattern.The method signature is consistent with the existing validation API design.
src/evo/deterministicmns.cpp (6)
442-447: Domain-port handling in AddMN follows atomic rollback pattern correctly.The implementation mirrors the CService handling:
- Atomic updates with rollback checkpoint (mnUniquePropertyMapSaved)
- Proper error messages including domain address
- Exception thrown on duplicate prevents partial state
Consistent with existing error handling strategy.
Based on learnings
502-505: Domain-port handling in UpdateMN maintains atomicity.The update logic correctly:
- Deletes old domain entries before adding new ones
- Uses the same atomic rollback pattern via updateNetInfo lambda
- Returns descriptive error strings for duplicate detection
- Follows the established pattern for CService updates
Based on learnings
Also applies to: 515-518
599-604: Domain-port removal follows established pattern.The implementation correctly:
- Uses DeleteUniqueProperty for domain entries
- Maintains atomic rollback checkpoint
- Provides descriptive error messages
Based on learnings
1147-1151: Domain duplicate check in CheckProRegTx is correct.The implementation correctly:
- Checks for domain uniqueness across the masternode list
- Allows reuse only when collateral matches (replacement scenario)
- Returns appropriate error code
Consistent with CService duplicate handling.
1234-1237: Domain duplicate check in CheckProUpServTx prevents conflicts.The implementation correctly prevents a service update from using a domain already registered to a different masternode, mirroring the CService duplicate check.
941-980: CheckPlatformFields extension correctly handles version-specific validation.The implementation correctly:
- Adds
is_extended_addrparameter to distinguish ExtAddr versioned transactions- Enforces zero platform ports for ExtAddr (ports subsumed by netInfo)
- Maintains existing validation for legacy versions
- Callers correctly derive
is_extended_addrfromnVersion >= ProTxVersion::ExtAddrThe version-aware validation ensures backward compatibility while enforcing new constraints for ExtAddr.
Also applies to: 1087-1087, 1211-1211
| #include <netbase.h> | ||
| #include <streams.h> | ||
| #include <util/pointer.h> | ||
| #include <util/strencodings.h> |
There was a problem hiding this comment.
Clang format check failed for this file.
The pipeline indicates clang-format differences. Please run clang-format to fix formatting issues.
🤖 Prompt for AI Agents
In src/test/evo_netinfo_tests.cpp around line 13, the include line formatting
does not comply with clang-format; run your project's clang-format configuration
on this file (e.g., clang-format -i src/test/evo_netinfo_tests.cpp or use your
IDE formatter) to fix the whitespace/ordering according to the repo style, then
re-run the pipeline and commit the reformatted file.
| std::vector<std::string> labels{SplitString(addr, '.')}; | ||
| if (labels.size() < 2) { | ||
| return DomainPort::Status::BadDotless; | ||
| } | ||
| for (const auto& label : labels) { | ||
| if (label.empty() || label.length() > 63) { | ||
| return DomainPort::Status::BadLabelLen; | ||
| } | ||
| if (label.at(0) == '-' || label.at(label.length() - 1) == '-') { | ||
| return DomainPort::Status::BadLabelCharPos; | ||
| } | ||
| } |
There was a problem hiding this comment.
@coderabbitai could this be re-worked to avoid reallocating / allocating for the vector of string; maybe using a view instead or something like that?
There was a problem hiding this comment.
@PastaPastaPasta Yes, this can be refactored to avoid the allocation. Instead of using SplitString which creates a std::vector<std::string>, we can iterate through the string once and validate each label on-the-fly:
DomainPort::Status DomainPort::ValidateDomain(const std::string& addr)
{
if (addr.length() > 253 || addr.length() < 4) {
return DomainPort::Status::BadLen;
}
if (!MatchCharsFilter(addr, SAFE_CHARS_RFC1035)) {
return DomainPort::Status::BadChar;
}
if (addr.at(0) == '.' || addr.at(addr.length() - 1) == '.') {
return DomainPort::Status::BadCharPos;
}
// Validate labels without allocating
size_t label_start = 0;
size_t label_count = 0;
for (size_t i = 0; i <= addr.length(); ++i) {
if (i == addr.length() || addr[i] == '.') {
// Found end of label
size_t label_len = i - label_start;
if (label_len == 0 || label_len > 63) {
return DomainPort::Status::BadLabelLen;
}
if (addr[label_start] == '-' || addr[i - 1] == '-') {
return DomainPort::Status::BadLabelCharPos;
}
++label_count;
label_start = i + 1;
}
}
if (label_count < 2) {
return DomainPort::Status::BadDotless;
}
return DomainPort::Status::Success;
}This approach:
- Iterates through the string once
- Validates each label in-place without storing it
- Avoids allocating a vector and individual label strings
- Maintains the same validation logic
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
There was a problem hiding this comment.
I think the intent of SplitString is more explicit and the current implementation as-is is preferable.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/rpc_netinfo.py (1)
1-1: Update duplicate-scope logic in ExtNetInfo::ProcessCandidate
ProcessCandidate’s global IsAddrPortDuplicate check blocks duplicates across all purposes, but tests expect only intra-purpose duplicates to be rejected. Scope the port-duplicate check to the specific purpose’s entries rather thanm_all_entries. [src/evo/netinfo.cpp:420]
♻️ Duplicate comments (2)
src/test/evo_netinfo_tests.cpp (1)
13-13: Clang-format failed for this fileAlready flagged previously; please run clang-format.
#!/bin/bash clang-format -i src/test/evo_netinfo_tests.cpp git diff -- src/test/evo_netinfo_tests.cppsrc/evo/netinfo.cpp (1)
416-446: Per-purpose duplicate rules still needed.The duplicate detection at line 433 applies universally across all purposes, but the intended behavior (based on past review comments and test expectations) requires different rules:
- CORE_P2P: Reject partial duplicates (same address, different port)
- PLATFORM_P2P: Allow partial duplicates, reject only exact addr:port duplicates
Apply this diff to implement per-purpose duplicate rules:
if (IsAddrPortDuplicate(candidate)) { return NetInfoStatus::Duplicate; } if (candidate.GetDomainPort().has_value() && purpose != NetInfoPurpose::PLATFORM_HTTPS) { // Domains only allowed for Platform HTTPS API return NetInfoStatus::BadInput; } if (auto it{m_data.find(purpose)}; it != m_data.end()) { // Existing entries list found, check limit auto& [_, entries] = *it; if (entries.size() >= MAX_ENTRIES_EXTNETINFO) { return NetInfoStatus::MaxLimit; } - if (IsAddrDuplicate(candidate, entries)) { + // For CORE_P2P, prohibit duplicates by address (even if port differs). + // For PLATFORM_* purposes, only exact addr:port duplicates are disallowed. + if (purpose == NetInfoPurpose::CORE_P2P && IsAddrDuplicate(candidate, entries)) { return NetInfoStatus::Duplicate; } entries.push_back(candidate);Based on past review comments.
🧹 Nitpick comments (6)
src/evo/netinfo.h (2)
109-182: DomainPort API looks solid; minor printf nitUse unsigned format for uint16_t to avoid sign-mismatch.
- std::string ToStringAddrPort() const { return strprintf("%s:%d", m_addr, m_port); } + std::string ToStringAddrPort() const { return strprintf("%s:%u", m_addr, m_port); }
223-231: NetInfoEntry (de)serialization includes Domain; add Domain round‑trip testSerialization branches for Service and Domain are correct. Please add a unit test that encodes/decodes a Domain entry to catch regressions, similar to the Service case in netinfo_ser.
I’ve proposed a test addition in src/test/evo_netinfo_tests.cpp below.
Also applies to: 241-251
src/test/evo_netinfo_tests.cpp (1)
292-326: Add Domain NetInfoEntry (de)ser round‑trip testMirror the Service round‑trip with a Domain entry to ensure stable serialization.
@@ BOOST_AUTO_TEST_CASE(netinfo_ser) { // NetInfoEntry should be able to read and write ADDRV2 addresses CService service{}; service.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"); BOOST_CHECK(service.IsValid() && service.IsTor()); @@ BOOST_CHECK(entry == entry2 && entry2.GetAddrPort().value() == service); } + + { + // NetInfoEntry should (de)serialize Domain entries + CDataStream ds(SER_DISK, CLIENT_VERSION | ADDRV2_FORMAT); + DomainPort dom; + BOOST_CHECK_EQUAL(dom.Set("server-1.example.com", 443), DomainPort::Status::Success); + NetInfoEntry e_dom{dom}, e2{}; + ds << NetInfoEntry::NetInfoType::Domain << dom; + ds >> e2; + BOOST_CHECK(e_dom == e2); + BOOST_CHECK(!e2.IsEmpty() && e2.IsTriviallyValid()); + BOOST_CHECK(e2.GetDomainPort().has_value()); + BOOST_CHECK_EQUAL(e2.ToStringAddrPort(), "server-1.example.com:443"); + }src/evo/netinfo.cpp (3)
61-68: Consider using a named constant for the platform HTTPS port.The hardcoded
443value appears multiple times in the codebase. Consider defining a constant likePLATFORM_HTTPS_PORTto improve maintainability.+static constexpr uint16_t PLATFORM_HTTPS_PORT{443}; + bool IsAllowedPlatformHTTPPort(uint16_t port) { switch (port) { - case 443: + case PLATFORM_HTTPS_PORT: return true; } return false; }
86-110: Consider single-pass label validation to avoid allocation.The current implementation allocates a
std::vector<std::string>viaSplitString(line 97). A single-pass validation approach would improve performance by eliminating this allocation.Based on past review comments.
Here's a single-pass approach that validates labels without allocating:
DomainPort::Status DomainPort::ValidateDomain(const std::string& addr) { if (addr.length() > DOMAIN_MAX_LEN || addr.length() < DOMAIN_MIN_LEN) { return DomainPort::Status::BadLen; } if (!MatchCharsFilter(addr, SAFE_CHARS_RFC1035)) { return DomainPort::Status::BadChar; } if (addr.front() == '.' || addr.back() == '.') { return DomainPort::Status::BadCharPos; } - std::vector<std::string> labels{SplitString(addr, '.')}; - if (labels.size() < 2) { - return DomainPort::Status::BadDotless; - } - for (const auto& label : labels) { - if (label.empty() || label.length() > DOMAIN_LABEL_MAX_LEN) { - return DomainPort::Status::BadLabelLen; - } - if (label.front() == '-' || label.back() == '-') { - return DomainPort::Status::BadLabelCharPos; - } - } + + // Validate labels without allocating + size_t label_start = 0; + size_t label_count = 0; + + for (size_t i = 0; i <= addr.length(); ++i) { + if (i == addr.length() || addr[i] == '.') { + // Found end of label + size_t label_len = i - label_start; + + if (label_len == 0 || label_len > DOMAIN_LABEL_MAX_LEN) { + return DomainPort::Status::BadLabelLen; + } + if (addr[label_start] == '-' || addr[i - 1] == '-') { + return DomainPort::Status::BadLabelCharPos; + } + + ++label_count; + label_start = i + 1; + } + } + + if (label_count < 2) { + return DomainPort::Status::BadDotless; + } + return DomainPort::Status::Success; }
487-489: Consider optimizing TLD extraction.The TLD validation allocates a vector via
SplitStringjust to check the last label. You can extract the TLD more efficiently:- if (const auto labels{SplitString(addr, '.')}; !MatchCharsFilter(labels.at(labels.size() - 1), SAFE_CHARS_ALPHA)) { + // Extract TLD (everything after the last dot) + if (const auto last_dot = addr.rfind('.'); last_dot != std::string::npos) { + std::string_view tld{addr.data() + last_dot + 1, addr.length() - last_dot - 1}; + if (!MatchCharsFilter(tld, SAFE_CHARS_ALPHA)) { + return NetInfoStatus::BadInput; + } + } else { return NetInfoStatus::BadInput; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/evo/deterministicmns.cpp(6 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/netinfo.cpp(13 hunks)src/evo/netinfo.h(5 hunks)src/evo/specialtxman.cpp(2 hunks)src/test/evo_netinfo_tests.cpp(8 hunks)test/functional/rpc_netinfo.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/specialtxman.cpp
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/netinfo.hsrc/test/evo_netinfo_tests.cppsrc/evo/deterministicmns.hsrc/evo/deterministicmns.cppsrc/evo/netinfo.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/netinfo.hsrc/evo/deterministicmns.hsrc/evo/deterministicmns.cppsrc/evo/netinfo.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/evo_netinfo_tests.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_netinfo.py
🧠 Learnings (1)
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/netinfo.hsrc/test/evo_netinfo_tests.cppsrc/evo/deterministicmns.cppsrc/evo/netinfo.cpp
🧬 Code graph analysis (4)
src/evo/netinfo.h (1)
src/evo/netinfo.cpp (24)
ValidateDomain(86-110)ValidateDomain(86-86)rhs(137-148)rhs(137-137)rhs(150-169)rhs(150-150)Validate(126-135)Validate(126-126)Validate(347-353)Validate(347-347)Validate(582-627)Validate(582-582)Set(112-124)Set(112-112)GetPort(189-200)GetPort(189-189)ToStringAddr(247-258)ToStringAddr(247-247)ToStringAddrPort(260-271)ToStringAddrPort(260-260)GetDomainPort(180-187)GetDomainPort(180-180)ValidateDomainPort(474-492)ValidateDomainPort(474-474)
src/test/evo_netinfo_tests.cpp (2)
src/evo/netinfo.h (2)
service(240-244)domain(246-250)src/evo/netinfo.cpp (4)
rhs(137-148)rhs(137-137)rhs(150-169)rhs(150-150)
test/functional/rpc_netinfo.py (1)
test/functional/feature_dip3_deterministicmns.py (1)
register_mn(247-251)
src/evo/netinfo.cpp (2)
src/evo/netinfo.h (6)
m_port(152-152)Validate(347-362)Validate(436-456)GetPort(262-300)service(240-244)domain(246-250)src/netaddress.h (1)
I2P_SAM31_PORT(118-118)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/netinfo.h
[error] 1-1: Clang-format differences found in src/evo/netinfo.h. Please run the formatter to apply changes.
src/test/evo_netinfo_tests.cpp
[error] 1-1: Clang-format differences found in src/test/evo_netinfo_tests.cpp. Please run the formatter to apply changes.
src/evo/deterministicmns.cpp
[error] 1-1: Clang-format differences found in src/evo/deterministicmns.cpp. Please run the formatter to apply changes.
src/evo/netinfo.cpp
[error] 1-1: Clang-format differences found in src/evo/netinfo.cpp. Please run the formatter to apply changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check_merge
- GitHub Check: predict_conflicts
- GitHub Check: Build slim container / Build container
- GitHub Check: Build container / Build container
🔇 Additional comments (21)
src/evo/deterministicmns.h (1)
445-456: Unique-property hashing for ADDRv2 CService is correctUsing ADDRV2_FORMAT only when !IsAddrV1Compatible() prevents truncation/collisions while preserving compatibility for v1. Good fix.
Based on learnings
src/evo/deterministicmns.cpp (6)
443-448: Include DomainPort in unique-properties on AddMNCorrectly mirrors CService path and rolls back on conflict. Good.
503-507: UpdateMN handles DomainPort add/remove atomicallyThe purge-then-insert sequence for DomainPort is consistent with CService and uses rollback on error. Good.
Also applies to: 516-520
600-605: RemoveMN deletes DomainPort from unique mapAligned with other property deletions; rollback on failure. Good.
1165-1169: Duplicate check on ProRegTx extended to DomainPortCorrectly enforces uniqueness across collaterals. Good.
1253-1255: Duplicate check on ProUpServTx includes DomainPortBlocks migrating to a duplicate used by other MNs. Good.
1-1: Ensure clang-format style compliance for src/evo/deterministicmns.cpp
Unable to verify formatting automatically; please run clang-format locally and commit any necessary style fixes per the pipeline.src/test/evo_netinfo_tests.cpp (2)
212-227: Domain validation rules tests look rightCovers privileged ports, .local, numeric TLDs, and privacy suffix misuse. LGTM.
436-497: DomainPort unit tests comprehensiveLabel/TLD/len/case normalization paths covered well. LGTM.
test/functional/rpc_netinfo.py (2)
350-358: Functional coverage for privacy + internet domainsGood end-to-end acceptance test for Tor/I2P in P2P fields and clear domain in HTTPS. LGTM.
544-578: Uniqueness tests exercise CService and DomainPortValidates duplicate rejection across IP:port, Tor/I2P, and domain:port. Nice.
src/evo/netinfo.h (1)
1-1: Verify code style with clang-format
The sandbox lacks clang-format, so please run locally:clang-format -i src/evo/netinfo.h git diff src/evo/netinfo.hand commit any formatting changes.
src/evo/netinfo.cpp (9)
21-44: LGTM! Constants are well-defined.The domain validation constants properly reflect RFC 1035 specifications, and the TLD blacklists correctly exclude reserved and privacy network domains.
112-135: LGTM! Domain setting and validation logic is correct.The implementation properly validates domains, converts to lowercase for case-insensitive deduplication, and ensures stored state consistency.
150-169: LGTM! Cross-type comparison is well-designed.The lexicographical comparison using
ToStringAddrPort()(lines 160-163) ensures consistent ordering betweenCServiceandDomainPortentries, which is important for duplicate detection and sorting.
180-187: LGTM! GetDomainPort follows established pattern.The implementation correctly mirrors
GetAddrPort()and maintains consistency with the existing API design.
221-226: LGTM! DomainPort validation correctly integrated.The type checking and validity validation for
DomainPortproperly mirrors theCServicehandling, maintaining consistency.
459-469: LGTM! I2P port restriction correctly implemented.The I2P port validation (lines 460-464) correctly enforces the SAM 3.1 limitation requiring port 0, while other networks properly require non-zero, non-bad ports. This aligns with the PR objectives.
506-533: LGTM! Domain and privacy network routing is well-structured.The multi-path routing correctly handles:
- Privacy TLDs (
.onion,.i2p) viaCNetAddr::SetSpecial→CService- Regular domains via
DomainPort- IP addresses via
Lookup→CServicewith CJDNS detectionThis design aligns with the PR objectives for supporting internet domains and privacy networks.
611-619: LGTM! Domain purpose restriction is correctly enforced.The validation correctly restricts
DomainPortentries toPLATFORM_HTTPSpurpose at both entry addition (line 423) and validation (line 612), ensuring consistency.
1-668: Format src/evo/netinfo.cpp with clang-formatThe pipeline flagged style violations in this file; clang-format isn’t available here—please run clang-format (or manually apply the project’s formatting rules) on src/evo/netinfo.cpp and commit the updated file.
3 participants
This pull request implements the following:
Additional Information
Depends on feat: introduce basic extended addresses implementation, stop using
platform{HTTP,P2P}Port#6666Due to SAM 3.1 and earlier not supporting the specification of ports (see bitcoin#22112), we do not allow defining a port for I2P and it must always be zero.
Tor and I2P depend on the ADDRv2 format for storing address information. By default, serialization logic will use ADDRv1, which will truncate that information. We cannot migrate the unique properties map to ADDRv2 as it's hashed so we opt to use ADDRv1 unless it is not an ADDRv1 compatible type, in which case we use ADDRv2.
This fix is validated by
test_uniqueness()inrpc_netinfo.py, which validates that the unique properties map correctly recognizes duplicates while allowing unique entries to be registered (false-positive duplicate detection is a sign of potential truncation and map contamination).Breaking Changes
None expected.
Checklist