8c47d599b8 doc: improve -debuglogfile help to be a bit clearer (jonatack)
20d89d6802 bench: document expected results in logging benchmarks (jonatack)
d8deba8c36 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack)
102b203349 bench: order the logging benchmark code by output (Jon Atack)
4b3fdbf6fe bench: update logging benchmark naming for clarity (Jon Atack)
4684aa8733 bench: allow logging benchmarks to be order-independent (Larry Ruane)
Pull request description:
Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697.
- make the logging benchmarks order-independent (Larry Ruane)
- add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks
- update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output
- add clarifying documentation to the logging benchmarks
- improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'`
Reviewers can run the logging benchmarks with:
```bash
./src/bench/bench_bitcoin -filter='LogP*.*'
```
ACKs for top commit:
LarryRuane:
ACK 8c47d599b8
martinus:
code review & tested ACK 8c47d599b8, here are my benchmark results:
achow101:
ACK 8c47d599b8
Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
and clarify the intention behind the -nodebuglogfile bench.
Co-authored-by: "kouloumos <kouloumosa@gmail.com>"
Co-authored-by: "Larry Ruane <larryruane@gmail.com>"
The global logging object instance is not re-created for each run, so when
multiple logging benchmarks are run, each one after the first one still has
the logging categories enabled from the previous ones. This commit disables
all categories at the start of each benchmark.
Optimizes coin selection by performing the "group outputs"
procedure only once, outside the "attempt selection" process.
Avoiding the repeated execution of the 'GroupOutputs' operation
that occurs on each coin eligibility filters (up to 8 of them);
then for every coin vector type plus one for all the coins together.
This also let us not perform coin selection over coin eligibility
filtered groups that don't add new elements.
(because, if the previous round failed, and the subsequent one has
the same coins, then this new round will fail again).
And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.
We can know before calling `Insert` whether the coin
will be accepted or not.
52f4d567d6 refactor: remove <util/system.h> include from wallet.h (furszy)
6c9b342c30 refactor: wallet, remove global 'ArgsManager' access (furszy)
d8f5fc4462 wallet: set '-walletnotify' script instead of access global args manager (furszy)
3477a28dd3 wallet: set keypool_size instead of access global args manager (furszy)
Pull request description:
Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object.
So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member.
Extra note:
In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including `util/system.h`.
ACKs for top commit:
achow101:
ACK 52f4d567d6
TheCharlatan:
Re-ACK 52f4d567d6
hebasto:
re-ACK 52f4d567d6
Tree-SHA512: 0cffd99b4dd4864bf618aa45aeaabbef2b6441d27b6dbb03489c4e013330877682ff17b418d07aa25fbe1040bdf2c67d7559bdeb84128c5437bf0e6247719016
511aa4f1c7 Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d25f7 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8bbda Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713961 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a02e Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f75763 Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece67b Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27841 Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff72476a Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf40213 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa0a6 Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)
Pull request description:
This is an alternative to #25354 (by my benchmarking, somewhat faster), subsumes #25712, and adds additional test vectors.
It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.
I thought about rebasing #25712 on top of this, but the changes before are fairly extensive, so redid it instead.
ACKs for top commit:
ajtowns:
ut reACK 511aa4f1c7
dhruv:
tACK crACK 511aa4f1c7
Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
Nothing has changed that would affect Bitcoin's usage of nanobench. Here is a detailed list of the changes
* Plenty of clang-tidy updates
* documentation updates
* faster Rng::shuffle
* Enable perf counters on older kernels
* Raise default minimum epoch time to 1ms (doesn't effect bitcoin's usage)
* Add support for custom information per benchmark
To directly return a CRIPEMD160 hash from data.
Incidentally, decoding this acronym:
* RIPEMD -> RIPE Message Digest
* RIPE -> RACE Integrity Primitives Evaluation
* RACE -> Research and Development in Advanced Communications Technologies in Europe
282019cd3d refactor: add kernel/cs_main.* (fanquake)
Pull request description:
One place to find / include `cs_main`.
No more:
> // Actually declared in validation.cpp; can't include because of circular dependency.
> extern RecursiveMutex cs_main;
Ultimately, no more need to include `validation.h` (which also includes (heavy/boost filled) `txmempool.h`) everywhere for `cs_main`. See #26087 for another example of why that is useful.
ACKs for top commit:
ajtowns:
ACK 282019cd3d
Tree-SHA512: 142835b794873e7a09c3246d6101843ae81ec0c6295e6873130c98a2abfa5f7282748d0f1a37237a779cc71c3bc0a75d03b20313ef5398c83d4814215cbc8287
2022917223 Add secp256k1_selftest call (Pieter Wuille)
3bfca788b0 Remove explicit enabling of default modules (Pieter Wuille)
4462cb0498 Adapt to libsecp256k1 API changes (Pieter Wuille)
9d47e7b71b Squashed 'src/secp256k1/' changes from 44c2452fd3..21ffe4b22a (Pieter Wuille)
Pull request description:
Now that libsecp256k1 has a release (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-December/021271.html), update the subtree to match it.
The changes themselves are not very impactful for Bitcoin Core, but include:
* It's no longer needed to specify whether contexts are for signing or verification or both (all contexts support everything), so make use of that in this PR.
* Verification operations can use the static context now, removing the need for some infrastructure in pubkey.cpp to make sure a context exists.
* Most modules are now enabled by default, so we can drop explicit enabling for them.
* CI improvements (in particular, MSVC and more recent MacOS)
* Introduction of an internal int128 type, which has no effect for GCC/Clang builds, but enables 128-bit multiplication in MSVC, giving a ~20% speedup there (but still slower than GCC/Clang).
* Release process changes (process documentation, changelog, ...).
ACKs for top commit:
Sjors:
ACK 2022917223, but 4462cb0498 could use more eyes on it.
achow101:
ACK 2022917223
jonasnick:
utACK 2022917223
Tree-SHA512: 8a9fe28852abe74abd6f96fef16a94d5a427b1d99bff4caab1699014d24698aab9b966a5364a46ed1001c07a7c1d825154ed4e6557c7decce952b77330a8616b
04528054fc [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb6 [miner] allow bypassing TestBlockValidity (glozow)
c058852308 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)
Pull request description:
Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.
ACKs for top commit:
kevkevinpal:
Tested ACK [0452805](04528054fc)
achow101:
ACK 04528054fc
stickies-v:
ACK 04528054f
Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5
f2fc03ec85 refactor: use braced init for integer constants instead of c style casts (Pasta)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/23810 for more context. This is broken out from that PR, as it is less breaking, and should be trivial to review and merge.
EDIT: Long term, the intention is to remove all C-style casts, as they can dangerously introduce reinterpret_casts. This is one step which removes a number of trivially removable C-style casts
ACKs for top commit:
aureleoules:
ACK f2fc03ec85
Tree-SHA512: 2fd11b92c9147e3f970ec3e130e3b3dce70e707ff02950a8c697d4b111ddcbbfa16915393db20cfc8f384bc76f13241c9b994a187987fcecd16a61f8cc0af14c
3a4f8bc242 bench: add benchmark for wallet 'AvailableCoins' function. (furszy)
Pull request description:
#### Rationale
`AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog.
As we are improving this process in #24699, #25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes).
#### Implementation Notes
There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types.
The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits.
ACKs for top commit:
achow101:
ACK 3a4f8bc242
hernanmarino:
ACK 3a4f8bc242
aureleoules:
ACK 3a4f8bc242
Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b
f1e89597c8 test: Drop no longer required bench output redirection (Hennadii Stepanov)
4dbcdf26a3 bench: Suppress output when running with `-sanity-check` option (Hennadii Stepanov)
Pull request description:
This change allows to simplify CI tests, and makes it easier to integrate the `bench_bitcoin` binary into CMake custom [targets](https://cmake.org/cmake/help/latest/command/add_custom_target.html) or [commands](https://cmake.org/cmake/help/latest/command/add_custom_command.html), as `COMMAND` does not support output redirection.
ACKs for top commit:
aureleoules:
tACK f1e89597c8. Ran as expected and is more practical than using an output redirection.
Tree-SHA512: 29086d428cccedcfd031c0b4514213cbc1670e35f955e8fd35cee212bc6f9616cf9f20d0cb984495390c4ae2c50788ace616aea907d44e0d6a905b9dda1685d8
The current BlockAssembler bench only tests on a mempool where all
transactions have 0 ancestors or descendants, which does not exercise
any of the package-handling logic in BlockAssembler
* Use SECP256K1_CONTEXT_NONE when creating signing context, as
SECP256K1_CONTEXT_SIGN is deprecated and unnecessary.
* Use secp256k1_static_context where applicable.
38941a703e refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov)
Pull request description:
This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360:
> why not move it to the right place, that is to `kernel/txmempool_entry.h`?
ACKs for top commit:
MarcoFalke:
review ACK 38941a703e📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
13d9760829 test: load wallet, coverage for crypted keys (furszy)
373c99633e refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85 refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e8121 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy)
Pull request description:
At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.
Note:
The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase`
can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading
process.
Includes test coverage for the following scenarios:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
ACKs for top commit:
achow101:
ACK 13d9760829
aureleoules:
ACK 13d9760829
Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b
files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
fa84df1f03 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy)
61c2265629 wallet: group AvailableCoins filtering parameters in a single struct (furszy)
f0f6a3577b RPC: listunspent, add "include immature coinbase" flag (furszy)
Pull request description:
Simple PR; adds a "include_immature_coinbase" flag to `listunspent` to include the immature coinbase UTXOs on the response. Requested by #25728.
ACKs for top commit:
danielabrozzoni:
reACK fa84df1f03
achow101:
ACK fa84df1f03
aureleoules:
reACK fa84df1f03
kouloumos:
reACK fa84df1f03
theStack:
Code-review ACK fa84df1f03
Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
db929893ef Faster -reindex by initially deserializing only headers (Larry Ruane)
c72de9990a util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane)
48a68908ba Add LoadExternalBlockFile() benchmark (Larry Ruane)
Pull request description:
### Background
During the first part of reindexing, `LoadExternalBlockFile()` sequentially reads raw blocks from the `blocks/blk00nnn.dat` files (rather than receiving them from peers, as with initial block download) and eventually adds all of them to the block index. When an individual block is initially read, it can't be immediately added unless all its ancestors have been added, which is rare (only about 8% of the time), because the blocks are not sorted by height. When the block can't be immediately added to the block index, its disk location is saved in a map so it can be added later. When its parent is later added to the block index, `LoadExternalBlockFile()` reads and deserializes the block from disk a second time and adds it to the block index. Most blocks (92%) get deserialized twice.
### This PR
During the initial read, it's rarely useful to deserialize the entire block; only the header is needed to determine if the block can be added to the block index immediately. This change to `LoadExternalBlockFile()` initially deserializes only a block's header, then deserializes the entire block only if it can be added immediately. This reduces reindex time on mainnet by 7 hours on a Raspberry Pi, which translates to around a 25% reduction in the first part of reindexing (adding blocks to the index), and about a 6% reduction in overall reindex time.
Summary: The performance gain is the result of deserializing each block only once, except its header which is deserialized twice, but the header is only 80 bytes.
ACKs for top commit:
andrewtoth:
ACK db929893ef
achow101:
ACK db929893ef
aureleoules:
ACK db929893ef - minor changes and new benchmark since last review
theStack:
re-ACK db929893ef
stickies-v:
re-ACK db929893e
Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
This change allows to simplify CI tests, and makes it easier to
integrate the `bench_bitcoin` binary into CMake custom targets or
commands, as `COMMAND` does not support output redirection
Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.
-----------------------
Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.
Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.
Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.
