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

mjdietzx · 2021-11-13T20:50:14Z

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

lsilva01 · 2021-11-13T21:09:11Z

Concept ACK.

DrahtBot · 2021-11-14T03:08:06Z

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

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.

mjdietzx · 2022-08-05T02:04:12Z

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

fanquake · 2022-10-03T09:49:38Z

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.

…ransaction.cpp

…erence

…mutating reference

…erence

…rence

…ference

…eference

…iValue` reference

mjdietzx · 2022-10-03T22:30:20Z

Thanks @fanquake, rebased.

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

DrahtBot · 2022-12-08T09:55:36Z

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

DrahtBot · 2023-03-08T00:24:53Z

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.

maflcko · 2023-06-07T16:02:31Z

src/test/fuzz/transaction.cpp

-    TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*entry=*/u);
-    TxToUniv(tx, /*block_hash=*/uint256::ONE, /*entry=*/u);
+    (void)TxToUniv(tx, /*block_hash=*/uint256::ZERO);
+    (void)TxToUniv(tx, /*block_hash=*/uint256::ONE);


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

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

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

mjdietzx mentioned this pull request Nov 13, 2021

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

Closed

DrahtBot added Refactoring RPC/REST/ZMQ Utils/log/libs Wallet labels Nov 13, 2021

DrahtBot mentioned this pull request Nov 14, 2021

rpc: Only allow specific types to be P2(W)SH wrapped in decodescript #23486

Merged

mjdietzx force-pushed the ret_UniValue_instead_of_pass_by_ref branch 3 times, most recently from a01a216 to 140767e Compare November 14, 2021 04:28

mjdietzx changed the title ~~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 Nov 14, 2021

mjdietzx force-pushed the ret_UniValue_instead_of_pass_by_ref branch 2 times, most recently from 5081cf9 to 82826eb Compare November 14, 2021 20:26

This was referenced Nov 14, 2021

rpc: deprecate top-level fee fields in getmempool RPCs #22689

Merged

Add fee rate distribution in -getinfo #22891

Closed

rpc: Add option to list transactions from oldest to newest in listtransactions RPC command #22775

Closed

mjdietzx marked this pull request as ready for review November 15, 2021 14:59

mjdietzx force-pushed the ret_UniValue_instead_of_pass_by_ref branch from 82826eb to 092ad6a Compare November 15, 2021 18:01

This was referenced Nov 16, 2021

refactor: Extract RipeMd160 #15294

Merged

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Merged

This was referenced Nov 27, 2021

wallet: Split stuff from rpcwallet #23602

Merged

[WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags #23599

Closed

DrahtBot added the Needs rebase label Dec 1, 2021

DrahtBot added the Needs rebase label May 18, 2022

mjdietzx added 14 commits October 3, 2022 11:22

refactor: ScriptToUniv return UniValue instead of mutating reference

4a72006

refactor: TxToJSON return UniValue instead of mutating reference

e9a58c2

refactor: TxToUniv return UniValue instead of mutating reference

0ebce55

refactor: prefer snake case, hashBlock renamed block_hash in rpc/rawt…

dc5d10e

…ransaction.cpp

refactor: remove code duplication b/w TxToJSON and TxToUniv

a5e0d35

refactor: entryToJSON return UniValue instead of mutating reference

352ebc3

refactor: SignTransaction return UniValue instead of mutating ref…

1fc2d0e

…erence

refactor: SignTransactionResultToJSON return UniValue instead of …

6b8db15

…mutating reference

refactor: TxInErrorToJSON return UniValue instead of mutating ref…

9a5c986

…erence

refactor: WalletTxToJSON return UniValue instead of mutating refe…

ea6ef02

…rence

refactor: ProcessSubScript return UniValue instead of mutating re…

370da17

…ference

refactor: GetWalletBalances return UniValue instead of mutating r…

a27abe9

…eference

refactor: ParseGetInfoResult return a value instead of mutating `Un…

908d609

…iValue` reference

refactor: RPCExecutor::request catch by reference can be const

df41d46

mjdietzx force-pushed the ret_UniValue_instead_of_pass_by_ref branch from 20b68de to df41d46 Compare October 3, 2022 22:30

DrahtBot removed the Needs rebase label Oct 3, 2022

This was referenced Oct 3, 2022

clang-tidy: fixup named argument comments #26238

Merged

refactor: Run type check against RPCArgs (1/2) #26039

Merged

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

DrahtBot mentioned this pull request Oct 17, 2022

rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second #26308

Merged

DrahtBot added the Needs rebase label Dec 8, 2022

achow101 closed this Apr 25, 2023

maflcko reviewed Jun 7, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

mjdietzx commented Nov 13, 2021 •

edited

Loading

Uh oh!

lsilva01 commented Nov 13, 2021

Uh oh!

DrahtBot commented Nov 14, 2021 •

edited

Loading

Uh oh!

mjdietzx commented Aug 5, 2022

Uh oh!

fanquake commented Oct 3, 2022

Uh oh!

mjdietzx commented Oct 3, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Dec 8, 2022

Uh oh!

DrahtBot commented Mar 8, 2023

Uh oh!

maflcko Jun 7, 2023

Uh oh!

mjdietzx Jun 7, 2023

Uh oh!

maflcko Jun 7, 2023

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

mjdietzx commented Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 commented Nov 13, 2021

Uh oh!

DrahtBot commented Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

mjdietzx commented Aug 5, 2022

Uh oh!

fanquake commented Oct 3, 2022

Uh oh!

mjdietzx commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 8, 2022

Uh oh!

DrahtBot commented Mar 8, 2023

Uh oh!

maflcko Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

mjdietzx Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

mjdietzx commented Nov 13, 2021 •

edited

Loading

DrahtBot commented Nov 14, 2021 •

edited

Loading

mjdietzx commented Oct 3, 2022 •

edited

Loading