In order to load the necessary data for migrating a legacy wallet
without the full LegacyScriptPubKeyMan, move the data storage and
loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now
subclasses that.
There are only a few call sites of these throughout the codebase, so
move the functionality into FastRandomContext, and rewrite all call sites.
This requires the callers to explicit construct FastRandomContext objects,
which do add to the verbosity, but also make potentially apparent locations
where the code can be improved by reusing a FastRandomContext object (see
further commit).
fa6beb8cfc ci: Clear unused /msan/llvm-project (MarcoFalke)
Pull request description:
Could help to fix the `no space left on device` that are sometimes seen.
ACKs for top commit:
theuni:
utACK fa6beb8cfc
Tree-SHA512: 0bedf4b26eed842c7bfa2caeac4df578cdbb00a658e8d0037b8b7b90150d8a9d1b8140437d1cf40b50d82a9085ea50cf9a010764c4439b2a03a457d399191319
The existing code uses GetRand(nMax), with a default value for nMax, where nMax is the
range of values (not the maximum!) that the output is allowed to take. This will always
miss the last possible value (e.g. GetRand<uint32_t>() will never return 0xffffffff).
Fix this, by moving the functionality largely in RandomMixin, and also adding a
separate RandomMixin::rand function, which returns a value in the entire (non-negative)
range of an integer.
The existing code provides two randomness mechanisms for test purposes:
- g_insecure_rand_ctx (with its wrappers InsecureRand*), which during tests is
initialized using either zeros (SeedRand::ZEROS), or using environment-provided
randomness (SeedRand::SEED).
- g_mock_deterministic_tests, which controls some (but not all) of the normal
randomness output if set, but then makes it extremely predictable (identical
output repeatedly).
Replace this with a single mechanism, which retains the SeedRand modes to control
all randomness. There is a new internal deterministic PRNG inside the random
module, which is used in GetRandBytes() when in test mode, and which is also used
to initialize g_insecure_rand_ctx. This means that during tests, all random numbers
are made deterministic. There is one exception, GetStrongRandBytes(), which even
in test mode still uses the normal PRNG state.
This probably opens the door to removing a lot of the ad-hoc "deterministic" mode
functions littered through the codebase (by simply running relevant tests in
SeedRand::ZEROS mode), but this isn't done yet.
Convert XoRoShiRo128PlusPlus into a full RandomMixin-based RNG class,
providing all utility functionality that FastRandomContext has. In doing so,
it is renamed to InsecureRandomContext, highlighting its non-cryptographic
nature.
To do this, a fillrand fallback is added to RandomMixin (where it is used by
InsecureRandomContext), but FastRandomContext still uses its own fillrand.
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
fa1bc7c88b scripted-diff: Log parameter interaction not thrice (MarcoFalke)
fafb7875e1 doc: Fix outdated dev comment about logging (MarcoFalke)
Pull request description:
Seems a bit overkill to log the words "parameter interaction" thrice, when at least once is enough. So do that.
Before:
```
2024-06-28T15:30:57Z [init.cpp:745] [InitParameterInteraction] InitParameterInteraction: parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0
```
After:
```
2024-06-28T15:47:27Z [init.cpp:745] [InitParameterInteraction] parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0
ACKs for top commit:
paplorinc:
ACK fa1bc7c88b
fjahr:
utACK fa1bc7c88b
TheCharlatan:
Nice, ACK fa1bc7c88b
hodlinator:
utACK fa1bc7c88b
Tree-SHA512: 83cd92e20dffa38737d4fd31764481284383e12671d9e4b33cfa496743c95c10921a113b1da2caafeb44fca3759a28a8e230df5e30c29fb55d5854ff1531382c
5d2fb14baf test: p2p: check that connecting to ourself leads to disconnect (Sebastian Falbesoner)
Pull request description:
This small PR adds test coverage for the scenario of connecting to ourself, leading to an immediate disconnect:
2f6dca4d1c/src/net_processing.cpp (L3729-L3735)
This logic has been first introduced by Satoshi in October 2009, together with a couple of other changes and a version bump to "v0.1.6 BETA" (see commit cc0b4c3b62).
ACKs for top commit:
kevkevinpal:
tACK [5d2fb14](5d2fb14baf)
maflcko:
ACK 5d2fb14baf
fjahr:
tACK 5d2fb14baf
tdb3:
ACK 5d2fb14baf
Tree-SHA512: 30fb8c82cef94701affeca386ecd59daa32231635fa770fe225feb69fdab2ffedbfa157edd563f65099ec209f2dafffc1154f7f9292c2ea68bbd114750904875
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>
Otherwise, if the background tip is not an ancestor of the snapshot, blocks in between that ancestor up to the height of the background tip will never be requested.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com>
The "connect to ourself" detection logic has been first introduced
by Satoshi in October 2009, together with a couple of other changes
and a version bump to "v0.1.6 BETA" (see commit
cc0b4c3b62).
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
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
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.
- This change ensures consistency in transaction size and weight calculation
within the wallet and prevents conversion overflow when calculating
`max_selection_weight`.
`CoinGrinder` will also produce change output, listing all the
Coin selection algorithms that produces change output is not maintainable,
just infer that remaining algorithms all might produce change.
