rpc: Add test-only RPCs under -test=<option> flag
#30717
Conversation
This adds the test-only rpcs that require bool arguments to `test-option-doc`
Adds helper function to extract bool arguments from test-only rpc included in the `-test` flags. Adds one implementation for CI passing
Adds test-only rpcs involving integer arguments to the `test-option-doc`
Adds helper function to extract int arguments from the test-only rpcs included in the `-test` flag. Added one function implementation for CI passing
Adds test-only RPCs to `test=<option>` involving Boolean arguments. Uses `HasTestOptionBool()` to extract boolean arguments
Refactors the test-only RPCs to be included in the `test=<option>` flag. Uses `HasTestOptionInt>` to extract integer arguments from the flags
Adds Helper function `HasTestOptionString` to extract string argument from test-only RPCs under `-test=<option>`. Removes the redundant check for chain in `acceptstalefeeestimates` as there is a similar check when passing this RPC using `-test` flag
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Looking at the discussion in #29007, I had the impression that it was suggested to move some RPC options to a "test-only" grouping, so I’m a bit surprised that this PR also moves a number of startup options to "test-only". At least a few from that list appear to be used in production, e.g. by people operating mempool explorers.
| "deprecatedrpc (Allows deprecated RPC method(s) to be used)", | ||
| "stopafterblockimport (Stop running after importing blocks from disk )", | ||
| "stopatheight (Stop running after reaching the given height in the main chain)", | ||
| "limitancestorcount (Do not accept transactions if number of in-mempool ancestors is <n> or more)", | ||
| "limitancestorsize (Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes)", | ||
| "limitdescendantcount (Do not accept transactions if any ancestor would have <n> or more in-mempool descendants)", | ||
| "limitdescendantsize (Do not accept transactions if any ancestor would have descendants exceeding <n> kilobytes)", |
There was a problem hiding this comment.
From what I understand, at least the startup options deprecatedrpc, stopatheight, limitancestorcount, limitancestorsize, limitdescendantcount, and limitdescendantsize are used in production by some users.
There was a problem hiding this comment.
In the arguments description in init.cpp they are categorised as DEBUG_ONLY AND DEBUG_TEST which is where I picked them up to be "test-only". There was not much documentation elsewhere to decide otherwise.
There was a problem hiding this comment.
I see. What’s the overarching intent here: is the purpose just to label them more explicitly as being for testing, or is the intent to restrict their use to test networks?
There was a problem hiding this comment.
We are restricting its use in test networks only specifically to regtest only . the -test flag has a check to assert use of regtest.
There was a problem hiding this comment.
Sorry, I have to withdraw limitancestorcount, limitancestorsize, limitdescendantcount, and limitdescendantsize. Both parties that I suspected to use these startup options, in fact, do not use them. We definitely used stopatheight at a prior employer while indexing the blockchain, and my understanding was that deprecatedrpc was meant to give users a heads-up that an RPC would be going away with the next release, but not completely requiring an immediate transitioning away from it.
There was a problem hiding this comment.
stopatheight is also frequently used for benchmarks on mainnet, most recently in #28280. The -checkXYZ startup options that can help debug issues on all networks and I've run some of them on several occasions on mainnet and signet.
Many of the options are debug-only because they are meant to only be used by devs testing new things, but that doesn't mean they can only be useful regtest - often it's helpful to test things on other networks too.
I think that restricting to regtest should only be done with options specifically introduced for the test suite that don't make sense anywhere else.
There was a problem hiding this comment.
makes sense. moving regtest only options to -test=<opt> would make the interface(dev-testing/more-debug-info vs regtest-only) less confusing. (other than test=addrman), regtest-only would include: fastprune, mocktime, acceptstalefeeestimates.
I think peertimeout and rpcdoccheck are only used in tests, so you could include them too
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs. Closing due to lack of interest. |
followup of #29007 , implements adding of test-only RPC options in
-testas discussed hereCurrently a general user has access to
test-onlyRPCs , where user might accidentally use it in production. This PR splits off test-only RPCs to be included in-testflag with additional restrictions of use inregtestsegregating the user facing and testing RPCs . These test-only RPCs are split off from args and have their own method to extract arguments .Current implementation of test-only RPCs follow this convention
-test=<command>=<argument>.The test-only rpcs with
ArgsManager::DEBUG_ONLYwithOptionsCategory::DEBUG_TESToptions have been added.Additional info
Two more test-only arguments
-checkpointsand-deprecatedrpc=<method>are excluded from this PR because of the requirement of their tests to be run on eithertestnetortestnet3.