refactor: Avoid UniValue copies #25429

UniValue copies have been a source of performance related bugs and regressions, so it would be good to remove the copy constructors.

This is the first change toward the goal. This change uses a const UniValue& where possible.

See also:

• https://github.com/bitcoin-core/univalue-subtree/pull/27
• https://github.com/bitcoin/bitcoin/pull/15974

Review note: const will compile, but kill performance when std::move or RVO is intended.

Concept ACK

cc @martinus

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26988 ([rpc]: Add test-only RPC addrmaninfo for new/tried table address count by stratospher)
• #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
• #26780 (rpc: simplify scan blocks by furszy)
• #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
• #26612 (refactor: RPC: pass named argument value as string_view by stickies-v)
• #26426 (WIP: Fix coinstatsindex overflow issue by fjahr)
• #25974 (test, build: Separate read_json function into its own module by hebasto)
• #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
• #25344 (New outputs argument for bumpfee/psbtbumpfee by rodentrabies)
• #15294 (refactor: Extract RipeMd160 by Empact)

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.

Nice! concept ACK. How about removing the copy constructor and copy assignment from the UniValue class, as done here? Fixing all copies would make the PR much bigger though.

I am actually not sure how to fix all copies. Sometimes the std lib will copy if asked to:

std::vector<UniValue> a;
std::vector<UniValue> b;
a.insert(a.end(), b.begin(), b.end());

I guess you could add an explicit 'clone' method (like rust) and use that when it's really necessary to make a copy. It will probably make some things much more verbose though.

a.insert(a.end(), b.begin(), b.end());

In that case you can use

a.insert(a.end(), std::make_move_iterator(b.begin()), std::make_move_iterator(b.end()));

But I there might still be case where copy constructor is necessary, I remember that I had difficulties in my old PR as well

Concept ACK

ACK bbbbcfdb8e87f3151229ebc8c7b812a420d89b14 - as a first step.

While the changes are correct, I'll try to make them easier to review and more complete. For now, please review actual UniValue-copy bugfixes, like https://github.com/bitcoin/bitcoin/pull/25464

Concept ACK.

<!--cf906140f33d8803c4a75a2196329ecb-->

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