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
Conversation
WalkthroughThis change introduces an optional ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
test/functional/feature_dip3_v19.py (1)
116-135: 🛠️ Refactor suggestionSimplify
test_revoke_protx()signatureFollowing up on the previous comment, the function doesn’t need
node_idxwhen it can derive it:-def test_revoke_protx(self, node_idx, revoke_mn: MasternodeInfo): +def test_revoke_protx(self, revoke_mn: MasternodeInfo): @@ - self.wait_until(lambda: self.nodes[node_idx].getconnectioncount() == 0) - self.connect_nodes(node_idx, 0) + self.wait_until(lambda: self.nodes[revoke_mn.nodeIdx].getconnectioncount() == 0) + self.connect_nodes(revoke_mn.nodeIdx, 0)This removes a parameter from all call-sites and eliminates the chance of mixing indices.
src/rpc/evo.cpp (1)
1-1:⚠️ Potential issueMerge conflict marker detected
Check Potential Conflictsjob flagged an unresolved conflict for this file. Please rebase / resolve before merging.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
test/functional/test_framework/test_framework.py (1)
1-1:⚠️ Potential issueMerge-conflict flag raised by CI
GitHub Actions reports a conflict on this file. Rebase onto current
developto keep the helper additions intact.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
[error] 1-1: Multiple PRs conflict on this file. Conflicting PRs require rebase before merging.
♻️ Duplicate comments (1)
src/rpc/evo.cpp (1)
1278-1282: …and again forprotx revokeSee previous two comments; consider centralising the “last optional bool” parsing.
🧹 Nitpick comments (14)
test/functional/feature_llmq_simplepose.py (1)
209-213: Possible flakiness:update_service()may spend unconfirmed funds chosen at random
update_service()can internally flipsubmittoFalse(→sendrawtransaction) half of the time.
Here the fee source (addr) has only the zero-conf output just created bysendtoaddress.
While Dash usually allows spending unconfirmed change, chains of descendants can occasionally hit the max-ancestor limit and make the test non-deterministic.Consider either
# give the wallet a confirmed coin first self.generate(self.nodes[0], 1)before the call, or pass
submit=Falseexplicitly and broadcast after a confirmation to keep the test deterministic.doc/release-notes-6720.md (1)
1-5: Minor markdown/style polishNit: to stay consistent with other release-notes files, add a trailing period at the end of the bullet and wrap the RPC names in back-ticks:
-* 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`. +* A new optional field `submit` has been introduced to the `protx revoke`, `protx update_registrar`, and `protx update_service` RPCs. It behaves identically to `submit` in `protx register` or `protx register_fund`.test/functional/feature_dip3_deterministicmns.py (6)
216-217: Consider supplyingfundsAddrexplicitly
update_registrar()will fall back toself.fundsAddrwhenfundsAddr=None.
Passing it explicitly (as done in other calls below) keeps the test self-contained and avoids surprises ifself.fundsAddrchanges earlier in the scenario.
240-242: Return type assertion could be stricter
register_fund()always returns a hex txid (32 bytes / 64 hex chars) unless an error is expected.
To catch malformed returns early, assert the length as well:-assert txid is not None +assert txid and len(txid) == 64
248-249: Missing assertion forproTxHashlengthSame rationale as above – validating the shape of the returned hash helps detect silent failures.
267-268: Potentially redundant empty stringsEmpty strings are treated as “unchanged” by the RPC layer, so passing
pubKeyOperator=""andvotingAddr=""is fine but unnecessary.
Omitting them keeps the test less noisy:-mn.update_registrar(node, submit=True, pubKeyOperator="", votingAddr="", rewards_address=payee, fundsAddr=mn.fundsAddr) +mn.update_registrar(node, submit=True, rewards_address=payee, fundsAddr=mn.fundsAddr)
274-275: No check on returned txid
update_service()can return either a txid orNone(if an assert was expected).
Adding a simple assert guards against silent mis-behaviour:assert mn.update_service(self.nodes[0], submit=True, ipAndPort=f'127.0.0.2:{mn.nodePort}') is not None
283-284: Same return value check for rollback callReplicate the assertion here for symmetry and safety.
test/functional/feature_dip3_v19.py (1)
97-99:node_idxargument is redundant and risks mismatchYou already pass the full
MasternodeInfoobject; itsnodeIdxcan be accessed insidetest_revoke_protx().
Removing the extra parameter prevents accidental divergence:-self.test_revoke_protx(evo_info_3.nodeIdx, self.mninfo[-1]) +self.test_revoke_protx(self.mninfo[-1])src/rpc/evo.cpp (3)
146-149: Minor: advertise the default more clearly in help outputUsing
RPCArg::Default{true}is technically correct, but the generated help text will simply read=truewithout explaining the behavioural change (“broadcast vs. return-hex”).
Consider switching toRPCArg::DefaultHint{"(default = true, tx is broadcast)"}to make the help self-explanatory.
1055-1058: Index arithmetic is getting fragileThe offset
paramIdx + 2assumes that the argument list will never change again.
We already had to adjust it once for Platform fields; another optional parameter will silently shift the index and mis-readsubmit.Consider parsing the last parameter positionally from
request.params.back()or, better, switch toFindKeyInRequest("submit")-style named look-ups to make future additions safe.
1170-1174: Same offset fragility as above
request.params[5]works today but will break if one more optional argument is inserted ahead ofsubmit. A small helper likebool GetSubmitArg(const JSONRPCRequest& req, unsigned idx)would remove this duplication.
test/functional/test_framework/test_framework.py (2)
1222-1228: Over-restrictive assertion on rpc-error expectationsThe pair-wise guard
if (expected_assert_code and not expected_assert_msg) or ...forces BOTH code and message.
assert_raises_rpc_erroralready lets tests match either. Dropping this guard would let callers check just what they care about and keeps helpers flexible.
1277-1340: Duplication & long signatures hint at missing abstraction
register_fund,register,revoke,update_registrar,update_serviceshare a lot of boiler-plate (argument validation,use_srtlogic, error handling).
Consider extracting a private helper (_call_protx) that:
- Builds the argument vector.
- Performs the assert / RPC / optional SRT broadcast.
This will slash ~300 lines, silence the R0913/R0914 warnings, and make future maintenance cheaper.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1277-1277: Too many arguments (16/5)
(R0913)
[refactor] 1277-1277: Too many positional arguments (16/5)
(R0917)
[refactor] 1277-1277: Too many local variables (22/15)
(R0914)
[refactor] 1336-1336: Too many arguments (7/5)
(R0913)
[refactor] 1336-1336: Too many positional arguments (7/5)
(R0917)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
doc/release-notes-6720.md(1 hunks)src/rpc/evo.cpp(11 hunks)test/functional/feature_dip3_deterministicmns.py(4 hunks)test/functional/feature_dip3_v19.py(3 hunks)test/functional/feature_llmq_simplepose.py(1 hunks)test/functional/test_framework/test_framework.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/functional/feature_llmq_simplepose.py (1)
test/functional/test_framework/test_framework.py (1)
update_service(1420-1474)
test/functional/test_framework/test_framework.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(127-143)test/functional/test_framework/test_node.py (1)
TestNode(56-791)
🪛 GitHub Actions: Check Potential Conflicts
test/functional/feature_llmq_simplepose.py
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
test/functional/feature_dip3_deterministicmns.py
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
src/rpc/evo.cpp
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
test/functional/feature_dip3_v19.py
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
test/functional/test_framework/test_framework.py
[error] 1-1: Merge conflict detected in this file across multiple PRs. Conflicting PRs require rebase before merging.
[error] 1-1: Multiple PRs conflict on this file. Conflicting PRs require rebase before merging.
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 320-320: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[refactor] 1217-1217: Too many arguments (17/5)
(R0913)
[refactor] 1217-1217: Too many positional arguments (17/5)
(R0917)
[refactor] 1217-1217: Too many local variables (23/15)
(R0914)
[refactor] 1277-1277: Too many arguments (16/5)
(R0913)
[refactor] 1277-1277: Too many positional arguments (16/5)
(R0917)
[refactor] 1277-1277: Too many local variables (22/15)
(R0914)
[refactor] 1336-1336: Too many arguments (7/5)
(R0913)
[refactor] 1336-1336: Too many positional arguments (7/5)
(R0917)
[refactor] 1378-1378: Too many arguments (9/5)
(R0913)
[refactor] 1378-1378: Too many positional arguments (9/5)
(R0917)
[refactor] 1420-1420: Too many arguments (11/5)
(R0913)
[refactor] 1420-1420: Too many positional arguments (11/5)
(R0917)
[refactor] 1420-1420: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (6)
src/rpc/evo.cpp (1)
323-350:✅ Verification successful
Signature change may break untouched call-sites
SignAndSendSpecialTx()now requires a 5-thfSubmitarg. All updated callers in this file compile, but any other call in the codebase still passing 4 args will now fail.Run the quick scan below to catch stragglers before CI does:
The list should be empty – any hit means you need to add the missing
fSubmitparameter.
🏁 Script executed:
#!/bin/bash # Find invocations that still use the old 4-argument form ast-grep --pattern $'SignAndSendSpecialTx($_, $_, $_, $_)' --lang cppLength of output: 70
🏁 Script executed:
#!/bin/bash # Locate all invocations of SignAndSendSpecialTx to verify argument counts rg --no-heading --line-number "SignAndSendSpecialTx\(" -n .Length of output: 901
No missing
fSubmitarguments in callers
A search across the codebase shows every invocation ofSignAndSendSpecialTxnow passes five parameters (includingfSubmit):
- src/rpc/evo.cpp:770
- src/rpc/evo.cpp:824
- src/rpc/evo.cpp:888
- src/rpc/evo.cpp:1067
- src/rpc/evo.cpp:1179
- src/rpc/evo.cpp:1286
No 4-argument calls remain. This change won’t break any existing call-sites.
test/functional/test_framework/test_framework.py (5)
1254-1260: Randomised path can cause flaky testsSelecting the
sendrawtransactionpath withrandom.getrandbits(1)means behaviour depends on global RNG state.
Although the framework seedsrandom, parallel/sharded runs (or future calls torandom) can still diverge, making failures hard to reproduce.Expose an explicit parameter (e.g.
via_srt=False) or drive the choice from the test, instead of hidden randomness.
1378-1410: Potential positional-argument driftFor
update_registrarthe helper appends[fundsAddr, submit]only whenfundsAddris present.
If RPC signature changed to expectsubmitbeforefundsAddr(PR description says “submit … placed earlier”), the call order will be wrong and fail silently in legacy nodes.Double-check the updated C++ RPC parser and adjust ordering if needed.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1378-1378: Too many arguments (9/5)
(R0913)
[refactor] 1378-1378: Too many positional arguments (9/5)
(R0917)
1420-1474: Same concerns apply toupdate_serviceVerify positional order of
submitvs. optional addresses to match the new RPC layout.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1420-1420: Too many arguments (11/5)
(R0913)
[refactor] 1420-1420: Too many positional arguments (11/5)
(R0917)
[refactor] 1420-1420: Too many local variables (17/15)
(R0914)
1188-1189: Good safety checkNice addition preventing
evo&legacyfrom both beingTrue.
47-48: Import looks correct
assert_raises_rpc_erroris now used extensively; the added import is appropriate and not duplicated.
| UpdateSpecialTxInputsHash(tx, payload); | ||
|
|
||
| uint256 hash = ::SerializeHash(payload); | ||
| payload.sig = key.Sign(hash, use_legacy); |
There was a problem hiding this comment.
CI failing – run clang-format
clang-format discrepancies were reported by the pipeline. Re-format this file to unblock the build.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 320-320: Clang format differences found. Code formatting does not match clang-format style. Please run clang-format to fix.
🤖 Prompt for AI Agents
In src/rpc/evo.cpp at line 320, the code formatting does not comply with the
project's clang-format style guidelines, causing CI failures. Run clang-format
on this file to automatically reformat the code according to the project's style
rules, then save the changes and commit the updated file to resolve the
formatting issues.
| def register(self, node: TestNode, submit: bool, collateral_txid: Optional[str] = None, collateral_vout: Optional[int] = None, | ||
| ipAndPort: Optional[str] = None, ownerAddr: Optional[str] = None, pubKeyOperator: Optional[str] = None, votingAddr: Optional[str] = None, | ||
| operator_reward: Optional[int] = None, rewards_address: Optional[str] = None, fundsAddr: Optional[str] = None, | ||
| platform_node_id: Optional[str] = None, platform_p2p_port: Optional[int] = None, platform_http_port: Optional[int] = None, | ||
| expected_assert_code: Optional[int] = None, expected_assert_msg: Optional[str] = None) -> Optional[str]: | ||
| if (expected_assert_code and not expected_assert_msg) or (not expected_assert_code and expected_assert_msg): | ||
| raise AssertionError("Intending to use assert_raises_rpc_error() but didn't specify code and message") | ||
|
|
||
| # We don't store these values internally so all register() calls must specify them in the argument list | ||
| if ipAndPort is None: | ||
| raise AssertionError("ipAndPort is not an optional parameter, must be specified!") | ||
|
|
||
| # EvoNode-specific fields are ignored for regular masternodes | ||
| if self.evo: | ||
| if platform_node_id is None: | ||
| raise AssertionError("EvoNode but platform_node_id is missing, must be specified!") | ||
| if platform_p2p_port is None: | ||
| raise AssertionError("EvoNode but platform_p2p_port is missing, must be specified!") | ||
| if platform_http_port is None: | ||
| raise AssertionError("EvoNode but platform_http_port is missing, must be specified!") | ||
|
|
||
| # Common arguments shared between regular masternodes and EvoNodes | ||
| args = [ | ||
| collateral_txid or self.collateral_txid, | ||
| collateral_vout or self.collateral_vout, | ||
| ipAndPort, | ||
| ownerAddr or self.ownerAddr, | ||
| pubKeyOperator or self.pubKeyOperator, | ||
| votingAddr or self.votingAddr, | ||
| operator_reward or self.operator_reward, | ||
| rewards_address or self.rewards_address, | ||
| ] | ||
| address_funds = fundsAddr or self.fundsAddr | ||
|
|
||
| # Use assert_raises_rpc_error if we expect to error out | ||
| use_assert: bool = bool(expected_assert_code and expected_assert_msg) | ||
|
|
||
| # Flip a coin and decide if we will submit the transaction directly or use sendrawtransaction | ||
| use_srt: bool = bool(random.getrandbits(1)) and submit and not use_assert | ||
| if use_srt: | ||
| submit = False | ||
|
|
||
| # Construct final command and arguments | ||
| if self.evo: | ||
| command = "register_evo" | ||
| args = args + [platform_node_id, platform_p2p_port, platform_http_port, address_funds, submit] # type: ignore | ||
| else: | ||
| command = "register_legacy" if self.legacy else "register" | ||
| args = args + [address_funds, submit] # type: ignore | ||
|
|
||
| if use_assert: | ||
| assert_raises_rpc_error(expected_assert_code, expected_assert_msg, node.protx, command, *args) | ||
| return None | ||
|
|
||
| ret: str = node.protx(command, *args) | ||
| if use_srt: | ||
| ret = node.sendrawtransaction(ret) | ||
|
|
||
| return ret | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating the submit parameter
submit is passed in as a boolean argument but is reassigned (submit = False) when use_srt is chosen.
This:
• Breaks the principle of not mutating caller-supplied inputs.
• Makes the later argument list harder to reason about because its meaning has changed mid-function.
Prefer a dedicated local, e.g.:
- if use_srt:
- submit = False
+ broadcast_now = submit and not use_srtand pass broadcast_now (or an explicit flag) to the RPC call.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1217-1217: Too many arguments (17/5)
(R0913)
[refactor] 1217-1217: Too many positional arguments (17/5)
(R0917)
[refactor] 1217-1217: Too many local variables (23/15)
(R0914)
🤖 Prompt for AI Agents
In test/functional/test_framework/test_framework.py around lines 1217 to 1276,
the boolean parameter 'submit' is reassigned within the function, which mutates
the caller-supplied input and complicates understanding of the argument's
meaning. To fix this, introduce a new local variable (e.g., 'broadcast_now') to
represent the adjusted submission behavior instead of modifying 'submit'. Use
this new variable in the argument list passed to the RPC call, leaving the
original 'submit' parameter unchanged throughout the function.
We store a fair amount of information about the masternode already in MasternodeInfo, so we should try and leverage that as best we can and require the user only input the values we don't store and they need to override.
*Not* submitting a transaction gives test the ability to check if a call will result in a *valid* transaction, it also lets us examine the raw transaction, manipulate it and check if transaction checks at broadcast time will catch it. This is relevant for extended addresses (specifically, ProUpServTx) but has been extended to all ProTx-creating RPCs for consistency's sake.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/functional/test_framework/test_framework.py (1)
1252-1256: Stop mutating the caller-suppliedsubmitargument (same issue as previously flagged)
submitis reassigned toFalsewhenuse_srtis chosen. This:• Changes the semantic meaning of an inbound parameter mid-function.
• Makes subsequent argument lists harder to reason about.
• Violates the principle of not modifying caller intent.A simple local flag avoids the mutation:
- use_srt: bool = bool(random.getrandbits(1)) and submit and not use_assert - if use_srt: - submit = False + use_srt: bool = bool(random.getrandbits(1)) and submit and not use_assert + broadcast_now = submit and not use_srtand pass
broadcast_now(or an explicitly named variable) to the final*argslist.This applies to all helper methods shown in the selected ranges.
Also applies to: 1295-1298, 1344-1347, 1385-1388, 1438-1441
🧹 Nitpick comments (1)
test/functional/test_framework/test_framework.py (1)
1251-1254: Randomised execution path will make tests non-deterministicThe coin-flip (
random.getrandbits(1)) means different CI runs might exercise different paths, complicating reproducibility of failures. Consider:• Seeding
randomonce per test with a fixed value derived from the test name / CLI option; or
• Allowing a per-test override to pin the behaviour.This keeps coverage benefits while preserving determinism when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
doc/release-notes-6720.md(1 hunks)src/rpc/evo.cpp(11 hunks)test/functional/feature_dip3_deterministicmns.py(4 hunks)test/functional/feature_dip3_v19.py(3 hunks)test/functional/feature_llmq_simplepose.py(1 hunks)test/functional/test_framework/test_framework.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/functional/feature_llmq_simplepose.py
- doc/release-notes-6720.md
- test/functional/feature_dip3_deterministicmns.py
- test/functional/feature_dip3_v19.py
- src/rpc/evo.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/functional/test_framework/test_framework.py (2)
test/functional/test_framework/util.py (2)
assert_raises_rpc_error(127-143)p2p_port(348-350)test/functional/test_framework/test_node.py (1)
TestNode(56-791)
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[refactor] 1218-1218: Too many arguments (17/5)
(R0913)
[refactor] 1218-1218: Too many positional arguments (17/5)
(R0917)
[refactor] 1218-1218: Too many local variables (23/15)
(R0914)
[refactor] 1274-1274: Too many arguments (16/5)
(R0913)
[refactor] 1274-1274: Too many positional arguments (16/5)
(R0917)
[refactor] 1274-1274: Too many local variables (22/15)
(R0914)
[refactor] 1329-1329: Too many arguments (7/5)
(R0913)
[refactor] 1329-1329: Too many positional arguments (7/5)
(R0917)
[refactor] 1371-1371: Too many arguments (9/5)
(R0913)
[refactor] 1371-1371: Too many positional arguments (9/5)
(R0917)
[refactor] 1413-1413: Too many arguments (11/5)
(R0913)
[refactor] 1413-1413: Too many positional arguments (11/5)
(R0917)
[refactor] 1413-1413: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: ClangFormat
- GitHub Check: check_merge
🔇 Additional comments (2)
test/functional/test_framework/test_framework.py (2)
1188-1189: Good defensive check on mutually-exclusive flags
Early-out assertion prevents invalidMasternodeInfo(evo=True, legacy=True)instantiation.
1207-1209: Nice touch: derivenodePortautomatically
Switching top2p_port(nodeIdx)removes duplicated port math throughout the call-sites.
…addresses` field, deprecate `service` field, allow submitting multiple addresses 7fdd642 docs: add documentation for deprecation, new field and renamed input (Kittywhiskers Van Gogh) 5c564c2 test: check pre-fork conditions for ProRegTx and ProUpServTx preparation (Kittywhiskers Van Gogh) 859ce5c test: validate common conditions for input validation (Kittywhiskers Van Gogh) a60c39a test: add functional test for `addresses` and deprecated `service` field (Kittywhiskers Van Gogh) e0d2a81 rpc: deprecate key "service" replaced by "addresses" (Kittywhiskers Van Gogh) d9247ad rpc: add new key "addresses" to allow reporting multiple entries (Kittywhiskers Van Gogh) 33e5f6a refactor: s/ipAndPort/coreP2PAddrs/g (Kittywhiskers Van Gogh) 4fd4e0e rpc: allow `ipAndPort` to accept multiple entries with arrays (Kittywhiskers Van Gogh) 8c28f30 rpc: spin-off `ipAndPort` and `platform{HTTP,P2P}Port` setters to util (Kittywhiskers Van Gogh) Pull request description: ## Motivation To enable including functional tests with the extended addresses (ExtAddr) implementation (i.e. [dash#6666](#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 #6665 * Depends on #6720 * Dependency for #6666 *⚠️ The format for the field replacing `service`, `addresses` is not stable and is changed in [dash#6666](#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](https://github.com/dashpay/dash/blob/da8a475dfac9ad80d566eef8cf119c8036d6ebcc/src/rpc/server.cpp#L370-L375)), 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 - [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 7fdd642 UdjinM6: utACK 7fdd642 Tree-SHA512: 75331a467e7170542349c94408b71f5d25ebd2bf418cb6bdf77866a342b9079ec795638535f67c6487739b37651bf8265dce66ef3b06e0732748a173441695ef
3 participants
Motivation
Continuing on the work of dash#6718, this pull request aims to do two things:
Leverage the amount of information stored in
MasternodeInfoto 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 inMasternodeInfoto construct the RPC callThis also comes with the bonus of
MasternodeInfoknowing 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 forsubmit, which is placed earlier and must be specified mandatorily).Allow testing both failure and success cases by introducing both support for
assert_raises_rpc_errorin 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(inrpc_netinfo.py, introduced in dash#6674) by a fair amount (diff) and could see further reduction in future iterations.Additional Information
Depends on test: consolidate masternode info tracking (
MasternodeInfo) in functional tests, implement helpers, add type annotations #6718Dependency for feat(rpc): allow reporting multiple addresses with new
addressesfield, deprecateservicefield, allow submitting multiple addresses #6674As
fundsAddris an optional field in some ProTx RPCs that trigger different behavior if not supplied (source), 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()andupdate_registrar(), transaction withholding (settingsubmittofalse) is not allowed as this would require settingfundsAddrto an empty string, which does not trigger the expected fallback behavior.This does not extend to
update_service()where a blank string is submitted asprotx update_servicedoes not seem to behave adversely when supplied a blank string.To test that the
submitargument works as intended in newly updated and existing RPCs, ifsubmitisTrueand no expected error code and message are supplied, at random,submitis set toFalseand the hex serialized signed transaction output is broadcast usingsendrawtransaction(seeuse_srdvariable).Breaking Changes
submithas been introduced to theprotx revoke,protx update_registrar,protx update_serviceRPCs. It behaves identically tosubmitinprotx registerorprotx register_fund.Checklist