rpc: Add test-only RPCs under -test=<option> flag #30717

pull Prabhat1308 wants to merge 8 commits into bitcoin:master from Prabhat1308:feat-refactor_test_only_rpcs changing 65 files +253 −154
  1. Prabhat1308 commented at 5:35 pm on August 26, 2024: none

    followup of #29007 , implements adding of test-only RPC options in -test as discussed here

    Currently a general user has access to test-only RPCs , where user might accidentally use it in production. This PR splits off test-only RPCs to be included in -test flag with additional restrictions of use in regtest segregating 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_ONLY with OptionsCategory::DEBUG_TEST options have been added.

    Additional info

    Two more test-only arguments -checkpoints and -deprecatedrpc=<method> are excluded from this PR because of the requirement of their tests to be run on either testnet or testnet3.

  2. rpc: Add rpcs involving bool arguments 
    This adds the test-only rpcs that require bool arguments to
`test-option-doc`
    e42fa10563
  3. rpc: Add helper function for bool argument rpcs 
    Adds helper function to extract bool arguments from test-only rpc
included in the `-test` flags. Adds one implementation for CI
passing
    04dc783fc3
  4. rpc: Add rpcs involving int arguments 
    Adds test-only rpcs involving integer arguments to the `test-option-doc`
    16be6dc4bd
  5. rpc: Add helper for rpcs with int arguments 
    Adds helper function to extract int arguments from the test-only
rpcs included in the `-test` flag. Added one function implementation
for CI passing
    3ec29e9a5e
  6. Refactor: Change test options with Boolean arguments 
    Adds test-only RPCs to `test=<option>` involving Boolean arguments.
Uses `HasTestOptionBool()` to extract boolean arguments
    9cb7cf242a
  7. Refactor: Change RPCs involving Integer arguments 
    Refactors the test-only RPCs to be included in the
`test=<option>` flag. Uses `HasTestOptionInt>` to
extract integer arguments from the flags
    aa37339b89
  8. rpc: Add Helper function for RPCs with string arguments 
    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
    0db66a702b
  9. Refactor: change functional test to work with testnet4 846014a07d
  10. DrahtBot commented at 5:35 pm on August 26, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30712 (fuzz: Add missing fuzz targets to cmake build by maflcko)
    • #30592 (Remove mempoolfullrbf by instagibbs)
    • #30454 (build: Introduce CMake-based build system by hebasto)
    • #30232 (refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options by luke-jr)
    • #29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts by fjahr)
    • #28802 (ArgsManager: support subcommand-specific options by ajtowns)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

    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.

  11. DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2024
  12. in src/common/args.cpp:701 in 846014a07d 
    696+    "stopafterblockimport (Stop running after importing blocks from disk )",
697+    "stopatheight (Stop running after reaching the given height in the main chain)",
698+    "limitancestorcount (Do not accept transactions if number of in-mempool ancestors is <n> or more)",
699+    "limitancestorsize (Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes)",
700+    "limitdescendantcount (Do not accept transactions if any ancestor would have <n> or more in-mempool descendants)",
701+    "limitdescendantsize (Do not accept transactions if any ancestor would have descendants exceeding <n> kilobytes)",
    murchandamus commented at 6:04 pm on August 26, 2024:
    From what I understand, at least the startup options deprecatedrpc, stopatheight, limitancestorcount, limitancestorsize, limitdescendantcount, and limitdescendantsize are used in production by some users.
    Prabhat1308 commented at 6:12 pm on August 26, 2024:
    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.
    murchandamus commented at 6:15 pm on August 26, 2024:
    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?
    Prabhat1308 commented at 6:19 pm on August 26, 2024:
    We are restricting its use in test networks only specifically to regtest only . the -test flag has a check to assert use of regtest.
    murchandamus commented at 6:45 pm on August 26, 2024:
    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.
    mzumsande commented at 7:46 pm on August 27, 2024:

    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.

    stratospher commented at 8:48 am on September 3, 2024:
    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
  13. murchandamus commented at 6:04 pm on August 26, 2024: contributor
    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.
  14. DrahtBot commented at 7:14 pm on August 26, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29266802249

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  15. DrahtBot added the label CI failed on Aug 26, 2024
  16. DrahtBot commented at 9:55 am on August 28, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  17. DrahtBot added the label Needs rebase on Aug 28, 2024
  18. achow101 commented at 2:34 pm on October 15, 2024: member

    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.

  19. achow101 closed this on Oct 15, 2024


Prabhat1308 DrahtBot murchandamus mzumsande stratospher achow101

Labels
RPC/REST/ZMQ Needs rebase CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me