Add warnings to createmultisig and addmultisig if using uncompressed keys #23113
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
laanwj
reviewed
Sep 28, 2021
Member
|
Concept ACK |
1 similar comment
|
Concept ACK |
Member
|
ACK 29a78ac431982aa34d909c526c3be6eeba97c34d |
luke-jr
reviewed
Oct 3, 2021
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Oct 10, 2021
Github-Pull: bitcoin#23113 Rebased-From: 9c63a8f9f87d89fa543d5cdb1e03301e20b042a3
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Oct 10, 2021
Github-Pull: bitcoin#23113 Rebased-From: 20675022ee6e83f5f13e4a28b79bf3f205873b74
instagibbs
reviewed
Oct 15, 2021
promag
reviewed
Oct 16, 2021
Contributor
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Code review ACK 29a78ac431982aa34d909c526c3be6eeba97c34d.
Contributor
|
Concept ACK |
Contributor
Author
|
Feedback addressed in new commit, which ensures the warning message is only returned if an address type is explicitly chosen by the user. Also adds the test @instagibbs requested. |
instagibbs
approved these changes
Oct 18, 2021
Member
|
@prayank23 hahaha one sec |
Contributor
|
@meshcollider does it make sense to fail instead? |
Contributor
Author
|
@promag I don't think so personally, a warning is sufficient IMO. |
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Merged
Contributor
Author
|
Rebased |
Member
|
ACK d5cab1a |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 11, 2021
…f using uncompressed keys d5cab1a Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson) e46fc93 Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson) d1a9742 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson) Pull request description: Fixes bitcoin#21368 Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different. ACKs for top commit: achow101: ACK d5cab1a Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
RandyMcMillan
pushed a commit
to RandyMcMillan/mempool-tab
that referenced
this pull request
Dec 23, 2021
…ltisig if using uncompressed keys 74c34e2 Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson) c1f9fa4 Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson) 6d8bc3c Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson) Pull request description: Fixes #21368 Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different. ACKs for top commit: achow101: ACK 74c34e2 Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
laanwj
added a commit
that referenced
this pull request
Jun 6, 2022
… in createmultisig 3a9b9bb test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) eaf6f63 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Fixes #25127 If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different. However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning: https://github.com/bitcoin/bitcoin/blob/192d639a6b1bd0feaa52e6ea4e63e33982704c32/src/rpc/output_script.cpp#L166-L169 So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type. ACKs for top commit: jonatack: ACK 3a9b9bb laanwj: Code review ACK 3a9b9bb Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 6, 2022
…-segwit in createmultisig 3a9b9bb test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) eaf6f63 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Fixes bitcoin#25127 If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, bitcoin#23113 added a warnings field which will warn the user why their address format is different. However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning: https://github.com/bitcoin/bitcoin/blob/192d639a6b1bd0feaa52e6ea4e63e33982704c32/src/rpc/output_script.cpp#L166-L169 So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type. ACKs for top commit: jonatack: ACK 3a9b9bb laanwj: Code review ACK 3a9b9bb Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 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.
Fixes #21368
Currently, if there are any uncompressed keys when calling
AddAndGetMultisigDestination, it will just default to a legacy address regardless of the chosenaddress_type. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add awarningsfield which will warn the user why their address format is different.