rpc: fix incorrect warning for address type p2sh-segwit in createmultisig#25220
Merged
laanwj merged 2 commits intobitcoin:masterfrom Jun 6, 2022
Merged
Conversation
704b064 to
794fb31
Compare
Member
laanwj
reviewed
May 31, 2022
794fb31 to
31e57fd
Compare
Contributor
|
I tested and left one review. Thanks for working on this. I'll test again later. Here are two easy test cases: EDIT: oops, just submitted the review that i had left pending |
furszy
reviewed
Jun 3, 2022
Member
furszy
left a comment
There was a problem hiding this comment.
Code review ACK 31e57fdf
Can apply the same changes to addmultisigaddress (wallet/rpc/addresses.cpp line 305).
Contributor
Author
|
Thanks, @furszy. Force-pushed addressing it. |
31e57fd to
f105d14
Compare
mruddy
reviewed
Jun 5, 2022
jonatack
reviewed
Jun 6, 2022
Member
There was a problem hiding this comment.
ACK with some suggested improvements, and "addmultisigaddress" should be mentioned in the commit message(s).
diff
diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
index d86499a35f..f4bb76f50f 100644
--- a/src/rpc/output_script.cpp
+++ b/src/rpc/output_script.cpp
@@ -163,11 +163,11 @@ static RPCHelpMan createmultisig()
result.pushKV("descriptor", descriptor->ToString());
UniValue warnings(UniValue::VARR);
- if (!request.params[2].isNull() && *descriptor->GetOutputType() != output_type) {
+ if (descriptor->GetOutputType() != output_type) {
// Only warns if the user has explicitly chosen an address type we cannot generate
warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
}
- if (warnings.size()) result.pushKV("warnings", warnings);
+ if (!warnings.empty()) result.pushKV("warnings", warnings);
return result;
},
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 66aeb0c962..f25ad59528 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -302,11 +302,11 @@ RPCHelpMan addmultisigaddress()
result.pushKV("descriptor", descriptor->ToString());
UniValue warnings(UniValue::VARR);
- if (!request.params[3].isNull() && *descriptor->GetOutputType() != output_type) {
+ if (descriptor->GetOutputType() != output_type) {
// Only warns if the user has explicitly chosen an address type we cannot generate
warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
}
- if (warnings.size()) result.pushKV("warnings", warnings);
+ if (!warnings.empty()) result.pushKV("warnings", warnings);
return result;
},
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 77f072dce0..716ee8f7ef 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -91,15 +91,17 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
assert 'warnings' not in result
# Generate addresses with the segwit types. These should all make legacy addresses
+ err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]
+
for addr_type in ['bech32', 'p2sh-segwit']:
- result = self.nodes[0].createmultisig(2, keys, addr_type)
+ result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type)
assert_equal(legacy_addr, result['address'])
- assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
+ assert_equal(result['warnings'], err_msg)
if self.is_bdb_compiled():
- result = wmulti0.addmultisigaddress(2, keys, '', addr_type)
+ result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type)
assert_equal(legacy_addr, result['address'])
- assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
+ assert_equal(result['warnings'], err_msg)…multisig and addmultisigaddress
…any warning for expected cases
f105d14 to
3a9b9bb
Compare
Contributor
Author
Member
|
ACK 3a9b9bb |
Member
|
Code review ACK 3a9b9bb |
Member
|
tagged for backport |
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
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jun 9, 2022
…multisig and addmultisigaddress Github-Pull: bitcoin#25220 Rebased-From: eaf6f63
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jun 9, 2022
…any warning for expected cases Github-Pull: bitcoin#25220 Rebased-From: 3a9b9bb
Merged
Member
|
Backported in #25316. |
maflcko
pushed a commit
that referenced
this pull request
Jul 8, 2022
4ebf6e3 p2p: always set nTime for self-advertisements (Martin Zumsande) 039ef21 tests: Use descriptor that requires both legacy and segwit (Andrew Chow) 5fd25eb tests: Calculate input weight more accurately (Andrew Chow) bd6d3ac windeploy: Renewed windows code signing certificate (Andrew Chow) 32fa522 test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) 7658055 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Backports: - #24454 - #25201 - #25220 - #25314 ACKs for top commit: LarryRuane: re-utACK 4ebf6e3 achow101: ACK 4ebf6e3 Tree-SHA512: add3999d0330b3442f3894fce38ad9b5adc75da7d681c949e1d052bac5520c2c6fb06eba98bfbeb4aa9a560170451d24bf00d08dddd4a3d080030ecb8ad61882
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #25127
If there are any uncompressed keys when calling
AddAndGetMultisigDestination, it will just default to a legacy address regardless of the chosenaddress_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 returnsScriptHashfor both legacy andP2SH_SEGWIT. So, sinceP2SH_SEGWITis different fromScriptHash, it returns the warning:bitcoin/src/rpc/output_script.cpp
Lines 166 to 169 in 192d639
So, to avoid this mistake I changed
OutputTypeFromDestinationtodescriptor->GetOutputType()to get the appropriate output type.