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

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 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 commented at 9:09 pm on November 13, 2021: contributor

Concept ACK.

DrahtBot added the label Refactoring on Nov 13, 2021

DrahtBot added the label RPC/REST/ZMQ on Nov 13, 2021

DrahtBot added the label Utils/log/libs on Nov 13, 2021

DrahtBot added the label Wallet on Nov 13, 2021

DrahtBot commented at 3:08 am on November 14, 2021: contributor

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 force-pushed on Nov 14, 2021

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

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

mjdietzx force-pushed on Nov 14, 2021

mjdietzx marked this as ready for review on Nov 15, 2021

mjdietzx force-pushed on Nov 15, 2021

DrahtBot added the label Needs rebase on Dec 1, 2021

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.

rajarshimaitra approved

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.

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

fanquake closed this on Mar 22, 2022

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

fanquake commented at 7:07 am on March 31, 2022: member

#24673 has now been merged.

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

fanquake reopened this on Apr 3, 2022

fanquake commented at 7:46 am on April 3, 2022: member

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

mjdietzx force-pushed on Apr 3, 2022

DrahtBot removed the label Needs rebase on Apr 3, 2022

mjdietzx force-pushed on Apr 3, 2022

DrahtBot added the label Needs rebase on Apr 4, 2022

mjdietzx force-pushed on Apr 4, 2022

DrahtBot removed the label Needs rebase on Apr 4, 2022

DrahtBot added the label Needs rebase on Apr 6, 2022

mjdietzx force-pushed on Apr 13, 2022

DrahtBot removed the label Needs rebase on Apr 14, 2022

laanwj removed the label Wallet on Apr 14, 2022

laanwj removed the label Utils/log/libs on Apr 14, 2022

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.

DrahtBot added the label Needs rebase on May 18, 2022

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

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.

refactor: `ScriptToUniv` return `UniValue` instead of mutating reference 4a72006952

refactor: `TxToJSON` return `UniValue` instead of mutating reference e9a58c2999

refactor: `TxToUniv` return `UniValue` instead of mutating reference 0ebce550c1

refactor: prefer snake case, hashBlock renamed block_hash in rpc/rawtransaction.cpp dc5d10e11c

refactor: remove code duplication b/w `TxToJSON` and `TxToUniv` a5e0d3544e

refactor: `entryToJSON` return `UniValue` instead of mutating reference 352ebc36ef

refactor: `SignTransaction` return `UniValue` instead of mutating reference 1fc2d0e4c4

refactor: `SignTransactionResultToJSON` return `UniValue` instead of mutating reference 6b8db15a03

refactor: `TxInErrorToJSON` return `UniValue` instead of mutating reference 9a5c986b1b

refactor: `WalletTxToJSON` return `UniValue` instead of mutating reference ea6ef02393

refactor: `ProcessSubScript` return `UniValue` instead of mutating reference 370da170b9

refactor: `GetWalletBalances` return `UniValue` instead of mutating reference a27abe973d

refactor: `ParseGetInfoResult` return a value instead of mutating `UniValue` reference 908d60973d

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

mjdietzx force-pushed on Oct 3, 2022

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.

DrahtBot removed the label Needs rebase on Oct 3, 2022

DrahtBot added the label Needs rebase on Dec 8, 2022

DrahtBot commented at 9:55 am on December 8, 2022: contributor

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

DrahtBot commented at 0:24 am on March 8, 2023: contributor

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.

achow101 closed this on Apr 25, 2023

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?

bitcoin locked this on Jun 6, 2024

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

Conflicts

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