Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Actually throw help when passed invalid number of params #15401

Merged
merged 2 commits into from Feb 25, 2019

Conversation

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 13, 2019

Can be tested by

  • running the included test against an old binary (compiled without this patch)
  • calling setban 1 "add" 3 4 5 6 7 8 9 0 in the gui
@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Feb 18, 2019
Copy link
Member

@promag promag left a comment

Tested with master binary and functional test fails.

src/rpc/util.cpp Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke force-pushed the Mf1902-rpcNumArgs branch from fa910b0 to fa4ce70 Feb 20, 2019
Copy link
Member

@promag promag left a comment

Tested ACK fa4ce70.

Could update license year in test/functional/rpc_getblockstats.py.

break;
}
}
return num_required_args <= num_args && num_args <= m_args.size();
Copy link
Member

@promag promag Feb 20, 2019

nit, could early return with if (num_args > m_args.size()) return false;.

Copy link
Member Author

@MarcoFalke MarcoFalke Feb 20, 2019

I think this is the wrong place to overoptimize

bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
{
size_t num_required_args = 0;
for (size_t n = m_args.size(); n > 0; --n) {
Copy link
Member

@promag promag Feb 20, 2019

nit, could change condition to n > num_args?

Copy link
Member Author

@MarcoFalke MarcoFalke Feb 20, 2019

No, that would be incorrect

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 25, 2019

Could update license year in test/functional/rpc_getblockstats.py.

Please, let's not start nitting about copyright years. In as far as they matter at all (we don't know!) updating these once per year is enough.

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 25, 2019

utACK fa4ce70

@laanwj laanwj merged commit fa4ce70 into bitcoin:master Feb 25, 2019
2 checks passed
laanwj added a commit that referenced this issue Feb 25, 2019
…params

fa4ce70 rpc: Actually throw help when passed invalid number of params (MarcoFalke)
fa05626 rpc: Add RPCHelpMan::IsValidNumArgs() (MarcoFalke)

Pull request description:

  Can be tested by

  * running the included test against an old binary (compiled without this patch)
  * calling `setban 1 "add" 3 4 5 6 7 8 9 0` in the gui

Tree-SHA512: aa6a25bbe6f40722913ea292252a62a4012c964eed9f4035335a2e2d13be98eb60f368e8a3251a104a26a62c08b2cb926b06e5ab1418ef1cf4abdd71d87c2919
@MarcoFalke MarcoFalke deleted the Mf1902-rpcNumArgs branch Feb 25, 2019
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue May 22, 2020
Summary:
 * rpc: Add RPCHelpMan::IsValidNumArgs()

This is a backport of Core [[bitcoin/bitcoin#15401 | PR15401]]

Depends on D6064

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D6220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants