Update univalue subtree #17324

Only change is a performance improvement. See https://github.com/bitcoin-core/univalue/pull/21#issue-333858474 and https://github.com/bitcoin-core/univalue/pull/15#issue-186748173

The last update was more than a year ago: #14164#issue-213911287

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

ACK fa439e88af944082875e1fdb1cd8bb5a74b88b56

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Can you specify which commit or line of code this refers to? Bitcoin Core is a major user of univalue, and we want to avoid a situation where Bitcoin's univalue diverges from the Debian/Ubuntu univalue.

Bench

Bench config

Bitcoinperf commit e671f5d5dfd49724af3b234ebfd1076345822e92 OS: Ubuntu Eoan YML:

---

# Cache a built bitcoin src directory and restore it from the cache on
# subsequent runs.
cache_build: false

# If true, the first git clone will be cached and copied from as necessary.
cache_git: true

# Set to false to make cache dropping optional and bypass various safety checks.
safety_checks: false

compilers:
  - clang
  - gcc

benches:
  build:
    num_jobs: 9

  microbench:
    filter: '.*Json.*'

to_bench:

  - gitref: 'fa439e88af944082875e1fdb1cd8bb5a74b88b56'

  - gitref: 'master'

<!--a722867cd34abeea1fadc8d60700f111-->

Gitian builds

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Ok. Thanks for the explanation It's useful to document why we're not taking certain changes from upstream.

ACK fa439e88af944082875e1fdb1cd8bb5a74b88b56

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Ok. Thanks for the explanation It's useful to document why we're not taking certain changes from upstream.

It would be better for the trees not to diverge. Will look at removing "clumsy goto" related code, and work to eliminate this code-divergence issue.

The so-called "clumsy goto" in this case looks like pretty reasonable use of goto...

We don't use goto anywhere else in the code base and this case can be replaced with a one line helper function that clears the Univalue and returns false.

Also, while it has definite uses in C, the use of goto is generally unnecessary in modern C++.

Speedup for gcc: https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=micro.gcc.RpcMempool&env=1&revs=50&equid=off&quarts=on&extr=on and: https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=micro.gcc.BlockToJsonVerbose&env=1&revs=50&equid=off&quarts=on&extr=on

No speedup for clang: https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=micro.clang.RpcMempool&env=1&revs=50&equid=off&quarts=on&extr=on and: https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=micro.clang.BlockToJsonVerbose&env=1&revs=50&equid=off&quarts=on&extr=on