-
Notifications
You must be signed in to change notification settings - Fork 35.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warnings to createmultisig and addmultisig if using uncompressed keys #23113
Add warnings to createmultisig and addmultisig if using uncompressed keys #23113
Conversation
Concept ACK |
1 similar comment
Concept ACK |
ACK 29a78ac |
Github-Pull: bitcoin#23113 Rebased-From: 9c63a8f
Github-Pull: bitcoin#23113 Rebased-From: 2067502
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 29a78ac.
Concept ACK |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2d21025
@prayank23 hahaha one sec |
@meshcollider does it make sense to fail instead? |
@promag I don't think so personally, a warning is sufficient IMO. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Rebased |
ACK d5cab1a |
…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
…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
… 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
…-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
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 awarnings
field which will warn the user why their address format is different.