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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
review ACK 28bd30a73bd4bff1dc44c89ae64a194cc1c526e4 📰
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 28bd30a73bd4bff1dc44c89ae64a194cc1c526e4 📰
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUj6LAv/esiGjhUnjDPkpTWtjOaUHG6aiNtEYHI/5WSG95syCi67C01fd7pgMh2X
8irGcd4oneNMs4mTcdudldqW+0gbIBEQNKodntgvon8cW3YudCJDUlA/1D1HAII29
9WxLdujkyzQcv+zb8UtleI6ir0WSKKLuBjW1KmJ+B+mU2jAy4b1cZv2+jnfQTWIZI
108EV85EpeW4JL2EfwkBOpRoGAA6acpruy7SFVE+U1Ae1QDTHLYSytO6iALucFQ43K
11D2rubN4Z6Tk/x37WAkKodytHIJkw/QSi07vYY0cvEVC1yuGM2RBM/DHepKBZ1PhR
12zkjU4+ng6sG5aHI9y+gsujSUHIDptyWlERprXHT31MJ1w6Cj5oY7Xm+ZcV5BI1P4
13qCsr4dkARENaXrwZtiScO3jsY7Frhjj4Ma03JOCus52R+Dpy3z/qFuVp5GNHxSjq
14uRfXm0rWoA4n5i0pLzhJy3j3FeFEJJlbGOJWjZg2xFP/NtCVf5M/4+SxhTsq0Fj0
15buwwVfj0
16=y2oM
17-----END PGP SIGNATURE-----
Timestamp of file with hash f308d8d97552a7b726d0344e7ac0ace93281130a51a3154b19a1b6b4e0c1e1d2 -
175- if (!ExtractDestinations(scriptPubKey, type, addresses, nRequired) || type == TxoutType::PUBKEY) {
176- out.pushKV("type", GetTxnOutputType(type));
177- return;
178- }
179+ std::vector<std::vector<unsigned char>> solns;
180+ TxoutType type = Solver(scriptPubKey, solns);
b0770cee942e17f6ed3d4
0 const TxoutType type{Solver(scriptPubKey, solns)};
215@@ -249,7 +216,7 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, bool include_add
216 out.pushKV("n", (int64_t)i);
217
218 UniValue o(UniValue::VOBJ);
219- ScriptPubKeyToUniv(txout.scriptPubKey, o, true, include_addresses);
220+ ScriptPubKeyToUniv(txout.scriptPubKey, o, true);
Named args could be nice wherever this function is invoked
0 ScriptPubKeyToUniv(txout.scriptPubKey, /* out */ o, /* include_hex */ true);
108@@ -133,15 +109,11 @@ FUZZ_TARGET_INIT(script, initialize_script)
109 (void)ScriptToAsmStr(script, true);
110
111 UniValue o1(UniValue::VOBJ);
112- ScriptPubKeyToUniv(script, o1, true, true);
113- ScriptPubKeyToUniv(script, o1, true, false);
114+ ScriptPubKeyToUniv(script, o1, true);
b0770cee942e17f6ed3d4ded wherever this function is called
0 ScriptPubKeyToUniv(script, /* out */ o1, /* include_hex */ true);
1753@@ -1754,7 +1754,7 @@ static RPCHelpMan gettransaction()
1754
1755 if (verbose) {
1756 UniValue decoded(UniValue::VOBJ);
1757- TxToUniv(*wtx.tx, uint256(), pwallet->chain().rpcEnableDeprecated("addresses"), decoded, false);
1758+ TxToUniv(*wtx.tx, uint256(), decoded, false);
b0770cee942e17f6ed3d4ded
0 TxToUniv(*wtx.tx, /* hashBlock */ uint256(), /* entry */ decoded, /* include_hex */ false);
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, bool include_addresses);
48-void ScriptToUniv(const CScript& script, UniValue& out, bool include_address);
49-void TxToUniv(const CTransaction& tx, const uint256& hashBlock, bool include_addresses, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);
50+void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
51+inline void ScriptToUniv(const CScript& script, UniValue& out) { ScriptPubKeyToUniv(script, out, /* include_hex */ true, /* include_address */ false); }
ScriptPubKeyToUniv and ScriptToUniv into a single function?
I thought about it (and agree it’s a good change). However I didn’t include it originally because I wanted to be conservative w/ the changes in this PR and thought keeping them as separate functions may have been “self documenting”
Anyways, I wanted to do this and since you were thinking the same thing I did it in this commit b456004de9ef61716140c9020c572416b46acbff - if we decide we don’t want it I can just revert this commit
re-ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2 🚅
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2 🚅
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh0XAv9F8CNbuDGfeUPadszE9aTIjsAhaztFE3LJmoJrRq9x9LBb0UUygTQBk6D
8HT4Lf69d2aD6Gf8yvhyhZzyXLGKmV9c+Z2WN40ydiYXc2rhZHLYgWRnBGoPCiLq5
94a+0K35aCdVZGcSE90W5YkZR58P3K5AfRdy/VNq4kAK3pRXM8DPyopfopfRWd6sy
10CUhqyrc3Uyq7SMLhZahCBJN9eNK38Z+H5UDaihPD/o0L2lMburvtpcNwfw9yq5WD
11KhQkaiXHGERk1/O55V4StNn1ZYxGiW01OgPSP1xTpd/O2VjKhnb7la1l5AF6MgY3
12RWow9P2f1hHv7KdqZjyWmsTwaCE53XCZ+kkHUqLkpNeIS90BWnATXf5kGmPTUWNp
13B5MZfHuaMhLYgVRi1qsQCpjBFu6h1VdDQZI1jg9aR39h1l/BMxc3sjQk4RiDz8e3
14yamCGcDRlCbncI69m0rNppX2E4HifB0d+w4OkEa9dugiYd1g6AIObs7ExlWyYuxz
15XjG3uMuH
16=vDaO
17-----END PGP SIGNATURE-----
Timestamp of file with hash e9ce8225e5bc9dceb1997a8dd371a81415ce8afa17aee27a36cf95c220ae6d1f -
63@@ -64,6 +64,12 @@ P2P and network changes
64 Updated RPCs
65 ------------
66
67+- The `-deprecatedrpc=addresses` flag has been removed entirely.
68+ The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`,
69+ `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`,
70+ `/rest/block` no longer return the fields: `addresses`, `reqSigs` which were
71+ previously deprecated in v22. (#22650)
IIUC gettransaction is only affected when verbose=true and some writing suggestions
0-- The `-deprecatedrpc=addresses` flag has been removed entirely.
1- The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`,
2- `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`,
3- `/rest/block` no longer return the fields: `addresses`, `reqSigs` which were
4- previously deprecated in v22. (#22650)
5+- The `-deprecatedrpc=addresses` configuration option has been removed. RPCs
6+ `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`,
7+ `gettransaction verbose=true` and REST endpoints `/rest/tx`, `/rest/getutxos`,
8+ `/rest/block` no longer return the `addresses` and `reqSigs` fields, which
9+ were previously deprecated in 22.0. (#22650)
ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2
Happy to re-ACK if you update.
1035@@ -1056,7 +1036,7 @@ static RPCHelpMan decodepsbt()
1036 {RPCResult::Type::STR, "asm", "The asm"},
1037 {RPCResult::Type::STR_HEX, "hex", "The hex"},
1038 {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
1039- {RPCResult::Type::STR, "address"," Bitcoin address if there is one"},
1040+ {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (only if a well-defined address exists)"},
b0770cee ignore unless you retouch
0 {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
(“Bitcoin” seems to be a proper noun here, look at the translations returned by git grep -ni "bitcoin address")
1168@@ -1189,7 +1169,7 @@ static RPCHelpMan decodepsbt()
1169 txout = input.non_witness_utxo->vout[psbtx.tx->vin[i].prevout.n];
1170
1171 UniValue non_wit(UniValue::VOBJ);
1172- TxToUniv(*input.non_witness_utxo, uint256(), non_wit, false);
1173+ TxToUniv(*input.non_witness_utxo, uint256(), /* entry */ non_wit, /* include_hex */ false);
0ba1fe4 (only if you retouch) missed one ;)
0 TxToUniv(*input.non_witness_utxo, /* hashBlock */ uint256(), /* entry */ non_wit, /* include_hex */ false);
ScriptToUniv() is un-inlined right after again in b456004. ACK b456004de9ef61716140c9020c572416b46acbff otherwise.
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
git diff b456004 92dc5e9, re-read the per-commit changes, and rebased to current master/debug build/checked unit+functional tests pass
NACK 92dc5e954f
No idea what this commit is trying to achieve. Maybe split this up into another pull? It is dangerous by creating undetectable silent merge conflicts and not to mention it is plain wrong.
review ACK 073035f5a6fae435b96426273f6d1930fc6a70e5 🛎
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 073035f5a6fae435b96426273f6d1930fc6a70e5 🛎
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh7xQwAwWkY8oJ9UjoxE2VWgb0Su6ZdvLoPJ5553LsZbiGFp9IrkHfIFzlpBPAd
8Ev/wwOv890C25NC1mtaQDFrA4dF05+L2M+EdhINEJDA1Rm+kLQFlAA7h8gXgWBVk
9XqgYj3PQfwN1d1c8eHb9FKdPjYsq/UQfq9xPuo+WldT+9w241jfHvn3D/beypVEt
106/+mC/tCIGyiG2tR1FYufz/jMcKx2wKSjgxGgqXHDEsq0TGAILH+3zsYnfqE15l6
11CsP19y12I6TVIGepRhdkeN9kuUV98xzggaTx9zcd1TTJ6+rGCgpVpNaD87cNbmcc
12vRGoHk4pWRXEFuqRC9wj2mpQCuUcx/iclzSFxi5klcrwFajFMhstRD5jWtnz++Ye
13UrNCTRFmsDhCYAKNDZ+963MPNXgWZUHf3ygXyTD9AtJb/6DPB7/kvMwjwEx36AcX
14V6loqHJjEa+qZsBoY/KTzrERmwuHhyE95Jx3yoFmU6u/NSvOckwsy7/pPvYBqLB4
155pmgOyDu
16=p5HW
17-----END PGP SIGNATURE-----
Timestamp of file with hash 70af3366d8fa8d42efbe696d981448bff0730a1f79b3417951cb6e66fe628fa9 -
1267 out.pushKV("redeem_script", r);
1268 }
1269 if (!output.witness_script.empty()) {
1270 UniValue r(UniValue::VOBJ);
1271- ScriptToUniv(output.witness_script, /* out */ r);
1272+ ScriptToUniv(output.witness_script, /* out */ r, /* include_address */ false);
102@@ -103,6 +103,6 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
103 (void)IsWitnessStandard(tx, coins_view_cache);
104
105 UniValue u(UniValue::VOBJ);
106- TxToUniv(tx, /* hashBlock */ uint256::ZERO, u);
107- TxToUniv(tx, /* hashBlock */ uint256::ONE, u);
108+ TxToUniv(tx, /* hashBlock */ uint256::ZERO, /* entry */ u);
I think you can drop one of these statements to make the fuzz test run ~twice as fast.
(In a separate commit, obviously)
Good point. I don’t see any need to exercise this code path from the fuzz test, which seems to be the only point of invoking TxToUniv again w/ /* hashBlock */ uint256::ONE (especially if it will run twice as fast)
0if (!hashBlock.IsNull())
1 entry.pushKV("blockhash", hashBlock.GetHex());
So I’ll add a commit removing TxToUniv(tx, /* hashBlock */ uint256::ONE, u); as suggested.
uint256{bool_val}
1753@@ -1754,7 +1754,7 @@ static RPCHelpMan gettransaction()
1754
1755 if (verbose) {
1756 UniValue decoded(UniValue::VOBJ);
1757- TxToUniv(*wtx.tx, uint256(), decoded, false);
1758+ TxToUniv(*wtx.tx, /* hashBlock */ uint256(), /* entry */ decoded, /* include_hex */ false);
/* arg= */ syntax, so that clang-tidy understands it as well.
494- {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (only if a well-defined address exists)"},
495- {RPCResult::Type::ARR, "addresses", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Array of bitcoin addresses",
496- {
497- {RPCResult::Type::STR, "address", "bitcoin address"},
498- }},
499+ {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
Agreed this could be a worthwhile cleanup, there are 6 occurrences of this exact string, 5 of which are in the same file src/rpc/rawtransaction.cpp
Do you have any suggestion on where this variable belongs or can you point me to any conventions in the code?
See:
0 static const std::vector<RPCResult> TransactionDescriptionString()
Rebased.
I think you can drop one of these statements to make the fuzz test run ~twice as fast.
Done c45b133
726@@ -727,7 +727,7 @@ static void MutateTx(CMutableTransaction& tx, const std::string& command,
727 static void OutputTxJSON(const CTransaction& tx)
728 {
729 UniValue entry(UniValue::VOBJ);
730- TxToUniv(tx, uint256(), entry);
731+ TxToUniv(tx, /* hashBlock */ uint256(), entry);
a0f2dab816fc9987fb177c02f5d801cf0b8ebbf9: I think this can be done in a follow-up.
If you decide to do it here, at least use the proper clang-tidy format, so that it can be checked by the clang compiler.
, /* hashBlock=*/ uint256{},
review ACK f1acb5434f 🐯
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK f1acb5434f 🐯
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgi5Qv/ZHz7rCf4k/qbLPVJkl28A1bwknL9PtYwyfKkTq4BUREjjGmQoQQbbUjs
8jkHWzJxVKVveR9i9D+v/oEuZLEh64BdRAiirB6UG40fuaEKI3eYiV9IvkrowjzyO
9R91NYDJzGSCzuqDbAg9OBnQShxtZKKQ/DENoxMayCOQXbDfwO5CFGy8gTB17s+hp
10DXJ2Rq9R2J7rYshpfRPaLbJf2ZxzZlJa9A/lqv+ls9FdCuet3jhjOs/Aoknd/f/I
113cZzK8skniakpyEq0TjCIwdwfaaJXZdMJyPIiRdGXIjVP3BMradUjdPpwGMdvZpF
12AnfCdkmxksCGy1EJbH3Gipbo/PF3Cp1oQ1uwE4+oq16wxOUoLGSeFqtAwg5mCSKt
13itvsYyQhP2Erm/FNwas/la+nFusyDlkDSOgMHpg0XFzdEoqslAM3g2IxWFg4FKPs
14qqvI1Jl44hfRkBErCnn7edYmTpA5uGOvYxwAJSns0WUQsh7h4MMTexM7+xOwbCOh
15NXsg0pdM
16=FPMp
17-----END PGP SIGNATURE-----
Timestamp of file with hash 1eb7fc34e86b60e73cdd3193dc2eeb609ebf87c032e3483f7067eb23fe71b73a -
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.
71- type_ret == TxoutType::NONSTANDARD ||
72- type_ret == TxoutType::NULL_DATA);
73- }
74- } else {
75- assert(required_ret >= 1 && required_ret <= 16);
76- assert((unsigned long)required_ret == addresses.size());
This assertion fails:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39152
It might be good to merge this pull request or apply the changes to this fuzz test early.
re-ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec 🐉
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec 🐉
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgkNQv/TUjva/T/Sp2WEolHXi+auEURUOvDAqnHI2HYJxTX603YbaeWPkvATkzl
83L12B+9CH60cpegoJVK0jesF7J1G2TryHs2L9LAb4X0GGnvD6Tr7+hx8BrJenZDT
951laMGIWvZxPGRnohFuVP/tp3/a0eBhtee6DGBFpJWOjT9p6cUJxF02eFtDwDYTN
10nxOTxzliV5TnQgQyPC0r4MShGRZbCZwqBbv1nwK+iB0q8qHMzAP5mS+B7B+DkhV8
11SpGrH2OIjk2RAmKP2nuD42THzz4alMUJPY6S2vuU9MPVaLapNhK0CB3vaJJewODN
126F6uqzMUQ2JWstjAtUUeNOp7pXndFw+8jk0UlocSxtfLhpfu2bLtTxyBFWog9MaF
13Czo9RxXhmFePu5t7lRCF/esSxrnXonsjXMaxZ3+ZdAm0nPGYecOjHvrg1p8dGLDv
14QlvAK7A2sNZsiNI8u7ggY3Abu3BGL/7Qf5AM0fOGXJQFVDjeXwqhj2kyLFAlcJUC
15l4UmCNZz
16=AVpG
17-----END PGP SIGNATURE-----
Timestamp of file with hash 1ef8757c6c687d7a663bef23283581d4e088670176d49a29213e78ffea20beec -
112@@ -113,9 +113,7 @@ FUZZ_TARGET_INIT(script, initialize_script)
113 UniValue o2(UniValue::VOBJ);
114 ScriptPubKeyToUniv(script, o2, false);
115 UniValue o3(UniValue::VOBJ);
116- ScriptToUniv(script, o3, true);
117- UniValue o4(UniValue::VOBJ);
118- ScriptToUniv(script, o4, false);
119+ ScriptToUniv(script, o3);
include_address=true/false checks on ScriptPubKeyToUniv(script, o4, fIncludeHex, include_address)
Code review ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec
Commit refactor: minor styling, prefer snake case and same line if should be squashed with the previous refactoring commit.
170- int nRequired;
171
172 out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
173- if (fIncludeHex)
174- out.pushKV("hex", HexStr(scriptPubKey));
175+ if (include_hex) out.pushKV("hex", HexStr(scriptPubKey));
return or return true/false, I increasingly add braces instead of one-lining it. The action stands out, and if a line raises while running gdb, it’s more isolated. Others may have a different opinion, so just a thought.
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
Labels
Wallet
RPC/REST/ZMQ
Utils/log/libs
Milestone
23.0