The scope of the lock should be limited to just guarding m_warnings as
anything listening on `NotifyAlertChanged` may execute code that
requires the lock as well.
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>
Add the possibility of giving more guidance to the creation of the
metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes
successfully creates a valid snapshot.
This also changes the asserts for the success case that were outdated,
and only didn't result in a crash because the fuzzer wasn't able
to reach this code before.
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.
f59e9057e2 depends: switch libevent to CMake (Cory Fields)
Pull request description:
Switches libevent in depends to be built with CMake.
ACKs for top commit:
TheCharlatan:
ACK f59e9057e2
willcl-ark:
ACK f59e9057e2
Tree-SHA512: 875bf9bc57653c78775a1f8192a2c964fea8f4490d733ff796d9efb00e786f0ca9a7c1a3fd610cda032273c4f2ae06394585b03567d5f241ab073c83a47cf927
33c48c106c validation: Check if mempool exists before asserting in ActivateSnapshot (TheCharlatan)
Pull request description:
The mempool is an optional component of the chainstate manager, so don't assume its presence and instead check if it is there first.
ACKs for top commit:
maflcko:
re-ACK 33c48c106c
fjahr:
ACK 33c48c106c
Tree-SHA512: 7a3568d5b7af45efa7bf54bae7bac1f00dc99bc9d47a744d73594f283c952be9500168f680d72f4aee09761da4e878ddca83ba675cdea8ee9e44eeff00ac09da
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
dea7afd5e4 lint: remove unneeded trailing line fix (willcl-ark)
4d942547a8 lint: ignore files ignored by git in mlc (willcl-ark)
Pull request description:
Updating to MLC v0.18.0 includes a new feature which will ignore all files ignored by git: `mlc --gitignore`.
This helps avoid false-positives flagged by this linter in non-project files, such as a developer might expect to have in their working directory (e.g. guix-builds, python venvs, etc.)
Top commit has no ACKs.
Tree-SHA512: 1752448e0c85abd3c73570a17cc69294de2248d7773c6499833ae33806f6c03f3f345261aa7b855a557b45982fbdcb8190e758d087c43b4fb0254fbb39173432
Updating to MLC v0.18.0 includes a new feature which will ignore all
files ignored by git: `--gitignore`.
This helps avoid false-positives flagged by this linter in non-project
files, such as a developer might expect to have in their directory (e.g.
guix-builds, python venvs, etc.)
2342b46c45 test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9 rpc: Avoid getchaintxstats invalid results (MarcoFalke)
Pull request description:
The `getchaintxstats` RPC reply during AU background download may return non-zero, but invalid, values for `window_tx_count` and `txrate`.
For example, `txcount` may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).
Also, `txcount` may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.
Fix all issues by skipping the returned value if either `txcount` is unset (equal to zero).
Also, skip `txcount` in the returned value, if it is unset (equal to zero).
Fixes https://github.com/bitcoin/bitcoin/issues/29328
ACKs for top commit:
fjahr:
re-ACK 2342b46c45
achow101:
ACK 2342b46c45
mzumsande:
ACK 2342b46c45
Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
926b8e39dc [doc] add release note for TRUC (glozow)
19a9b90617 use version=3 instead of v3 in debug strings (glozow)
881fac8e60 scripted-diff: change names from V3 to TRUC (glozow)
a573dd2617 [doc] replace mentions of v3 with TRUC (glozow)
089b5757df rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89 rename policy/v3_policy.* to policy/truc_policy.* (glozow)
Pull request description:
Adds a release note for TRUC policy which will be live in v28.0.
For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904
I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.
ACKs for top commit:
instagibbs:
reACK 926b8e39dc
achow101:
ACK 926b8e39dc
ismaelsadeeq:
Code review ACK 926b8e39dc
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f4 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407e assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c0118 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)
Pull request description:
This was discovered in a discussion in #29996
If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.
While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.
The behavior change described above is in the second commit.
The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.
The third commit removes an unnecessary restart introduced in #29428.
ACKs for top commit:
mzumsande:
re-ACK 2f9bde6
alfonsoromanz:
Re-ACK 2f9bde69f4. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
achow101:
ACK 2f9bde69f4
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
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
8ec24bdad8 test: Added coverage to Block not found error using gettxoutsetinfo (kevkevinpal)
Pull request description:
#### Description
There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.
You can see there are no tests that do the following by doing the below
`grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/`
which leads to no results
ACKs for top commit:
achow101:
ACK 8ec24bdad8
tdb3:
ACK 8ec24bdad8
kristapsk:
ACK 8ec24bdad8
brunoerg:
crACK 8ec24bdad8
alfonsoromanz:
Re ACK 8ec24bdad8
Tree-SHA512: 2c61c681e7304c679cc3d7dd13af1b795780e85716c25c7423d68104e253d01271e048e21bc21be35dbc7ec1a4fde94e439542f3cfd669fe5a16478c5fa982ab
e38eadb2c2 test: change comments to `self.log.info` for `test_addnode_getaddednodeinfo` (brunoerg)
c838e3b610 test: add coverage for `node` field of `getaddednodeinfo` RPC (brunoerg)
Pull request description:
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.
ACKs for top commit:
kevkevinpal:
ACK [e38eadb](e38eadb2c2)
achow101:
ACK e38eadb2c2
tdb3:
re ACK e38eadb2c2
BrandonOdiwuor:
Code Review ACK e38eadb2c2
rkrux:
tACK [e38eadb](e38eadb2c2)
Tree-SHA512: e9f768b7aa86e58b0b0ced089ead57040ff9a5204493da1ab99c8bc897b6dcdce7c856855f74c52010fceef19af1e12a39eee9f8f2e7294b42476b6f980fe754
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
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
9ec2c53701 Revert "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
Pull request description:
As suggested in https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200786670, this PR reverts the recently added test #30362 that causes frequent CI failures. A TODO is added in the functional test file to re-add it later when the race condition is fixed.
ACKs for top commit:
mzumsande:
utACK 9ec2c53701
brunoerg:
utACK 9ec2c53701
tdb3:
ACK 9ec2c53701
Tree-SHA512: df211ab194dc47f2ff8192f3827382974db922ed9fa54bc44fac75de4edfb3af43c1340cd5434b15b0b573f7b0ddd4451a0bbbbd7deaf7f4244e4865b9d5977e
The migration process reloads the wallet after all failures.
This commit tests the behavior by trying to obtain a new address
after a decryption failure during migration.
In SetupLegacyScriptPubKeyMan, a base LegacyDataSPKM will be created if
the database has the format "bdb_ro" (i.e. the wallet was opened only
for migration purposes).
All of the loading functions are now called with a LegacyDataSPKM object
instead of LegacyScriptPubKeyMan.
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.
