script: return proper error for CScriptNum errors
#34381
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34381. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Neat. Unit tests and functional tests pass on my end. tACK 6b307a2 You can remove |
6b307a2 to
3e7c250
Compare
Just removed it since it's now unused. |
|
tACK 3e7c250 |
It will be used for errors related to CScriptNum (e.g. overflow or encoding errors). Currently, we simply return unknown error for these errors.
3e7c250 to
5777cb8
Compare
|
Force-pushed addressing #34381 (comment) |
5777cb8 to
6f7b432
Compare
|
|
||
| static ScriptErrorDesc script_errors[]={ | ||
| {SCRIPT_ERR_OK, "OK"}, | ||
| {SCRIPT_ERR_UNKNOWN_ERROR, "UNKNOWN_ERROR"}, |
There was a problem hiding this comment.
Interesting, is this now unreachable?
There was a problem hiding this comment.
I think so. I don't see a explicit scenario for this unknown error.
When evaluating a script, the current code is bad for analyzing some errors because it returns
SCRIPT_ERR_UNKNOWN_ERRORfor errors that are clearly known.CScriptNumhas two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for anyCScriptNumerror.