Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference #23507

pull mjdietzx wants to merge 14 commits into bitcoin:master from mjdietzx:ret_UniValue_instead_of_pass_by_ref changing 17 files +92 −148
  1. mjdietzx commented at 8:50 PM on November 13, 2021: contributor

    Now functions return a UniValue instead of mutating a parameter that's passed by reference.

    After this PR, any time a UniValue is passed by reference, it should be const. This is the case everywhere in the codebase now (please double check me on this) except for SoftForkDescPushBack (which I didn't want to touch), and the RPC request handling logic.

    Motivation for this PR was based on a suggestion in #22924 review:

    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.

    I figured it's simple enough, why not do this across the entire codebase. Hopefully I didn't get carried away

  2. mjdietzx cross-referenced this on Nov 13, 2021 from issue refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag by mjdietzx
  3. lsilva01 commented at 9:09 PM on November 13, 2021: contributor

    Concept ACK.

  4. DrahtBot added the label Refactoring on Nov 13, 2021
  5. DrahtBot added the label RPC/REST/ZMQ on Nov 13, 2021
  6. DrahtBot added the label Utils/log/libs on Nov 13, 2021
  7. DrahtBot added the label Wallet on Nov 13, 2021
  8. DrahtBot commented at 3:08 AM on November 14, 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:

    • #26039 (rpc: Run type check against RPCArgs (1/2) by MarcoFalke)
    • #23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)
    • #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)

    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.

  9. DrahtBot cross-referenced this on Nov 14, 2021 from issue rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by maflcko
  10. mjdietzx force-pushed on Nov 14, 2021
  11. mjdietzx force-pushed on Nov 14, 2021
  12. mjdietzx force-pushed on Nov 14, 2021
  13. mjdietzx renamed this:
    Refactor: Improve API design of `ScriptToUniv`, `TxToUniv`
    Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating the parameter/reference
    on Nov 14, 2021
  14. mjdietzx renamed this:
    Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating the parameter/reference
    Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference
    on Nov 14, 2021
  15. DrahtBot cross-referenced this on Nov 14, 2021 from issue rpc: Add RPC help for getblock verbosity level 3 by kiminuo
  16. DrahtBot cross-referenced this on Nov 14, 2021 from issue rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh
  17. DrahtBot cross-referenced this on Nov 14, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  18. DrahtBot cross-referenced this on Nov 14, 2021 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky
  19. mjdietzx force-pushed on Nov 14, 2021
  20. mjdietzx force-pushed on Nov 14, 2021
  21. DrahtBot cross-referenced this on Nov 14, 2021 from issue rpc: deprecate top-level fee fields in getmempool RPCs by josibake
  22. DrahtBot cross-referenced this on Nov 15, 2021 from issue Add fee rate distribution in -getinfo by ghost
  23. DrahtBot cross-referenced this on Nov 15, 2021 from issue rpc: Add option to list transactions from oldest to newest in `listtransactions` RPC command by ben-kaufman
  24. mjdietzx marked this as ready for review on Nov 15, 2021
  25. mjdietzx force-pushed on Nov 15, 2021
  26. DrahtBot cross-referenced this on Nov 16, 2021 from issue refactor: Extract RipeMd160 by Empact
  27. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by maflcko
  28. DrahtBot cross-referenced this on Nov 18, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by maflcko
  29. DrahtBot cross-referenced this on Nov 27, 2021 from issue wallet: Split stuff from rpcwallet by maflcko
  30. DrahtBot cross-referenced this on Nov 27, 2021 from issue [WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by maflcko
  31. DrahtBot added the label Needs rebase on Dec 1, 2021
  32. in src/test/fuzz/script.cpp:109 in 092ad6a030 outdated 
     107 | @@ -108,12 +108,7 @@ FUZZ_TARGET_INIT(script, initialize_script)
 108 |      (void)ScriptToAsmStr(script, false);
 109 |      (void)ScriptToAsmStr(script, true);
    maflcko commented at 3:07 PM on December 10, 2021:

    unrelated, but could also use ConsumeBool here

    mjdietzx commented at 6:46 PM on April 2, 2022:

    This has been done in this commit fae3f178238df96554dc2495e040f5580b55408a

    Marking as resolved.

  33. rajarshimaitra approved
  34. rajarshimaitra commented at 7:17 AM on December 15, 2021: contributor

    Concept + tACK https://github.com/bitcoin/bitcoin/pull/23507/commits/092ad6a03090172e61ac95a328b9d429e043e8ff

    Agree that this makes code much simpler. Never was a fan of argument modification. 😅

    Tested that all tests are passing. No unexpected behavior change.

  35. fanquake commented at 11:37 AM on March 22, 2022: member

    I'm going to close this for now. Let's re-open / followup once we figure out the status of, and potentially merge #22924. (I'm doing a rebase at the moment).

  36. fanquake closed this on Mar 22, 2022
  37. mjdietzx commented at 3:09 PM on March 25, 2022: contributor

    Sounds good, I'll start my review of #24673 today, and will rebase this on top of that PR if it makes sense - then we can see where this PR goes from there

  38. fanquake commented at 7:07 AM on March 31, 2022: member

    #24673 has now been merged.

  39. mjdietzx commented at 8:12 PM on April 2, 2022: contributor

    I've rebased this PR, but I have not force pushed yet. IIRC I need to make sure I re-open the PR before force pushing. But, I can't figure out how to re-open (I think it may be due to "This pull request is closed, but the mjdietzx:ret_UniValue_instead_of_pass_by_ref branch has unmerged commits.")

    Can anyone advise on how I should move forward? Should I create a new branch and just reference this PR (leave it closed)?

  40. fanquake reopened this on Apr 3, 2022
  41. fanquake commented at 7:46 AM on April 3, 2022: member

    @mjdietzx you should be able to force push to the branch now.

  42. mjdietzx force-pushed on Apr 3, 2022
  43. DrahtBot removed the label Needs rebase on Apr 3, 2022
  44. mjdietzx force-pushed on Apr 3, 2022
  45. mjdietzx force-pushed on Apr 3, 2022
  46. DrahtBot added the label Needs rebase on Apr 4, 2022
  47. mjdietzx force-pushed on Apr 4, 2022
  48. DrahtBot removed the label Needs rebase on Apr 4, 2022
  49. DrahtBot cross-referenced this on Apr 5, 2022 from issue add `(none)` in -getinfo `Warnings:` if no warning returned by ghost
  50. DrahtBot cross-referenced this on Apr 5, 2022 from issue rest: Use query parameters to control resource loading by stickies-v
  51. DrahtBot added the label Needs rebase on Apr 6, 2022
  52. mjdietzx force-pushed on Apr 13, 2022
  53. mjdietzx force-pushed on Apr 13, 2022
  54. DrahtBot removed the label Needs rebase on Apr 14, 2022
  55. laanwj removed the label Wallet on Apr 14, 2022
  56. laanwj removed the label Utils/log/libs on Apr 14, 2022
  57. laanwj commented at 7:52 AM on April 14, 2022: member

    Concept ACK. Returning out-only return values n is a clear improvement in API usability.

  58. DrahtBot cross-referenced this on May 16, 2022 from issue refactor: Remove `NO_THREAD_SAFETY_ANALYSIS` from non-test/benchmarking code by hebasto
  59. DrahtBot cross-referenced this on May 18, 2022 from issue rpc, wallet: add abandoned field for all categories of transaction in ListTransaction by brunoerg
  60. DrahtBot added the label Needs rebase on May 18, 2022
  61. mjdietzx commented at 2:04 AM on August 5, 2022: contributor

    Does anyone want this PR enough to review it if I rebase? If so I will do a careful rebase/review. If not I'm ok with closing this

  62. fanquake commented at 9:49 AM on October 3, 2022: member

    Does anyone want this PR enough to review it if I rebase? If so I will do a careful rebase/review. If not I'm ok with closing this

    I think this change is worthwhile. If you're happy to do a rebase I'll review it. If not, I'll pick this up.

  63. refactor: `ScriptToUniv` return `UniValue` instead of mutating reference 4a72006952
  64. refactor: `TxToJSON` return `UniValue` instead of mutating reference e9a58c2999
  65. refactor: `TxToUniv` return `UniValue` instead of mutating reference 0ebce550c1
  66. refactor: prefer snake case, hashBlock renamed block_hash in rpc/rawtransaction.cpp dc5d10e11c
  67. refactor: remove code duplication b/w `TxToJSON` and `TxToUniv` a5e0d3544e
  68. refactor: `entryToJSON` return `UniValue` instead of mutating reference 352ebc36ef
  69. refactor: `SignTransaction` return `UniValue` instead of mutating reference 1fc2d0e4c4
  70. refactor: `SignTransactionResultToJSON` return `UniValue` instead of mutating reference 6b8db15a03
  71. refactor: `TxInErrorToJSON` return `UniValue` instead of mutating reference 9a5c986b1b
  72. refactor: `WalletTxToJSON` return `UniValue` instead of mutating reference ea6ef02393
  73. refactor: `ProcessSubScript` return `UniValue` instead of mutating reference 370da170b9
  74. refactor: `GetWalletBalances` return `UniValue` instead of mutating reference a27abe973d
  75. refactor: `ParseGetInfoResult` return a value instead of mutating `UniValue` reference 908d60973d
  76. refactor: `RPCExecutor::request` catch by reference can be const df41d46249
  77. mjdietzx force-pushed on Oct 3, 2022
  78. mjdietzx commented at 10:30 PM on October 3, 2022: contributor

    Thanks @fanquake, rebased.

    I am also going to do a thorough re-review since I did this a while ago. Also I dropped fcffbdefd2d5f5ec9bba59f55f5b1db6e9a9965c for now, I need to take some time to go through @MarcoFalke's #25464 before I rebase this commit.

  79. DrahtBot removed the label Needs rebase on Oct 3, 2022
  80. DrahtBot cross-referenced this on Oct 3, 2022 from issue clang-tidy: fixup named argument comments by fanquake
  81. DrahtBot cross-referenced this on Oct 4, 2022 from issue refactor: Run type check against RPCArgs (1/2) by maflcko
  82. DrahtBot cross-referenced this on Oct 6, 2022 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky
  83. DrahtBot cross-referenced this on Oct 17, 2022 from issue rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second by andrewtoth
  84. DrahtBot added the label Needs rebase on Dec 8, 2022
  85. DrahtBot commented at 9:55 AM on December 8, 2022: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  86. DrahtBot commented at 12:24 AM on March 8, 2023: 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.
  87. achow101 closed this on Apr 25, 2023
  88. in src/test/fuzz/transaction.cpp:104 in df41d46249 
      99 | @@ -100,7 +100,6 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
 100 |      (void)AreInputsStandard(tx, coins_view_cache);
 101 |      (void)IsWitnessStandard(tx, coins_view_cache);
 102 |  
 103 | -    UniValue u(UniValue::VOBJ);
 104 | -    TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*entry=*/u);
 105 | -    TxToUniv(tx, /*block_hash=*/uint256::ONE, /*entry=*/u);
 106 | +    (void)TxToUniv(tx, /*block_hash=*/uint256::ZERO);
 107 | +    (void)TxToUniv(tx, /*block_hash=*/uint256::ONE);
    maflcko commented at 4:02 PM on June 7, 2023:

    I think this is a bugfix for the fuzz test, which eats too much memory (especially under msan)

    mjdietzx commented at 4:13 PM on June 7, 2023:

    Oh man I'm curious how you ended up back in this PR!? Is that just from your memory?

    Anyways lmk if you want me to do anything. I'd be happy to rebase this, open a PR with just this commit, etc.. whatever you suggest. Also feel free to just cherry-pick that commit or whatever is most efficient for you as well incase you are just commenting this for documentation purposes

    maflcko commented at 4:53 PM on June 7, 2023:

    Maybe just split out the TxToUniv change in a new pull?

  89. bitcoin locked this on Jun 6, 2024
Contributors
mjdietzxlsilva01DrahtBotmaflckorajarshimaitrafanquakelaanwj
Labels
RefactoringRPC/REST/ZMQNeeds rebase
Linked (view graph)
#22924 refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag#24673 refactor: followup of remove -deprecatedrpc=addresses flag#25464 rpc: Reduce Univalue push_backV peak memory usage in listtransactions
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