[RPC] Correct issues with budget commands#965
Merged
Conversation
random-zebra
previously approved these changes
Jul 27, 2019
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
ACK 55408b8
Ivoking preparebudget before the wallet has fully started, correctly returns the error.
Successfully prepared and submitted budget on testnet.
3 tasks
CaveSpectre11
force-pushed
the
RPCBudgetCrash
branch
from
55408b8 to
8d4bbce
Compare
Author
|
Rebased on top of PR #964 (refactoring) for ease of merging later. Given the complications/complexity of the merge; I'm retesting. |
CaveSpectre11
force-pushed
the
RPCBudgetCrash
branch
from
8d4bbce to
0041fd2
Compare
Author
CaveSpectre11
force-pushed
the
RPCBudgetCrash
branch
from
6208039 to
4b14327
Compare
Collaborator
|
needs rebase now that #964 is merged |
CaveSpectre11
force-pushed
the
RPCBudgetCrash
branch
from
4b14327 to
7261134
Compare
Author
Rebase done |
Fuzzbawls
approved these changes
Aug 8, 2019
|
Merging... |
random-zebra
added a commit
that referenced
this pull request
Aug 8, 2019
7261134 [Review] Convert runtime_error to JSONRPCError (Cave Spectre) f7ac53b [RPC] Correct issues with budget commands (Cave Spectre) Pull request description: ### **Release notes** - [RPC] Fix potential wallet crashes in budget commands - [RPC] Remove unnecessary conditionals in parameter checking of budget commands ### **Details** **wallet segfault** `preparebudget`, invoked before the wallet has fully started, caused a segfault and crashed. This was due to the lack of checking for `pwalletMain` before referencing cs_wallet. This is solved with a throw error to tell the user to try again after the wallet has started. Furthermore; both `preparebudget` and `submitbudget` has a check for `pindexPrev` to conditionalize the assignment of nBlockMin (which references `pindexPrev->nHeight`); however later checks, `pindexPrev->nHeight` is referenced even if the pindexPrev check failed; which would have caused a crash if pindexPrev was NULL. This would occur if `chainActive->Tip()` returns null. Given the logic of `getnextsuperblock`, the check added to preparebudget and submitbudget will generate a throw error if there is no active chain tip. **Remove unnecessary conditionals and code** `pindexPrev` is loaded from `chainActive.Tip()` pindexPrev->nHeight is checked multiple times in the parameter checking. For ease of reading, pindexPrev->nHeight is now saved in a variable as it is used multiple times in the execution of the RPC commands in question. Likewise, two function calls are used to determine the constant length of a budget cycle `Params().GetBudgetCycleBlocks()`; and is used several times. Storing this information in a constant is more efficient then several calls for the information. nBlockMin is determined (the next superblock). However later nNext was defined to be the exact same thing. It's not necessary to build them both. By nature of validating that the chosen budget cycle block is greater than the current block, and validating the number of cycles is greater than zero; the need to check the end of the budget cycle is unnecessary. Having a separate throw message for choosing the wrong block (not a budget block), and specifying a block that has passed is unnecessary. These have been combined so that the user, putting in the wrong block, is informed, in both cases, what the next budget block is. ### **Note** This PR was originally part of #964, but split out to distinguish between these changes and the other's refactoring. ACKs for top commit: random-zebra: ACK 7261134 Fuzzbawls: ACK 7261134 Tree-SHA512: b0ccabae52d02767971104353b48d4efb76926017d7099b52b341d525710aa3a70d343c92c260048bb1c288990a914730a4dd0832578f07e96abb384d628cd4e
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Release notes
Details
wallet segfault
preparebudget, invoked before the wallet has fully started, caused a segfault and crashed. This was due to the lack of checking forpwalletMainbefore referencing cs_wallet. This is solved with a throw error to tell the user to try again after the wallet has started.Furthermore; both
preparebudgetandsubmitbudgethas a check forpindexPrevto conditionalize the assignment of nBlockMin (which referencespindexPrev->nHeight); however later checks,pindexPrev->nHeightis referenced even if the pindexPrev check failed; which would have caused a crash if pindexPrev was NULL. This would occur ifchainActive->Tip()returns null. Given the logic ofgetnextsuperblock, the check added to preparebudget and submitbudget will generate a throw error if there is no active chain tip.Remove unnecessary conditionals and code
pindexPrevis loaded fromchainActive.Tip()pindexPrev->nHeight is checked multiple times in the parameter checking. For ease of reading, pindexPrev->nHeight is now saved in a variable as it is used multiple times in the execution of the RPC commands in question. Likewise, two function calls are used to determine the constant length of a budget cycleParams().GetBudgetCycleBlocks(); and is used several times. Storing this information in a constant is more efficient then several calls for the information.nBlockMin is determined (the next superblock). However later nNext was defined to be the exact same thing. It's not necessary to build them both.
By nature of validating that the chosen budget cycle block is greater than the current block, and validating the number of cycles is greater than zero; the need to check the end of the budget cycle is unnecessary.
Having a separate throw message for choosing the wrong block (not a budget block), and specifying a block that has passed is unnecessary. These have been combined so that the user, putting in the wrong block, is informed, in both cases, what the next budget block is.
Note
This PR was originally part of #964, but split out to distinguish between these changes and the other's refactoring.