test: negative/unknown `rpcserialversion` should throw an init error #25731

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2022-07-rpcserialversion-init-error changing 1 files +5 −0

brunoerg commented at 10:02 PM on July 28, 2022: contributor

This PR adds test coverage for the following init errors: https://github.com/bitcoin/bitcoin/blob/41205bf442254d17bc7885f3b2693749da714a0e/src/init.cpp#L1025-L1030
DrahtBot added the label Tests on Jul 28, 2022

in test/functional/feature_segwit.py:617 in 14a1a9ac43 outdated

 608 | @@ -609,6 +609,12 @@ def run_test(self):
 609 |                  assert_equal(self.nodes[1].gettransaction(txid, True)["txid"], txid)
 610 |                  assert_equal(self.nodes[1].listtransactions("*", 1, 0, True)[0]["txid"], txid)
 611 |  
 612 | +        self.log.info('Test negative and unknown rpcserialversion throw an init error')
 613 | +        self.stop_node(0)
 614 | +        self.nodes[0].assert_start_raises_init_error(["-rpcserialversion=-1"], "Error: rpcserialversion must be non-negative.")
 615 | +        self.nodes[0].assert_start_raises_init_error(["-rpcserialversion=100"], "Error: Unknown rpcserialversion requested.")
 616 | +
 617 | +

stickies-v commented at 1:26 PM on August 1, 2022:

nit: PEP8 prescribes a single blank line before a method

brunoerg commented at 1:44 PM on August 1, 2022:

Yeah, thanks!

stickies-v commented at 1:40 PM on August 1, 2022: contributor

No strong opinion - asking more as a question: is there any risk involved for the user by providing invalid -rpcserialversion values? If not, is it worth it to add this functional test?
brunoerg commented at 1:54 PM on August 1, 2022: contributor

@stickies-v, there is no risk for the user because there are these verifications, so covering them in the functional tests ensure if user types a negative or unknown value to rpcserialversion, he will not be harmed. See other similar PRs like: #25511, #25451, #25358.
test: negative/unknown `rpcserialversion` should throw an init error 155344960b
brunoerg force-pushed on Aug 1, 2022
stickies-v commented at 2:13 PM on August 1, 2022: contributor

I understand we validate user input in init.cpp, my question was more about whether or not we should test that the user input is indeed being validated. Based on a quick search, it looks like we don't do that for all parameters (e.g. "-dbcache", "-par") and I wonder if that's just because no one's gotten to it yet, or because we should only do it for critical parameters (i.e. where wrong input can be dangerous). Because it looks like for -rpcserialversion, it's not critical.

So I guess I'm +0 on this, don't think there's a real need to test this, but if we generally want to move towards testing all input validation, then this would make sense.
brunoerg commented at 2:45 PM on August 1, 2022: contributor

@stickies-v, I understand your point but I believe that we should cover in the tests as much as possible, even if it is not something extremely critical. If there is a validation and costs only few lines to test it, why not? And I think there are other parameters which don't have test coverage because no one got it yet, I've done this for other ones: (https://github.com/bitcoin/bitcoin/pull/25511, #25451, #25358.).
luke-jr commented at 11:12 PM on August 9, 2022: member

Concept ACK
MarcoFalke merged this on Aug 10, 2022
MarcoFalke closed this on Aug 10, 2022
sidhujag referenced this in commit 56aad28ea3 on Aug 10, 2022
bitcoin locked this on Aug 10, 2023

Contributors

Labels