In many cases, it is known at compile time how many bits are requested from
randbits. Provide a variant of randbits that accepts this number as a template,
to make sure the compiler can make use of this knowledge. This is used immediately
in rand32() and randbool(), and a few further call sites.
The previous randbits code would, when requesting more randomness than available
in its random bits buffer, discard the remaining entropy and generate new.
Benchmarks show that it's usually better to first consume the existing randomness
and only then generate new ones. This adds some complexity to randbits, but it
doesn't weigh up against the reduced need to generate more randomness.
Rather than make all the useful types of randomness be exclusive to
FastRandomContext, move it to a separate RandomMixin class where it can be reused by
other RNGs.
A Curiously Recurring Template Pattern (CRTP) is used for this, to provide the ability
for individual RNG classes to override one or more randomness functions, without
needing the runtime-cost of virtual classes.
Specifically, RNGs are expected to only provide fillrand and rand64, while all others
are derived from those:
- randbits
- randrange
- randbytes
- rand32
- rand256
- randbool
- rand_uniform_delay
- rand_uniform_duration
- min(), max(), operator()(), to comply with C++ URBG concept.
55eea003af test: Make blockencodings_tests deterministic (AngusP)
4c99301220 test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)
Pull request description:
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.
This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
ACKs for top commit:
marcofleon:
Code review ACK 55eea003af. I ran the `blockencodings` unit test and no issues with the new test case.
dergoegge:
Code review ACK 55eea003af
glozow:
ACK 55eea003af
Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
e009bf681c Don't use iterator addresses in IteratorComparator (dergoegge)
Pull request description:
See #29018.
Stability for `txorphan` is now >90%. `mini_miner` needs further investigation, stability still low (although slightly improved by this PR) at ~62%.
ACKs for top commit:
marcofleon:
Tested ACK e009bf681c. Using afl++, stability for `txorphan` went from 82% to ~94% and for `mini_miner` it went from 84% to 97%. I ran them both using the corpora in qa-assets.
glozow:
utACK e009bf681c
Tree-SHA512: 6d0a20fd7ceedca8e702d8adde5fca500d8b0187147aee8d43b4e9eb5176dcacf60180f42a7158f037d18dbb27e479b6c069a0f3c912226505cbff5aa073a415
4d81b4de33 fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75ea97 fuzz: simplify FuzzedSock::m_peek_data (Vasil Dimov)
Pull request description:
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
---
This is a followup to https://github.com/bitcoin/bitcoin/pull/30211, more specifically:
https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366
ACKs for top commit:
marcofleon:
ACK 4d81b4de33. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of `Recv` in `FuzzedSock`.
dergoegge:
Code review ACK 4d81b4de33
brunoerg:
utACK 4d81b4de33
Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd
AddToBlock was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
Since its behavior was changed in 400b151, I've named the variable accordingly.
This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a measurable speed increase for many iterations.
> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=1000
before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 155,558.97 | 6,428.43 | 0.1% | 1.10 | `AssembleBlock`
after:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 148,083.68 | 6,752.94 | 0.1% | 1.10 | `AssembleBlock`
Co-authored-by: furszy <mfurszy@protonmail.com>
a74b0f93ef Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db44d refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b70489 Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce7637ad refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef0e2 Add missing include for mining interface (Sjors Provoost)
Pull request description:
Followups from #30200
Fixes:
- `std::unique_ptr` needs `#include <memory>` (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)
- Drop lock from createNewBlock that was spuriously added
- Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)
Refactor:
- Use CHECK_NONFATAL to avoid single-use symbol (refactor)
- move output argument `state` to the end of `testBlockValidity`, see https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176
ACKs for top commit:
AngusP:
Code Review ACK a74b0f93ef
itornaza:
Tested ACK a74b0f93ef
ryanofsky:
Code review ACK a74b0f93ef. Just new error string is added since last review, and a commit message was updated
Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)
Pull request description:
This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.
Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.
Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.
ACKs for top commit:
achow101:
ACK 73f0a6cbd0
ryanofsky:
Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
tdb3:
cr ACK 73f0a6cbd0
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
a9c7300135 move-only: refactor CreateTransactionInternal (josibake)
adc6ab25bb wallet: use CRecipient instead of CTxOut (josibake)
Pull request description:
Broken out from #28201
---
In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to:
* Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see 797e21c8c1)
* Use the new `GetSerializeSizeForRecipient` to move the serialize size calculation to *before* coin selection and the output creation to *after* coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected
Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like `git diff -w --color-moved=dimmed-zebra`
ACKs for top commit:
S3RK:
reACK a9c7300135
achow101:
ACK a9c7300135
rkrux:
tACK [a9c7300](a9c7300135)
Tree-SHA512: 412e1764b98f7428c8530c3a68f55e32063d6b66ab2ff613e1c7e12d49b049807cb60055cfe7f7e8ffe7ac7f0f9931427cbfd3efe7d4f97a5a0f6d1bf1aaac58
PermsToSymbolicString will convert from fs::perms to string type
'rwxrwxrwx'.
InterpretPermString will convert from a user-supplied "perm string" such
as 'owner', 'group' or 'all, into appropriate fs::perms.
The goal of interfaces is to eventually run in their own process,
so we can't use EXCLUSIVE_LOCKS_REQUIRED in their declaration.
However TestBlockValidaty will crash (in its call to ConnectBlock)
if the tip changes from under the proposed block.
Have the testBlockValidity implementation hold the lock instead,
and non-fatally check for this condition.
7d3662fbe3 i2p: fix log when an interruption happens during `Accept` (brunoerg)
3d3a83fab2 i2p: log errors properly according to their severity (brunoerg)
Pull request description:
This PR improves and fixes i2p logs (joint work with vasild).
- It replaces `LogPrint` to `LogPrintLevel` so we can log according to the severity.
- Fix log when interruption happens during `Accept`. Before this PR, when an interruption happens, it just logs "Error accepting:", no reason is logged as it does for other situations. This PR changes it to log "Accept interrupted".
- Log errors according to the severity. Stuff like creating SAM session, destroying SAM session, etc... are logged as 'debug'.
ACKs for top commit:
achow101:
ACK 7d3662fbe3
marcofleon:
ACK 7d3662fbe3.
vasild:
ACK 7d3662fbe3
Tree-SHA512: 1c3d92108dbc22833f37a78e18b4efd723433d10f28166d17c74eab884cd97e908b4e0a0908fd16288df895eb2eb480f781de37b2ec6a6d414abfb71e0c86fe2
72b226882f wallet: notify when preset + automatic inputs exceed max weight (furszy)
Pull request description:
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
ACKs for top commit:
achow101:
ACK 72b226882f
tdb3:
re ACK for 72b226882f
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882f
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
2721d64989 chainparams: Add achow101 DNS seeder (Ava Chow)
Pull request description:
I wrote a [DNS seeder](https://github.com/achow101/dnsseedrs) and have been running it for the past 2 months now. I believe it is ready/good enough to be used as an additional DNS seeder for all of our supported public networks.
ACKs for top commit:
laanwj:
ACK 2721d64989
1440000bytes:
~~reACK 2721d64989~~
mzumsande:
ACK 2721d64989
willcl-ark:
reACK 2721d64989
Tree-SHA512: 857a6cf7dd33962f0008a89db4d6b57d3c6aa622704cdcca6ab710babeead3a2970d9a6fa190949c7bbf7cb7d006e814d6314be3d8c8180eed29013c7c1ac7e1
a9716c53f0 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834f rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ce rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e7 rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f971 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436 rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5a rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb681678 Introduce Mining interface (Sjors Provoost)
Pull request description:
Introduce a `Mining` interface for the `getblocktemplate`, `generateblock` and other mining RPCs to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
ACKs for top commit:
tdb3:
re ACK a9716c53f0
itornaza:
Code review and std-tests ACK a9716c53f0
ryanofsky:
Code review ACK a9716c53f0 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
e3dc64f499 build: add -Wundef (fanquake)
82b43955f7 refactor: use #ifdef HAVE_SOCKADDR_UN (fanquake)
40cd7585a0 randomenv: use ifdef over if (fanquake)
7839503b30 zmq: use #ifdef ENABLE_ZMQ (fanquake)
79e197b175 build: Suppress warnings from boost and capnproto in multiprocess code (Ryan Ofsky)
Pull request description:
Turn on `-Wundef`.
[> Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero.](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wundef).
Note that this is still beneficial with CMake, and may even be nice to have enabled prior, to catch any change in behaviour.
If we end up with this enabled, it should probably be enough to fix #16419.
ACKs for top commit:
hebasto:
ACK e3dc64f499, I have reviewed the code and it looks OK.
Tree-SHA512: 73436ead07f3a09ba0d30f7105df50d9b2ec8452f11e866bc1c7ebc10c005772ee77fedaa125f444175663c04dfc472f98c2699c63711da356089b66a8cc3e0a
randomenv.cpp:48:5: warning: 'HAVE_VM_VM_PARAM_H' is not defined, evaluates to 0 [-Wundef]
randomenv.cpp:51:5: warning: 'HAVE_SYS_RESOURCES_H' is not defined, evaluates to 0 [-Wundef]
randomenv.cpp:424:5: error: 'HAVE_SYSCTL' is not defined, evaluates to 0 [-Werror,-Wundef]
Without this change there are errors from boost like:
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/expired_slot.hpp:23:28: error: 'what' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/detail/signal_template.hpp:750:32: error: 'lock_pimpl' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/connection.hpp:150:22: error: 'connected' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
There do not seem to be errors from capnproto currently, but add a suppression
for it, too, to be consistent with other libraries.
1245d1388b netbase: extend CreateSock() to support creating arbitrary sockets (Vasil Dimov)
Pull request description:
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place.
The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102
ACKs for top commit:
achow101:
ACK 1245d1388b
tdb3:
re ACK 1245d1388b
theStack:
re-ACK 1245d1388b
Tree-SHA512: cc86b56121293ac98959aed0ed77812d20702ed7029b5a043586f46e74295779c5354bb0d5f9e80be6c29e535df980d34c1dbf609064fb7ea3e5ca0f0ed54d6b
