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

pull mjdietzx wants to merge 5 commits into bitcoin:master from mjdietzx:refactor_ScriptToUniv changing 10 files +51 −48

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
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?

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.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on Dec 1, 2021
DrahtBot commented at 1:06 PM on March 21, 2022: member
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- 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

Contributors

Labels

Refactoring Needs rebase

Linked (view graph)

#22650 Remove -deprecatedrpc=addresses flag and corresponding code/logic #23507 Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference #24673 refactor: followup of remove -deprecatedrpc=addresses flag

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

Conflicts