rpc: deprecate `addresses` and `reqSigs` from rpc outputs

mjdietzx commented at 8:07 pm on November 2, 2020: contributor

Considering the limited applicability of reqSigs and the confusing output of 1 in all cases except bare multisig, the addresses and reqSigs outputs are removed for all rpc commands.

add a new sane “address” field (for outputs that have an identifiable address, which doesn’t include bare multisig)
with -deprecatedrpc: leave “reqSigs” and “addresses” intact (with all weird/wrong behavior they have now)
without -deprecatedrpc: drop “reqSigs” and “addresses” entirely always.

Note: Some light refactoring done to allow us to very easily delete a few chunks of code (marked with TODOs) when we remove this deprecated behavior.

Using IsDeprecatedRPCEnabled in core_write.cpp caused some circular dependencies involving core_io

Circular dependencies were caused by rpc/util unnecessarily importing node/coinstats and node/transaction. Really what rpc/util needs are some fundamental type/helper-function definitions. So this was cleaned up to make more sense.

This fixes #20102.

in src/rpc/rawtransaction.cpp:562 in 30259787c3 outdated

556@@ -560,7 +557,6 @@ static RPCHelpMan decodescript()
557                             {RPCResult::Type::STR, "asm", "String representation of the script public key"},
558                             {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"},
559                             {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
560-                            {RPCResult::Type::NUM, "reqSigs", "The required signatures (always 1)"},

jonatack commented at 8:10 pm on November 2, 2020:

Would these RPC removals need to go through the usual deprecation process?

(See doc/JSON-RPC-interface.md#versioning for more, or search for pulls or commits with the word “deprecate”.)

DrahtBot added the label Docs on Nov 2, 2020

DrahtBot added the label RPC/REST/ZMQ on Nov 2, 2020

mjdietzx force-pushed on Nov 2, 2020

in test/util/data/txcreatemultisig1.json:18 in a6843f0263 outdated

14@@ -15,7 +15,6 @@
15             "scriptPubKey": {
16                 "asm": "2 02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397 021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d 02df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb485 3 OP_CHECKMULTISIG",
17                 "hex": "522102a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff39721021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d2102df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb48553ae",
18-                "reqSigs": 2,

unknown commented at 9:54 pm on November 2, 2020:

Not sure about this change because reqSigs will be 2 in this case while testing

mjdietzx commented at 10:18 pm on November 2, 2020:

Based on the discussion in the issue #20102 it seemed no one cares/uses bare multisig, and that we could just totally get rid of reqSigs. I’m not sure if this is really true, but figured I’d remove it and see what feedback I get in the PR

MarcoFalke commented at 8:31 am on November 3, 2020: member

Is anyone using this for p2sh-multisig (passing the redeemscript into decodescript)?

luke-jr commented at 6:00 pm on November 13, 2020: member

“addresses” for multisigs don’t make sense without “reqSigs”

IMO if we deprecate/remove the latter, we should also be sure to remove the former with it.

(“addresses” should remain for actual address outputs)

mjdietzx commented at 7:24 pm on November 15, 2020: contributor

“addresses” for multisigs don’t make sense without “reqSigs”

IMO if we deprecate/remove the latter, we should also be sure to remove the former with it.

(“addresses” should remain for actual address outputs)

If we were to remove addresses for multisigs it seems to me we’d be able to get rid of ExtractDestinations entirely, as this is the only thing it’s used for currently, and ExtractDestination can be used instead.

It also makes me think {RPCResult::Type::ARR, "addresses", "array of bitcoin addresses"} is a bit misleading, and would ideally be a single {RPCResult::Type::STR, "address", "bitcoin address"}.

We’d be able to get rid of a nice chunk of code (and potentially confusing behavior). But I’m new to this repo, and I need some guidance on what should be done here. And also if I need to deprecate stuff and how I should proceed if so.

Note: there is always the easy option of just fixing the reqSigs behavior instead of removing it. But, I guess it’s unused and we can get rid of some cruft by removing it. Please advise!

sipa commented at 7:29 pm on November 15, 2020: member

Concept ACK on removing both “reqSigs” and “addresses”, but it’ll need to go through the RPC deprecation mechanism.

In other places we have already removed similar fields. Especially “addresses” is a bizarre and misleading leftover from a time when addresses were only ever P2PKH and just “key identifiers”. Now (… since P2SH in 2012) addresses are encodings of a specific scriptPubKey it makes no sense whatsoever that multisig is treated as “multiple addresses”.

luke-jr commented at 7:45 pm on November 15, 2020: member

Note: there is always the easy option of just fixing the reqSigs behavior instead of removing it.

If it’s broken, we should fix it (for backporting) before removing.

It also makes me think {RPCResult::Type::ARR, “addresses”, “array of bitcoin addresses”} is a bit misleading, and would ideally be a single {RPCResult::Type::STR, “address”, “bitcoin address”}.

I’m not sure the array format is entirely/hypothetically useless. Might as well leave it alone (or at least propose this change separately).

MarcoFalke removed the label Docs on Nov 16, 2020

mjdietzx force-pushed on Nov 18, 2020

mjdietzx commented at 10:44 pm on November 18, 2020: contributor

I’ve re-worked this PR, and force pushed, based on the convo/feedback in this thread. Broken into two commits:

https://github.com/bitcoin/bitcoin/pull/20286/commits/f5d3cf6ceb3bb578bfce36959744ee7035eaf6c0 “fixes” reqSigs. Instead of defaulting to 1 which is confusing (and possibly incorrect?) reqSigs is not included in rpc outputs unless the transaction is a bare multisig.
https://github.com/bitcoin/bitcoin/pull/20286/commits/c0545cc866a69da7ee30bf7cd5b9a84262da5778 deprecates reqSigs as discussed, where reqSigs is totally removed from rpc outputs, and addresses is no longer returned for bare multsigs. https://github.com/bitcoin/bitcoin/pull/20286/commits/c0545cc866a69da7ee30bf7cd5b9a84262da5778#diff-80ada085034954b397668e082d78cd40519aa9b162ec52101651dd48c48a17b3L18-L24 basically becomes a functional test, where we check the default case, and -deprecatedrpc=reqSigs are both correct

Note: This PR turned out to be more involved than I expected. I’ll probably need some more feedback and pointers to get this across the finish line.

Also: we’ll be able to delete a few chunks of code when we can get rid of -deprecatedrpc=reqSigs. I indicate this in the source with TODO comments.

sipa commented at 10:52 pm on November 18, 2020: member

@mjdietzx I think it’s a bit weird to fix "addresses"/"reqSigs", as it’s making a semantics change simultaneously with a deprecation.

My suggestion would be to:

add a new sane "address" field (for outputs that have an identifiable address, which doesn’t include bare multisig)
with -deprecatedrpc: leave "reqSigs" and "addresses" intact (with all weird/wrong behavior they have now)
without -deprecatedrpc: drop "reqSigs" and "addresses" entirely, always.

That means that anyone who is currently relying on the weird `“reqSigs”: `` behavior for unknown address types (for who knows what reason) isn’t affected if they set the deprecation option.

mjdietzx commented at 0:34 am on November 19, 2020: contributor

I like the approach you’ve outlined @sipa. Any objections before I go ahead and do this @luke-jr? I know you weren’t quite convinced re: addresses => address.

luke-jr commented at 0:51 am on November 19, 2020: member

I’d prefer to keep a single-item array (better compatibility, and future-proof), but I don’t care enough to object.

DrahtBot commented at 4:19 am on November 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21544 (rpc: Missing doc updates for bumpfee psbt update by MarcoFalke)
#21366 (refactor: replace util::Ref with std::any (C++17) by theStack)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#21245 (rpc: Add level 3 verbosity to getblock RPC call. by fyquah)
#20808 (test: Run rpc_generateblock.py even with wallet disabled by nginocchio)

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.

mjdietzx commented at 7:05 pm on November 20, 2020: contributor

I’ll need to re-work some of the functional tests (will get this committed soon) now that addresses is removed by default, but https://github.com/bitcoin/bitcoin/pull/20286/commits/162e7f0969bf593e66ec2e59e24c4c555cd101f7 updates this PR to be as @sipa suggested

mjdietzx force-pushed on Nov 23, 2020

mjdietzx renamed this:
~~rpc: remove `reqSigs` from rpc outputs~~
rpc: deprecate `addresses` and `reqSigs` from rpc outputs
on Nov 23, 2020

mjdietzx force-pushed on Nov 23, 2020

mjdietzx commented at 6:28 pm on November 23, 2020: contributor

Piggy backing off of #20211, now that we have:

0bool ExtractDestination(const CScript& scriptPubKey, TxoutType& typeRet, CTxDestination& addressRet);

Could we do something like:

 0TxoutType type;
 1CTxDestination dest;
 2ExtractDestination(m_script, type, dest);
 3switch (type) {
 4case TxoutType::PUBKEYHASH:
 5case TxoutType::SCRIPTHASH: return OutputType::LEGACY;
 6case TxoutType::WITNESS_V0_SCRIPTHASH:
 7case TxoutType::WITNESS_V0_KEYHASH:
 8case TxoutType::WITNESS_V1_TAPROOT:
 9case TxoutType::WITNESS_UNKNOWN: return OutputType::BECH32;
10case TxoutType::PUBKEY:
11case TxoutType::MULTISIG:
12case TxoutType::NONSTANDARD:
13case TxoutType::NULL_DATA: return nullopt; // no default case, so the compiler can warn about missing cases
14}
15assert(false);

Instead of what we have now:

 0CTxDestination dest;
 1ExtractDestination(m_script, dest);
 2switch (dest.which()) {
 3  case 1 /* PKHash */:
 4  case 2 /* ScriptHash */: return OutputType::LEGACY;
 5  case 3 /* WitnessV0ScriptHash */:
 6  case 4 /* WitnessV0KeyHash */:
 7  case 5 /* WitnessUnknown */: return OutputType::BECH32;
 8  case 0 /* CNoDestination */:
 9  default: return nullopt;
10}

Note: I messed around with this, and probably made some mistakes here, but didn’t see any unit tests or functional tests fail. So maybe some test coverage is needed here as well?

mjdietzx force-pushed on Nov 24, 2020

mjdietzx commented at 6:28 pm on November 24, 2020: contributor

This PR is ready for review again. Squashed into two commits (bc https://github.com/bitcoin/bitcoin/pull/20286/commits/07ff0f7d15b99a40ec09f1315bcfcd9e4a6099a2 refactor was required to fix a circular dependency lint error introduced by using IsDeprecatedRPCEnabled() in ScriptPubKeyToUniv())

mjdietzx force-pushed on Nov 28, 2020

mjdietzx commented at 6:32 pm on December 15, 2020: contributor

Hey, this PR seems a bit dead in the water. Does anyone have suggestions on how I can help move this through the review process?

My guess is this change is making it hard to review: https://github.com/bitcoin/bitcoin/pull/20286/files#diff-d3f64e14005fbfea3d4f72b076764ac897d0df451de0ab2fb1c57a5f87cd793bR23-R26

But that is the most straightforward way I could see to get rid of a circular dependency and allow me to use IsDeprecatedRPCEnabled in core_write.cpp here: https://github.com/bitcoin/bitcoin/pull/20286/files#diff-48fa743e653b0ceebb1e454766c5a2e70981a44503eca6adf24895016db9eb26R164

Any thoughts/suggestions?

in src/rpc/rawtransaction.cpp:135 in 1bbe210727 outdated

131@@ -132,7 +132,7 @@ static RPCHelpMan getrawtransaction()
132                                      {
133                                          {RPCResult::Type::STR, "asm", "the asm"},
134                                          {RPCResult::Type::STR, "hex", "the hex"},
135-                                         {RPCResult::Type::NUM, "reqSigs", "The required sigs"},
136+                                         {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=reqSigs is passed) Number of required signatures (only present for bare multisig"},

sipa commented at 6:34 pm on December 15, 2020:

I don’t think it’s technically true anymore in the current PR that this is only present for bare multisig (here and elsewhere).

in src/rpc/blockchain.cpp:1107 in c81ffe5228 outdated

1103@@ -1104,8 +1104,8 @@ static RPCHelpMan gettxout()
1104                             {
1105                                 {RPCResult::Type::STR_HEX, "asm", ""},
1106                                 {RPCResult::Type::STR_HEX, "hex", ""},
1107-                                {RPCResult::Type::NUM, "reqSigs", "Number of required signatures"},
1108-                                {RPCResult::Type::STR_HEX, "type", "The type, eg pubkeyhash"},
1109+                                {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=reqSigs is passed) Number of required signatures (only present for bare multisig"},

sipa commented at 6:42 pm on December 15, 2020:

This change seems to be in the wrong commit.

sipa commented at 6:46 pm on December 15, 2020: member

Concept and approach ACK on the first commit. The circular dependency fix seems weird to me as the CoinStatsHashType really belongs together with GetUTXOStates; moving it out makes its purpose very unclear, and makes the code more confusing, not less. The goal of the circular dependency checker is that such dependencies are usually a sign of bad design or hard-to-follow code, and we want to avoid introducing that. But if the cure is worse than the disease, just add an exception.

mjdietzx force-pushed on Dec 15, 2020

mjdietzx commented at 10:19 pm on December 15, 2020: contributor

Concept and approach ACK on the first commit. The circular dependency fix seems weird to me as the CoinStatsHashType really belongs together with GetUTXOStates; moving it out makes its purpose very unclear, and makes the code more confusing, not less. The goal of the circular dependency checker is that such dependencies are usually a sign of bad design or hard-to-follow code, and we want to avoid introducing that. But if the cure is worse than the disease, just add an exception.

Ah yeah that’s much better 👍. I addressed this, and your other comments in this commit: 811947c0527fa868157352434a5c310609dd5c63

DrahtBot added the label Needs rebase on Dec 15, 2020

mjdietzx force-pushed on Dec 16, 2020

mjdietzx commented at 2:09 am on December 16, 2020: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Squashed and rebased

DrahtBot removed the label Needs rebase on Dec 16, 2020

in src/Makefile.am:512 in 1cca632e59 outdated

507   scheduler.cpp \
508   script/descriptor.cpp \
509   script/sign.cpp \
510   script/signingprovider.cpp \
511   script/standard.cpp \
512+  shutdown.cpp \

MarcoFalke commented at 8:36 am on December 16, 2020:

Can you explain what this is doing and why?

Also, the ci is failing

mjdietzx commented at 3:51 pm on December 16, 2020:

Since I’ve added rpc/server.cpp to libbitcoin_common_a_SOURCES, and rpc/server.cpp in turn includes shutdown.h and invokes StartShutdown() here, I need to include shutdown.cpp in libbitcoin_common_a_SOURCES

Otherwise we’ll get a compile error:

0Undefined symbols for architecture x86_64:
1  "StartShutdown()", referenced from:
2      std::__1::__function::__func<stop()::$_3, std::__1::allocator<stop()::$_3>, UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) in libbitcoin_common.a(libbitcoin_common_a-server.o)
3ld: symbol(s) not found for architecture x86_64
4clang: error: linker command failed with exit code 1 (use -v to see invocation)

mjdietzx commented at 4:03 pm on December 16, 2020:

Looks like I made a mistake resolving a merge conflict when I rebased to master, which caused CI to fail.

The CI error was:

0Good job! The circular dependency "policy/fees -> txmempool -> validation -> policy/fees" is no longer present.
1458Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in test/lint/lint-circular-dependencies.sh
2459to make sure this circular dependency is not accidentally reintroduced.

I had a single merge conflict in lint-circular-dependencies.sh where: "policy/fees -> txmempool -> validation -> policy/fees" needed to be removed, but I re-added it instead of removing it when resolving the merge conflict.

Fixed now: https://github.com/bitcoin/bitcoin/pull/20286/files#diff-ee6a5803b78eb12f095b391894014b5fb2b836cef29943980b3ab684861c1f2dR23

mjdietzx force-pushed on Dec 16, 2020

mjdietzx force-pushed on Dec 22, 2020

mjdietzx commented at 3:39 am on December 22, 2020: contributor

Looks like this ones gonna miss the v0.21.0 boat 🚢

Either way, want to see if there’s anything I can do to help move this review forward. All feedback in this review until now has been addressed, and I squashed into single commit

in src/core_write.cpp:152 in d6932139c2 outdated

151-    if (include_address && ExtractDestination(script, address) && type != TxoutType::PUBKEY) {
152+
153+    if (ExtractDestination(script, type, address) && include_address && type != TxoutType::PUBKEY) {
154         out.pushKV("address", EncodeDestination(address));
155     }
156+    out.pushKV("type", GetTxnOutputType(type));

MarcoFalke commented at 8:22 am on December 22, 2020:

Why are you changing the order?

sipa commented at 4:06 am on December 27, 2020:

This doesn’t seem addressed: https://github.com/bitcoin/bitcoin/pull/20286/files#r547134259 (it’s ok if you disagree with the comment, or there is a good reason to - but do respond if you can).

mjdietzx commented at 7:54 pm on December 27, 2020:

Edit: After thinking about it more, I agree - this is refactoring that doesn’t belong in this PR. I reverted these refactors

~~The first thing done in ExtractDestination is~~ ~~std::vector vSolutions;~~ ~~whichType = Solver(scriptPubKey, vSolutions);~~ ~~so prior to this change, Solver code was duplicated and running twice (see the removed code in this diff).~~

By changing the order in the if statement from include_address && ExtractDestination(script, address => ExtractDestination(script, type, address) && include_address we are guaranteed to always have type set by ExtractDestination, and can get rid of the duplicated Solver code here.

in src/core_write.cpp:164 in d6932139c2 outdated

171 
172-    if (!ExtractDestinations(scriptPubKey, type, addresses, nRequired) || type == TxoutType::PUBKEY) {
173-        out.pushKV("type", GetTxnOutputType(type));
174-        return;
175+    // TODO: from v0.22 ("addresses" and "reqSigs" deprecated) this entire if statement should be removed
176+    if (IsDeprecatedRPCEnabled("reqSigs")) { // cruft...

MarcoFalke commented at 8:23 am on December 22, 2020:

Seems like a layer violation. You can add a ScriptPubKeyToUniv to rpc/blockchain.h, which calls this ScriptPubKeyToUniv and passes an additional boolean parameter with the value of IsDeprecatedRPCEnabled('reqsiqs')

in src/core_write.cpp:181 in d6932139c2 outdated

194-        a.push_back(EncodeDestination(addr));
195+    if (ExtractDestination(scriptPubKey, type, address) && type != TxoutType::PUBKEY) {
196+        out.pushKV("address", EncodeDestination(address));
197     }
198-    out.pushKV("addresses", a);
199+    out.pushKV("type", GetTxnOutputType(type));

MarcoFalke commented at 8:23 am on December 22, 2020:

Same

mjdietzx commented at 7:58 pm on December 27, 2020:

I changed the order here so that when the deprecation period is over, we just simply delete the if (fIncludeDeprecated) { branch. It also cleaned things up a bit as we don’t need to push out.pushKV("type", GetTxnOutputType(type)); twice. The behavior is the same though

mjdietzx commented at 9:02 pm on December 27, 2020:

Edit: I got rid of any unnecessary refactors/changes here

Anyways, if you think this is a bad idea or doesn’t belong in this PR, I have no problem undoing these changes and trying to stick to the original code as closely as possible - just lmk. Part of the reason I added some fuzz tests in this PR #20772 was to help build confidence since these two functions have to be touched in this PR either way

in src/script/standard.cpp:224 in d6932139c2 outdated

220@@ -221,6 +221,12 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
221     return false;
222 }
223 
224+bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet) {

MarcoFalke commented at 8:24 am on December 22, 2020:

0bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
1{

MarcoFalke commented at 8:25 am on December 22, 2020: member

I still don’t like changing the build system for a minor rpc change

mjdietzx force-pushed on Dec 26, 2020

in src/core_write.cpp:164 in 45d411a13f outdated

171-    if (!ExtractDestinations(scriptPubKey, type, addresses, nRequired) || type == TxoutType::PUBKEY) {
172-        out.pushKV("type", GetTxnOutputType(type));
173-        return;
174+    // TODO: from v0.22 ("addresses" and "reqSigs" deprecated) this entire if statement should be removed
175+    const std::vector<std::string> enabled_methods = gArgs.GetArgs("-deprecatedrpc");
176+    if (find(enabled_methods.begin(), enabled_methods.end(), "reqSigs") != enabled_methods.end()) {

sipa commented at 4:07 am on December 27, 2020:

I’d think that “addresses” being removed is a more prominent change, so perhaps the deprecatedrpc name should be “addressed” ?

in src/rpc/rawtransaction.cpp:137 in 45d411a13f outdated

131@@ -132,7 +132,7 @@ static RPCHelpMan getrawtransaction()
132                                      {
133                                          {RPCResult::Type::STR, "asm", "the asm"},
134                                          {RPCResult::Type::STR, "hex", "the hex"},
135-                                         {RPCResult::Type::NUM, "reqSigs", "The required sigs"},
136+                                         {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=reqSigs is passed) Number of required signatures"},
137                                          {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
138                                          {RPCResult::Type::ARR, "addresses", "",

sipa commented at 4:09 am on December 27, 2020:

This should probably also get a changed comment.

in src/rpc/rawtransaction.cpp:495 in 45d411a13f outdated

489@@ -490,7 +490,7 @@ static RPCHelpMan decoderawtransaction()
490                                 {
491                                     {RPCResult::Type::STR, "asm", "the asm"},
492                                     {RPCResult::Type::STR_HEX, "hex", "the hex"},
493-                                    {RPCResult::Type::NUM, "reqSigs", "The required sigs"},
494+                                    {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=reqSigs is passed) Number of required signatures"},
495                                     {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
496                                     {RPCResult::Type::ARR, "addresses", "",

sipa commented at 4:09 am on December 27, 2020:

Same here.

in src/rpc/rawtransaction.cpp:552 in 45d411a13f outdated

547@@ -548,7 +548,7 @@ static RPCHelpMan decodescript()
548                     {
549                         {RPCResult::Type::STR, "asm", "Script public key"},
550                         {RPCResult::Type::STR, "type", "The output type (e.g. "+GetAllOutputTypes()+")"},
551-                        {RPCResult::Type::NUM, "reqSigs", "The required signatures"},
552+                        {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=reqSigs is passed) Number of required signatures"},
553                         {RPCResult::Type::ARR, "addresses", "",

sipa commented at 4:09 am on December 27, 2020:

And here.

in src/rpc/rawtransaction.cpp:562 in 45d411a13f outdated

558@@ -559,7 +559,6 @@ static RPCHelpMan decodescript()
559                             {RPCResult::Type::STR, "asm", "String representation of the script public key"},
560                             {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"},
561                             {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
562-                            {RPCResult::Type::NUM, "reqSigs", "The required signatures (always 1)"},
563                             {RPCResult::Type::ARR, "addresses", "(always length 1)",

sipa commented at 4:09 am on December 27, 2020:

And here.

in src/wallet/rpcwallet.cpp:3676 in 45d411a13f outdated

3672@@ -3672,9 +3673,9 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
3673             // Always report the pubkey at the top level, so that `getnewaddress()['pubkey']` always works.
3674             if (subobj.exists("pubkey")) obj.pushKV("pubkey", subobj["pubkey"]);
3675             obj.pushKV("embedded", std::move(subobj));
3676-        } else if (which_type == TxoutType::MULTISIG) {
3677-            // Also report some information on multisig scripts (which do not have a corresponding address).
3678-            // TODO: abstract out the common functionality between this logic and ExtractDestinations.
3679+        } else if (find(enabled_methods.begin(), enabled_methods.end(), "reqSigs") != enabled_methods.end() &&

sipa commented at 4:14 am on December 27, 2020:

It’s pretty ugly to duplicate this logic in many places. I’d suggest just using IsDeprecatedRPCEnabled, in the top-level RPC code, and pass down a boolean to wherever it’s needed (e.g. in core_write, make ScriptPubKeyToUniv take a boolean to indicate whether to not to include reqSigs / addresses).

mjdietzx commented at 8:58 pm on December 27, 2020:

I was able to do this how you and @MarcoFalke suggested in this commit https://github.com/bitcoin/bitcoin/pull/20286/commits/dba1053a5e0c74e53ded3d42c3826373474ba805. However, I’m a little confused by what’s going on in rpcwallet.cpp. I’m able to use IsDeprecatedRPCEnabled here when I build and test locally (macOS), and it seems some of the CI test runs pass. However, some of the CI runs throw a compile error in this case. Any ideas or suggestions on the approach I should take for rpcwallet?

in src/wallet/rpcwallet.cpp:3678 in 45d411a13f outdated

3672@@ -3672,9 +3673,9 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
3673             // Always report the pubkey at the top level, so that `getnewaddress()['pubkey']` always works.
3674             if (subobj.exists("pubkey")) obj.pushKV("pubkey", subobj["pubkey"]);
3675             obj.pushKV("embedded", std::move(subobj));
3676-        } else if (which_type == TxoutType::MULTISIG) {
3677-            // Also report some information on multisig scripts (which do not have a corresponding address).
3678-            // TODO: abstract out the common functionality between this logic and ExtractDestinations.
3679+        } else if (find(enabled_methods.begin(), enabled_methods.end(), "reqSigs") != enabled_methods.end() &&
3680+                   which_type == TxoutType::MULTISIG) {
3681+            // TODO: from v0.22 ("addresses" and "reqSigs" deprecated) this entire else if statement should be removed

sipa commented at 4:15 am on December 27, 2020:

Note that the next major release after 0.21 is going to be v22 (not v0.22); see #20223.

mjdietzx force-pushed on Dec 27, 2020

in test/util/data/txcreatemultisig1.json:18 in 726cec1384 outdated

20-                "addresses": [
21-                    "1FoG2386FG2tAJS9acMuiDsKy67aGg9MKz",
22-                    "1FXtz9KU8JNmQDyHdiEm5HDiALuP3zdHvV",
23-                    "14LuavcBbXZYJ6Tsz3cAUQj9SuQoL2xCQX"
24-                ]
25+                "address": "1FoG2386FG2tAJS9acMuiDsKy67aGg9MKz",

mjdietzx commented at 4:47 pm on December 28, 2020:

Please note this behavior for bare multisigs. I’m not totally sure what we want address to be in this case. For now I just made it the first address that ExtractDestinations sets in addresses. But maybe address should not be included for type == TxoutType::MULTISIG?

Looking forward to when we get rid of the deprecatedrpc=addresses logic, we’ll end up removing ExtractDestinations completely. And at that point we won’t necessarily have an addresses array for type == TxoutType::MULTISIG - so not including address in the response for bare multisigs would seem to be the long-term (and maybe more correct) solution?

sipa commented at 4:48 pm on December 28, 2020:

Bare multisig outputs should not have any address, because there is no address that corresponds to their scriptPubKey.

mjdietzx commented at 5:07 pm on December 28, 2020:

Ok, done https://github.com/bitcoin/bitcoin/pull/20286/commits/98b7080d7eeece1081eed3c68dd092e974ad37ee

in src/core_io.h:47 in 98b7080d7e outdated

43@@ -44,8 +44,8 @@ UniValue ValueFromAmount(const CAmount& amount);
44 std::string FormatScript(const CScript& script);
45 std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
46 std::string SighashToStr(unsigned char sighash_type);
47-void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
48+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex, bool fIncludeAddresses);

sipa commented at 5:38 pm on December 28, 2020:

Nit: follow the style guide in doc/developer-notes.md for new code (use snake_case, not fHungarianNotation).

in src/core_write.cpp:174 in 98b7080d7e outdated

170@@ -169,17 +171,21 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
171         return;
172     }
173 
174-    out.pushKV("reqSigs", nRequired);
175+    if (type != TxoutType::MULTISIG)

sipa commented at 5:39 pm on December 28, 2020:

Nit: follow coding style in doc/developer-notes.md for new code ({} around if/then/else blocks if spanning more than one line).

in src/core_write.cpp:175 in 98b7080d7e outdated

170@@ -169,17 +171,21 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
171         return;
172     }
173 
174-    out.pushKV("reqSigs", nRequired);
175+    if (type != TxoutType::MULTISIG)
176+        out.pushKV("address", EncodeDestination(addresses[0]));

sipa commented at 5:41 pm on December 28, 2020:

It’d be better to use ExtractDestination here, I think, so you can avoid the need for special casing bare multisig.

in src/rpc/blockchain.cpp:14 in 98b7080d7e outdated

10@@ -11,7 +11,6 @@
11 #include <chainparams.h>
12 #include <coins.h>
13 #include <consensus/validation.h>
14-#include <core_io.h>

sipa commented at 5:41 pm on December 28, 2020:

Removing this seems incorrect, there are still core_io functions used in this .cpp file.

in src/rpc/blockchain.cpp:1124 in 98b7080d7e outdated

1118@@ -1120,9 +1119,10 @@ static RPCHelpMan gettxout()
1119                             {
1120                                 {RPCResult::Type::STR_HEX, "asm", ""},
1121                                 {RPCResult::Type::STR_HEX, "hex", ""},
1122-                                {RPCResult::Type::NUM, "reqSigs", "Number of required signatures"},
1123-                                {RPCResult::Type::STR_HEX, "type", "The type, eg pubkeyhash"},
1124-                                {RPCResult::Type::ARR, "addresses", "array of bitcoin addresses",
1125+                                {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Number of required signatures"},
1126+                                {RPCResult::Type::STR, "type", "The type, eg pubkeyhash"},
1127+                                {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (not included for bare multisig)"},

sipa commented at 5:44 pm on December 28, 2020:

I think this comment is more confusing than helpful. Multisig outputs do not have an address; that’s exactly what we’re trying to address by removing the weird “addresses” reported for it. It’s true of course that no “address” is listed, but there is also not one for P2PK, or whatever other weird scripts someone might invent. Perhaps just say “(only if a well-defined address exists)” ?

~~EDIT: I realize that the current code also reports an address for P2PK outputs. That’s equally wrong IMO, and behavior that should be removed for the new “address” field.~~

EDEDITIT: My EDIT above was wrong.

in src/core_io.h:49 in 98b7080d7e outdated

43@@ -44,8 +44,8 @@ UniValue ValueFromAmount(const CAmount& amount);
44 std::string FormatScript(const CScript& script);
45 std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
46 std::string SighashToStr(unsigned char sighash_type);
47-void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
48+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex, bool fIncludeAddresses);
49 void ScriptToUniv(const CScript& script, UniValue& out, bool include_address);
50-void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);
51+void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool fIncludeAddresses = true, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);

sipa commented at 5:50 pm on December 28, 2020:

Making the fIncludeAddresses field optional here makes this code a bit hard to review, as any call site that isn’t changed may now reinterpret the true in e.g. TxToUniv(tx, hash, entry, true) as fIncludeAddresses rather than include_hex. There aren’t that many call sites, so I’d suggest just making the new argument mandatory, and changing them all.

in src/rpc/blockchain.cpp:1780 in 98b7080d7e outdated

1777@@ -1778,6 +1778,11 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
1778     }
1779 }
1780 
1781+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex)

sipa commented at 6:21 pm on December 28, 2020:

There is only one invocation of this function in this .cpp file. Maybe just inline it?

in src/rpc/net.cpp:228 in 98b7080d7e outdated

224@@ -225,12 +225,12 @@ static RPCHelpMan getpeerinfo()
225         obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to);
226         obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from);
227         if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) {
228-            // addnode is deprecated in v0.21 for removal in v0.22
229+            // addnode is deprecated in v0.21 for removal in v22

sipa commented at 6:22 pm on December 28, 2020:

Nit: no need to change unrelated code. I just mentioned the version numbering change as an FYI.

in src/rpc/rawtransaction.cpp:9 in 98b7080d7e outdated

5@@ -6,7 +6,6 @@
6 #include <chain.h>
7 #include <coins.h>
8 #include <consensus/validation.h>
9-#include <core_io.h>

sipa commented at 6:22 pm on December 28, 2020:

This file is still used.

in src/rpc/rawtransaction.cpp:135 in 98b7080d7e outdated

129@@ -132,9 +130,10 @@ static RPCHelpMan getrawtransaction()
130                                      {
131                                          {RPCResult::Type::STR, "asm", "the asm"},
132                                          {RPCResult::Type::STR, "hex", "the hex"},
133-                                         {RPCResult::Type::NUM, "reqSigs", "The required sigs"},
134+                                         {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Number of required signatures"},
135                                          {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
136-                                         {RPCResult::Type::ARR, "addresses", "",
137+                                         {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (not included for bare multisig)"},

sipa commented at 6:23 pm on December 28, 2020:

Same comment about mentioning bare multisig here being confusing.

in src/rpc/rawtransaction.cpp:494 in 98b7080d7e outdated

488@@ -490,9 +489,10 @@ static RPCHelpMan decoderawtransaction()
489                                 {
490                                     {RPCResult::Type::STR, "asm", "the asm"},
491                                     {RPCResult::Type::STR_HEX, "hex", "the hex"},
492-                                    {RPCResult::Type::NUM, "reqSigs", "The required sigs"},
493+                                    {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Number of required signatures"},
494                                     {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
495-                                    {RPCResult::Type::ARR, "addresses", "",
496+                                    {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (not included for bare multisig)"},

sipa commented at 6:23 pm on December 28, 2020:

And here.

in src/rpc/rawtransaction.cpp:551 in 98b7080d7e outdated

547@@ -548,8 +548,9 @@ static RPCHelpMan decodescript()
548                     {
549                         {RPCResult::Type::STR, "asm", "Script public key"},
550                         {RPCResult::Type::STR, "type", "The output type (e.g. "+GetAllOutputTypes()+")"},
551-                        {RPCResult::Type::NUM, "reqSigs", "The required signatures"},
552-                        {RPCResult::Type::ARR, "addresses", "",
553+                        {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (not included for bare multisig)"},

sipa commented at 6:24 pm on December 28, 2020:

And here.

in src/test/fuzz/script.cpp:7 in 98b7080d7e outdated

3@@ -4,10 +4,10 @@
4 
5 #include <chainparams.h>
6 #include <compressor.h>
7-#include <core_io.h>

sipa commented at 6:25 pm on December 28, 2020:

Still used.

in src/test/fuzz/script.cpp:10 in 98b7080d7e outdated

 3@@ -4,10 +4,10 @@
 4 
 5 #include <chainparams.h>
 6 #include <compressor.h>
 7-#include <core_io.h>
 8 #include <core_memusage.h>
 9 #include <policy/policy.h>
10 #include <pubkey.h>
11+#include <rpc/blockchain.h>

sipa commented at 6:26 pm on December 28, 2020:

This is pretty ugly. The fuzz test shouldn’t be using command-line options at all; just invoke the explicit-addresses version once with false and once with true, if you need to.

sipa commented at 6:36 pm on December 28, 2020: member

The compile error is probably due to a library dependency order (wallet/rpcwallet and rpc/server are in different libraries, so they may not be able to use each other in both directions). Try using pwallet->chain().rpcEnableDeprecated("addresses").

in src/script/standard.cpp:224 in 98b7080d7e outdated

220@@ -221,6 +221,7 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
221     return false;
222 }
223 
224+// TODO: from v22 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed

sipa commented at 7:22 pm on December 28, 2020:

This is incorrect. Current master will become v22. This functionality can be removed in v23.

MarcoFalke commented at 8:22 pm on December 28, 2020: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

mjdietzx force-pushed on Dec 28, 2020

in src/core_write.cpp:178 in 9fc577a6ab outdated

171@@ -169,17 +172,22 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
172         return;
173     }
174 
175-    out.pushKV("reqSigs", nRequired);
176+    if (ExtractDestination(scriptPubKey, address)) {

sipa commented at 8:48 pm on December 28, 2020:

See my earlier edit here: #20286 (review)

For “address” we shouldn’t report anything for P2PK outputs either, as the address that’s reported currently is really the P2PKH one for the same pubkey (and sending to that P2PKH address would have a distinct scriptPubKey). This is due to historical reasons where addresses used to also be “pubkey id” (that’s the same reason as why “addresses” were reported for bare multisig; those addresses in modern meaning have nothing to do with that multisig address).

Can you add a && GetScriptForDestination(address) == scriptPubKey? That would guarantee that whatever address is extracted exactly matches the output.

mjdietzx commented at 9:22 pm on December 28, 2020:

I pushed a commit as you suggest: https://github.com/bitcoin/bitcoin/pull/20286/commits/97fbaffc4aea265d638895531dc3b3c84669c561

I’m a little confused though. Aren’t we already not reporting anything for P2PK bc we return here https://github.com/bitcoin/bitcoin/blob/master/src/core_write.cpp#L167, if type == TxoutType::PUBKEY?

Also, I want to know if this problem could affect us anywhere else. It seems ExtractDestination already handles this https://github.com/bitcoin/bitcoin/blob/master/src/core_write.cpp#L151 (only pushes an address if type != TxoutType::PUBKEY) – but if not, rawtransaction.cpp invokes ScriptToUniv which may need this check as well

Last, I’m trying to figure out if anything needs to be done in rpcwallet, here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L3669

sipa commented at 9:31 pm on December 28, 2020:

I’m a little confused though. Aren’t we already not reporting anything for P2PK bc we return here https://github.com/bitcoin/bitcoin/blob/master/src/core_write.cpp#L167, if type == TxoutType::PUBKEY?

You’re right, this change isn’t needed. It seems the specific P2PK case was already addressed earlier.

Also, I want to know if this problem could affect us anywhere else. It seems ExtractDestination already handles this https://github.com/bitcoin/bitcoin/blob/master/src/core_write.cpp#L151 (only pushes an address if type != TxoutType::PUBKEY) – but if not, rawtransaction.cpp invokes ScriptToUniv which may need this check as well

The code you’re linking to is ScriptToUniv, not ExtractDestination. ExtractDestination does produce output for P2PK outputs. It makes sense to see what other call sites may be relying on this P2PK behavior though; if there aren’t any, or they are all bogus, it may be easier to just drop support for P2PK in ExtractDestinations.

Last, I’m trying to figure out if anything needs to be done in rpcwallet, here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L3669

That looks like a bug, but a hard to trigger one. You’d need a P2PK script inside a P2SH or P2WSH script in a wallet for that.

mjdietzx commented at 9:45 pm on December 28, 2020:

OK, I’ll go ahead and revert the commit since it isn’t needed.

The code you’re linking to is ScriptToUniv, not ExtractDestination. ExtractDestination does produce output for P2PK outputs. It makes sense to see what other call sites may be relying on this P2PK behavior though; if there aren’t any, or they are all bogus, it may be easier to just drop support for P2PK in ExtractDestinations.

Ah yeah good point. Well I’ll read through the code and try to figure out if an issue needs to be created. I guess if ExtractDestination(s) dropped support for P2PK it may also fixrpcwallet (if it does turn out to be a bug) https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L3669. It’ll take me some time to fully understand all this and test some things out

mjdietzx force-pushed on Dec 28, 2020

MarcoFalke commented at 9:55 pm on December 28, 2020: member

Btw, you don’t have to mention me in the commit message. All I did was some minor review suggestion. :sweat_smile:

mjdietzx force-pushed on Dec 28, 2020

mjdietzx commented at 10:02 pm on December 28, 2020: contributor

Btw, you don’t have to mention me in the commit message. All I did was some minor review suggestion. 😅

Ah ok, I think GH automatically added that since I committed your suggestion, I just took it out.

Anyways thank you @MarcoFalke @sipa for all the review so far. I know this has been a kinda brutal review process for a minor change - I’m learning so write it off as an investment in a future contributor 😬

in src/wallet/rpcwallet.cpp:3675 in 4f4b466c73 outdated

3671@@ -3671,9 +3672,8 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
3672             // Always report the pubkey at the top level, so that `getnewaddress()['pubkey']` always works.
3673             if (subobj.exists("pubkey")) obj.pushKV("pubkey", subobj["pubkey"]);
3674             obj.pushKV("embedded", std::move(subobj));
3675-        } else if (which_type == TxoutType::MULTISIG) {
3676-            // Also report some information on multisig scripts (which do not have a corresponding address).
3677-            // TODO: abstract out the common functionality between this logic and ExtractDestinations.
3678+        } else if (which_type == TxoutType::MULTISIG && pwallet->chain().rpcEnableDeprecated("addresses")) {

sipa commented at 9:30 pm on December 29, 2020:

Just noticed this: please don’t remove this. This isn’t doing anything confusing (it’s reporting actual pubkeys, not addresses), and isn’t restricted to bare multisig. Reporting detailed information about properly wrapped multisig isn’t something we should remove.

mjdietzx commented at 2:30 am on December 30, 2020:

Thanks for catching that - I reverted this change and updated the docs. re-squashed

mjdietzx force-pushed on Dec 30, 2020

sipa commented at 11:04 pm on January 21, 2021: member

utACK 3c2b24b8f5798976e4d9b6b873f423e955098b47

in src/bitcoin-tx.cpp:728 in 3c2b24b8f5 outdated

724@@ -725,7 +725,7 @@ static void MutateTx(CMutableTransaction& tx, const std::string& command,
725 static void OutputTxJSON(const CTransaction& tx)
726 {
727     UniValue entry(UniValue::VOBJ);
728-    TxToUniv(tx, uint256(), entry);
729+    TxToUniv(tx, uint256(), entry, false); // TODO: bitcoin-tx doesn't go through the addresses deprecation phase currently, defaults to new behavior. Do we want to add a deprecation flag here?

MarcoFalke commented at 1:17 pm on January 22, 2021:

e2b814e315f011b6afea26a1ac10aa9f31b5fddb: This needs release notes

0    TxToUniv(tx, uint256(), entry, /* include_addresses */ false);

in src/core_io.h:49 in 3c2b24b8f5 outdated

43@@ -44,8 +44,8 @@ UniValue ValueFromAmount(const CAmount& amount);
44 std::string FormatScript(const CScript& script);
45 std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
46 std::string SighashToStr(unsigned char sighash_type);
47-void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
48+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex, bool include_addresses);
49 void ScriptToUniv(const CScript& script, UniValue& out, bool include_address);
50-void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);
51+void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_addresses, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);

MarcoFalke commented at 1:24 pm on January 22, 2021:

e2b814e315f011b6afea26a1ac10aa9f31b5fddb:

This is not safe in light of silent merge conflicts. Passing an array of bool args (some with default values) without named args can lead to bugs. I think there are two options:

Remove all default args (might be controversial)
Change the order so that no two neighbouring args have the same type

I think you can just add the new arg to the beginning (or close to it). It will be removed anyway, so any style concerns that the arg is “too early” are irrelevant.

mjdietzx commented at 9:21 pm on January 31, 2021:

I updated TxToUniv to use this wrapper approach, so it is now used in both cases. As to why I went with this wrapper approach in the first place, it was suggested earlier in the review (unless I mis-interpreted something):

I'd suggest just using IsDeprecatedRPCEnabled, in the top-level RPC code, and pass down a boolean to wherever it's needed (e.g. in core_write, make ScriptPubKeyToUniv take a boolean to indicate whether to not to include reqSigs / addresses).

I know it reduces the diff a bit and meant we didn’t have changes in as many files. I think it got rid of a circular include dependency as well (since we don’t have to include rpc/server in a bunch of places now). – and it may have even been necessary to allow this PR to happen without touching the build system as including rpc/server in some files was causing trouble. I don’t entirely remember, but know it was done for sane reasons, and I think you had suggested something like this early in the review (although your earlier review comments aren’t showing in this PR now)

mjdietzx commented at 9:58 pm on January 31, 2021:

I meant for this to be a response to your other comment: “Can you explain why you are using this wrapper approach for ScriptPubKeyToUniv but not for TxToUniv. Either use it for both or none.”

in src/rpc/blockchain.cpp:1121 in 3c2b24b8f5 outdated

1119@@ -1120,9 +1120,10 @@ static RPCHelpMan gettxout()
1120                             {
1121                                 {RPCResult::Type::STR_HEX, "asm", ""},

MarcoFalke commented at 1:36 pm on January 22, 2021:

unrelated nit

0                                {RPCResult::Type::STR, "asm", ""},

in src/script/standard.cpp:220 in 3c2b24b8f5 outdated

220@@ -221,6 +221,7 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
221     return false;

MarcoFalke commented at 1:42 pm on January 22, 2021:

same commit:

Should probably remove the comment. “Multisig txns have more than one address…”

MarcoFalke approved

MarcoFalke commented at 1:55 pm on January 22, 2021: member

review ACK 3c2b24b8f5798976e4d9b6b873f423e955098b47 🛡

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 3c2b24b8f5798976e4d9b6b873f423e955098b47 🛡
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjqnQv7BJdIKCHx584yF7InMc3RLX3f+MwsObnyUTwb6SLgnrJ/1MR/4ge1ksuG
 8d9lmNZm3sGqZ75q7Go8AyfkVSXqngmyL3tI04wjjpcO7B+Xu1hE+Rc9O4reAabag
 9yjKsKkrtsT0UkeMYT2eB7pFXdwgfaPjUSDMjuGi9erpzhDTh0ej3PEdD/I/+Q0HH
10/FkYcMK4gecdTzPhMiktFp9eWfcBuEq9YEOrpNJK0gRgK9kQdyQb8ui7dYfUhpQw
11+t6WHaoYzJwIHxdoY7D29aL5RexblXASgdvD26U2U2FNgbOVTyZIzaHfWXn9l5OK
12neU7/7kyDNxAYm4E2RDq+BK/aX0SOC6RjSKR+0RtKcKMA0Tl+Uf9tNfJbW7gdRTd
13vHDrfqyBMLJvYY16SteV9p7h22eDeVZ8zogNc+zFTvq03OjKPTTb+4VRdzz/J4yf
147Zqjm03AR6VVumUJJTG7IB/dRTouxJB2cvC8Fr5r0cHeDyxX7lGEsjpSlVMqISuY
15oUegAZXN
16=yCxp
17-----END PGP SIGNATURE-----

Timestamp of file with hash ee289e0483353e4cddc99875c3df748a70e7aeff1a9baf49bc6acdaa7ba688c3 -

in doc/release-notes.md:81 in 3c2b24b8f5 outdated

75+  The `-deprecatedrpc=addresses` flag must be passed for these fields to be
76+  included in the RPC response. This flag/option will be available until v23, at which
77+  point the deprecation will be removed entirely. Note that these fields are attributes of
78+  the `scriptPubKey` object returned in the RPC response. However, in the response
79+  of `decodescript` these fields are top-level attributes, and included again as attributes
80+  of the `scriptPubKey` object. (#20286)

MarcoFalke commented at 2:34 pm on January 22, 2021:

Also, missing release notes for rest

in src/rpc/blockchain.cpp:1783 in 3c2b24b8f5 outdated

1778@@ -1778,6 +1779,11 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
1779     }
1780 }
1781 
1782+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex)
1783+{
1784+    ScriptPubKeyToUniv(scriptPubKey, out, fIncludeHex, IsDeprecatedRPCEnabled("addresses"));
1785+}

MarcoFalke commented at 2:40 pm on January 22, 2021:

Can you explain why you are using this wrapper approach for ScriptPubKeyToUniv but not for TxToUniv. Either use it for both or none.

mjdietzx commented at 3:53 pm on January 25, 2021: contributor

Thanks for the review @MarcoFalke I will get all your comments addressed in the next few days (I’m getting through a busy period w my company right now so slightly slow)

mjdietzx commented at 9:23 pm on January 31, 2021: contributor

@MarcoFalke your remaining comments are all addressed in this commit 2b74c73. I haven’t squashed yet because I’m not sure if that would make reviewers lives more difficult or not. Anyways I’ll squash it as appropriate once I get the “please squash” and address any remaining feedback

in src/test/fuzz/transaction.cpp:112 in 2b74c73051 outdated

107@@ -108,10 +108,10 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
108         }
109     }
110     if (!skip_tx_to_univ) {
111-        TxToUniv(tx, /* hashBlock */ {}, u, true);
112-        TxToUniv(tx, /* hashBlock */ {}, u, false);
113+        TxToUniv(tx, /* hashBlock */ {}, /* include_addresses */ true, u);
114+        TxToUniv(tx, /* hashBlock */ {}, false, u);

MarcoFalke commented at 7:56 am on February 1, 2021:

nit: could mention /* include_addresses */ here as well for symmetry

mjdietzx commented at 5:14 pm on February 1, 2021:

squashed, and I included this nit in the push

MarcoFalke commented at 7:58 am on February 1, 2021: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

I review from scratch or with range-diff, so squashing will simplify review for me. I think we generally prefer to have a squashed and ready version on every push.

mjdietzx force-pushed on Feb 1, 2021

DrahtBot added the label Needs rebase on Feb 11, 2021

MarcoFalke commented at 5:22 pm on February 11, 2021: member

re-ACK 732ad2d87bd15db2f42313e0f92b2857f0837f60 only changes are to address feedback 🤗

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 732ad2d87bd15db2f42313e0f92b2857f0837f60 only changes are to address feedback 🤗
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUin6gv/UeOu6b4VrhEqVZzrvybDcAxYkJ96F6+fuwbmf6b4WrQor81jOCcUvkqy
 8HfQKi45XS8zF+zfMlvZWc8CrDzluMtvpSxqV94aVwBojVB30jQZkEdxUFlc7Cups
 9KMtz4QBXqZxNtU4hWy342IYZdAJlndjWJHdOom0JNFFs7mKvrerh3WwGc1VKv0t0
10098eTDGZuM/nzmrcy8LriWaYD4knxpBi7VLRHPZHDUWxfM7LFlAtKGHWlkZ0Rwqk
1119ornEMPtVERHtFKouKSXHL4cx1qA9lPnUvY6RY653aqeKHtBMglBUoFM5tVOx5n
12+k345Z347dhi23hkiN3sYDHITCYA6W7nvRIvPxGxM8+Tndpx/4W5+JHLxOAQwOmH
13cLnPQhK1TAl5MQ5k9/8Tb61I1ReXd8UjND2VsY662qtbI8iuaroqnDblnA0xua98
14NofIkPZa6S3rQ5Oa1zZWXHEmQKItiqul7E6Fi2SEMiVHQGKSGWLoTO6HIOLWneZn
15lR+JWihV
16=uo81
17-----END PGP SIGNATURE-----

Timestamp of file with hash 5b7ea3b2ea13987cc0104b0f945e60e835ec583f2b93a088dc340f7e6587fe4b -

mjdietzx force-pushed on Feb 11, 2021

DrahtBot removed the label Needs rebase on Feb 11, 2021

DrahtBot added the label Needs rebase on Feb 16, 2021

sipa commented at 8:06 pm on February 19, 2021: member

Needs rebase.

mjdietzx force-pushed on Feb 21, 2021

DrahtBot removed the label Needs rebase on Feb 21, 2021

MarcoFalke commented at 9:38 am on February 22, 2021: member

re-ACK 0d83a7326385bf012c1a34d766b96af11fa88979 only change is rebase 🤸

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 0d83a7326385bf012c1a34d766b96af11fa88979 only change is rebase 🤸
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhYywwAjgK4ihIQ39KVE2lzOU0k1tOoysgSg2R0ecgHzTn7dVymB6zzYYv6EUxX
 8MSafRTRy9PoTxs7yIijFvI7KeYGMNVfjoz31UvB17ecxPAUetuHR8spLAmmVtdf/
 9po8kLlJ91r88oUK/of56Mt9jzrIo/UPnjuoznCeRaYgTDo4QQoB/t9l+ZJqA/JCK
10Z//4UJDu3cV3fVSa1bnoj+V0aw2GRGt33D7ZFo6+aav3ISbT2E+yMcarBR5Osb3A
11nQbXU0DhVb5xv9HOl3tNFQA2i1TllJ63uDSgB+J0ylA9muEmfQJ+6hclkjEzppop
12JS3gMNPC1OWU6tj4KCwK/F6tPdSr4sef217IwyQOQpdLU+zn6ojIIPdThPCPBJ0U
13pToy32H+zeX4lQJV/3H2st8ylGs4ba2vIHe0Heo6hdOqkUPmk0q/NOBYLK0gRljh
14H/y728bGj+aAw4GWh48x0B+3WgRylmjgw+Zk6Zsppg4P6V3nK79lIkgKSKESnzHx
15gX/dFts/
16=l+z1
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1e142a9aeeb7dbbc160362a7c3f047702eee9212dda1795b35be97cc096c15f8 -

sanket1729 approved

sanket1729 commented at 7:42 am on March 2, 2021: contributor

Code review ACK 0d83a7326385bf012c1a34d766b96af11fa88979 . The address feild is indeed a much better RPC output.

MarcoFalke commented at 9:50 am on March 2, 2021: member

@sipa Mind to re-ACK? Should be close to merge, hopefully.

DrahtBot added the label Needs rebase on Mar 3, 2021

mjdietzx force-pushed on Mar 4, 2021

DrahtBot removed the label Needs rebase on Mar 4, 2021

DrahtBot added the label Needs rebase on Mar 15, 2021

rpc: deprecate `addresses` and `reqSigs` from rpc outputs

1) add a new sane "address" field (for outputs that have an
   identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
   (with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
   always.

085b3a7299

doc: Add release notes for -deprecatedrpc=addresses and bitcoin-tx 90ae3d8ca6

mjdietzx force-pushed on Mar 23, 2021

MarcoFalke commented at 3:17 pm on March 23, 2021: member

re-ACK 90ae3d8ca68334ec712d67b21a8d4721c6d79788 📢

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 90ae3d8ca68334ec712d67b21a8d4721c6d79788 📢
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg6IAv+Ol7ckNRTmCxL9Ko6DB0jxK62hxEeAODWHiXIrB9y/1wT7WScw8VIBSRn
 83p9VIwDE/+WXoU1OUXJcUfszI/vGoWkZO/uO91fQ3ZmN683+55zkh+5t6qIuSXWU
 99aqb4EZEhiJtwa1NZOBohZCSdU1c/ogwUN0jn2WNhZwoGR7fccpay5L8K3cLpu3Y
1083pCYZ29WrxGeKgq1iV+mW018oh6+/7OvgBEujnM+4qerRDHps/J8fyNBYGuIY8a
11iK32n//OmKxP57lY1kbHlUjL5KXW/EWtQiu7srqJsOoi0+ypmCGvDepF3gpbnOVV
12OKchPIDkOPBggEQzAF6AeLPUtr+/K6+sZkpImlmo/xvF0KRuwHrUh8f18N/2vZws
13zyC5pkSLP+ChVXfW7HnjjmRC3ax5y3bR42XJm5wIXYk4+QLmXMQPqMe+RzxZVr+4
14SNaQ8vWQZSSbl+X01TGgyTatrF7dwYtgpLRSImf6C5SXZXB16VLCLoqx+zr3EI3/
15gMbBV9Eo
16=TB20
17-----END PGP SIGNATURE-----

Timestamp of file with hash a41966e5952e2cb0d54de33ddc00b633b1c94ca2c9647f9886491c77a18134ee -

DrahtBot removed the label Needs rebase on Mar 23, 2021

MarcoFalke merged this on Mar 29, 2021

MarcoFalke closed this on Mar 29, 2021

sidhujag referenced this in commit 8585c3fbe2 on Mar 29, 2021

kristapsk referenced this in commit 95da4e51a3 on May 25, 2021

domob1812 referenced this in commit 1207c00a47 on Aug 9, 2021

kristapsk referenced this in commit c96df1c58f on Sep 8, 2021

meshcollider referenced this in commit d6492d4ed0 on Sep 28, 2021

bitcoinfacts referenced this in commit 0fdb8a4a07 on Dec 31, 2021

bitcoinfacts referenced this in commit d58992f678 on Dec 31, 2021

DrahtBot locked this on Aug 16, 2022

rpc: deprecate addresses and reqSigs from rpc outputs #20286

Conflicts

rpc: deprecate `addresses` and `reqSigs` from rpc outputs #20286