| Age | Commit message (Collapse) | Author |
|---|
|
""_hex literals
8756ccd71218c8e013181473720b10d3c4a94957 scripted-diff: Replace ParseHex[<std::byte>]("str") -> "str"_hex[_u8] (Hodlinator)
9cb687351f7ff50d19b5c5997ed69cfdab75bbf2 refactor: Prepare for ParseHex -> ""_hex scripted-diff (Hodlinator)
50bc017040ae300c795e3709233b80619db24518 refactor: Hand-replace some ParseHex -> ""_hex (Hodlinator)
5b74a849cf5c54543280ba6488ae7f87361b1e2f util: Add consteval ""_hex[_v][_u8] literals (l0rinc)
dc5f6f681275f56ff389500e3dd98fbe791f4a45 test refactor: util_tests - parse_hex clean up (Hodlinator)
2b5e6eff36abe4c23b8789ef1babfafedc90b973 refactor: Make XOnlyPubKey tolerate constexpr std::arrays (Hodlinator)
403d86f1ccf0b73f042d42a9722bb007ba8c7a31 refactor: vector -> span in CCrypter (Hodlinator)
bd0830bbd4105af1953b6b897ba6bc35098cbe13 refactor: de-Hungarianize CCrypter (Hodlinator)
d99c81697148a9695c0fba614dff9fbe728a3acd refactor: Improve CCrypter related lines (Hodlinator)
7e1d9a84689d77a9349a3a09fd5f9dd3f9c293aa refactor: Enforce lowercase hex digits for consteval uint256 (Hodlinator)
Pull request description:
Motivation:
* Validates and converts the hex string into bytes at compile time instead of at runtime like `ParseHex()`.
* Eliminates runtime dependencies: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2214432177, https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592108480
* Has stricter requirements than `ParseHex()` (disallows whitespace and uppercase hex digits) and replaces it in a bunch of places.
* Makes it possible to derive other compile time constants.
* Minor: should shave off a few runtime CPU cycles.
`""_hex` produces `std::array<std::byte>` as the momentum in the codebase is to use `std::byte` over `uint8_t`.
Also makes `uint256` hex string constructor disallow uppercase hex digits. Discussed: https://github.com/bitcoin/bitcoin/pull/30560#discussion_r1701323070
Surprisingly does not change the size of the Guix **bitcoind** binary (on x86_64-linux-gnu) by 1 single byte.
Spawned already merged PRs: #30436, #30482, #30532, #30560.
ACKs for top commit:
l0rinc:
ACK 8756ccd71218c8e013181473720b10d3c4a94957
stickies-v:
Rebase re-ACK 8756ccd71218c8e013181473720b10d3c4a94957, no changes since a096215c9c71a2ec03e76f1fd0bcdda0727996e0
ryanofsky:
Code review ACK 8756ccd71218c8e013181473720b10d3c4a94957, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment
Tree-SHA512: 9b2011b7c37e0ef004c669f8601270a214b388916316458370f5902c79c2856790b1b2c7c123efa65decad04886ab5eff95644301e0d84358bb265cf1f8ec195
|
|
The following scripted-diff commit will replace ParseHex("...") with "..."_hex_u8, but this replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte is accepted.
For example, it is currently necessary to use _hex_v_u8 when calling CScript operator<< because that operator does not currently support std::array or std::byte.
Conversely, it is incorrect to use _hex_v instead of _hex in net_processing.cpp for the MakeAndPushMessage argument, because if the argument is a std::vector it is considered variable-length and serialized with a size prefix, but if the argument is a std::array or Span is it considered fixed length and serialized without a prefix.
By the same logic, it is also safe to change the NUMS_H constant in pubkey.cpp from a std::vector to std::array because it is never serialized.
|
|
|
|
948238a683b6c99f4e91114aa75680c6c2d73714 test: Remove FastRandomContext global (Ryan Ofsky)
fa0fe08eca48064b2a42789571fea017e455d820 scripted-diff: [test] Use g_rng/m_rng directly (MarcoFalke)
fa54cab4734f02422f28fdffc0f11e6d3d51b8f0 test: refactor: Accept any RandomNumberGenerator in RandMoney (MarcoFalke)
68f77dd21e4aaf4f09d36d6e5ddd7d260824b94b test: refactor: Pass rng parameters to test functions (Ryan Ofsky)
fa19af555dff6d6c722caf36319b158699d2aa95 test: refactor: Move g_insecure_rand_ctx.Reseed out of the helper that calls MakeRandDeterministicDANGEROUS (MarcoFalke)
3dc527f4602297ffcec3a578eadc480a620d01ec test: refactor: Give unit test functions access to test state (Ryan Ofsky)
fab023e177d7eaef73902869ae1c95693f1e268b test: refactor: Make unsigned promotion explicit (MarcoFalke)
fa2cb654eca8dd6ed89101cd6d199ba1de0b81e0 test: Add m_rng alias for the global random context (MarcoFalke)
fae7e3791c9ed8053166773fcfb583ad19d006dd test: Correct the random seed log on a prevector test failure (MarcoFalke)
Pull request description:
This is mostly a style-cleanup for the tests' random generation:
1) `g_insecure_rand_ctx` in the tests is problematic, because the name is a leftover when the generator was indeed insecure. However, now the generator is *deterministic*, because the seed is either passed in or printed (c.f. RANDOM_CTX_SEED). Stating that deterministic randomness is insecure in the tests seems redundant at best. Fix it by just using `m_rng` for the name.
2) The global random context has many one-line aliases, such as `InsecureRand32`. This is problematic, because the same line of code may use the context directly and through a wrapper at the same time. For example in net_tests (see below). This inconsistency is harmless, but confusing. Fix it by just removing the one-line aliases.
```
src/test/net_tests.cpp: auto msg_data_1 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(100000));
````
3) The wrapper for randmoney has the same problem that the same unit test uses the context directly and through a wrapper at the same time. Also, it has a single type of Rng hardcoded. Fix it by accepting any type.
ACKs for top commit:
hodlinator:
ACK 948238a683b6c99f4e91114aa75680c6c2d73714
ryanofsky:
Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since last review were changing a comments a little bit.
marcofleon:
Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since my last review are the improvements in `prevector_tests`.
Tree-SHA512: 69c6b46a42cb743138ee8c87ff26a588dbe083e3efb3dca49b8a133ba5d3b09e8bf01c590ec7e121a7d77cb1fd7dcacd927a9ca139ac65e1f7c6d1ec46f93b57
|
|
41051290ab3b6c36312cec26a27f787cea9961b4 cmake: Ignore build subdirectories within source directory (Hennadii Stepanov)
6ce50fd9d0ae6850d54bf883e7a7c1bcb6912c5c doc: Update for CMake-based build system (Hennadii Stepanov)
9730288a0cd3f33021ef00fb2d95e5216d10ab61 ci: Migrate CI scripts to CMake (Hennadii Stepanov)
c360837ca5c91c9878ae8088bb5482e96fd87c96 cmake, lint: Adjust `lint_includes_build_config` (Hennadii Stepanov)
3885441ee0d35a40904995ede68120fea471dde7 cmake: Add presets for native Windows builds (Hennadii Stepanov)
7681746b20dd58e7d3e6d2852f07fb876383a133 cmake: Add vcpkg manifest file (Hennadii Stepanov)
8b6f1c4353836bae6aa683cbc65251165bd031ba cmake: Add `Coverage` and `CoverageFuzz` scripts (Hennadii Stepanov)
65bdbc1ff23b0a817f4d9a4682e6f630c9bbdd59 cmake: Add `docs` build target (Hennadii Stepanov)
fb75ebbc33557ddd56f505100ad3631a0028eb86 cmake: Add compiler diagnostic flags (Hennadii Stepanov)
e821f0a37a026fa0480c7f6f6c938da7c77e0d52 cmake: Migrate Guix build scripts to CMake (Hennadii Stepanov)
747adb6ffe9b06d476fc5eaebbaf9a62b91a78c5 cmake: Add `Maintenance` module (Hennadii Stepanov)
1f60b30df0cb58a7381a1bfbd6d34f002232e862 cmake: Add `APPEND_{CPP,C,CXX,LD}FLAGS` cache variables (Hennadii Stepanov)
2b43c45b13ad00cfd9928a03da8a480977724cb1 cmake: Add `AddWindowsResources` module (Hennadii Stepanov)
973a3b0c5dcbf6b3fd155b2dda4c2e94a0b0ee5f cmake: Implement `install` build target (Hennadii Stepanov)
84ac35cfd4dfa6f235f6e5a00b571846358f45ce cmake: Add cross-compiling support (Hennadii Stepanov)
0d01c228a7d39bb4918b5cb9f6db25cb8c30567a build: Generate `toolchain.cmake` in depends (Hennadii Stepanov)
91a799247dc5e4627e6b2f221669c8ff9238bc8d depends: Add host-specific `cmake_system_version` variables (Hennadii Stepanov)
9b31209b4caaa02b3044acd2375a7f595cdbd520 depends: Rename `cmake_system` -> `cmake_system_name` (Hennadii Stepanov)
4a5208a81d5bfeef270c64d48dce3444d6d03511 Revert "build, qt: Do not install *.prl files" (Hennadii Stepanov)
6522af62af1c3a6e2525bfffdb2295751b6fa49b depends: Amend handling flags environment variables (Hennadii Stepanov)
90cec4d251a541adfc5953e24dc01840a8cb4af2 cmake: Add `MULTIPROCESS` option (Hennadii Stepanov)
bb1a450dcb111746869547c8b538b5d2472cf8e6 cmake: Build `bitcoin-chainstate` executable (Hennadii Stepanov)
aed38ea58cbde068fe12b5299b246b4e3649a09c cmake: Build `bitcoinkernel` library (Hennadii Stepanov)
975d67369b8f3a33a21fd7618c299c0ec138292c cmake: Build `test_bitcoin-qt` executable (Hennadii Stepanov)
10fcc668a3430b72eaf7effc83f00cedeb27c7dc cmake: Add `WITH_DBUS` option (Hennadii Stepanov)
5bb5a4bc75a523e30eab561763927252ce105c4d cmake: Add `libqrencode` optional package support (Hennadii Stepanov)
57a6e2ef4abbfd2b12ee6489366bc6609bead263 cmake: Build `bitcoin-qt` executable (Hennadii Stepanov)
30f642952cb5bf39479bdbe467b3950f0d09324a cmake: Add `WERROR` option (Hennadii Stepanov)
c98d4a4c341e524348d0342e145d439816a44c83 cmake: Add `REDUCE_EXPORTS` option (Hennadii Stepanov)
a01cb6e63ff3940f0773b37e2fe1e148f17acad9 cmake: Add `HARDENING` option (Hennadii Stepanov)
a8a2e364acf55bbe18404ab21f852d52257bcb6d cmake: Add Python-based tests (Hennadii Stepanov)
3d853795707c5a1828dcd09c1f68bb07dee472cd cmake: Add fuzzing options (Hennadii Stepanov)
908530e312a3d4561f9c1feeb2a76ce899f21c68 cmake: Add `SANITIZERS` option (Hennadii Stepanov)
8bb0e85631e7c1bee16e136454b2466776be1d65 cmake: Build `bench_bitcoin` executable (Hennadii Stepanov)
801735163a81650619a6c9587e8f1df9ee182694 cmake: Add external signer support (Hennadii Stepanov)
353e0c9e9679864a777e17c1bb7c6ba8b6eac96d cmake: Add `systemtap-sdt` optional package support (Hennadii Stepanov)
d2fda82b4954f4af7e7d340cf42b9cb34d96cde1 cmake: Add `libzmq` optional package support (Hennadii Stepanov)
ae7b39a0e106d798b6e9cc03ee783d9081e41480 cmake: Add `libminiupnpc` optional package support (Hennadii Stepanov)
6480e1dcdb03f43ce3d0aad96b8668d017d11750 cmake: Add `libnatpmp` optional package support (Hennadii Stepanov)
e73e9304a11af65f9b086460ff349f9f700709ce cmake: Build `bitcoin-util` executable (Hennadii Stepanov)
027c6d7caa0355c35b00f2689eddccc3d1227aef cmake: Build `bitcoin-tx` executable (Hennadii Stepanov)
d10c5c34c3d899db8bcff47ac8c6ba396a6da4b6 cmake: Add wallet functionality (Hennadii Stepanov)
ab2e99b0d95714e16a7d1a1313d7da938b0485cb cmake: Create test suite for `ctest` (Hennadii Stepanov)
959370bd76d30ced34208db45fb4fd097fbad31b cmake: Build `test_bitcoin` executable (Hennadii Stepanov)
b27bf9700dbbfa9a0243815f78c8b62abe78d8bc cmake: Build `bitcoin-cli` executable (Hennadii Stepanov)
a9813df826c885b1609e55a83d87cd9cbc90adfd cmake: Build `bitcoind` executable (Hennadii Stepanov)
97829ce2d5a8dc3b0307b5d57c6686b96b7cf850 cmake: Add `FindLibevent` module (Hennadii Stepanov)
3118e40c6157c8ab9e264518d1065d2b0fc07795 cmake: Build `bitcoin_consensus` library (Hennadii Stepanov)
809a2f192903145f88f30bc10d3cf1ab9ed06881 cmake: Build `bitcoin_util` static library (Hennadii Stepanov)
0a9a521a704ca8a27124c1498a86e87ad46d4c34 cmake: Build `bitcoin_crypto` library (Hennadii Stepanov)
958971f476a29cb5bb76f3ae80ae968317ca1930 cmake: Build `univalue` static library (Hennadii Stepanov)
752747fda801f2c0ecce91c96bcc9ef93e27462b cmake: Generate `obj/build.h` header (Hennadii Stepanov)
1f0a78edf3cd2c24236ac512acf420eb9ed86ab3 cmake: Build `minisketch` static library (Hennadii Stepanov)
12bfbc81540f037c95e7796ae0b9f05ce3fb1b4a cmake: Build `leveldb` static library (Hennadii Stepanov)
51985c5304dfc52bd45f505b3115989637d79ff5 cmake: Build `crc32c` static library (Hennadii Stepanov)
db7a198f29c62c5f762eaa25d1d83c57e2f1e211 cmake: Build `secp256k1` subtree (Hennadii Stepanov)
dbb7ed14e8562439238eec70b202c50f172e3def cmake: Add `ccache` support (Hennadii Stepanov)
cedfdf6c72535d0797a271c6bb9d84c4b406a8ea cmake: Redefine/adjust per-configuration flags (Hennadii Stepanov)
b6b5e732c8b49a2cc14f34ac72b2189389c6b27d cmake: Add global compiler and linker flags (Hennadii Stepanov)
f98327931bd0b5d90678ddd1770e9862266b396e cmake: Add `TryAppendLinkerFlag` module (Hennadii Stepanov)
4a0af29697b62d32af6f60d3ec70cd2ed4d7243c cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov)
35cffc497d8db3cf3eee35c1513e3435558f056b cmake: Add POSIX threads support (Hennadii Stepanov)
fd72d00ffe34c84e292b305f6797201040d31a72 cmake: Add position independent code support (Hennadii Stepanov)
07069e2bb0bbdacf16cf34efd3a33390de030217 cmake: Add introspection module (Hennadii Stepanov)
27d687fc1f6aceaed7725e1e904a093ead68d6e6 cmake: Add `config/bitcoin-config.h` support (Hennadii Stepanov)
fe5cdace5ffba46fb7981efb816621962d3873e3 cmake: Print compiler and linker flags in summary (Hennadii Stepanov)
70683884c5fd78dbf7816434464e6511b9d4e486 cmake: Introduce interface libraries to encapsulate common flags (Hennadii Stepanov)
a2317e27b7fb86df4e32cd1674c06e09cb808248 cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov)
Pull request description:
This PR introduces a new CMake-based build system, which is a drop-in replacement for the current Autotools-based build system.
ML announcement: https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo
As discussed during the recent CoreDev meetup in April, the switch from Autotools to CMake is intended to happen as soon as possible after branching 28.x off, which means that 29.0 will be built using CMake.
This PR branch is essentially the [staging branch](https://github.com/hebasto/bitcoin/tree/cmake-staging), with every change reviewed and tested by a group of contributors, including (in alphabetical order):
- [**achow101**](https://github.com/achow101)
- [**fanquake**](https://github.com/fanquake)
- [**maflcko**](https://github.com/maflcko)
- [**m3dwards**](https://github.com/m3dwards)
- [**pablomartin4btc**](https://github.com/pablomartin4btc)
- [**real-or-random**](https://github.com/real-or-random)
- [**ryanofsky**](https://github.com/ryanofsky)
- [**sipsorcery**](https://github.com/sipsorcery)
- [**TheCharlatan**](https://github.com/TheCharlatan)
- [**theStack**](https://github.com/theStack)
- [**theuni**](https://github.com/theuni)
- [**vasild**](https://github.com/vasild)
Reviewing in a separate staging repo was suggested in https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431798320.
The accompanying changes to the OSS-Fuzz project are available in https://github.com/hebasto/oss-fuzz/pull/8.
Please refer to the [build options parity table](https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e). The "auto" value is no longer available; non-default values must be specified explicitly. Additionally, the new default values have been chosen to suit the everyday build experience for the majority of developers.
System requirements for using the CMake-based build system:
- CMake >= 3.22 (if not available in your system's repository, it can be downloaded from https://cmake.org/download/)
- a build tool of your choice:
- any Make (GNU Make is no longer a requirement); GNU Make is still required to build depends
- Ninja (https://ninja-build.org/)
- MSBuild
- Xcode
A note for Windows users: The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager).
---
We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented separately. Bugs in the scripts and errors in the updated documentation should be the focus of this PR. Please be advised that comments not aligned with this PR's goal may be ignored.
Thank you all for your understanding.
ACKs for top commit:
maflcko:
review ACK 41051290ab3b6c36312cec26a27f787cea9961b4 π₯
sipsorcery:
ACK 41051290ab3b6c36312cec26a27f787cea9961b4.
vasild:
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
TheCharlatan:
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
pablomartin4btc:
tACK 41051290ab3b6c36312cec26a27f787cea9961b4
i-am-yuvi:
tACK [`4105129`](https://github.com/bitcoin/bitcoin/commit/41051290ab3b6c36312cec26a27f787cea9961b4)
theuni:
ACK 41051290ab3b6c36312cec26a27f787cea9961b4.
fanquake:
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Tree-SHA512: 6c1445054436c6c00ad63bfa0f19d64091a2b25c9bd694f85bf2218ac358ffb774d6c000685b3ca1e9b50401babed989fa2a0694b774c211d226bfd1944c9b39
|
|
18d65d27726bf9fc7629b8e794047a10c9cf6156 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329c3bf38e47a07434e2a3c0031f808d0 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544b538dba9823bcd5754d074272bfc04 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1e5d5af6779c3e4befe514c9dafb8ff node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737e77ee85812cc328c4ddfaea7147533 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe3f94770bdf73dedd8bda0193a44627 test: unittest chainstatemanager_args (stickies-v)
Pull request description:
Since fad2991ba073de0bd1f12e42bf0fbaca4a265508, `uint256S` has been [deprecated](https://github.com/bitcoin/bitcoin/pull/30482/commits/fad2991ba073de0bd1f12e42bf0fbaca4a265508#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_
This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.
The main behaviour change introduced in this PR is:
- `-minimumchainwork` will raise an error when input is longer than 64 hex digits
- `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
- test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits
After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
ACKs for top commit:
hodlinator:
re-ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
l0rinc:
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
achow101:
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
ryanofsky:
Code review ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
|
|
Drop g_insecure_rand_ctx
|
|
-BEGIN VERIFY SCRIPT-
# Use m_rng in unit test files
ren() { sed -i "s:\<$1\>:$2:g" $( git grep -l "$1" src/test/*.cpp src/wallet/test/*.cpp src/test/util/setup_common.cpp ) ; }
ren InsecureRand32 m_rng.rand32
ren InsecureRand256 m_rng.rand256
ren InsecureRandBits m_rng.randbits
ren InsecureRandRange m_rng.randrange
ren InsecureRandBool m_rng.randbool
ren g_insecure_rand_ctx m_rng
ren g_insecure_rand_ctx_temp_path g_rng_temp_path
-END VERIFY SCRIPT-
|
|
Accepting any Rng in RandMoney makes tests more flexible to use a
different Rng. Also, passing in the Rng clarifies the call sites, so
that they all use g_rand_ctx explicitly and consistently.
|
|
Add FastRandomContext parameter to the utility function
AddTestCoin(), and a few local test functions and classes.
|
|
MakeRandDeterministicDANGEROUS
The global g_insecure_rand_ctx will be removed in the future, so
removing it from this helper is useful.
Also, tying the two concepts of the global internal RNGState and the
global test-only rng context is a bit confusing, because tests can
simply use the m_rng, if it exists. Also, tests may seed more than one
random context, or none at all, or a random context of a different type.
Fix all issues by moving the Reseed call to the two places where it is
used.
|
|
Removes dependency on unsafe and deprecated uint256S.
This makes parsing more strict, by requiring RANDOM_CTX_SEED
to be a string of up to 64 hex digits (optionally prefixed with
"0x"), whereas previously any string would be accepted, with
non-hex characters silently ignored and input longer than
64 characters (ignoring "0x" prefix) silently trimmed.
Can be tested with:
$ RANDOM_CTX_SEED=z ./src/test/test_bitcoin --log_level=all --run_test=timeoffsets_tests/timeoffsets_warning -- -printtoconsole=1 | grep RANDOM_CTX_SEED
RANDOM_CTX_SEED must consist of up to 64 hex digits ("0x" prefix allowed), it was set to: 'z'.
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
|
|
The two names point to the same object, but having the reference now
allows easier removal of the global in the future.
|
|
This speeds up the fuzz target, which allows "valid" inputs. It does not
affect the "INVALID" fuzz target.
|
|
|
|
|
|
The re-init is expensive, so skip it if there is no need.
Also, add an even faster fuzz target utxo_snapshot_invalid, which does
not need any re-init at all.
|
|
It is expensive to construct, and only one test uses it.
Fix both issues by disallowing the construction and moving it to the
single test that uses it.
|
|
bf0efb4fc72d3c49a2c498c944e55466dfa046dc scripted-diff: Modernize naming of nChainTx and nTxCount (Fabian Jahr)
72e5d1be1f4491565249d43e836ee42cfd858866 test: Add basic check for nChainTx type (Fabian Jahr)
dc2938e9799d79696d1db2438ef33d90542d984b chainparams: Change nChainTx to uint64_t (Fabian Jahr)
Pull request description:
This picks up the work from #29331 and closes #29258.
This simply changes the type and addresses the comments from #29331 by changing the type in all relevant places and removing unnecessary casts. This also adds an extremely simple unit test.
Additionally this modernizes the name of `nChainTx` which helps reviewers check all use of the symbol and can make silent merge conflicts.
ACKs for top commit:
maflcko:
only rebase in scripted-diff, re-ACK bf0efb4fc72d3c49a2c498c944e55466dfa046dc π
glozow:
reACK bf0efb4fc72 via range-diff
Tree-SHA512: ee4020926d0800236fe655d0c7b127215ab36b553b04d5f91494f4b7fac6e1cfe7ee298b07c0983db5a3f4786932acaa54f5fd2ccd45f2fcdcfa13427358dc3b
|
|
73e3fa10b4dd63b7767d6b6f270df66971067341 doc + test: Correct uint256 hex string endianness (Hodlinator)
Pull request description:
This PR is a follow-up to #30436.
Only changes test-code and modifies/adds comments.
Byte order of hex string representation was wrongfully documented as little-endian, but are in fact closer to "big-endian" (endianness is a memory-order concept rather than a numeric concept). `[arith_]uint256` both store their data in arrays with little-endian byte order (`arith_uint256` has host byte order within each `uint32_t` element).
**uint256_tests.cpp** - Avoid using variable from the left side of the condition in the right side. Credits to @maflcko: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688273553
**setup_common.cpp** - Skip needless ArithToUint256-conversion. Credits to @stickies-v: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688621638
---
<details>
<summary>
## Logical reasoning for endianness
</summary>
1. Comparing an `arith_uint256` (`base_uint<256>`) to a `uint64_t` compares the beginning of the array, and verifies the remaining elements are zero.
```C++
template <unsigned int BITS>
bool base_uint<BITS>::EqualTo(uint64_t b) const
{
for (int i = WIDTH - 1; i >= 2; i--) {
if (pn[i])
return false;
}
if (pn[1] != (b >> 32))
return false;
if (pn[0] != (b & 0xfffffffful))
return false;
return true;
}
```
...that is consistent with little endian ordering of the array.
2. They have the same endianness (but `arith_*` has host-ordering of each `uint32_t` element):
```C++
arith_uint256 UintToArith256(const uint256 &a)
{
arith_uint256 b;
for(int x=0; x<b.WIDTH; ++x)
b.pn[x] = ReadLE32(a.begin() + x*4);
return b;
}
```
### String conversions
The reversal of order which happens when converting hex-strings <=> uint256 means strings are actually closer to big-endian, see the end of `base_blob<BITS>::SetHexDeprecated`:
```C++
unsigned char* p1 = m_data.data();
unsigned char* pend = p1 + WIDTH;
while (digits > 0 && p1 < pend) {
*p1 = ::HexDigit(trimmed[--digits]);
if (digits > 0) {
*p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
p1++;
}
}
```
Same reversal here:
```C++
template <unsigned int BITS>
std::string base_blob<BITS>::GetHex() const
{
uint8_t m_data_rev[WIDTH];
for (int i = 0; i < WIDTH; ++i) {
m_data_rev[i] = m_data[WIDTH - 1 - i];
}
return HexStr(m_data_rev);
}
```
It now makes sense to me that `SetHexDeprecated`, upon receiving a shorter hex string that requires zero-padding, would pad as if the missing hex chars where towards the end of the little-endian byte array, as they are the most significant bytes. "Big-endian" string representation is also consistent with the case where `SetHexDeprecated` receives too many hex digits and discards the leftmost ones, as a form of integer narrowing takes place.
### How I got it wrong in #30436
Previously I used the less than (`<`) comparison to prove endianness, but for `uint256` it uses `memcmp` and thereby gives priority to the *lower* bytes at the beginning of the array.
```C++
constexpr int Compare(const base_blob& other) const { return std::memcmp(m_data.data(), other.m_data.data(), WIDTH); }
```
`arith_uint256` is different in that it begins by comparing the bytes from the end, as it is using little endian representation, where the bytes toward the end are more significant.
```C++
template <unsigned int BITS>
int base_uint<BITS>::CompareTo(const base_uint<BITS>& b) const
{
for (int i = WIDTH - 1; i >= 0; i--) {
if (pn[i] < b.pn[i])
return -1;
if (pn[i] > b.pn[i])
return 1;
}
return 0;
}
```
(The commit documents that `base_blob::Compare()` is doing lexicographic ordering unlike the `arith_*`-variant which is doing numeric ordering).
</details>
ACKs for top commit:
paplorinc:
ACK 73e3fa10b4dd63b7767d6b6f270df66971067341
ryanofsky:
Code review ACK 73e3fa10b4dd63b7767d6b6f270df66971067341
Tree-SHA512: 121630c37ab01aa7f7097f10322ab37da3cbc0696a6bbdbf2bbd6db180dc5938c7ed91003aaa2df7cf4a4106f973f5118ba541b5e077cf3588aa641bbd528f4e
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/nChainTx/m_chain_tx_count/g' $(git grep -l 'nChainTx' ./src)
sed -i 's/nTxCount/tx_count/g' $(git grep -l 'nTxCount' ./src)
-END VERIFY SCRIPT-
|
|
Follow-up to #30436.
uint256 string representation was wrongfully documented as little-endian due to them being reversed by GetHex() etc, and base_blob::Compare() giving most significance to the beginning of the internal array. They are closer to "big-endian", but this commit tries to be even more precise than that.
uint256_tests.cpp - Avoid using variable from the left side of the condition in the right side.
setup_common.cpp - Skip needless ArithToUint256-conversion.
|
|
|
|
This adds a first version of the overall linearization interface, which given
a DepGraph constructs a good linearization, by incrementally including good
candidate sets (found using AncestorCandidateFinder and SearchCandidateFinder).
|
|
This introduces a bespoke fuzzing-focused serialization format for DepGraphs,
and then tests that this format can represent any graph, roundtrips, and then
uses that to test the correctness of DepGraph itself.
This forms the basis for future fuzz tests that need to work with interesting
graphs.
|
|
TestingSetup
f46b2202560a76b473e229b77303b8f877c16cac fuzz: Use BasicTestingSetup for coins_view target (TheCharlatan)
9e2a723d5da4fc277a42fed37424f578e348ebf8 test: Add arguments for creating a slimmer setup (TheCharlatan)
Pull request description:
This adds arguments to some of the testing setup constructors for creating an environment without networking and a validation interface. This is useful for improving the performance of the utxo snapshot fuzz test, which constructs a new TestingSetup on each iteration.
Using this slimmed down `TestingSetup` in future might also make the tests a bit faster when run in aggregate.
ACKs for top commit:
maflcko:
review ACK f46b2202560a76b473e229b77303b8f877c16cac
dergoegge:
utACK f46b2202560a76b473e229b77303b8f877c16cac
Tree-SHA512: 9dc62512b127b781fc9e2d8ef2b5a9b06ebb927a8294b6d872001c553984a7eb1f348e0257b32435b34b5505b5d0323f73bdd572a673da272d3e1e8538ab49d6
|
|
length
09ce3501fa2ea2885a857e380eddb74605f7038c fix: Make TxidFromString() respect string_view length (Hodlinator)
01e314ce0ae30228742b6f19d2f12a050ab97e4d refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator)
2f5577dc2e7ba668798a89a2f6ef72795db6c285 test: uint256 - Garbage suffixes and zero padding (Hodlinator)
f11f816800ac520064a1e96871d0b4cc9601ced7 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator)
f0eeee2dc1329b0647df09bea9ccc0395bb82698 test: Add test for TxidFromString() behavior (Ryan Ofsky)
Pull request description:
### Problem
Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`.
### Solution
Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in.
(PR was prompted by comment in https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200 (referring to https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378)).
ACKs for top commit:
maflcko:
re-ACK 09ce3501fa2ea2885a857e380eddb74605f7038c π
paplorinc:
ACK 09ce3501fa2ea2885a857e380eddb74605f7038c
ryanofsky:
Code review ACK 09ce3501fa2ea2885a857e380eddb74605f7038c. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.
Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581
|
|
|
|
Adds more testing options for creating an environment without networking
and a validation interface. This is useful for improving the performance
of the utxo snapshot fuzz test, which constructs a new TestingSetup on
each iteration.
|
|
16bd283b3ad05daa41259a062aee0fc05b463fa6 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c14855fe2cba15a32245572b693dc18c4e net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c1302c986e344c7f44bb0b352213d5dc8 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net_processing.cpp#L1662-L1683))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes #30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
tdb3:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
glozow:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
dergoegge:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
|
|
fa690c8e532672f7ab53be6f7a0bb3070858745e test: [refactor] Pass TestOpts (MarcoFalke)
Pull request description:
Currently optional test context setup settings are passed by adding a new optional argument to the constructors. For example `extra_args`. This is problematic, because:
* Adding more optional settings in the future requires touching all affected constructors, increasing their verbosity.
* Setting only a later option requires setting the earlier ones.
* Clang-tidy named args passed to `std::make_unique` are not checked.
Fix all issues by adding a new struct `TestOpts`, which holds all options. Notes:
* The chain type is not an option in the struct for now, because the default values vary.
* The struct holds all possible test options globally. Not all fields may be used by all constructors. Albeit harmless, it is up to the test author to not set a field that is unused.
ACKs for top commit:
kevkevinpal:
utACK [fa690c8](https://github.com/bitcoin/bitcoin/pull/30407/commits/fa690c8e532672f7ab53be6f7a0bb3070858745e)
dergoegge:
utACK fa690c8e532672f7ab53be6f7a0bb3070858745e
TheCharlatan:
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e
Tree-SHA512: 8db8efa5dff854a73757d3f454f8f902e41bb4358f5f9bae29dbb3e251e20ee93489605de51d0822ba31d97835cd15526a29c075278dd6a8bbde26134feb4f49
|
|
test/util/net.cpp
e233ec036dc972a5847ab769ad22d418fd9404d1 refactor: Use designated initializer (Hodlinator)
Pull request description:
Block was recently touched (e2d1f84858485650ff743753ffa5c679f210a992) and the codebase recently switched to C++20 which allows this to improve robustness.
Follow-up suggested in https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014
ACKs for top commit:
maflcko:
ACK e233ec036dc972a5847ab769ad22d418fd9404d1
Tree-SHA512: ce3a18f513421e923710a43c8f97db1badb7ff5c6bdbfd62d9543312d2225731db5c14bef16feb47c43b84fad4dc24485086634b680feba422d2b7b363e13fa6
|
|
Initiating an outbound network connection currently involves the
following steps after the socket connection is established (see
`CConnman::OpenNetworkConnection` method):
1. set up node state
2. queue VERSION message
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message
(step 2) is received and processed locally *before* the node object
is added to the connection manager's `m_nodes` vector (step 3). In this
case, the self-connect remains undiscovered, as the detection doesn't
find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion`
call out of `InitializeNode` and doing that in the `SendMessages` method
instead, which is only called for `CNode` instances in `m_nodes`.
Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on
how to fix this.
|
|
606a7ab862470413ced400aa68a94fd37c8ad3d3 kernel: De-globalize signature cache (TheCharlatan)
66d74bfc45ae0f743084475ac3bbfb4355bb6ec2 Expose CSignatureCache class in header (TheCharlatan)
021d38822c0e6a1b9497bcb20401c5c37e1bb84d kernel: De-globalize script execution cache hasher (TheCharlatan)
13a3661aba95b54b822c99ecbb695b14a22536d2 kernel: De-globalize script execution cache (TheCharlatan)
ab14d1d6a4a8ef5fe5013150e6c5ebcb5f5e4ea9 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 606a7ab862470413ced400aa68a94fd37c8ad3d3
glozow:
reACK 606a7ab
ryanofsky:
Code review ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3. Just small formatting, include, and static_assert changes since last review.
Tree-SHA512: e7f3ee41406e3b233832bb67dc3a63c4203b5367e5daeed383df9cb590f227fcc62eae31311029c077d5e81b273a37a88a364db3dee2efe91bb3b9c9ddc8a42e
|
|
|
|
Block was recently touched (e2d1f84858485650ff743753ffa5c679f210a992) and the codebase recently switched to C++20 which allows this to improve robustness.
|
|
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.
|
|
ce8094246ee95232e9d84f7e37f3c0a43ef587ce random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561ecfdf81855e6df6b9ad3d8843cdfa2 random: use LogError for init failure (Pieter Wuille)
97e16f57042cab07e5e73f6bed19feec2006e4f7 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd68064e402e8eceea3df9474bb7afd48 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b5e9fdd01e8b220c08a3ccde5cf584c net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966bc501db8bf6a3809633f0b82e6ae547 random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf0b46f3ea9e62992ade989860f086c8 random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3e189369d59144b524cb2808cbef8c3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d95fc9447e64c098dcadb6b8a2f1f2ee net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999d7e1a87efaf6bcb222186f0dcd87ec random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858485650ff743753ffa5c679f210a992 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e12a1fdace7998d75b4daf8b67d7028 tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104caf9952393f9dac2a36539d964077f random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065fc1864f879248d1e1444588e27076b random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b608bdf396d8f2d758a792f869c2cd2a xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f66269c04633167def01f82c74ea730 random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd96c1f626def4755e0e1b5aaac94d3e random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658fed0d3e4552e8b02a6902cb31c572e random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da05f74ffb6a2ac20b7d9efefbe29634 random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b60d7a67a9720a84a685f16e3f05b06 random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6a6a159779f572f4c3a06170f955ed8 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde202ad508baf553817c5b38fdd2d4a0c random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e845e623e202e3eefe1cc2010e9b514 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 ce8094246ee95232e9d84f7e37f3c0a43ef587ce
maflcko:
re-ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce π
hodlinator:
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
dergoegge:
utACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
|
|
926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746 [doc] add release note for TRUC (glozow)
19a9b90617419f68d0f1c90ee115b5220be99a16 use version=3 instead of v3 in debug strings (glozow)
881fac8e609be17eb71bd9a54c0284b304e2e2e2 scripted-diff: change names from V3 to TRUC (glozow)
a573dd261748d2a80560f73db08f7dca788c7fcf [doc] replace mentions of v3 with TRUC (glozow)
089b5757dff39a9a06cdb625aaced9beeb72958d rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89d93441645250c40c3980aeb0c3b664 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 https://github.com/bitcoin/bitcoin/pull/30272/commits/926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
achow101:
ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
ismaelsadeeq:
Code review ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
|
|
invalid chain
2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407ef546fa50d18b2ffbd67b7417797064 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c011863d69e7785673283e4c9033fbcd5ac 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 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. 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 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/SingleV3Checks/SingleTRUCChecks/g' $(git grep -l 'SingleV3Checks')
sed -i 's/PackageV3Checks/PackageTRUCChecks/g' $(git grep -l 'PackageV3Checks')
sed -i 's/PV3C/PTRUCC/g' src/policy/truc_policy.h
sed -i 's/V3_MAX_VSIZE/TRUC_MAX_VSIZE/g' $(git grep -l 'V3_MAX_VSIZE')
sed -i 's/V3_CHILD_MAX_VSIZE/TRUC_CHILD_MAX_VSIZE/g' $(git grep -l 'V3_CHILD_MAX_VSIZE')
sed -i 's/V3_DESCENDANT_LIMIT/TRUC_DESCENDANT_LIMIT/g' $(git grep -l 'V3_DESCENDANT_LIMIT')
sed -i 's/V3_ANCESTOR_LIMIT/TRUC_ANCESTOR_LIMIT/g' $(git grep -l 'V3_ANCESTOR_LIMIT')
sed -i 's/CheckMempoolV3Invariants/CheckMempoolTRUCInvariants/g' $(git grep -l 'CheckMempoolV3Invariants')
-END VERIFY SCRIPT-
|
|
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.
|
|
|
|
|
|
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.
|
|
This is preparation for making it more generally accessible.
|
|
|
|
Make use of C++20 functions in XoRoShiRo128PlusPlus.
|
