feat(rpc): allow reporting multiple addresses with new `addresses` field, deprecate `service` field, allow submitting multiple addresses #6674

kwvg · 2025-05-18T17:00:44Z

Motivation

To enable including functional tests with the extended addresses (ExtAddr) implementation (i.e. dash#6666), we need the ability to specify and retrieve multiple addresses. The current RPCs do not allow for that and this pull request aims to remedy it.

This particular pull request does not include:

Compatibility logic to allow reporting Platform-specific addresses using legacy fields (will be submitted after ExtAddr impl)
Compatibility logic to allow deriving Platform ports from extended address entries to populate deprecated fields (will be submitted after ExtAddr impl)
The ability to submit multiple Platform addresses (done in conjunction with ExtAddr impl)

Additional Information

Depends on refactor: model interface after MnNetInfo and support switching impls, add new ProTx version #6665
Depends on test: implement helper functions for ProTx creation, allow working with assert_raises_rpc_error, add ability to withhold submission to all ProTx creation RPCs #6720
Dependency for feat: introduce basic extended addresses implementation, stop using platform{HTTP,P2P}Port #6666
⚠️ The format for the field replacing service, addresses is not stable and is changed in dash#6666 to allow reporting Platform-specific addresses. This should be acceptable as the whole extended addresses changeset across multiple PRs is expected to be part of a major release but is mentioned here for the sake of posterity.
As IsDeprecatedRPCEnabled() is defined in libbitcoin_node (see rpc/server.cpp, source), it is not available to libbitcoin_common, where special transaction and network information source files are included. To get around this, a practically identical function, IsServiceDeprecatedRPCEnabled() is defined and used in non-RPC code.
- It is located in evo/netinfo.cpp instead of a more natural rpc/evo_util.cpp to avoid unnecessary circular dependencies.

Breaking Changes

The input field ipAndPort has been renamed to coreP2PAddrs.
- coreP2PAddrs can now, in addition to accepting a string, accept an array of strings, subject to validation rules.
The key service has been deprecated for some RPCs (decoderawtransaction, decodepsbt, getblock, getrawtransaction,
gettransaction, masternode status (only for the dmnState key), protx diff, protx listdiff) and has been replaced
with the field addresses.
- The deprecated field can be re-enabled using -deprecatedrpc=service but is liable to be removed in future versions
  of Dash Core.
- This change does not affect masternode status (except for the dmnState key) as service does not represent a payload
  value but the external address advertised by the active masternode.

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

github-actions · 2025-05-28T15:10:38Z

This pull request has conflicts, please rebase.

…nfo`) in functional tests, implement helpers, add type annotations 0532baf refactor: add helper for finding collateral vout (Kittywhiskers Van Gogh) 89b2548 refactor: add helper for transaction burial (Kittywhiskers Van Gogh) f3777c5 refactor: introduce address generation helper, make constructor minimal (Kittywhiskers Van Gogh) fa56126 refactor: add type annotations for `MasternodeInfo` members (Kittywhiskers Van Gogh) dfca0ee refactor: use `MasternodeInfo` in feature_dip3_deterministicmns.py (Kittywhiskers Van Gogh) cd7ffcf refactor: store only the node index in `MasternodeInfo` (Kittywhiskers Van Gogh) 07c7319 refactor: use `nodeIdx` instead of `node.index` with `MasternodeInfo` (Kittywhiskers Van Gogh) 08e81d8 refactor: store P2P port instead of full address in `MasternodeInfo` (Kittywhiskers Van Gogh) 3e25b30 refactor: store the funding address in `MasternodeInfo` (Kittywhiskers Van Gogh) 08d18d7 refactor: annotate usage of `MasternodeInfo` (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on functional tests for [dash#6666](#6666) (which in turn, relies on the foundation built by [dash#6674](#6674), see `rpc_netinfo.py`, [source](https://github.com/dashpay/dash/blob/c788531d35c48820af951799ff8b67ee2dadb8b3/test/functional/rpc_netinfo.py)), the existing test infrastructure, while well optimized for testing interactions _between_ masternodes, didn't offer enough flexibility for testing _creation_ of masternodes. The tests that need to be implemented for extended addresses mimic `feature_dip3_deterministicmns.py` ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/feature_dip3_deterministicmns.py)) in objective and while taking cues from it, it was found that instead of the `MasternodeInfo` used in most of the codebase ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1138-L1153)), `feature_dip3_deterministicmns.py` implements its own object, `Masternode` ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/feature_dip3_deterministicmns.py#L223-L227)). In a similar vein, `rpc_netinfo.py`, as implemented in c788531, implemented its own variant, `Node` ([source](https://github.com/dashpay/dash/blob/c788531d35c48820af951799ff8b67ee2dadb8b3/test/functional/rpc_netinfo.py#L31-L194)) to address the testing needs for extended addresses. It became clear that without additional intervention, the test suite would have three different ways of representing masternode information (`MasternodeInfo`, `Masternode`, `Node`) and that it would be more beneficial to consolidate all three approaches together. So, taking cue from `Node` (from `rpc_netinfo.py`, not included in `develop` as of this writing, [source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/rpc_netinfo.py)), this pull request aims to clean up, consolidate and improve upon `MasternodeInfo` in an attempt to remove the need for `Node`. ## Additional Information * PEP 526 ("Syntax for Variable Annotations") prohibit annotation of variables defined by a `for` or `with` statement ([source](https://peps.python.org/pep-0526/#where-annotations-aren-t-allowed)), to get around that, the syntax for type comments ([source](https://peps.python.org/pep-0484)) as defined in PEP 484 ("Type Hints") have been used instead. * The decision to remove `node` from `MasternodeInfo` despite its convenience was rooted in the observation that there are cases where `nodeIdx` and `node` are assigned separately ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1480), [source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1499)), which highlighted the fact that `MasternodeInfo` has no control over the lifetime of the `TestNode` instance as it is ultimately controlled by `BitcoinTestFramework`. To avoid potential error and to make it syntactically clear that ownership is vests with `BitcoinTestFramework`, `node` has been removed and replaced with `get_node()`, which alongside some basic sanity checks, is just an alias for `self.nodes[mn.nodeIdx]` but is still less tedious to type or read. * The reason why functions like `generate_addresses()` accept a separate `TestNode` ([source](https://github.com/dashpay/dash/blob/c5444b3f7c06aa5b060fd542773df6dd5a9bae1f/test/functional/test_framework/test_framework.py#L1160-L1174)) is due to the practice of using a single node to instantiate all masternodes instead of keeping them as self-contained instances that mine their own blocks and fund their own collaterals (for instance, `DashTestFramework` uses `nodes[0]` to setup all the masternodes even if the masternodes themselves had a different index). * The decision to replace `addr` with `nodePort` has been inspired by the need to register masternodes with non-loopback addresses (like in `feature_dip3_deterministicmns.py` or `rpc_netinfo.py`). As a node that is intended to be used will always need the same standardized loopback address, we can safely resort to only storing the port number and save a few `p2p_port()` calls along the way. ## Breaking Changes None expected. Affects only functional tests. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 0532baf UdjinM6: utACK 0532baf Tree-SHA512: c2b5b0f9200c2714159a5e0f6f52174e38eae426c01620cf15e9adb8b8e67bbb078d658edb21b052a1fc9896ba098cffa02248aaa980be5d5cb145e9929c4568

github-actions · 2025-06-12T17:20:32Z

This pull request has conflicts, please rebase.

…ow working with `assert_raises_rpc_error`, add ability to withhold submission to all ProTx creation RPCs 28aaf1d test: allow `assert_raises_rpc_error` with helper functions (Kittywhiskers Van Gogh) 5aeec2a test: validate that the 'submit' argument works as intended (Kittywhiskers Van Gogh) 43318a4 rpc: extend the ability to withhold submission to all ProTxes (Kittywhiskers Van Gogh) 968698b test: add helper for `update_service{,_evo}` calls (Kittywhiskers Van Gogh) 1493d67 test: add helper for `update_registrar{,_legacy}` calls (Kittywhiskers Van Gogh) 6ba685f test: add helper for `revoke` calls (Kittywhiskers Van Gogh) aa96ace test: add helper for `register_fund{,_evo,_legacy}` calls (Kittywhiskers Van Gogh) dbce9c9 test: add helper for `register{,_evo,_legacy}` calls (Kittywhiskers Van Gogh) Pull request description: ## Motivation Continuing on the work of [dash#6718](#6718), this pull request aims to do two things: * Leverage the amount of information stored in `MasternodeInfo` to make ProTx creation simpler by only needing to specify the values that need to be _updated_ instead of needing to read most of the fields in `MasternodeInfo` to construct the RPC call * This also comes with the bonus of `MasternodeInfo` knowing _which_ RPC call to make depending on whether the node is legacy regular, basic regular or basic evonode. * As ordering is managed by `register{,_fund}`/`revoke`/`update_{registrar,service}()`, the arguments can be supplied in any desired order (or in RPC call order as the arguments have been arranged in that way except for `submit`, which is placed earlier and must be specified mandatorily). * Allow testing both failure and success cases by introducing both support for `assert_raises_rpc_error` in these functions _and_ implementing support for withholding submission of a ProTx for non-ProRegTxes. These changes benefit the functional tests needed for extended addresses, where we need to test for _changes_ in input validation logic based on deployment status, evaluating both success and failure cases. For instance, this pull request reduces the amount of logic in `Node` (in `rpc_netinfo.py`, introduced in [dash#6674](#6674)) by a fair amount ([diff](https://github.com/dashpay/dash/compare/c788531d35c48820af951799ff8b67ee2dadb8b3..232eb493484b5632b48320d6bbce6c459ee17602#diff-8bacb0d5b8909182c16ac6696c69f805c5cddc3deebc8eb4c0fac77b37aabd5d)) and could see further reduction in future iterations. ## Additional Information * Depends on #6718 * Dependency for #6674 * As `fundsAddr` is an optional field in some ProTx RPCs that trigger different behavior if not supplied ([source](https://github.com/dashpay/dash/blob/ee34525451fbc3efffba6afadf812d2b7f5b7782/src/rpc/evo.cpp#L1237-L1246)), special handling has been introduced to make sure that if this behavior is desired, the argument itself is omitted as replacing it with a blank string does not trigger the intended behavior. * This means that for `revoke()` and `update_registrar()`, transaction withholding (setting `submit` to `false`) is not allowed as this would require setting `fundsAddr` to an empty string, which does not trigger the expected fallback behavior. * This does not extend to `update_service()` where a blank string is submitted as `protx update_service` does not seem to behave adversely when supplied a blank string. * To test that the `submit` argument works as intended in newly updated and existing RPCs, if `submit` is `True` and no expected error code and message are supplied, at random, `submit` is set to `False` and the hex serialized signed transaction output is broadcast using `sendrawtransaction` (see `use_srd` variable). ## Breaking Changes * A new optional field `submit` has been introduced to the `protx revoke`, `protx update_registrar`, `protx update_service` RPCs. It behaves identically to `submit` in `protx register` or `protx register_fund`. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 28aaf1d UdjinM6: utACK 28aaf1d Tree-SHA512: 2346c3ebcd9b4384a4884f0ad2e1f675fc23fc15575973109233770f006e1af98d3c22c0bbb819589a6f97e277a580e4f90b39d940fbe99f8a86961783d72e8e

github-actions · 2025-06-18T13:41:37Z

This pull request has conflicts, please rebase.

github-actions · 2025-06-29T18:57:15Z

This pull request has conflicts, please rebase.

…switching impls, add new ProTx version 3e65628 evo: introduce new ProTx version for extended addresses (`ExtNetInfo`) (Kittywhiskers Van Gogh) b4fa7a2 evo: add helper to get highest ProTx version based on deployment status (Kittywhiskers Van Gogh) c7948e6 evo: allow deciding `NetInfoInterface` impl based on object version (Kittywhiskers Van Gogh) 22d9e93 evo: use interface shared ptr for `netInfo` instead of implementation (Kittywhiskers Van Gogh) f212caa evo: introduce `NetInfoInterface`, make `MnNetInfo` an implementation (Kittywhiskers Van Gogh) 0b04978 evo: make `netInfo` a shared pointer (Kittywhiskers Van Gogh) 0a4e726 util: add deep comparison helpers for shared_ptr (Kittywhiskers Van Gogh) c4f1175 evo: cleanup CDeterministicMN{,List,ListDiff,StateDiff} ser logic (Kittywhiskers Van Gogh) Pull request description: ## Motivation To enable the switch to extended addresses, the current `CService` entry was first encapsulated in `MnNetInfo` and exposed through a public interface (see [dash#6627](#6627)) that restricted access to the underlying type, then the underlying type of the address was abstracted away through `NetInfoEntry` (see [dash#6629](#6629)), allowing us to store different types of addresses. This pull request finally abstracts away `MnNetInfo` as an implementation detail and sets the stage for alternate implementations (i.e. extended addresses) to be selected in based on the version number. This should allow the switchover logic to be insulated from most of the codebase. ## Additional Information * Depends on #6669 * Depends on #6629 * Dependency for #6674 * Since [dash#6579](#6579) got rid of legacy `CDeterministicMNState` serialization, there is no more logic that can attempt legacy deserialization, making the version check ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/evo/deterministicmns.h#L75-L78)) vestigial. We can safely remove the leftover version fields and simplify the serialization logic. It is done as part of this PR because the serialization version is manipulated with `OverrideStream` to ensure proper ADDRv2 serialization ([source](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/addrman.cpp#L175)) and as the extended implementation uses it for that reason, the otherwise-vestigial version-based check could now trigger a crash, so it has been removed. * ~~As implementing operators in virtual classes (needed to allow switching implementations) is not possible (unlike regular pointers where a `lhs && rhs ? *lhs == *rhs : lhs == rhs` would be enough), a separate equality function `NetInfoInterface::IsEqual()` has been implemented that attempts to downcast `lhs` and `rhs` to the same type and returns the implementation-specific equality result.~~ A solution that uses `typeid` to help define comparison operators has been implemented but `std::shared_ptr` does not deep compare, to aid with that, the deep comparison helpers `util::shared_ptr_{,not_}equal` have been implemented and can be used with any pair of `shared_ptr`s so long as their underlying types define the necessary operators. Care needs to be taken to ensure that all equality comparisons involving `netInfo` use those functions. * A new ProTx version has been introduced for the upcoming extended addresses implementation. It is only applicable to `CProRegTx` and `CProUpServTx` as no other special transaction stores `netInfo` and is therefore, unaffected by the switchover. No code has been implemented in this PR that uses this new version, that is reserved for the PR that introduces the extended implementation. * Additionally, as the transaction version is used to determine the underlying implementation, make sure that `nVersion` is set *before* calling `MakeNetInfo()` (also remember to call `MakeNetInfo()` as the default constructor will set `netInfo` to `nullptr` and a missing call will result in a `nullptr` dereference when `netInfo` is eventually accessed). This is only relevant if a transaction is being constructed in-place as deserialization logic handles this for you. * ~~An earlier approach involved setting the new version only for `CProRegTx` and `CProUpServTx`. In light of _multiple_ ProTx types being able to modify the version of the masternode state (`pdmnState`), this may prove to be more dangerous than beneficial. In light of that, the version bump now affects all ProTx types but is still only effective in `CProRegTx` and `CProUpServTx`.~~ See [comment](#6665 (comment)). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3e65628 Tree-SHA512: 2675712351c9bbcf8bff2c6f032df40a52503abc62f8cd74b4b4f70bbcca0b68a32c1f4e1310c8a674bad1e605a2949337a7ab862a2129c4753cf8f4dbfa8789

coderabbitai · 2025-07-09T14:02:57Z

Walkthrough

This set of changes updates the handling, validation, and serialization of masternode network address information across the codebase. The legacy single-string service field is deprecated and replaced with a new addresses field, which supports multiple addresses and is always present in relevant RPC outputs and JSON serializations. The RPC interface for registering and updating masternodes now accepts an array parameter coreP2PAddrs instead of a single ipAndPort string. Helper functions for processing and validating network address and port inputs are introduced and integrated. Corresponding updates are made to documentation, test frameworks, and functional tests to reflect and verify the new behavior and data structures.

✨ Finishing Touches

📝 Generate Docstrings

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/functional/rpc_netinfo.py (1)

255-276: Simplify dictionary key existence checks.

Use the more Pythonic key in dict pattern instead of key in dict.keys().

-        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['updatedMNs'][0][proregtx_hash].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['updatedMNs'][0][proregtx_hash]

Apply the same pattern to all other occurrences in lines 269-276 and 291-295.

Also applies to: 291-295

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9f52ac and 798dea3.

📒 Files selected for processing (21)

contrib/seeds/makeseeds.py (1 hunks)
doc/release-notes-6665.md (1 hunks)
src/Makefile.am (2 hunks)
src/coinjoin/client.cpp (1 hunks)
src/evo/core_write.cpp (2 hunks)
src/evo/dmnstate.cpp (2 hunks)
src/evo/netinfo.cpp (3 hunks)
src/evo/netinfo.h (4 hunks)
src/evo/simplifiedmns.cpp (2 hunks)
src/rpc/coinjoin.cpp (1 hunks)
src/rpc/evo.cpp (14 hunks)
src/rpc/evo_util.cpp (1 hunks)
src/rpc/evo_util.h (1 hunks)
src/rpc/masternode.cpp (1 hunks)
src/rpc/quorums.cpp (1 hunks)
test/functional/feature_dip3_deterministicmns.py (1 hunks)
test/functional/rpc_netinfo.py (1 hunks)
test/functional/rpc_quorum.py (1 hunks)
test/functional/test_framework/test_framework.py (10 hunks)
test/functional/test_runner.py (1 hunks)
test/util/data/non-backported.txt (1 hunks)

🧰 Additional context used

🧠 Learnings (21)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

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.

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.

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.

test/functional/rpc_netinfo.py (2)

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.

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.

src/evo/simplifiedmns.cpp (3)

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.

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.

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.

test/util/data/non-backported.txt (2)

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.

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

test/functional/rpc_quorum.py (2)

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.

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.

src/rpc/masternode.cpp (1)

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.

contrib/seeds/makeseeds.py (1)

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.

src/evo/core_write.cpp (2)

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.

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.

src/coinjoin/client.cpp (1)

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

src/evo/netinfo.h (1)

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.

src/Makefile.am (1)

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

src/evo/netinfo.cpp (3)

Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.

Learnt from: kwvg
PR: dashpay/dash#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.

Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.

src/rpc/coinjoin.cpp (1)

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.

test/functional/test_runner.py (1)

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.

src/evo/dmnstate.cpp (3)

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.

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.

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.

doc/release-notes-6665.md (2)

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

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.

test/functional/feature_dip3_deterministicmns.py (3)

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.

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.

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.

src/rpc/quorums.cpp (2)

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.

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.

src/rpc/evo.cpp (4)

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.

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.

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

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.

src/rpc/evo_util.cpp (4)

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.

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

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.

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.

test/functional/test_framework/test_framework.py (2)

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.

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.

🧬 Code Graph Analysis (3)

test/functional/rpc_quorum.py (1)

test/functional/test_framework/util.py (1)

assert_equal (64-69)

src/coinjoin/client.cpp (3)

src/wallet/interfaces.cpp (15)

m_wallet (152-155)

m_wallet (160-160)

m_wallet (169-169)

m_wallet (170-170)

m_wallet (176-176)

m_wallet (177-177)

m_wallet (433-436)

m_wallet (437-440)

m_wallet (512-512)

m_wallet (513-513)

m_wallet (514-514)

m_wallet (515-515)

m_wallet (516-516)

m_wallet (521-521)

m_wallet (563-563)

src/coinjoin/interfaces.cpp (2)

obj (50-53)

obj (50-50)

src/coinjoin/client.h (2)

obj (229-229)

obj (352-352)

test/functional/feature_dip3_deterministicmns.py (3)

test/functional/test_framework/test_framework.py (2)

update_service (1413-1467)

generate (798-801)

src/rpc/evo.cpp (2)

protx_info (1490-1551)

protx_info (1490-1490)

test/functional/test_framework/util.py (1)

assert_equal (64-69)

🪛 Ruff (0.11.9)

test/functional/rpc_netinfo.py

255-255: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

256-256: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

257-257: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

258-258: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

259-259: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

269-269: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

270-270: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

271-271: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

272-272: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

273-273: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

276-276: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

291-291: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

292-292: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

293-293: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

294-294: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

295-295: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

🪛 GitHub Actions: Clang Diff Format Check

src/evo/netinfo.cpp

[error] 242-251: Clang format differences found: function MnNetInfo::ToJson() formatting does not match style guidelines.

src/rpc/coinjoin.cpp

[error] 413-514: Clang format differences found: large block of code formatting inconsistent with style guidelines.

src/rpc/evo.cpp

[error] 69-150: Clang format differences found: multiple RPC argument list formatting inconsistencies.

[error] 393-630: Clang format differences found: multiple RPCHelpMan function formatting inconsistencies.

[error] 912-978: Clang format differences found: protx update_service and update_service_evo RPC functions formatting inconsistent.

🪛 LanguageTool

doc/release-notes-6665.md

[grammar] ~1-~1: Use correct spacing
Context: Updated RPCs ------------ * The input field ipAndPort has been re...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

[grammar] ~5-~5: There might be a mistake here.
Context: ...ed to coreP2PAddrs. * coreP2PAddrs can now, in addition to accepting a string,...

(QB_NEW_EN_OTHER)

[grammar] ~5-~5: Use correct spacing
Context: ...array of strings, subject to validation rules. * The key service has been deprecated f...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

[grammar] ~7-~7: Use correct spacing
Context: ...transaction, decodepsbt, getblock, getrawtransaction, gettransaction, masternode status(only for thedmnS...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

[grammar] ~8-~8: Use correct spacing
Context: ...x diff, protx listdiff) and has been replaced with the field addresses`. * The deprecat...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

[grammar] ~10-~10: Use correct spacing
Context: ...` but is liable to be removed in future versions of Dash Core. * This change does not aff...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

[grammar] ~12-~12: Use correct spacing
Context: ... key) as service does not represent a payload value but the external address advertised by ...

(QB_NEW_EN_OTHER_ERROR_IDS_5)

⏰ 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: win64-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_multiprocess-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_sqlite-build / Build source

🔇 Additional comments (48)

test/util/data/non-backported.txt (1)

27-27: LGTM! Appropriate addition to non-backported files.

The addition of src/rpc/evo_util.* is correct since these files contain Dash-specific evolution RPC utilities that don't exist in Bitcoin Core.

test/functional/test_runner.py (1)

236-236: LGTM! Proper integration of new test into test suite.

The addition of rpc_netinfo.py to the BASE_SCRIPTS list correctly integrates the new test for network address handling validation into the default test suite.

test/functional/rpc_quorum.py (1)

31-31: LGTM! Correct transition from deprecated service field to addresses.

The change from member["service"] to member['addresses'][0] properly updates the test to use the new addresses field format, aligning with the broader deprecation of the service field.

contrib/seeds/makeseeds.py (1)

167-167: LGTM! Correct adaptation to new addresses field format.

The change from mn['state']['service'] to mn['state']['addresses'][0] properly updates the seed generation script to use the new addresses field format, ensuring compatibility with the updated masternode data structure.

src/Makefile.am (2)

303-303: LGTM! Proper addition of new header to build system.

The addition of rpc/evo_util.h to the BITCOIN_CORE_H list ensures the new utility header is properly included in the build process.

804-804: LGTM! Correct integration of new source file.

The addition of rpc/evo_util.cpp to the libbitcoin_common_a_SOURCES list properly integrates the new utility source file into the common library, ensuring it's compiled and available for use by the RPC system.

src/evo/core_write.cpp (2)

70-73: LGTM: Proper implementation of service field deprecation.

The conditional inclusion of the "service" field only when IsServiceDeprecatedRPCEnabled() returns true, alongside the unconditional addition of the "addresses" field, correctly implements the deprecation pattern described in the PR objectives.

120-123: LGTM: Consistent deprecation pattern applied.

The same deprecation pattern is correctly applied to CProUpServTx::ToJson(), ensuring consistency across ProTx transaction types.

test/functional/feature_dip3_deterministicmns.py (2)

275-275: LGTM: Correct update for new coreP2PAddrs parameter.

The test correctly updates to use the new coreP2PAddrs parameter as a list, which aligns with the API changes that replace the single ipAndPort string with an array of addresses.

280-281: LGTM: Proper validation of new addresses field.

The assertions correctly validate the first element of the addresses array in both protx_info and masternode list outputs, which is the expected behavior for the new multi-address format.

src/rpc/quorums.cpp (1)

209-212: RPC deprecation flag usage is consistent

The use of IsDeprecatedRPCEnabled("service") in RPC files and IsServiceDeprecatedRPCEnabled() in Evo code is intentional. The latter is simply a wrapper (per src/evo/netinfo.h) for use outside of RPC handlers. No changes needed here—approving as-is.

src/rpc/coinjoin.cpp (1)

438-444: LGTM: Documentation correctly reflects API changes.

The RPC help documentation properly documents the deprecation of the "service" field and the addition of the new "addresses" field, which aligns with the broader API changes in this PR.

src/rpc/masternode.cpp (1)

626-629: LGTM: Correct implementation of service field deprecation pattern.

The implementation properly follows the deprecation pattern by:

Conditionally including the deprecated address field only when IsDeprecatedRPCEnabled("service") is true

Always including the new addresses field with comprehensive network information via ToJson()

Maintaining backward compatibility while enabling the transition to multiple address support

This change aligns with the PR objectives and is consistent with similar modifications across the codebase.

src/coinjoin/client.cpp (1)

1886-1889: LGTM: Consistent implementation of service field deprecation.

The implementation correctly follows the deprecation pattern by:

Conditionally including the deprecated service field only when rpcEnableDeprecated("service") is true

Always including the new addresses field with comprehensive network information

Using the appropriate wallet chain interface to check the deprecation flag

This change is consistent with the similar modification in src/rpc/masternode.cpp and aligns with the broader effort to replace single-address service fields with multi-address addresses fields.

src/evo/simplifiedmns.cpp (2)

9-9: Include reorganization looks good.

The include reorganization and addition of evo/netinfo.h and evo/specialtx.h are appropriate for the new JSON serialization functionality.

Also applies to: 11-12

84-87: Consistent implementation of service field deprecation.

The conditional inclusion of the "service" field based on IsServiceDeprecatedRPCEnabled() and the unconditional inclusion of the "addresses" field aligns perfectly with the PR objectives to deprecate the service field while maintaining backward compatibility.

src/evo/netinfo.cpp (3)

14-14: Include addition is appropriate.

The addition of univalue.h is necessary for the new JSON serialization functionality.

38-43: Well-implemented JSON conversion function.

The ArrFromService function correctly converts a CService to a JSON array containing the address and port string. The implementation is clean and follows the expected JSON format.

45-49: Correct implementation of deprecated RPC flag checking.

The IsServiceDeprecatedRPCEnabled function properly checks for the "service" flag in the deprecatedrpc arguments, enabling backward compatibility control as intended.

src/evo/dmnstate.cpp (2)

42-45: Consistent deprecation pattern implementation.

The conditional inclusion of the "service" field and unconditional inclusion of the "addresses" field in CDeterministicMNState::ToJson properly implements the deprecation strategy while maintaining backward compatibility.

79-82: Consistent implementation in diff serialization.

The same deprecation pattern is correctly applied in CDeterministicMNStateDiff::ToJson, ensuring consistency across all JSON serialization methods.

src/evo/netinfo.h (4)

16-16: Appropriate forward declaration.

The forward declaration of UniValue is a good practice to avoid unnecessary include dependencies in the header file.

56-57: Good documentation and function declaration.

The comment clearly explains the purpose of the function and its relationship to the existing IsDeprecatedRPCEnabled("service") function. The declaration is appropriate for use outside RPC code.

149-149: Proper interface extension.

Adding the ToJson method as a pure virtual function to the NetInfoInterface is the correct approach for extending the interface to support JSON serialization.

206-206: Consistent override declaration.

The ToJson method override in MnNetInfo properly implements the interface requirement.

src/rpc/evo_util.h (4)

5-6: Proper include guard implementation.

The include guards are correctly implemented following the expected naming convention.

8-8: Appropriate forward declaration.

The forward declaration of UniValue follows the same pattern as other headers in the codebase.

10-11: Well-designed template function declaration.

The ProcessNetInfoCore template function declaration is clear and includes the necessary parameters for processing network info with an optional flag.

13-14: Complementary template function declaration.

The ProcessNetInfoPlatform template function declaration appropriately handles platform-specific P2P and HTTP port parameters.

doc/release-notes-6665.md (1)

1-14: LGTM!

The release notes clearly document the RPC interface changes, including the renaming of ipAndPort to coreP2PAddrs and the deprecation of the service field.

src/rpc/evo_util.cpp (2)

16-58: LGTM!

The ProcessNetInfoCore function correctly handles both string and array inputs for core P2P addresses with comprehensive validation and descriptive error messages.

60-79: Well-structured port validation logic.

The use of a lambda function to avoid code duplication between P2P and HTTP port validation is a good design choice. Port range validation is correctly implemented.
src/rpc/evo.cpp (3)

718-718: Good refactoring to use centralized validation logic.

The replacement of direct AddEntry calls with ProcessNetInfoCore improves code maintainability and ensures consistent validation across all RPC methods.

Also applies to: 1009-1009

750-750: Excellent centralization of platform port validation.

Replacing manual validation with ProcessNetInfoPlatform ensures consistent port validation across all RPC methods and eliminates code duplication.

Also applies to: 1020-1020

95-109: Fix formatting inconsistencies in RPC argument definitions

The CI is failing due to mismatched indentation and line breaks in the RPCArg initializers for coreP2PAddrs and coreP2PAddrs_update. Please run clang-format on this section to align it with the surrounding code style:

• File: src/rpc/evo.cpp
– Lines ~95–109: RPCArg entry for {"coreP2PAddrs", …}
– Lines ~110–124: RPCArg entry for {"coreP2PAddrs_update", …}

Suggested command:
clang-format -i src/rpc/evo.cpp
Then commit the updated formatting to resolve the CI failures.
test/functional/rpc_netinfo.py (2)

29-144: Well-structured test helper class.

The Node class provides excellent abstraction for masternode lifecycle management in tests, with proper handling of both regular and EvoNode types.

172-214: Comprehensive input validation test coverage.

The validation tests thoroughly cover edge cases including invalid array inputs, empty strings, port range validation, and legacy format handling.

test/functional/test_framework/test_framework.py (11)

26-26: Good addition of Union import for flexible type annotations.

The Union import is necessary for the new parameter type annotations that support both string and list inputs, improving API flexibility while maintaining type safety.

1219-1219: Excellent parameter redesign for multiple address support.

The change from ipAndPort: Optional[str] to coreP2PAddrs: Union[str, List[str], None] is well-designed, providing backward compatibility while enabling multiple address specifications. The parameter name is also more descriptive and aligned with the broader codebase changes.

1239-1239: Consistent default value handling.

The fallback logic coreP2PAddrs or [f'127.0.0.1:{self.nodePort}'] properly converts the default behavior to use a list format, maintaining consistency with the new parameter type while preserving the original functionality.

1274-1274: Consistent method signature update.

The register_fund method signature is updated consistently with the register method, maintaining API coherence across the MasternodeInfo class.

1302-1302: Proper default value handling maintained.

The same consistent pattern for default value handling is applied here, ensuring uniform behavior across all updated methods.

1413-1413: Complete method signature consistency.

All three core methods (register, register_fund, update_service) now have consistent parameter signatures, providing a uniform API for address specification.

1445-1445: Uniform implementation pattern.

The same robust default value handling pattern is consistently applied across all methods, demonstrating good code maintainability.

1646-1646: Proper usage update for new parameter format.

The change from single string to list format (['127.0.0.1:%d' % node_p2p_port]) correctly adapts to the new parameter type while maintaining the same functional behavior.

1650-1650: Consistent parameter name update.

The parameter name is properly updated from ipAndPort to coreP2PAddrs to match the new method signature.

1712-1712: Systematic usage pattern update.

The conversion to list format is consistently applied throughout the codebase, ensuring uniform behavior.

1718-1718: Complete and consistent parameter updates.

All method calls are systematically updated to use the new parameter name, ensuring the refactoring is complete and maintains functionality.

Also applies to: 1721-1721, 1733-1733

src/evo/netinfo.cpp

src/rpc/evo_util.cpp

PastaPastaPasta · 2025-07-09T18:09:46Z

test/functional/rpc_netinfo.py

+    node: TestNode
+    platform_nodeid: str = ""
+
+    def __init__(self, node: TestNode, is_evo: bool):


@coderabbitai, please investigate if any of these test cases can semi-trivially / easily be converted into unit tests via c++ unit testing, instead of being inside functional tests

🧩 Analysis chain

@PastaPastaPasta I'll investigate the test cases to see which ones can be converted to C++ unit tests. Let me examine the underlying code structure first.

Based on my analysis of the code and test cases, here's what I found regarding converting functional tests to C++ unit tests:

Test Cases That Can Be Converted to C++ Unit Tests:

1. Port Validation Logic (from test_validation_common())

What it tests: Port range validation (1-65535) for platformP2PPort and platformHTTPPort

C++ location: ProcessNetInfoPlatform() in src/rpc/evo_util.cpp

Unit test approach: Create unit tests that directly call ProcessNetInfoPlatform() with various port values

Benefits: Much faster execution, no blockchain setup needed

2. Core P2P Address Validation (from test_validation_common() and test_validation_legacy())

What it tests: Array validation, string validation, empty string detection

C++ location: ProcessNetInfoCore() in src/rpc/evo_util.cpp

Unit test approach: Unit test the validation logic with various UniValue inputs

Benefits: Isolated testing of validation logic without RPC overhead

3. JSON Output Format Validation (from test_deprecation())

What it tests: Presence/absence of "service" vs "addresses" fields

C++ location: Various ToJson() methods in src/evo/ files

Unit test approach: Create mock objects and test JSON output directly

Benefits: Test serialization logic independently

Test Cases That Should Remain Functional Tests:

1. Integration with RPC Layer

Why: Tests the complete RPC request/response cycle

Examples: Testing that protx register properly calls validation functions

2. Masternode Registration/Update Flow

Why: Tests complex interactions between multiple systems

Examples: Testing masternode lifecycle with network restarts

3. Deprecation Flag Integration

Why: Tests command-line flag processing and RPC configuration

Examples: Testing -deprecatedrpc=service behavior

Recommended Approach:

Create new unit test file: src/test/netinfo_tests.cpp

Move validation logic tests: Port validation and address validation

Keep integration tests: Full RPC workflow and masternode lifecycle tests

Add JSON serialization tests: Test ToJson() methods independently

This would reduce functional test execution time while maintaining comprehensive coverage of the integration aspects that truly require a full node environment.

🏁 Script executed:

#!/bin/bash # First, let's examine the ProcessNetInfoCore and ProcessNetInfoPlatform functions # that handle the validation logic cat src/rpc/evo_util.cpp

Length of output: 3884

🏁 Script executed:

#!/bin/bash # Look at the ToJson methods that format the output rg -A 20 "ToJson.*const" src/evo/ | head -50

Length of output: 3195

🏁 Script executed:

#!/bin/bash # Check if there are existing unit tests for similar validation logic fd -e cpp -e h . src/test/

Length of output: 7601

🏁 Script executed:

#!/bin/bash # Look for existing unit tests that test validation functions rg -A 5 -B 5 "BOOST_AUTO_TEST_CASE.*valid" src/test/

Length of output: 14610

🏁 Script executed:

#!/bin/bash # Examine the validation logic in ProcessNetInfoCore more closely ast-grep --pattern $'template <typename T1> void ProcessNetInfoCore(T1& ptx, const UniValue& input, const bool optional) { $$$ }'

Length of output: 132

🏁 Script executed:

#!/bin/bash # Look at the platform validation logic ast-grep --pattern $'template <typename T1> void ProcessNetInfoPlatform(T1& ptx, const UniValue& input_p2p, const UniValue& input_http) { $$$ }'

Length of output: 147

🏁 Script executed:

#!/bin/bash # Check what validation functions exist in the netinfo code rg -A 10 -B 5 "valid.*port\|port.*valid" src/evo/netinfo.cpp

Length of output: 62

🏁 Script executed:

#!/bin/bash # Look at the error handling patterns to understand what can be unit tested rg -A 3 -B 3 "throw.*JSONRPCError\|JSONRPCError.*throw" src/rpc/evo_util.cpp

Length of output: 78

Based on my detailed analysis of the code, here's what I found regarding converting functional tests to C++ unit tests:

Test Cases That Can Be Converted to C++ Unit Tests:

1. Port Validation Logic (Highest Priority)

Current location: test_validation_common() in functional test

C++ function: ProcessNetInfoPlatform() in src/rpc/evo_util.cpp

What to test: Port range validation (1-65535), string vs number input handling

Unit test approach: Create test cases that directly call ProcessNetInfoPlatform() with mock CProRegTx/CProUpServTx objects

2. Core P2P Address Array Validation (High Priority)

Current location: test_validation_common() and test_validation_legacy()

C++ function: ProcessNetInfoCore() in src/rpc/evo_util.cpp

What to test: Array vs string input, empty string detection, array element validation

Unit test approach: Test with various UniValue inputs (arrays, strings, mixed types)

3. JSON Serialization Logic (Medium Priority)

Current location: test_deprecation() in functional test

C++ functions: Various ToJson() methods in src/evo/ files

What to test: Conditional "service" field inclusion, "addresses" field presence

Unit test approach: Mock the deprecation flag and test JSON output directly

Specific Unit Test Implementation:

I notice there's already a src/test/evo_netinfo_tests.cpp file. Let me examine it:

The existing unit tests likely already cover some network info functionality, but we can add specific tests for the new validation functions. Here's what I recommend:

New Unit Tests to Add:

// In src/test/evo_netinfo_tests.cpp or new file BOOST_AUTO_TEST_CASE(ProcessNetInfoPlatform_port_validation) { // Test port range validation CProRegTx ptx; ptx.netInfo = std::make_unique<MnNetInfo>(); // Valid ports BOOST_CHECK_NO_THROW(ProcessNetInfoPlatform(ptx, UniValue(1), UniValue(65535))); // Invalid ports BOOST_CHECK_EXCEPTION(ProcessNetInfoPlatform(ptx, UniValue(0), UniValue(22201)), JSONRPCError, [](const JSONRPCError& e) { return e.code == RPC_INVALID_PARAMETER; }); // String input for ports BOOST_CHECK_NO_THROW(ProcessNetInfoPlatform(ptx, UniValue("22200"), UniValue("22201"))); } BOOST_AUTO_TEST_CASE(ProcessNetInfoCore_address_validation) { CProRegTx ptx; ptx.netInfo = std::make_unique<MnNetInfo>(); // Valid string input BOOST_CHECK_NO_THROW(ProcessNetInfoCore(ptx, UniValue("127.0.0.1:19999"), false)); // Valid array input UniValue arr(UniValue::VARR); arr.push_back("127.0.0.1:19999"); BOOST_CHECK_NO_THROW(ProcessNetInfoCore(ptx, arr, false)); // Invalid: empty string when not optional BOOST_CHECK_EXCEPTION(ProcessNetInfoCore(ptx, UniValue(""), false), JSONRPCError, [](const JSONRPCError& e) { return e.code == RPC_INVALID_PARAMETER; }); }

Test Cases That Should Remain Functional:

Full RPC Integration: Testing complete protx register workflows

Masternode Lifecycle: Registration, updates, destruction with blockchain

Node Restart Integration: Testing masternode behavior across restarts

Multi-node Interaction: Testing masternode visibility across nodes

Benefits of This Approach:

Faster execution: Unit tests run in milliseconds vs seconds for functional tests

Isolated testing: Test specific validation logic without blockchain setup

Better coverage: Can test edge cases more easily

Clearer failure diagnosis: Unit test failures pinpoint exact validation issues

The functional tests should focus on integration scenarios while unit tests handle the detailed validation logic testing.

🏁 Script executed:

#!/bin/bash # Let's look at the existing evo_netinfo_tests.cpp to see what's already tested cat src/test/evo_netinfo_tests.cpp

Length of output: 9636

🏁 Script executed:

#!/bin/bash # Let's look at the specific error handling patterns rg -n "throw JSONRPCError" src/rpc/evo_util.cpp

Length of output: 975

🏁 Script executed:

#!/bin/bash # Let's see the specific validation functions and what they test rg -A 5 -B 5 "ParseInt32V\|port.*1.*65535" src/rpc/evo_util.cpp

Length of output: 65

🏁 Script executed:

#!/bin/bash # Let's look at the AddEntry function to understand what it validates rg -A 10 -B 5 "AddEntry" src/evo/netinfo.cpp

Length of output: 451

🏁 Script executed:

#!/bin/bash # Let's check what the NIS (NetInfoStatus) enum contains rg -A 10 -B 5 "enum.*NetInfoStatus\|NetInfoStatus.*enum" src/evo/netinfo.h

Length of output: 76

🏁 Script executed:

#!/bin/bash # Let's look at the specific validation test cases in the functional test rg -A 10 -B 5 "def test_validation" test/functional/rpc_netinfo.py

Length of output: 2459

Perfect! Now I have a comprehensive understanding of the code structure. Let me provide you with a detailed analysis of what can be converted to unit tests.

Analysis Summary

Looking at the existing code, I can see that:

src/test/evo_netinfo_tests.cpp already provides excellent unit test coverage for the low-level network validation logic in MnNetInfo::AddEntry()

src/rpc/evo_util.cpp contains higher-level RPC parameter processing functions that are currently only tested through functional tests

test/functional/rpc_netinfo.py tests the full RPC integration stack

Test Cases That Can Be Converted to C++ Unit Tests:

1. RPC Parameter Validation in ProcessNetInfoCore (High Priority)

Current functional test location: test_validation_common()
C++ function: ProcessNetInfoCore() in src/rpc/evo_util.cpp
What to unit test:

UniValue type validation (string vs array vs invalid types)

Empty string/array handling when optional=false

Array element validation (non-string elements, empty strings)

JSONRPCError exception throwing with correct error codes

Example unit test:

BOOST_AUTO_TEST_CASE(ProcessNetInfoCore_parameter_validation) { CProRegTx ptx; ptx.netInfo = std::make_unique<MnNetInfo>(); // Test invalid parameter types BOOST_CHECK_EXCEPTION(ProcessNetInfoCore(ptx, UniValue(123), false), JSONRPCError, [](const JSONRPCError& e) { return e.code == RPC_INVALID_PARAMETER && e.message.find("must be string or array") != std::string::npos; }); // Test empty string when not optional BOOST_CHECK_EXCEPTION(ProcessNetInfoCore(ptx, UniValue(""), false), JSONRPCError, [](const JSONRPCError& e) { return e.message.find("Empty param for coreP2PAddrs not allowed") != std::string::npos; }); // Test array with non-string elements UniValue arr(UniValue::VARR); arr.push_back(UniValue(123)); BOOST_CHECK_EXCEPTION(ProcessNetInfoCore(ptx, arr, false), JSONRPCError, [](const JSONRPCError& e) { return e.message.find("must be string") != std::string::npos; }); }

2. Platform Port Validation in ProcessNetInfoPlatform (High Priority)

Current functional test location: test_validation_common()
C++ function: ProcessNetInfoPlatform() in src/rpc/evo_util.cpp
What to unit test:

Port range validation (1-65535)

String vs number input handling

Error message formatting

Edge cases (0, 65536, negative numbers)

Example unit test:

BOOST_AUTO_TEST_CASE(ProcessNetInfoPlatform_port_validation) { CProRegTx ptx; ptx.netInfo = std::make_unique<MnNetInfo>(); // Test valid port ranges BOOST_CHECK_NO_THROW(ProcessNetInfoPlatform(ptx, UniValue(1), UniValue(65535))); BOOST_CHECK_EQUAL(ptx.platformP2PPort, 1); BOOST_CHECK_EQUAL(ptx.platformHTTPPort, 65535); // Test invalid port ranges BOOST_CHECK_EXCEPTION(ProcessNetInfoPlatform(ptx, UniValue(0), UniValue(22201)), JSONRPCError, [](const JSONRPCError& e) { return e.message.find("must be a valid port [1-65535]") != std::string::npos; }); // Test string input BOOST_CHECK_NO_THROW(ProcessNetInfoPlatform(ptx, UniValue("22200"), UniValue("22201"))); // Test invalid parameter types UniValue arr(UniValue::VARR); BOOST_CHECK_EXCEPTION(ProcessNetInfoPlatform(ptx, arr, UniValue(22201)), JSONRPCError, [](const JSONRPCError& e) { return e.message.find("must be number") != std::string::npos; }); }

3. JSON Output Formatting with Deprecation Flags (Medium Priority)

Current functional test location: test_deprecation()
C++ functions: Various ToJson() methods in src/evo/ files
What to unit test:

Conditional "service" field inclusion based on IsServiceDeprecatedRPCEnabled()

"addresses" field presence and format

JSON structure consistency

Test Cases That Should Remain Functional:

1. Full RPC Integration

Complete protx register workflow

Parameter passing through RPC layer

Integration with command-line argument parsing

2. Masternode Lifecycle Integration

Registration, updates, destruction with blockchain

Node restart behavior

Masternode visibility across network

3. Multi-node Scenarios

Deprecation flag behavior across different nodes

Network synchronization of masternode info

Recommended Implementation:

Add to src/test/evo_netinfo_tests.cpp:

// Add these test cases to the existing file BOOST_AUTO_TEST_CASE(ProcessNetInfoCore_validation) { /* ... */ } BOOST_AUTO_TEST_CASE(ProcessNetInfoPlatform_validation) { /* ... */ } BOOST_AUTO_TEST_CASE(ToJson_deprecation_flags) { /* ... */ }

Benefits of This Approach:

~70% faster execution: Unit tests for parameter validation run in microseconds

Better isolation: Test specific validation logic without blockchain overhead

Comprehensive edge case coverage: Easier to test boundary conditions

Clearer failure diagnosis: Unit test failures pinpoint exact validation issues

Conclusion:

About 40-50% of the current functional test logic can be effectively converted to unit tests, specifically the parameter validation and JSON formatting logic. The remaining integration tests should stay functional to ensure proper RPC workflow and blockchain integration.

The existing evo_netinfo_tests.cpp already provides excellent coverage for the underlying network validation, so we mainly need to add tests for the RPC parameter processing layer.

🧠 Learnings used

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.

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.

Learnt from: kwvg PR: dashpay/dash#6543 File: src/wallet/receive.cpp:240-251 Timestamp: 2025-02-06T14:34:30.466Z Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

PastaPastaPasta

Generally looks good, I like the suggestions by CodeRabbit. Let's apply those or similar, and I'm happy to approve.

IsDeprecatedRPCEnabled() is defined in RPC code but we construct UniValue objects outside of RPC code that are impacted. They are linked in binaries that do not include RPC logic. So, we need to implement a function that's functionally identical, IsServiceDeprecatedRPCEnabled().

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

test/functional/rpc_netinfo.py (3)

255-259: Apply static analysis style improvements for dictionary membership checks.

The static analysis correctly identifies that .keys() is unnecessary when checking dictionary membership. Using key in dict is more pythonic and efficient than key in dict.keys().

-        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['updatedMNs'][0][proregtx_hash].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['updatedMNs'][0][proregtx_hash]

-        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['updatedMNs'][0][proregtx_hash].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['updatedMNs'][0][proregtx_hash]

-        assert "service" in masternode_status.keys()
+        assert "service" in masternode_status

-        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['updatedMNs'][0][proregtx_hash].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['updatedMNs'][0][proregtx_hash]

Also applies to: 269-273, 276-276, 291-295

123-129: Fix indentation issues in list comprehensions.

The static analysis correctly identifies indentation issues in the transaction creation.

         # Create transaction to spend old collateral and fee change
         raw_tx = self.node.createrawtransaction([
-                { 'txid': self.mn.collateral_txid, 'vout': self.mn.collateral_vout },
-                { 'txid': address_funds_unspent['txid'], 'vout': address_funds_unspent['vout'] }
-            ], [
-                {self.mn.collateral_address: float(self.mn.get_collateral_value())},
-                {self.mn.fundsAddr: float(address_funds_value - Decimal(0.001))}
-            ])
+             { 'txid': self.mn.collateral_txid, 'vout': self.mn.collateral_vout },
+             { 'txid': address_funds_unspent['txid'], 'vout': address_funds_unspent['vout'] }
+         ], [
+             {self.mn.collateral_address: float(self.mn.get_collateral_value())},
+             {self.mn.fundsAddr: float(address_funds_value - Decimal(0.001))}
+         ])

29-143: Consider unit test conversion for validation logic.

Based on previous review discussions, the validation logic tested in this file could benefit from C++ unit tests for faster execution and better isolation. The Node class provides good abstraction, but the underlying validation functions (ProcessNetInfoCore() and ProcessNetInfoPlatform()) could be tested more efficiently as unit tests.

As discussed in previous comments, would you like me to help identify specific validation test cases that could be converted to C++ unit tests in src/test/evo_netinfo_tests.cpp? This would significantly improve test execution speed while maintaining comprehensive coverage.

The integration aspects (full RPC workflow, masternode lifecycle, node restarts) should remain as functional tests, but parameter validation logic could be unit tested.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 798dea3 and 7fdd642.

📒 Files selected for processing (21)

contrib/seeds/makeseeds.py (1 hunks)
doc/release-notes-6665.md (1 hunks)
src/Makefile.am (2 hunks)
src/coinjoin/client.cpp (1 hunks)
src/evo/core_write.cpp (2 hunks)
src/evo/dmnstate.cpp (2 hunks)
src/evo/netinfo.cpp (3 hunks)
src/evo/netinfo.h (4 hunks)
src/evo/simplifiedmns.cpp (2 hunks)
src/rpc/coinjoin.cpp (1 hunks)
src/rpc/evo.cpp (14 hunks)
src/rpc/evo_util.cpp (1 hunks)
src/rpc/evo_util.h (1 hunks)
src/rpc/masternode.cpp (1 hunks)
src/rpc/quorums.cpp (1 hunks)
test/functional/feature_dip3_deterministicmns.py (1 hunks)
test/functional/rpc_netinfo.py (1 hunks)
test/functional/rpc_quorum.py (1 hunks)
test/functional/test_framework/test_framework.py (10 hunks)
test/functional/test_runner.py (1 hunks)
test/util/data/non-backported.txt (1 hunks)

✅ Files skipped from review due to trivial changes (1)

src/rpc/coinjoin.cpp

🚧 Files skipped from review as they are similar to previous changes (19)

test/util/data/non-backported.txt
contrib/seeds/makeseeds.py
test/functional/test_runner.py
test/functional/rpc_quorum.py
src/evo/core_write.cpp
src/rpc/masternode.cpp
src/rpc/quorums.cpp
src/evo/netinfo.cpp
test/functional/feature_dip3_deterministicmns.py
src/evo/dmnstate.cpp
src/evo/netinfo.h
src/Makefile.am
doc/release-notes-6665.md
src/coinjoin/client.cpp
src/evo/simplifiedmns.cpp
src/rpc/evo_util.h
src/rpc/evo_util.cpp
src/rpc/evo.cpp
test/functional/test_framework/test_framework.py

🧰 Additional context used

📓 Path-based instructions (1)

test/functional/**/*.py

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

CLAUDE.md

🧠 Learnings (2)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

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.

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.

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.

Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.853Z
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.

test/functional/rpc_netinfo.py (12)

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.

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.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be implemented in src/fuzz/

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench

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.

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.853Z
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.

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.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.332Z
Learning: Favor extension over modification of the Bitcoin Core foundation

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety

🪛 Ruff (0.11.9)

test/functional/rpc_netinfo.py

255-255: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

256-256: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

257-257: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

258-258: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

259-259: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

269-269: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

270-270: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

271-271: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

272-272: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

273-273: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)

276-276: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

291-291: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

292-292: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

293-293: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

294-294: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

295-295: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

🪛 Flake8 (7.2.0)

test/functional/rpc_netinfo.py

[error] 124-124: continuation line over-indented for hanging indent

(E126)

[error] 126-126: continuation line under-indented for hanging indent

(E121)

[error] 129-129: closing bracket does not match indentation of opening bracket's line

(E123)

⏰ 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). (5)

GitHub Check: win64-build / Build source
GitHub Check: arm-linux-gnueabihf / Build depends
GitHub Check: x86_64-apple-darwin / Build depends
GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends

🔇 Additional comments (2)

test/functional/rpc_netinfo.py (2)

172-214: LGTM: Comprehensive validation testing covers all edge cases.

The validation tests thoroughly cover:

Invalid array element types and empty strings

Port range validation (1-65535)

Mainnet port rejection

Array size limits

Non-numeric port inputs

The test logic correctly validates the new ProcessNetInfoCore() and ProcessNetInfoPlatform() functions with appropriate error codes and messages.

215-296: LGTM: Deprecation testing covers all affected RPC endpoints.

The deprecation tests comprehensively verify:

Presence of addresses field in all relevant RPC responses

Absence of service field by default

Presence of service field with -deprecatedrpc=service flag

Coverage of multiple RPC endpoints (getrawtransaction, masternode status, protx diff, protx listdiff)

The test correctly handles the special case where service remains in the root masternode status response as it's relevant for the host node.

PastaPastaPasta

utACK 7fdd642

UdjinM6

utACK 7fdd642

UdjinM6 · 2025-07-15T17:20:07Z

src/rpc/evo.cpp

-                "Can be set to an empty string, which will require a ProUpServTx afterwards."}
+            {"ipAndPort", RPCArg::Type::ARR, RPCArg::Optional::NO,
+                "Array of addresses in the form \"ADDR:PORT\". Must be unique on the network.\n"
+                "Can be set to an empty string, which will require a ProUpServTx afterwards.",


nit:

Suggested change

"Can be set to an empty string, which will require a ProUpServTx afterwards.",

"Can be set to an empty string or an empty array, which will require a ProUpServTx afterwards.",

kwvg mentioned this pull request May 18, 2025

feat: introduce basic extended addresses implementation, stop using platform{HTTP,P2P}Port #6666

Open

5 tasks

kwvg force-pushed the netinfo_rpc1 branch from 6760805 to e72e381 Compare May 19, 2025 20:16

kwvg mentioned this pull request May 19, 2025

refactor: model interface after MnNetInfo and support switching impls, add new ProTx version #6665

Merged

5 tasks

kwvg added this to the 23 milestone May 20, 2025

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label May 20, 2025

kwvg force-pushed the netinfo_rpc1 branch from e72e381 to 9a45d36 Compare May 22, 2025 18:43

github-actions bot added the needs rebase label May 28, 2025

kwvg force-pushed the netinfo_rpc1 branch from 9a45d36 to 28aa283 Compare May 28, 2025 20:04

github-actions bot removed the needs rebase label May 28, 2025

kwvg force-pushed the netinfo_rpc1 branch from 28aa283 to 8a385d8 Compare May 28, 2025 20:05

kwvg force-pushed the netinfo_rpc1 branch from 8a385d8 to c788531 Compare June 6, 2025 17:43

kwvg mentioned this pull request Jun 9, 2025

test: consolidate masternode info tracking (MasternodeInfo) in functional tests, implement helpers, add type annotations #6718

Merged

5 tasks

github-actions bot added the needs rebase label Jun 12, 2025

kwvg mentioned this pull request Jun 15, 2025

test: implement helper functions for ProTx creation, allow working with assert_raises_rpc_error, add ability to withhold submission to all ProTx creation RPCs #6720

Merged

5 tasks

kwvg force-pushed the netinfo_rpc1 branch from c788531 to cb3ed74 Compare June 15, 2025 11:00

github-actions bot removed the needs rebase label Jun 15, 2025

kwvg force-pushed the netinfo_rpc1 branch from cb3ed74 to 232eb49 Compare June 15, 2025 11:12

github-actions bot added the needs rebase label Jun 18, 2025

kwvg force-pushed the netinfo_rpc1 branch from 232eb49 to 89431dc Compare June 18, 2025 15:34

github-actions bot removed the needs rebase label Jun 18, 2025

github-actions bot added the needs rebase label Jun 29, 2025

kwvg force-pushed the netinfo_rpc1 branch from 89431dc to 594ff7a Compare July 1, 2025 11:37

github-actions bot added needs rebase and removed needs rebase labels Jul 1, 2025

kwvg force-pushed the netinfo_rpc1 branch from 594ff7a to 99f0584 Compare July 7, 2025 17:11

github-actions bot removed the needs rebase label Jul 7, 2025

kwvg force-pushed the netinfo_rpc1 branch from 99f0584 to 798dea3 Compare July 9, 2025 13:44

kwvg marked this pull request as ready for review July 9, 2025 13:57

coderabbitai bot reviewed Jul 9, 2025

View reviewed changes

src/evo/netinfo.cpp Show resolved Hide resolved

kwvg requested review from knst, UdjinM6, PastaPastaPasta and thephez July 9, 2025 15:07

PastaPastaPasta reviewed Jul 9, 2025

View reviewed changes

src/rpc/evo_util.cpp Show resolved Hide resolved

PastaPastaPasta reviewed Jul 9, 2025

View reviewed changes

src/rpc/evo_util.cpp Outdated Show resolved Hide resolved

PastaPastaPasta reviewed Jul 9, 2025

View reviewed changes

kwvg added 9 commits July 10, 2025 16:42

rpc: spin-off ipAndPort and platform{HTTP,P2P}Port setters to util

8c28f30

rpc: allow ipAndPort to accept multiple entries with arrays

4fd4e0e

refactor: s/ipAndPort/coreP2PAddrs/g

33e5f6a

rpc: add new key "addresses" to allow reporting multiple entries

d9247ad

test: add functional test for addresses and deprecated service field

a60c39a

test: validate common conditions for input validation

859ce5c

test: check pre-fork conditions for ProRegTx and ProUpServTx preparation

5c564c2

docs: add documentation for deprecation, new field and renamed input

7fdd642

kwvg force-pushed the netinfo_rpc1 branch from 798dea3 to 7fdd642 Compare July 11, 2025 19:00

kwvg requested a review from PastaPastaPasta July 11, 2025 19:03

coderabbitai bot reviewed Jul 11, 2025

View reviewed changes

PastaPastaPasta approved these changes Jul 15, 2025

View reviewed changes

UdjinM6 approved these changes Jul 15, 2025

View reviewed changes

PastaPastaPasta merged commit b334565 into dashpay:develop Jul 15, 2025
31 of 38 checks passed

	"Can be set to an empty string, which will require a ProUpServTx afterwards.",
	"Can be set to an empty string or an empty array, which will require a ProUpServTx afterwards.",

feat(rpc): allow reporting multiple addresses with new addresses field, deprecate service field, allow submitting multiple addresses #6674

feat(rpc): allow reporting multiple addresses with new addresses field, deprecate service field, allow submitting multiple addresses #6674

Uh oh!

Conversation

kwvg commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Additional Information

Breaking Changes

Checklist

Uh oh!

github-actions bot commented May 28, 2025

Uh oh!

github-actions bot commented Jun 12, 2025

Uh oh!

github-actions bot commented Jun 18, 2025

Uh oh!

github-actions bot commented Jun 29, 2025

Uh oh!

coderabbitai bot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Jul 9, 2025

Choose a reason for hiding this comment

Test Cases That Can Be Converted to C++ Unit Tests:

1. Port Validation Logic (from test_validation_common())

2. Core P2P Address Validation (from test_validation_common() and test_validation_legacy())

3. JSON Output Format Validation (from test_deprecation())

Test Cases That Should Remain Functional Tests:

1. Integration with RPC Layer

2. Masternode Registration/Update Flow

3. Deprecation Flag Integration

Recommended Approach:

Test Cases That Can Be Converted to C++ Unit Tests:

1. Port Validation Logic (Highest Priority)

2. Core P2P Address Array Validation (High Priority)

3. JSON Serialization Logic (Medium Priority)

Specific Unit Test Implementation:

New Unit Tests to Add:

Test Cases That Should Remain Functional:

Benefits of This Approach:

Analysis Summary

Test Cases That Can Be Converted to C++ Unit Tests:

1. RPC Parameter Validation in ProcessNetInfoCore (High Priority)

2. Platform Port Validation in ProcessNetInfoPlatform (High Priority)

3. JSON Output Formatting with Deprecation Flags (Medium Priority)

Test Cases That Should Remain Functional:

1. Full RPC Integration

2. Masternode Lifecycle Integration

3. Multi-node Scenarios

Recommended Implementation:

Benefits of This Approach:

Conclusion:

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

feat(rpc): allow reporting multiple addresses with new `addresses` field, deprecate `service` field, allow submitting multiple addresses #6674

feat(rpc): allow reporting multiple addresses with new `addresses` field, deprecate `service` field, allow submitting multiple addresses #6674

kwvg commented May 18, 2025 •

edited

Loading

coderabbitai bot commented Jul 9, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

1. Port Validation Logic (from `test_validation_common()`)

2. Core P2P Address Validation (from `test_validation_common()` and `test_validation_legacy()`)

3. JSON Output Format Validation (from `test_deprecation()`)

1. RPC Parameter Validation in `ProcessNetInfoCore` (High Priority)

2. Platform Port Validation in `ProcessNetInfoPlatform` (High Priority)