31cc5006c3 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e833 init: Improve chainstate init db error messages (TheCharlatan)
cd093049dd init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f85d7 init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce880a3 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842ff1 init: Remove unneeded argument for mempool_opts checks (stickies-v)
Pull request description:
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
ACKs for top commit:
achow101:
ACK 31cc5006c3
mzumsande:
Code Review / lightly tested ACK 31cc5006c3
BrandonOdiwuor:
Code Review ACK 31cc5006c3.
stickies-v:
ACK 31cc5006c3
Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b
33381ea530 scripted-diff: Modernize nLocalServices to m_local_services (Fabian Jahr)
Pull request description:
The type of the `nLocalServices` variable was changed to `std::atomic<ServiceFlags>` in #30807 and I suggested the variable name to get updated with a scripted diff along with it. It wasn't included in the PR but I am still suggesting to do it as a follow-up since I had already prepared the commit.
ACKs for top commit:
sipa:
utACK 33381ea530
achow101:
ACK 33381ea530
furszy:
utACK 33381ea530
jonatack:
ACK 33381ea530
theStack:
ACK 33381ea530
Tree-SHA512: 407ea9eac694f079aa5b5c1611b5874d7a0897ba6bc3aa0570be94afe1bf3a826657b6890b6597c03c063e95b9dc868f0bdfbfc41e77ec7e06f5b045bf065c71
It is bad, because it is both printed for non-GUI users and does not
convey additional information.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
e9d60af988 refactor: Replace init retry for loop with if statement (TheCharlatan)
c1d8870ea4 refactor: Move most of init retry for loop to a function (TheCharlatan)
781c01f580 init: Check mempool arguments in AppInitParameterInteractions (TheCharlatan)
Pull request description:
The for loop around the chain loading logic in `init.cpp` allows users of the GUI to retry once on failure with reindexing without having to manually set the reindex flag on startup. However this current mechanism has problems:
* It is badly documented and has led to confusion among developers and bugs making it into master. Examples:
* https://github.com/bitcoin/bitcoin/pull/28830/files#r1598392660
* https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121
* It can only ever iterate once, making the choice of a for loop questionable.
* With its large scope it is easy for re-entry bugs to sneak in. Example:
* https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601589963
Attempt to fix this by moving the bulk of the logic into a separate function and replacing the for loop with a simpler `if` statement.
The diff's in this pull request are best reviewed with `--color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files.
ACKs for top commit:
maflcko:
review ACK e9d60af988🚸
josibake:
crACK e9d60af988
achow101:
ACK e9d60af988
ryanofsky:
Code review ACK e9d60af988. Nice change to make AppInitMain shorter and more understandable.
Tree-SHA512: 5e5c0a5fd1b32225346450f8482f0ae8792e1557cdab1518112c1a3ec3a4400b64f5796692245cc5bf2f9010bb97b3a9558f07626a285ccd6ae525dd671ead13
5c7cacf649 ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da doc: Remove mention of natpmp build options (laanwj)
061c3e32a2 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa build: Drop libnatpmp from build system (laanwj)
7b04709862 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c6 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cd net: Add PCP and NATPMP implementation (laanwj)
d72df63d16 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b net: Add netif utility (laanwj)
754e425438 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649
achow101:
ACK 5c7cacf649
vasild:
ACK 5c7cacf649
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
The for loop has been a long standing source of confusion and bugs, both
because its purpose is not clearly documented and because the body of
the for loop contains a lot of logic.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
This makes it clearer which state is being mutated by the function and
facilitates getting rid of the for loop in the following commit. Move
creation of the required options into the function too, such that the
function takes fewer arguments and is more self-contained.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
7942951e3f Remove unused g_best_block (Ryan Ofsky)
e3a560ca68 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c Remove unused CRPCSignals (Sjors Provoost)
dca923150e Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121 rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05 Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf160 node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbb refactor: rename BlockKey to BlockRef (Sjors Provoost)
Pull request description:
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
`waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
There's a commit to add (direct) tests for the methods that are about to be refactored:
- `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
- `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`
This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.
The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).
The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.
`RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.
Finally `g_best_block` is also dropped.
ACKs for top commit:
achow101:
ACK 7942951e3f
ryanofsky:
Code review ACK 7942951e3f. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3f
Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
Previously, when an invalid port was specified
in `-rpcbind`, the `SplitHostPort()` return value
in `HTTPBindAddresses()` was ignored and attempt
would be made to bind to the default rpcbind port
(with the host/port treated as a host).
This rearranges port checking code in
`AppInitMain()` to handle the invalid
port before reaching `HTTPBindAddresses()`.
Also adjusts associated functional tests.
Also signal m_tip_block_cv when StopRPC is called, for
consistency with g_best_block_cv. This is handled in
StopRPC instead of OnRPCStopped() because the latter
is deleted in a later commit.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
bb3b980dfd validation: drop maximum -dbcache (Sjors Provoost)
Pull request description:
Due to recent UTXO set growth, the current maximum value for `-dbcache` of 16GB is ~just months away from being~ insufficient (for those who wish to complete IBD with the UTXO set held in RAM).
This drops the limit. It also adds a warning that it's up to users to check that they have enough RAM.
Fixes #28249.
---
A previous version of this PR increased the maximum to 64GB. It also made startup abort if the value provided is too high, rather than quietly round it down. But this didn't get much support.
ACKs for top commit:
achow101:
ACK bb3b980dfd
tdb3:
ACK bb3b980dfd
BenWestgate:
crACK bb3b980dfd.
Tree-SHA512: 8515fff468c2387a0b04bd9523ab1df46d6325738588b7550fabddbb8624817a583d95b95ea246407f9f0ff3e43e760cf7334621bec6af79710176328528a3ef
fc7b507e9a tidy: add clang-tidy `modernize-use-starts-ends-with` check (Roman Zeyde)
Pull request description:
ACKs for top commit:
jonatack:
re-ACK fc7b507e9a only change since my previous ACK is the commit message
achow101:
ACK fc7b507e9a
stickies-v:
ACK fc7b507e9a
hebasto:
ACK fc7b507e9a, I have reviewed the code and it looks OK.
Tree-SHA512: 334e0ff91b9b108a57cdfc12ee53685b792d377e11124c7c394b8f681a8168a8d65a56c7f884555238e65e97e9ad62ede52b79219ce258979e54abdd76721df1
The thread handle is never used by the ChainstateManager, so move it out
and into the node context. Users of the kernel library now no longer
have to manually join the thread when destructing the ChainstateManager.
992f83bb6f test: add coverage for assumeUTXO honest peers disconnection (furszy)
6d5812e5c8 assumeUTXO: fix peers disconnection during sync (furszy)
Pull request description:
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (`NODE_NETWORK`), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness from the AssumeUTXO
node.
This lack of response occurs because nodes ignore `getdata` requests when they do
not have the block data available (further discussion can be found in #30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal `NODE_NETWORK_LIMITED` support. Then, full-node (`NODE_NETWORK`)
support will be re-enabled once the background chain sync is completed.
Thanks mzumsande for a post-#30385 convo too.
Testing notes:
Just cherry-pick the second commit (bb08c22) on master.
It will fail there, due to the IBD node requesting historical blocks to the snapshot
node - which is bad because the snapshot node will ignore the requests and
stall + disconnect after some time.
ACKs for top commit:
achow101:
ACK 992f83bb6f
naumenkogs:
ACK 992f83bb6f
mzumsande:
ACK 992f83bb6f
Tree-SHA512: fef525d1cf3200c2dd89a346be9c82d77f2e28ddaaea1f490a435e180d1a47a371cadea508349777d740ab56e94be536ad8f7d61cc81f6550c58b609b3779ed3
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
d4c7c4009d init: error out if -maxconnections is negative (Sergi Delgado Segura)
c773649481 init: improves file descriptors accounting and docs (Sergi Delgado Segura)
29008a7ff4 init: fixes fd accounting regarding poll/select (Sergi Delgado Segura)
Pull request description:
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixes #18911
ACKs for top commit:
sr-gi:
> ACK [d4c7c40](d4c7c4009d)
naumenkogs:
ACK d4c7c4009d
vasild:
ACK d4c7c4009d
TheCharlatan:
ACK d4c7c4009d
Tree-SHA512: 1524d10c8ad8f354f6ab9c244699adbcdae2dd7aba37de5b8f9e177c629e8a2cce0f6e8117e076dde3a592f5283bd30a4201db96a3c011e335c02d1fde7414bc
Add `-ipcbind` option to `bitcoin-node` to listen on an IPC socket and accept
connections from other processes. In the future, there will be an `-ipcconnect`
option added to `bitcoin-wallet` and `bitcoin-node` to allow wallet and gui
processes to connect to the node and access it.
Example usage:
src/bitcoin-node -regtest -debug -ipcbind=unix
src/bitcoin-wallet -regtest -ipcconnect=unix info
src/bitcoin-gui -regtest -ipcconnect=unix
src/bitcoin-mine -regtest -ipcconnect=unix
The current logic for file descriptor accounting is pretty convoluted and hard
to follow. This is partially caused by the lack of documentation plus non-intuitive
variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain
(as shown in he previous commit).
Redefine some of the constants, plus document what are we accounting for so this can be
extended more easily
Remove FreeBSD workaround to #2695
We are computing our file descriptors limits based on whether we use
poll or select. However, we are taking that into account only partially
(subtracting from fd_max in one case, but from nFD later on). Moreover,
nBind is also only accounted for partially.
Simplify and fix this logic
6bfa26048d testnet: Add timewarp attack prevention for Testnet4 (Fabian Jahr)
0100907ca1 testnet: Add Testnet4 difficulty adjustment rules fix (Fabian Jahr)
74a04f9e7a testnet: Introduce Testnet4 (Fabian Jahr)
Pull request description:
To supplement the [ongoing conceptual discussion about a testnet reset](https://groups.google.com/g/bitcoindev/c/9bL00vRj7OU/m/9yCPo3uUBwAJ) I have drafted a move to v4 including a fix to the difficulty adjustment mechanism, which was part of the motivation that started the discussion.
Conceptual considerations:
- The conceptual discussion about doing a testnet4 or softforking the fix into testnet3 is outside of the scope of this PR and I would ask reviewers to contribute their opinions on this on the ML instead. However, I am happy to adapt this PR to a softfork change on testnet3 if there is consensus for that instead.
- The difficulty adjustment fix suggested here touches the `CalculateNextWorkRequired` function and uses the same logic used in `GetNextWorkRequired` to find the last previous block that was not mined with difficulty 1 under the exceptionf. An alternative fix briefly mentioned on the mailing list by Jameson Lopp would be to "restrict the special testnet minimum difficulty rule so that it can't be triggered on the block right before a difficulty retarget". That would also fix the issue but I find my suggestion here a bit more elegant.
ACKs for top commit:
jsarenik:
tACK 6bfa26048d
achow101:
ACK 6bfa26048d
murchandamus:
tACK 6bfa26048d
Tree-SHA512: 0b8b69a621406a944da5be551b863d065358ba94d85dd3b80d83c412660e230ee93b27316081fbee9b4851cc4ff8585db64c7dfa26cb5148ac835663f2712c3d
bca346a970 net: require P2P binds to succeed (Vasil Dimov)
af552534ab net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov)
9a7e5f4d68 net: don't extra bind for Tor if binds are restricted (Vasil Dimov)
Pull request description:
Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail".
Fixes https://github.com/bitcoin/bitcoin/issues/22726
Fixes https://github.com/bitcoin/bitcoin/issues/22727
ACKs for top commit:
achow101:
ACK bca346a970
cbergqvist:
ACK bca346a970
pinheadmz:
ACK bca346a970
Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603
This avoids situations during a reindex in which shutdown
doesn't finish since SyncWithValidationInterfaceQueue is
called by the load block thread when the scheduler is already stopped.
606a7ab862 kernel: De-globalize signature cache (TheCharlatan)
66d74bfc45 Expose CSignatureCache class in header (TheCharlatan)
021d38822c kernel: De-globalize script execution cache hasher (TheCharlatan)
13a3661aba kernel: De-globalize script execution cache (TheCharlatan)
ab14d1d6a4 validation: Don't error if maxsigcachesize exceeds uint32::max (TheCharlatan)
Pull request description:
The validation caches are currently setup independently from where the rest of the validation code is initialized. This makes their ownership semantics unclear. There is also no clear enforcement on when and in what order they need to be initialized. The caches are always initialized in the `BasicTestingSetup` although a number of tests don't actually need them.
Solve this by moving the caches from global scope into the `ChainstateManager` class. This simplifies the usage of the kernel library by no longer requiring manual setup of the caches prior to using the `ChainstateManager`. Tests that need to access the caches can instantiate them independently.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 606a7ab862
glozow:
reACK 606a7ab
ryanofsky:
Code review ACK 606a7ab862. Just small formatting, include, and static_assert changes since last review.
Tree-SHA512: e7f3ee41406e3b233832bb67dc3a63c4203b5367e5daeed383df9cb590f227fcc62eae31311029c077d5e81b273a37a88a364db3dee2efe91bb3b9c9ddc8a42e
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
Use this opportunity to make SignatureCache RAII styled
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
ce8094246e random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561 random: use LogError for init failure (Pieter Wuille)
97e16f5704 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd6 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966b random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d9 net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999 random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104 random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065 random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b6 xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658 random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde2 random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e random: write rand256() in function of fillrand() (Pieter Wuille)
Pull request description:
This PR contains a number of vaguely-related improvements to the random module.
The specific changes and more detailed rationale is in the commit messages, but the highlights are:
* `XoRoShiRo128PlusPlus` (previously a test-only RNG) moves to random.h and becomes `InsecureRandomContext`, which is even faster than `FastRandomContext` but non-cryptographic. It also gets all helper randomness functions (`randrange`, `fillrand`, ...), making it a lot more succinct to use.
* During tests, **all** randomness is made deterministic (except for `GetStrongRandBytes`) but non-repeating (like `GetRand()` used to be when `g_mock_deterministic_tests` was used), either fixed, or from a random seed (overridden by env var).
* Several infrequently used top-level functions (`GetRandMillis`, `GetRandMicros`, `GetExponentialRand`) are converted into member functions of `FastRandomContext` (and `InsecureRandomContext`).
* `GetRand<T>()` (without argument) can now return the maximum value of the type (previously e.g. `GetRand<uint32_t>()` would never return 0xffffffff).
ACKs for top commit:
achow101:
ACK ce8094246e
maflcko:
re-ACK ce8094246e🐈
hodlinator:
ACK ce8094246e
dergoegge:
utACK ce8094246e
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
323ce30308 Moved the repeated -printpriority fetching out of AddToBlock (Lőrinc)
Pull request description:
`AddToBlock` was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/6fd89647-7b6c-4f44-bd04-98d16cd2a938">
This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a small speed increase for many iterations.
> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=10000
before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 156,460.15 | 6,391.40 | 0.1% | 11.03 | `AssembleBlock`
after:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 149,289.55 | 6,698.39 | 0.3% | 10.97 | `AssembleBlock`
---
The slight speedup shows up in CI as well:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/3be779c9-2dce-4a96-ae5f-cab5435bd72f">
ACKs for top commit:
maflcko:
ACK 323ce30308
achow101:
ACK 323ce30308
tdb3:
re ACK 323ce30308
furszy:
utACK 323ce30308
Tree-SHA512: c2a0aab429646453ad0470956529f1cac8c38778c4c53f82c92c6cbaaaeb69f3d3603c0014ff097844b151e9da7caa2371a4676244caea96527cb540e66825a3
If only `-bind=addr:port` is given (without `-bind=...=onion`) then we
would bind to `addr:port` _and_ to `127.0.0.1:8334` in addition which
may be unexpected, assuming the semantic of `-bind=addr:port` is
"bind _only_ to `addr:port`".
Change the above to not do the additional bind: if only
`-bind=addr:port` is given (without `-bind=...=onion`) then bind to
`addr:port` (only). If we are creating a Tor hidden service then use
`addr:port` as target (same behavior as before
https://github.com/bitcoin/bitcoin/pull/19991).
This allows disabling binding on the onion port.
Fixes https://github.com/bitcoin/bitcoin/issues/22726
f1478c0545 mempool: move LoadMempool/DumpMempool to node (Cory Fields)
6d242ff1e9 kernel: remove mempool_persist.cpp (Cory Fields)
Pull request description:
DumpMempool/LoadMempool are not necessary for the kernel.
Noticed while working on instantiated logging.
I suppose these could have been left in on purpose, but I'm assuming it was probably just an oversight.
ACKs for top commit:
TheCharlatan:
Re-ACK f1478c0545
glozow:
ACK f1478c0545
stickies-v:
ACK f1478c0545
Tree-SHA512: 5825da0cf2e67470524eb6ebe397eb90755a368469a25f184df99ab935b3eb6d89eb802b41a6c3661e869bba3bbfa8ba9d95281bc75ebbf790ec5d9d1f79c66f
