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.

Sign up for GitHub

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

Jump to bottom

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

Merged
merged 4 commits into from
Sep 28, 2021
Merged

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

merged 4 commits into from
Sep 28, 2021

Conversation

mjdietzx
Copy link
Contributor

@mjdietzx mjdietzx commented Aug 6, 2021

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)

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

DrahtBot commented Aug 7, 2021 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Aug 7, 2021
Closed
@fanquake fanquake added this to the 23.0 milestone Aug 7, 2021
This was referenced Aug 7, 2021
Closed
Closed
Merged
Closed
@fanquake
Copy link
Member

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Aug 12, 2021
Merged
@theStack
Copy link
Contributor

Concept ACK

This was referenced Aug 20, 2021
Closed
Merged
@maflcko
Copy link
Member

maflcko commented Aug 25, 2021

review ACK 28bd30a 📰

Show signature and timestamp

Signature:

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

review ACK 28bd30a73bd4bff1dc44c89ae64a194cc1c526e4 📰
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj6LAv/esiGjhUnjDPkpTWtjOaUHG6aiNtEYHI/5WSG95syCi67C01fd7pgMh2X
irGcd4oneNMs4mTcdudldqW+0gbIBEQNKodntgvon8cW3YudCJDUlA/1D1HAII29
WxLdujkyzQcv+zb8UtleI6ir0WSKKLuBjW1KmJ+B+mU2jAy4b1cZv2+jnfQTWIZI
8EV85EpeW4JL2EfwkBOpRoGAA6acpruy7SFVE+U1Ae1QDTHLYSytO6iALucFQ43K
D2rubN4Z6Tk/x37WAkKodytHIJkw/QSi07vYY0cvEVC1yuGM2RBM/DHepKBZ1PhR
zkjU4+ng6sG5aHI9y+gsujSUHIDptyWlERprXHT31MJ1w6Cj5oY7Xm+ZcV5BI1P4
qCsr4dkARENaXrwZtiScO3jsY7Frhjj4Ma03JOCus52R+Dpy3z/qFuVp5GNHxSjq
uRfXm0rWoA4n5i0pLzhJy3j3FeFEJJlbGOJWjZg2xFP/NtCVf5M/4+SxhTsq0Fj0
buwwVfj0
=y2oM
-----END PGP SIGNATURE-----

Timestamp of file with hash f308d8d97552a7b726d0344e7ac0ace93281130a51a3154b19a1b6b4e0c1e1d2 -

@maflcko
Copy link
Member

maflcko commented Aug 25, 2021

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

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

Thanks @MarcoFalke, release notes have been corrected

@maflcko
Copy link
Member

maflcko commented Aug 25, 2021

re-ACK 3add69b 🚅

Show signature and timestamp

Signature:

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

re-ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2 🚅
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh0XAv9F8CNbuDGfeUPadszE9aTIjsAhaztFE3LJmoJrRq9x9LBb0UUygTQBk6D
HT4Lf69d2aD6Gf8yvhyhZzyXLGKmV9c+Z2WN40ydiYXc2rhZHLYgWRnBGoPCiLq5
4a+0K35aCdVZGcSE90W5YkZR58P3K5AfRdy/VNq4kAK3pRXM8DPyopfopfRWd6sy
CUhqyrc3Uyq7SMLhZahCBJN9eNK38Z+H5UDaihPD/o0L2lMburvtpcNwfw9yq5WD
KhQkaiXHGERk1/O55V4StNn1ZYxGiW01OgPSP1xTpd/O2VjKhnb7la1l5AF6MgY3
RWow9P2f1hHv7KdqZjyWmsTwaCE53XCZ+kkHUqLkpNeIS90BWnATXf5kGmPTUWNp
B5MZfHuaMhLYgVRi1qsQCpjBFu6h1VdDQZI1jg9aR39h1l/BMxc3sjQk4RiDz8e3
yamCGcDRlCbncI69m0rNppX2E4HifB0d+w4OkEa9dugiYd1g6AIObs7ExlWyYuxz
XjG3uMuH
=vDaO
-----END PGP SIGNATURE-----

Timestamp of file with hash e9ce8225e5bc9dceb1997a8dd371a81415ce8afa17aee27a36cf95c220ae6d1f -

jonatack
jonatack reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 3add69b

Happy to re-ACK if you update.

src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Show resolved Hide resolved
src/test/fuzz/script.cpp Show resolved Hide resolved
src/wallet/rpcwallet.cpp Show resolved Hide resolved
src/core_io.h Outdated Show resolved Hide resolved
doc/release-notes.md Outdated Show resolved Hide resolved
@mjdietzx mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from 3add69b to b456004 Compare August 25, 2021 20:00
@mjdietzx
Copy link
Contributor Author

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 😬

This was referenced Aug 25, 2021
Merged
Open
jonatack
jonatack reviewed Sep 2, 2021
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

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 b456004 otherwise.

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
@mjdietzx mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from b456004 to 92dc5e9 Compare September 3, 2021 02:28
@mjdietzx
Copy link
Contributor Author

mjdietzx commented Sep 3, 2021

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

jonatack commented Sep 3, 2021

Code review re-ACK 92dc5e9 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

@DrahtBot DrahtBot mentioned this pull request Sep 8, 2021
Merged
@mjdietzx mjdietzx mentioned this pull request Sep 8, 2021
Closed
maflcko
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
@mjdietzx mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from ba551e1 to c45b133 Compare September 14, 2021 14:18
@mjdietzx
Copy link
Contributor Author

Rebased.

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

Done c45b133

maflcko
maflcko reviewed Sep 14, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK f1acb54 🐯

Show signature and timestamp

Signature:

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

review ACK f1acb5434f 🐯
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgi5Qv/ZHz7rCf4k/qbLPVJkl28A1bwknL9PtYwyfKkTq4BUREjjGmQoQQbbUjs
jkHWzJxVKVveR9i9D+v/oEuZLEh64BdRAiirB6UG40fuaEKI3eYiV9IvkrowjzyO
R91NYDJzGSCzuqDbAg9OBnQShxtZKKQ/DENoxMayCOQXbDfwO5CFGy8gTB17s+hp
DXJ2Rq9R2J7rYshpfRPaLbJf2ZxzZlJa9A/lqv+ls9FdCuet3jhjOs/Aoknd/f/I
3cZzK8skniakpyEq0TjCIwdwfaaJXZdMJyPIiRdGXIjVP3BMradUjdPpwGMdvZpF
AnfCdkmxksCGy1EJbH3Gipbo/PF3Cp1oQ1uwE4+oq16wxOUoLGSeFqtAwg5mCSKt
itvsYyQhP2Erm/FNwas/la+nFusyDlkDSOgMHpg0XFzdEoqslAM3g2IxWFg4FKPs
qqvI1Jl44hfRkBErCnn7edYmTpA5uGOvYxwAJSns0WUQsh7h4MMTexM7+xOwbCOh
NXsg0pdM
=FPMp
-----END PGP SIGNATURE-----

Timestamp of file with hash 1eb7fc34e86b60e73cdd3193dc2eeb609ebf87c032e3483f7067eb23fe71b73a -

src/bitcoin-tx.cpp Outdated Show resolved Hide resolved
src/test/fuzz/transaction.cpp Outdated Show resolved Hide resolved
@DrahtBot DrahtBot mentioned this pull request Sep 14, 2021
Merged
@mjdietzx mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from c45b133 to f1acb54 Compare September 14, 2021 20:20
@mjdietzx
Copy link
Contributor Author

I got rid of two commits I thought were slightly unnecessary and would be better off in a followup refactor PR:
c45b133 fuzz: remove an unuseful TxToUniv invocation to speedup fuzz/transaction.cpp
a0f2dab 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
maflcko approved these changes Sep 15, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

my previous review is now valid

@DrahtBot DrahtBot mentioned this pull request Sep 15, 2021
Closed
@mjdietzx mjdietzx force-pushed the finalize-remove-reqsigs-from-rpcs branch from f1acb54 to 43cd6b8 Compare September 24, 2021 19:23
@mjdietzx
Copy link
Contributor Author

Rebased

@mjdietzx mjdietzx mentioned this pull request Sep 25, 2021
Closed
@maflcko
Copy link
Member

maflcko commented Sep 25, 2021

re-ACK 43cd6b8 🐉

Show signature and timestamp

Signature:

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

re-ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec 🐉
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgkNQv/TUjva/T/Sp2WEolHXi+auEURUOvDAqnHI2HYJxTX603YbaeWPkvATkzl
3L12B+9CH60cpegoJVK0jesF7J1G2TryHs2L9LAb4X0GGnvD6Tr7+hx8BrJenZDT
51laMGIWvZxPGRnohFuVP/tp3/a0eBhtee6DGBFpJWOjT9p6cUJxF02eFtDwDYTN
nxOTxzliV5TnQgQyPC0r4MShGRZbCZwqBbv1nwK+iB0q8qHMzAP5mS+B7B+DkhV8
SpGrH2OIjk2RAmKP2nuD42THzz4alMUJPY6S2vuU9MPVaLapNhK0CB3vaJJewODN
6F6uqzMUQ2JWstjAtUUeNOp7pXndFw+8jk0UlocSxtfLhpfu2bLtTxyBFWog9MaF
Czo9RxXhmFePu5t7lRCF/esSxrnXonsjXMaxZ3+ZdAm0nPGYecOjHvrg1p8dGLDv
QlvAK7A2sNZsiNI8u7ggY3Abu3BGL/7Qf5AM0fOGXJQFVDjeXwqhj2kyLFAlcJUC
l4UmCNZz
=AVpG
-----END PGP SIGNATURE-----

Timestamp of file with hash 1ef8757c6c687d7a663bef23283581d4e088670176d49a29213e78ffea20beec -

meshcollider
meshcollider reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider 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 43cd6b8

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

src/test/fuzz/script.cpp
ScriptToUniv(script, o3, true);
UniValue o4(UniValue::VOBJ);
ScriptToUniv(script, o4, false);
ScriptToUniv(script, o3);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
jonatack reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@meshcollider meshcollider merged commit d6492d4 into bitcoin:master Sep 28, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

@meshcollider meshcollider meshcollider left review comments

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
RPC/REST/ZMQ Utils/log/libs Wallet
Projects
None yet
Milestone
23.0
Development

Successfully merging this pull request may close these issues.

Remove -deprecatedrpc=addresses flag and corresponding code/logic
7 participants
@mjdietzx @DrahtBot @fanquake @theStack @maflcko @jonatack @meshcollider