rpc: Improve signrawtransaction error reporting #16251
Merged
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
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
Contributor
Author
|
With this patch, partial signature error message looks like: instead of: Mismatching scripts give a new error of either: or instead of an apparently successful operation (but unchanged rawtx) with embedded error of: |
ajtowns
force-pushed
the
201906-signrawerror
branch
2 times, most recently
from
89e4919 to
073a2f8
Compare
ajtowns
force-pushed
the
201906-signrawerror
branch
from
073a2f8 to
f7e4676
Compare
maflcko
reviewed
Jul 3, 2019
ajtowns
force-pushed
the
201906-signrawerror
branch
from
b601b68 to
f7e4676
Compare
Contributor
|
Concept ACK, looks right to me after an initial read |
ajtowns
force-pushed
the
201906-signrawerror
branch
from
f7e4676 to
b5e9bd9
Compare
instagibbs
reviewed
Sep 9, 2019
ajtowns
force-pushed
the
201906-signrawerror
branch
from
b5e9bd9 to
363038a
Compare
…/witnessScript This adds checks to ensure the redeemScript/witnessScript actually correspond to the provided scriptPubKey, and, if both are provided, that they are sensibly related to each other. Thanks to github user passionofvc for raising this issue.
Thanks to Danial Jaffy (tipu) for reporting this issue.
ajtowns
force-pushed
the
201906-signrawerror
branch
from
363038a to
ec4c793
Compare
Member
|
utACK ec4c793 |
Contributor
meshcollider
left a comment
meshcollider
left a comment
There was a problem hiding this comment.
utACK ec4c793
I really like this change
Member
|
Code Review ACK ec4c793 |
fanquake
changed the title
fanquake
added a commit
that referenced
this pull request
Sep 11, 2019
ec4c793 signrawtransaction*: improve error for partial signing (Anthony Towns) 3c481f8 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript (Anthony Towns) Pull request description: Two fixes for `signrawtransactionwith{key,wallet}` (in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required. Fixes: #13218 Fixes: #14823 ACKs for top commit: instagibbs: utACK ec4c793 achow101: Code Review ACK ec4c793 meshcollider: utACK ec4c793 Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35
fanquake
added a commit
that referenced
this pull request
Aug 15, 2020
7966aa4 Add variables for repeated scripts (MeshCollider) fec8336 Remove GetScriptForWitness function (MeshCollider) b887060 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider) Pull request description: As per this TODO in the code: > TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes. The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that. ACKs for top commit: instagibbs: ACK 7966aa4 jonatack: Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4` achow101: re-ACK 7966aa4 only changes since last is rebase. Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 16, 2020
…tination 7966aa4 Add variables for repeated scripts (MeshCollider) fec8336 Remove GetScriptForWitness function (MeshCollider) b887060 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider) Pull request description: As per this TODO in the code: > TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes. The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: bitcoin#16251 is even better than this check, please review that. ACKs for top commit: instagibbs: ACK bitcoin@7966aa4 jonatack: Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4` achow101: re-ACK 7966aa4 only changes since last is rebase. Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
This was referenced Dec 9, 2021
This was referenced Dec 12, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 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.
Two fixes for
signrawtransactionwith{key,wallet}(in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required.Fixes: #13218
Fixes: #14823