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
  1. 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

  2. DrahtBot added the label Refactoring on Sep 8, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on Sep 8, 2021
  4. DrahtBot added the label Utils/log/libs on Sep 8, 2021
  5. DrahtBot added the label Wallet on Sep 8, 2021
  6. DrahtBot added the label Needs rebase on Sep 9, 2021
  7. mjdietzx renamed this:
    refactor: merge ScriptPubKeyToUniv and ScriptToUniv into a single function
    refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag
    on Sep 14, 2021
  8. mjdietzx marked this as a draft on Sep 14, 2021
  9. 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

  10. mjdietzx cross-referenced this on Sep 28, 2021 from issue Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx
  11. mjdietzx force-pushed on Sep 29, 2021
  12. mjdietzx force-pushed on Sep 29, 2021
  13. fanquake removed the label Needs rebase on Sep 29, 2021
  14. 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

  15. mjdietzx marked this as ready for review on Sep 29, 2021
  16. 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

  17. 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

  18. MarcoFalke approved
  19. MarcoFalke removed the label Wallet on Sep 29, 2021
  20. MarcoFalke removed the label RPC/REST/ZMQ on Sep 29, 2021
  21. MarcoFalke removed the label Utils/log/libs on Sep 29, 2021
  22. MarcoFalke added the label Needs rebase on Sep 29, 2021
  23. DrahtBot removed the label Needs rebase on Sep 29, 2021
  24. mjdietzx force-pushed on Sep 29, 2021
  25. 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

  26. mjdietzx force-pushed on Sep 29, 2021
  27. DrahtBot commented at 8:42 PM on September 29, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  28. DrahtBot cross-referenced this on Sep 30, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  29. DrahtBot cross-referenced this on Sep 30, 2021 from issue rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) by kiminuo
  30. DrahtBot added the label Needs rebase on Oct 19, 2021
  31. mjdietzx force-pushed on Oct 25, 2021
  32. mjdietzx commented at 3:24 PM on October 25, 2021: contributor

    Rebased, fixed some merge conflicts from #22918

  33. DrahtBot removed the label Needs rebase on Oct 25, 2021
  34. DrahtBot cross-referenced this on Oct 25, 2021 from issue rpc: Add RPC help for getblock verbosity level 3 by kiminuo
  35. DrahtBot cross-referenced this on Oct 25, 2021 from issue rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh
  36. 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.

  37. 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

  38. DrahtBot cross-referenced this on Nov 12, 2021 from issue rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by MarcoFalke
  39. mjdietzx force-pushed on Nov 13, 2021
  40. refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function b34171186b
  41. refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash 6caf9812d1
  42. refactor: named args whenever ScriptToUniv or TxToUniv invoked 9d61af23b4
  43. refactor: de-duplicate common help text for parsed destination address 63a657ccff
  44. fuzz: speedup ScriptToUniv and TxToUniv tests by dropping redundant call d4675f410f
  45. mjdietzx force-pushed on Nov 13, 2021
  46. mjdietzx commented at 8:11 PM on November 13, 2021: contributor

    Addressed indentation nit and rebased to master

  47. mjdietzx force-pushed on Nov 13, 2021
  48. mjdietzx cross-referenced this on Nov 13, 2021 from issue Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference by mjdietzx
  49. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke
  50. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by MarcoFalke
  51. DrahtBot cross-referenced this on Nov 27, 2021 from issue [WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by MarcoFalke
  52. 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

  53. DrahtBot commented at 11:03 AM on December 1, 2021: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  54. DrahtBot added the label Needs rebase on Dec 1, 2021
  55. DrahtBot commented at 1:06 PM on March 21, 2022: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->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.
  56. MarcoFalke added the label Up for grabs on Mar 22, 2022
  57. MarcoFalke removed the label Up for grabs on Mar 22, 2022
  58. 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?

  59. fanquake cross-referenced this on Mar 25, 2022 from issue refactor: followup of remove -deprecatedrpc=addresses flag by fanquake
  60. 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).

  61. 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

  62. 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.

  63. mjdietzx closed this on Mar 25, 2022
  64. MarcoFalke referenced this in commit a2e1590f67 on Mar 31, 2022
  65. bitcoin locked this on Mar 25, 2023
Contributors
mjdietzxMarcoFalkeDrahtBotlaanwjfanquake
Labels
RefactoringNeeds 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
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC