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

mjdietzx commented at 5:12 pm on September 8, 2021: contributor

This is a follow-up on top of #22650. Based on review there, we thought it would be an improvement to merge ScriptPubKeyToUniv and ScriptToUniv into a single function, but this didn’t seem to belong in that PR. There are a few other minor cleanups here as well focused on style and making some fuzz tests run faster

DrahtBot added the label Refactoring on Sep 8, 2021

DrahtBot added the label RPC/REST/ZMQ on Sep 8, 2021

DrahtBot added the label Utils/log/libs on Sep 8, 2021

DrahtBot added the label Wallet on Sep 8, 2021

DrahtBot added the label Needs rebase on Sep 9, 2021

mjdietzx renamed this:
~~refactor: merge ScriptPubKeyToUniv and ScriptToUniv into a single function~~
refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag
on Sep 14, 2021

mjdietzx marked this as a draft on Sep 14, 2021

mjdietzx commented at 8:28 pm on September 14, 2021: contributor

Marking this as a draft for now. I will be moving some of the refactoring/cleanup that was originally included in #22650 to this PR. So once that is done and I’ve rebased this PR, I will reopen it

mjdietzx force-pushed on Sep 29, 2021

fanquake removed the label Needs rebase on Sep 29, 2021

mjdietzx commented at 3:15 am on September 29, 2021: contributor

Now that #22650 is merged, I rebased and finished this. This PR follows up with some cleanup, and addresses all the comments in the first PR that were left for now.

Just so we have them at-hand, I took all the original comments from #22650’s review, and am putting them here.

@jonatack: maybe I’m missing something but perhaps merge ScriptPubKeyToUniv and ScriptToUniv into a single function

Done b9ad1d9

@jonatack: Named args could be nice wherever this function is invoked

and

@MarcoFalke: 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

Done e9f3f1b

@MarcoFalke: as a follow-up it could make sense to de-duplicate the common help text

Done fe71ac8

@MarcoFalke: I think you can drop one of these statements to make the fuzz test run ~twice as fast. An alternative (to test both code paths) would be to deserialize a bool from the fuzzing input and use uint256{bool_val}

and

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

Done e749168

mjdietzx marked this as ready for review on Sep 29, 2021

in src/test/fuzz/transaction.cpp:108 in e749168e49 outdated

104@@ -103,6 +105,5 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
105     (void)IsWitnessStandard(tx, coins_view_cache);
106 
107     UniValue u(UniValue::VOBJ);
108-    TxToUniv(tx, /* hashBlock */ uint256::ZERO, u);
109-    TxToUniv(tx, /* hashBlock */ uint256::ONE, u);
110+    TxToUniv(tx, /* hashBlock= */ uint256{fuzzed_data_provider.ConsumeBool()}, /* entry= */ u, fuzzed_data_provider.ConsumeBool());

MarcoFalke commented at 8:03 am on September 29, 2021:

not a fan of re-using the same buffer, thus nVersion for this bool.

try { ds >> bool{}; } might be nicer

mjdietzx commented at 3:16 pm on September 29, 2021:

Makes sense, done 08543bf. I may have butchered this, so if you want to see any other changes here just lmk. Happy to fix up any nits bc I have a feeling there is a cleaner way to do this that I’m just not aware of

in src/wallet/rpcwallet.cpp:1765 in e749168e49 outdated

1761@@ -1762,7 +1762,7 @@ static RPCHelpMan gettransaction()
1762 
1763     if (verbose) {
1764         UniValue decoded(UniValue::VOBJ);
1765-        TxToUniv(*wtx.tx, uint256(), decoded, false);
1766+        TxToUniv(*wtx.tx, /* hashBlock= */ uint256(), /* entry= */ decoded, /* include_hex= */ false);

MarcoFalke commented at 8:06 am on September 29, 2021:

nit: Maybe before using named args, fix the argument name first?

0        TxToUniv(*wtx.tx, /*block_hash=*/uint256{}, /*entry=*/decoded, /*include_hex=*/false);

mjdietzx commented at 3:15 pm on September 29, 2021:

Added a commit, done here: be3651a. I had wanted to do this but seemed like hashBlock was used pretty regularly so I thought there might be some convention I wasn’t aware of - I guess not so I’m happy to be able to do this

MarcoFalke approved

MarcoFalke removed the label Wallet on Sep 29, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Sep 29, 2021

MarcoFalke removed the label Utils/log/libs on Sep 29, 2021

MarcoFalke added the label Needs rebase on Sep 29, 2021

DrahtBot removed the label Needs rebase on Sep 29, 2021

mjdietzx force-pushed on Sep 29, 2021

in src/test/fuzz/transaction.cpp:111 in 08543bff7c outdated

106+    bool include_hex;
107+    try {
108+        ds >> block_hash;
109+        ds >> include_hex;
110+    } catch (const std::ios_base::failure&) {
111+        return;

MarcoFalke commented at 3:15 pm on September 29, 2021:

If you initialize the bools to false, you can remove the early return (This is also what FuzzedDataProvider would do)

mjdietzx commented at 3:16 pm on September 29, 2021:

That’s a good idea. Ok, one sec

mjdietzx force-pushed on Sep 29, 2021

DrahtBot commented at 8:42 pm on September 29, 2021: 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:

#23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
#23546 (scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by MarcoFalke)
#23486 (rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by MarcoFalke)
#23320 (rpc: Add RPC help for getblock verbosity level 3 by kiminuo)
#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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 added the label Needs rebase on Oct 19, 2021

mjdietzx force-pushed on Oct 25, 2021

mjdietzx commented at 3:24 pm on October 25, 2021: contributor

Rebased, fixed some merge conflicts from #22918

DrahtBot removed the label Needs rebase on Oct 25, 2021

in src/core_write.cpp:151 in b387c280ad outdated

154 
155-    out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
156-    if (include_hex) out.pushKV("hex", HexStr(scriptPubKey));
157+    out.pushKV("asm", ScriptToAsmStr(script));
158+    if (include_hex) {
159+      out.pushKV("hex", HexStr(script));

laanwj commented at 2:48 pm on November 10, 2021:

indentation nit

mjdietzx commented at 3:35 pm on November 13, 2021:

Nice catch, done.

in src/core_io.h:56 in 570cbb7274 outdated

52@@ -53,8 +53,7 @@ UniValue ValueFromAmount(const CAmount amount);
53 std::string FormatScript(const CScript& script);
54 std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
55 std::string SighashToStr(unsigned char sighash_type);
56-void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
57-void ScriptToUniv(const CScript& script, UniValue& out);
58-void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);
59+void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false);

laanwj commented at 3:27 pm on November 10, 2021:

I wonder why we don’t return the UniValue (or optional<UniValue> in case it can fail) instead of having it as second parameter. It would seem better API design to me.

mjdietzx commented at 3:37 pm on November 13, 2021:

I think this is a great suggestion. And at first glance it seems to be straightforward enough to include in this PR. I’ll follow up shortly

mjdietzx commented at 8:10 pm on November 13, 2021:

I am leaning towards not including this change in this PR. I do think it is a great opportunity to cleanup and improve the code quality, so I would like to follow up with another PR doing this. This pattern is also used all over the place, not just ScriptToUniv, but TxToUniv, entryToJSON, and probably more. Anyways I want to spend some more time on this and don’t want to end up doing too much in this PR

mjdietzx commented at 8:52 pm on November 13, 2021:

Can you take a quick look at the last two commits in this draft PR #23507?

a) is this along the lines of what you are thinking? b) do you think it is worthwhile for me to do this across the codebase in #23507, eg entryToJSON, TxToJSON, etc… can be improved in the same way

Feel free to answer those in the linked PR and not here

mjdietzx force-pushed on Nov 13, 2021

refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function b34171186b

refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash 6caf9812d1

refactor: named args whenever ScriptToUniv or TxToUniv invoked 9d61af23b4

refactor: de-duplicate common help text for parsed destination address 63a657ccff

fuzz: speedup ScriptToUniv and TxToUniv tests by dropping redundant call d4675f410f

mjdietzx force-pushed on Nov 13, 2021

mjdietzx commented at 8:11 pm on November 13, 2021: contributor

Addressed indentation nit and rebased to master

mjdietzx force-pushed on Nov 13, 2021

in src/rpc/blockchain.cpp:1277 in 63a657ccff outdated

1273@@ -1274,7 +1274,7 @@ static RPCHelpMan gettxout()
1274                     {RPCResult::Type::STR, "asm", ""},
1275                     {RPCResult::Type::STR_HEX, "hex", ""},
1276                     {RPCResult::Type::STR, "type", "The type, eg pubkeyhash"},
1277-                    {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
1278+                    {RPCResult::Type::STR, "address", /* optional */ true, DESTINATION_ADDRESS},

MarcoFalke commented at 10:09 am on December 1, 2021:

it might be better to de-duplicate the whole asm/hex/type/address blob. Though, maybe create a separate pull for doc cleanups vs code refactorings?

MarcoFalke commented at 11:47 am on December 1, 2021:

See for example e7507f333b or f8911de619d41a3cc21771d62ab077713f1405c5 on how to do this.

mjdietzx commented at 3:06 pm on December 9, 2021:

I think this is a good suggestion. I will do this as a followup as you suggest. So I will get rid of 63a657ccff959f7d86f423607f69aa9f7507dbdb when I rebase and have a doc cleanup followup with these suggestions

DrahtBot commented at 11:03 am on December 1, 2021: member

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Dec 1, 2021

DrahtBot commented at 1:06 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke added the label Up for grabs on Mar 22, 2022

MarcoFalke removed the label Up for grabs on Mar 22, 2022

MarcoFalke commented at 11:20 am on March 22, 2022: member

I guess it is hard to tell how much of this is still relevant without doing a rebase?

fanquake commented at 2:53 pm on March 25, 2022: member

I’ve rebased some of the commits here in #24673. Haven’t done 63a657ccff959f7d86f423607f69aa9f7507dbdb as we might be able to follow up differently there, and do some de-duplication, see #22924 (review).

mjdietzx commented at 3:06 pm on March 25, 2022: contributor

Thanks @fanquake - it seems like the best path forward is closing this PR in favor of #24673? If so I’ll close this and test/review the new PR

fanquake commented at 3:08 pm on March 25, 2022: member

Thanks @fanquake - it seems like the best path forward is closing this PR in favor of #24673? If so I’ll close this and test/review the new PR

Sure, lets do that for now, and then potentially follow up with #23507 etc.

mjdietzx closed this on Mar 25, 2022

MarcoFalke referenced this in commit a2e1590f67 on Mar 31, 2022

DrahtBot locked this on Mar 25, 2023

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

Conflicts