Skip to content
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

rpc: deprecate addresses and reqSigs from rpc outputs #20286

Merged
merged 2 commits into from Mar 29, 2021

Conversation

mjdietzx
Copy link
Contributor

@mjdietzx mjdietzx commented Nov 2, 2020

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.

  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.

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.

@maflcko
Copy link
Member

maflcko commented Nov 3, 2020

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

@luke-jr
Copy link
Member

luke-jr commented Nov 13, 2020

"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
Copy link
Contributor Author

"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
Copy link
Member

sipa commented Nov 15, 2020

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
Copy link
Member

luke-jr commented Nov 15, 2020

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).

@mjdietzx
Copy link
Contributor Author

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

  1. f5d3cf6 "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.
  2. c0545cc deprecates reqSigs as discussed, where reqSigs is totally removed from rpc outputs, and addresses is no longer returned for bare multsigs. c0545cc#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
Copy link
Member

sipa commented Nov 18, 2020

@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
Copy link
Contributor Author

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
Copy link
Member

luke-jr commented Nov 19, 2020

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 19, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor Author

I'll need to re-work some of the functional tests (will get this committed soon) now that addresses is removed by default, but 162e7f0 updates this PR to be as @sipa suggested

@mjdietzx mjdietzx changed the title rpc: remove reqSigs from rpc outputs rpc: deprecate addresses and reqSigs from rpc outputs Nov 23, 2020
@mjdietzx mjdietzx force-pushed the remove-reqsigs-from-rpcs branch 2 times, most recently from be593ad to fc484eb Compare November 23, 2020 18:05
@mjdietzx
Copy link
Contributor Author

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

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

Could we do something like:

TxoutType type;
CTxDestination dest;
ExtractDestination(m_script, type, dest);
switch (type) {
case TxoutType::PUBKEYHASH:
case TxoutType::SCRIPTHASH: return OutputType::LEGACY;
case TxoutType::WITNESS_V0_SCRIPTHASH:
case TxoutType::WITNESS_V0_KEYHASH:
case TxoutType::WITNESS_V1_TAPROOT:
case TxoutType::WITNESS_UNKNOWN: return OutputType::BECH32;
case TxoutType::PUBKEY:
case TxoutType::MULTISIG:
case TxoutType::NONSTANDARD:
case TxoutType::NULL_DATA: return nullopt; // no default case, so the compiler can warn about missing cases
}
assert(false);

Instead of what we have now:

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

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?

@maflcko
Copy link
Member

maflcko commented Feb 22, 2021

re-ACK 0d83a73 only change is rebase 🤸

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 0d83a7326385bf012c1a34d766b96af11fa88979 only change is rebase 🤸
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhYywwAjgK4ihIQ39KVE2lzOU0k1tOoysgSg2R0ecgHzTn7dVymB6zzYYv6EUxX
MSafRTRy9PoTxs7yIijFvI7KeYGMNVfjoz31UvB17ecxPAUetuHR8spLAmmVtdf/
po8kLlJ91r88oUK/of56Mt9jzrIo/UPnjuoznCeRaYgTDo4QQoB/t9l+ZJqA/JCK
Z//4UJDu3cV3fVSa1bnoj+V0aw2GRGt33D7ZFo6+aav3ISbT2E+yMcarBR5Osb3A
nQbXU0DhVb5xv9HOl3tNFQA2i1TllJ63uDSgB+J0ylA9muEmfQJ+6hclkjEzppop
JS3gMNPC1OWU6tj4KCwK/F6tPdSr4sef217IwyQOQpdLU+zn6ojIIPdThPCPBJ0U
pToy32H+zeX4lQJV/3H2st8ylGs4ba2vIHe0Heo6hdOqkUPmk0q/NOBYLK0gRljh
H/y728bGj+aAw4GWh48x0B+3WgRylmjgw+Zk6Zsppg4P6V3nK79lIkgKSKESnzHx
gX/dFts/
=l+z1
-----END PGP SIGNATURE-----

Timestamp of file with hash 1e142a9aeeb7dbbc160362a7c3f047702eee9212dda1795b35be97cc096c15f8 -

Copy link
Contributor

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member

maflcko commented Mar 2, 2021

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

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.
@maflcko
Copy link
Member

maflcko commented Mar 23, 2021

re-ACK 90ae3d8 📢

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 90ae3d8ca68334ec712d67b21a8d4721c6d79788 📢
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg6IAv+Ol7ckNRTmCxL9Ko6DB0jxK62hxEeAODWHiXIrB9y/1wT7WScw8VIBSRn
3p9VIwDE/+WXoU1OUXJcUfszI/vGoWkZO/uO91fQ3ZmN683+55zkh+5t6qIuSXWU
9aqb4EZEhiJtwa1NZOBohZCSdU1c/ogwUN0jn2WNhZwoGR7fccpay5L8K3cLpu3Y
83pCYZ29WrxGeKgq1iV+mW018oh6+/7OvgBEujnM+4qerRDHps/J8fyNBYGuIY8a
iK32n//OmKxP57lY1kbHlUjL5KXW/EWtQiu7srqJsOoi0+ypmCGvDepF3gpbnOVV
OKchPIDkOPBggEQzAF6AeLPUtr+/K6+sZkpImlmo/xvF0KRuwHrUh8f18N/2vZws
zyC5pkSLP+ChVXfW7HnjjmRC3ax5y3bR42XJm5wIXYk4+QLmXMQPqMe+RzxZVr+4
SNaQ8vWQZSSbl+X01TGgyTatrF7dwYtgpLRSImf6C5SXZXB16VLCLoqx+zr3EI3/
gMbBV9Eo
=TB20
-----END PGP SIGNATURE-----

Timestamp of file with hash a41966e5952e2cb0d54de33ddc00b633b1c94ca2c9647f9886491c77a18134ee -

@maflcko maflcko merged commit 1c7be9a into bitcoin:master Mar 29, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 29, 2021
kristapsk added a commit to kristapsk/bitcoin-scripts that referenced this pull request May 25, 2021
domob1812 added a commit to xaya/democrit that referenced this pull request Aug 9, 2021
Bitcoin 22.x changed how the scriptPubKey JSON reports the address, see
bitcoin/bitcoin#20286.  Update the checker code
to support the new format.
kristapsk added a commit to kristapsk/bitcoin-scripts that referenced this pull request Sep 8, 2021
meshcollider added a commit that referenced this pull request Sep 28, 2021
…code/logic

43cd6b8 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2 refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638 rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)

Pull request description:

  Resolves #21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.

   `-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).

  Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)

ACKs for top commit:
  MarcoFalke:
    re-ACK 43cd6b8 🐉
  meshcollider:
    Code review ACK 43cd6b8
  jonatack:
    ACK 43cd6b8 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit

Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
bitcoinfacts added a commit to bitcoinfacts/GlacierProtocol that referenced this pull request Dec 31, 2021
bitcoinfacts added a commit to bitcoinfacts/GlacierProtocol that referenced this pull request Dec 31, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove reqSigs from output of decoderawtransaction and other RPCs
7 participants