[Qt] Check return value of addr.GetKeyID(keyid) on custom change address change #10972
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
practicalswift
changed the title
Contributor
|
utACK 65bb605. |
Contributor
|
utACK 65bb605b71f0494d23d61f545f7638df60b23e10 |
Contributor
|
This looks like it will be an instant crash if you put a P2SH address in the field. |
practicalswift
force-pushed
the
GetKeyID-assertion
branch
from
65bb605 to
37c4144
Compare
Contributor
Author
|
@gmaxwell Thanks for reviewing and clarifying that the old code does not assume I've now changed to |
Contributor
|
I think the code there just doesn't correctly handle p2sh keys in general. Probably needs a more extensive fixup than that, though the assert seems clearly not right. :) |
Contributor
|
Likely not neccessary if #11184 lands. |
Contributor
Author
|
@TheBlueMatt Thanks for the notification! @dooglus PR is the better choice. Closing this PR! :-) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 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.
Check return value of
addr.GetKeyID(keyid)on custom change address change. Prior to this commit this was the only place where the return value ofGetKeyID(…)was unchecked.Clarify our assumption of
addr.GetKeyID(keyid)beingtruein this context via an assertion.(Note to reviewers: Let me know if this assumption is not being made. If the assumption is not made then the case of
!addr.GetKeyID(keyid)should obviously be handled more gracefully than with anassert:-))