Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx · Pull Request #22650 · bitcoin/bitcoin

mjdietzx · 2021-08-06T14:29:53Z

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)

DrahtBot · 2021-08-07T05:09:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23091 (fuzz: fix checks on number of required sigs and keys in multisig scripts by mjdietzx)
#22951 (consensus: move amount.h into consensus by fanquake)
#22918 (rpc: Add level 3 verbosity to getblock RPC call (rpc: Add level 3 verbosity to getblock RPC call. #21245 modified) by kiminuo)
#20295 (rpc: getblockfrompeer by Sjors)
#16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)

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.

fanquake · 2021-08-12T02:24:38Z

Concept ACK

theStack · 2021-08-13T08:35:59Z

Concept ACK

maflcko · 2021-08-25T08:51:39Z

review ACK 28bd30a73bd4bff1dc44c89ae64a194cc1c526e4 📰

Show signature and timestamp

Signature:

Timestamp of file with hash f308d8d97552a7b726d0344e7ac0ace93281130a51a3154b19a1b6b4e0c1e1d2 -

maflcko · 2021-08-25T08:55:18Z

Oh, in the release notes it should say v22 and bitcoin-tx had no behaviour change at all, so no release notes needed?!

mjdietzx · 2021-08-25T13:07:50Z

Thanks @MarcoFalke, release notes have been corrected

maflcko · 2021-08-25T14:32:13Z

re-ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2 🚅

Show signature and timestamp

Signature:

Timestamp of file with hash e9ce8225e5bc9dceb1997a8dd371a81415ce8afa17aee27a36cf95c220ae6d1f -

jonatack

ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2

Happy to re-ACK if you update.

src/core_write.cpp

src/test/fuzz/script.cpp

src/wallet/rpcwallet.cpp

src/core_io.h

doc/release-notes.md

mjdietzx · 2021-08-25T20:06:11Z

Thanks for the review @jonatack - I made all your suggestions. I had to edit some commits and force push. I'm hoping this didn't throw a wrench in your review @MarcoFalke . If it did and you know a better way I could've done it please lmk so I can avoid this in the future 😬

jonatack

Great cleanup. Rebased to current master and debug-built each commit. It may be good to drop 7f7a2d9 "Inline ScriptToUniv", i.e. merge the two commits, since ScriptToUniv() is un-inlined right after again in b456004. ACK b456004de9ef61716140c9020c572416b46acbff otherwise.

src/rpc/rawtransaction.cpp

mjdietzx · 2021-09-03T02:30:15Z

Thanks for the re review @jonatack !

It may be good to drop 7f7a2d9 "Inline ScriptToUniv", i.e. merge the two commits, since ScriptToUniv() is un-inlined right after again in b456004

I squashed these two commits together. And then addressed both of your comments

jonatack · 2021-09-03T13:12:04Z

Code review re-ACK 92dc5e954fdb974160198f0c97eff3e7e51c1372 only changes are documentation and squashing two commits to one, checked git diff b456004 92dc5e9, re-read the per-commit changes, and rebased to current master/debug build/checked unit+functional tests pass

src/test/fuzz/transaction.cpp

src/wallet/rpcwallet.cpp

src/rpc/rawtransaction.cpp

mjdietzx · 2021-09-14T14:21:58Z

Rebased.

I think you can drop one of these statements to make the fuzz test run ~twice as fast.

Done c45b133

maflcko

review ACK f1acb5434f 🐯

Show signature and timestamp

Signature:

Timestamp of file with hash 1eb7fc34e86b60e73cdd3193dc2eeb609ebf87c032e3483f7067eb23fe71b73a -

src/bitcoin-tx.cpp

src/test/fuzz/transaction.cpp

mjdietzx · 2021-09-14T20:25:07Z

I got rid of two commits I thought were slightly unnecessary and would be better off in a followup refactor PR:
c45b13389a3540a07c0f64f63cb36f538f8e2f1c fuzz: remove an unuseful TxToUniv invocation to speedup fuzz/transaction.cpp
a0f2dab816fc9987fb177c02f5d801cf0b8ebbf9 refactor: named args whenever ScriptToUniv, ScriptPubKeyToUniv, TxToUniv are invoked

I will be making the changes @MarcoFalke suggested in the followup PR as the only remaining requested changes from this review were related to those last commits.

maflcko

my previous review is now valid

src/test/fuzz/script.cpp

mjdietzx · 2021-09-24T19:23:54Z

Rebased

maflcko · 2021-09-25T07:38:29Z

re-ACK 43cd6b8 🐉

Show signature and timestamp

Signature:

Timestamp of file with hash 1ef8757c6c687d7a663bef23283581d4e088670176d49a29213e78ffea20beec -

meshcollider

Code review ACK 43cd6b8

Commit refactor: minor styling, prefer snake case and same line if should be squashed with the previous refactoring commit.

meshcollider · 2021-09-28T01:07:34Z

src/test/fuzz/script.cpp

-    ScriptToUniv(script, o3, true);
-    UniValue o4(UniValue::VOBJ);
-    ScriptToUniv(script, o4, false);
+    ScriptToUniv(script, o3);


This should really be replaced with include_address=true/false checks on ScriptPubKeyToUniv(script, o4, fIncludeHex, include_address)

Good point, this isn't really needed. But because the followup PR #22924 merges ScriptPubKeyToUniv and ScriptToUniv into a single function it will be getting rid of this anyways. So this will be removed in the followup

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

src/core_write.cpp

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from b798f8b to 28bd30a Compare August 6, 2021 15:22

DrahtBot added RPC/REST/ZMQ Utils/log/libs Wallet labels Aug 6, 2021

DrahtBot mentioned this pull request Aug 7, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

fanquake added this to the 23.0 milestone Aug 7, 2021

This was referenced Aug 7, 2021

rpc: add path to gethdkey #22341

Closed

rpc: Add level 3 verbosity to getblock RPC call. #21245

Closed

rpc: getblockfrompeer #20295

Merged

assumeutxo #15606

Closed

DrahtBot mentioned this pull request Aug 12, 2021

rpc: deprecate top-level fee fields in getmempool RPCs #22689

Merged

This was referenced Aug 20, 2021

policy/rbf: don't return "incorrect" replaceability status #22665

Closed

scripted-diff: Use generate* from TestFramework #22788

Merged

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from 28bd30a to 3add69b Compare August 25, 2021 13:06

jonatack reviewed Aug 25, 2021

View reviewed changes

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from 3add69b to b456004 Compare August 25, 2021 20:00

This was referenced Aug 25, 2021

doc: Fix RPC result documentation #22798

Merged

Implement BIP 370 PSBTv2 #21283

Open

jonatack reviewed Sep 2, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from b456004 to 92dc5e9 Compare September 3, 2021 02:28

DrahtBot mentioned this pull request Sep 8, 2021

rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) #22918

Merged

mjdietzx mentioned this pull request Sep 8, 2021

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

Closed

DrahtBot added the Needs rebase label Sep 9, 2021

maflcko reviewed Sep 9, 2021

View reviewed changes

src/test/fuzz/transaction.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

maflcko reviewed Sep 9, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from ba551e1 to c45b133 Compare September 14, 2021 14:18

maflcko reviewed Sep 14, 2021

View reviewed changes

src/bitcoin-tx.cpp Outdated Show resolved Hide resolved

src/test/fuzz/transaction.cpp Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Sep 14, 2021

DrahtBot mentioned this pull request Sep 14, 2021

consensus: move amount.h into consensus #22951

Merged

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from c45b133 to f1acb54 Compare September 14, 2021 20:20

maflcko approved these changes Sep 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 15, 2021

scripted-diff: update license URLs to https #22975

Closed

DrahtBot added the Needs rebase label Sep 23, 2021

maflcko reviewed Sep 24, 2021

View reviewed changes

src/test/fuzz/script.cpp Outdated Show resolved Hide resolved

mjdietzx added 4 commits September 24, 2021 14:22

rpc: remove deprecated addresses and reqSigs from rpc outputs

8721638

refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv

d64deac

refactor: minor styling, prefer snake case and same line if

2b1fdc2

doc: add release notes for removal of the -deprecatedrpc=addresses flag

43cd6b8

mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from f1acb54 to 43cd6b8 Compare September 24, 2021 19:23

DrahtBot removed the Needs rebase label Sep 24, 2021

mjdietzx requested a review from jonatack September 24, 2021 21:29

mjdietzx mentioned this pull request Sep 25, 2021

fuzz: fix checks on number of required sigs and keys in multisig scripts #23091

Closed

meshcollider reviewed Sep 28, 2021

View reviewed changes

jonatack reviewed Sep 28, 2021

View reviewed changes

src/core_write.cpp Show resolved Hide resolved

meshcollider merged commit d6492d4 into bitcoin:master Sep 28, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Conversation

mjdietzx commented Aug 6, 2021

Uh oh!

DrahtBot commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Aug 12, 2021

Uh oh!

theStack commented Aug 13, 2021

Uh oh!

maflcko commented Aug 25, 2021

Uh oh!

maflcko commented Aug 25, 2021

Uh oh!

mjdietzx commented Aug 25, 2021

Uh oh!

maflcko commented Aug 25, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Aug 25, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Sep 3, 2021

Uh oh!

jonatack commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Sep 14, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Sep 14, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjdietzx commented Sep 24, 2021

Uh oh!

maflcko commented Sep 25, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

DrahtBot commented Aug 7, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading