rpc: Fix estimatesmartfee to properly handle a null estimate_mode arg #12940
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empact
force-pushed
the
fix-estimatesmartfee-null-estimatemode
branch
2 times, most recently
from
13fb692 to
fcc6abe
Compare
Empact
force-pushed
the
fix-estimatesmartfee-null-estimatemode
branch
2 times, most recently
from
b4b0edc to
232df46
Compare
2 tasks
This expands the capability of that method and enables it to takeover the same checks from RPCTypeCheck and RPCTypeCheckObj Note the new test on line 69 is consistent with that on line 86 via De Morgan’s law.
The estimate_mode arg is optional in documentation and handling, but the RPCTypeCheck applied to it did not allow null, therefore its presence was enforced. Note this only affected null values, not entirely absent arguments because RPCTypeCheck only tests against the params list as exists and no farther, so this is a narrow bug. This also replaces redundant testing via RPCTypeCheck & RPCTypeCheckArgument in favor of non-redundant calls to RPCTypeCheckArgument. The prior redundancy existed due to the prior limitation of the RPCTypeCheckArgument not being able to allow null. Added some call and light functional testing on estimatesmartfee and estimaterawfee as well.
Empact
force-pushed
the
fix-estimatesmartfee-null-estimatemode
branch
from
232df46 to
e9bab19
Compare
Contributor
| Needs rebase |
Contributor
| There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened. |
maflcko
pushed a commit
that referenced
this pull request
Mar 11, 2020
111880a [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley) Pull request description: This adds light functional coverage to estimaterawfee - a subset of the testing applied to estimatesmartfee, and argument validation testing to both estimaterawfee and estimatesmartfee. One valid estimatesmartfee signature test is commented out because it fails currently. Extracted from #12940 Top commit has no ACKs. Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 11, 2020
…tesmartfee 111880a [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley) Pull request description: This adds light functional coverage to estimaterawfee - a subset of the testing applied to estimatesmartfee, and argument validation testing to both estimaterawfee and estimatesmartfee. One valid estimatesmartfee signature test is commented out because it fails currently. Extracted from bitcoin#12940 Top commit has no ACKs. Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
…tesmartfee 111880a [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley) Pull request description: This adds light functional coverage to estimaterawfee - a subset of the testing applied to estimatesmartfee, and argument validation testing to both estimaterawfee and estimatesmartfee. One valid estimatesmartfee signature test is commented out because it fails currently. Extracted from bitcoin#12940 Top commit has no ACKs. Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The estimate_mode arg is optional in documentation and handling,
but the RPCTypeCheck applied to it did not allow null, therefore
its presence was enforced. Note this only affected null values,
not entirely absent arguments because RPCTypeCheck only tests
against the params list as exists and no farther, so this is a
narrow bug.
This also replaces redundant testing via RPCTypeCheck &
RPCTypeCheckArgument in favor of non-redundant calls to
RPCTypeCheckArgument. The prior redundancy existed due
to the prior limitation of the RPCTypeCheckArgument not
being able to allow null.
Added some call and light functional testing on estimatesmartfee
and estimaterawfee as well.