The two tests are doing different things - `xor_roundtrip_random_chunks` does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The `xor_bytes_reference` test makes sure the optimized xor implementation behaves in every imaginable scenario exactly as the simplest possible obfuscation - with random chunks, random alignment, random data, random key.
Since we're touching the file, other related small refactors were also applied:
* `nullpt` typo fixed;
* manual byte-by-byte xor key creations were replaced with `_hex` factories;
* since we're only using 64 bit keys in production, smaller keys were changed to reflect real-world usage;
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Since 31 byte xor-keys are not used in the codebase, using the common size (8 bytes) makes the benchmarks more realistic.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
f5647c6c5ae85e9469cfc5df6fcac23752e1695a depends: fix libevent _WIN32_WINNT usage (fanquake)
Pull request description:
Starting with version 13.x, the mingw headers will define the value of
`NTDDI_VERSION`, based on the value of `_WIN32_WINNT`, if that version is <
Windows 10. Given that libevent was undefining our `_WIN32_WINNT`, and
redefining it to a value < Windows 10 (`0x0501`), `NTDDI_VERSION` was also
being defined to that value, leading to functions not being exposed in
the mingw-w64 headers; see here: 9c2668ef77/mingw-w64-headers/include/iphlpapi.h (L36-L41).
Imports a commit from usptream ([a14ff91254f40cf36e0fee199e26fb11260fab49](a14ff91254)).
Fixes#32707.
ACKs for top commit:
willcl-ark:
crACK f5647c6c5ae85e9469cfc5df6fcac23752e1695a
Tree-SHA512: eb429457a4af6191dd27ef3d1087667c5304ff0f49d4c6824883651e3c2dbab5d9784fa1f170402f23cd9238005c5214e0a71a4160562a59dfa35618dc702132
This argument might have been used in the legacy wallets, but I don't
see any implementation using this argument in the SQLite wallets.
Removing it cleans up the code a bit.
4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 test: document HOST for get_previous_releases.py (Sjors Provoost)
609203d5075c5083c0922174e1acfe6bf3279600 test: stop signing previous releases >= v28.2 (Sjors Provoost)
c6dc2c29f8288065096e8ac145c1d8acfd5c8151 test: replace v28.0 with notarized v28.2 (Sjors Provoost)
5bd73d96a3a7914b2345260d3460156c04f3f899 test: fix macOS detection (Sjors Provoost)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/31407 macOS guix builds are signed and notarized. This was included in v29 and backported to 28.x.
This PR bumps the v28.0 previous release binary to v28.2 and adjusts the test that uses it. Additionally it no longer manually code signs binaries >= v28.2.
While testing on an M4 mac and redownloading all the binaries, I noticed that `platform == "arm64-apple-darwin"` doesn't actually work. This initially used `args.platform` in #26694, but that was changed to just `platform` in #32219.
So the first commit switches this to use `args.host`. I manually tested on Intel macOS 13.7.6 that code-signing still isn't needed there (when downloading using a script).
Also documented that you can set `HOST`.
ACKs for top commit:
m3dwards:
ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9
maflcko:
review ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 🚏
Tree-SHA512: b4803d39a21cb622fd2388a0528b76d2b502956e2505385d3da201143b0afcf6f9d71c8c28937f27b70d2588fb6da677da058bdcd67b90fb53617acc3a727818
61e800e75cffa256ccdbc2ffc7a1739c00880ce0 test: headers sync timeout (stringintech)
Pull request description:
When reviewing PR #32051 and considering which functional tests might need to be adapted/extended accordingly, I noticed there appears to be limited functional test coverage for header sync timeouts and thought it might be helpful to add one.
This test attempts to cover two scenarios:
1. **Normal peer timeout behavior:** When a peer fails to respond to initial getheaders requests within the timeout period, it should be disconnected and the node should attempt to sync headers from the next available peer.
2. **Noban peer behavior:** When a peer with noban privileges times out, it should remain connected while the node still attempts to sync headers from other peers.
ACKs for top commit:
maflcko:
re-ACK 61e800e75cffa256ccdbc2ffc7a1739c00880ce0 🗝
stratospher:
reACK 61e800e7.
Tree-SHA512: b8a867e7986b6f0aa00d81a84b205f2bf8fb2e6047a2e37272e0244229d1f43020e9031467827dabbfe7849a91429f2685e00a25356e2ed477fa1a035fa0b1fd
28416f367a5d720d89214aa8ef94013caa1d1a40 test: fix intermittent failure in rpc_invalidateblock.py (stratospher)
Pull request description:
resolves#32965.
node1 (with 24 blocks) causes node0 (with 6 blocks + 1 extra header) to silently reorg. so move the subtest to a point before the 20 blocks are generated so that node1's state doesn't cause node0 to silently reorg.
ACKs for top commit:
maflcko:
lgtm ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
mzumsande:
Code Review ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
Tree-SHA512: f6cc682b8e5416125f887c094d5e291dd37f0bfc41d7c0de218f3e24fa1ea0cd642f7a1e362f3127f68cde725a67f3054501326b9bd25f0caa9a05de7d0052b0
This adds a large simulation fuzz test for all TxOrphanage public interface
functions, using a mix of comparison with expected behavior (in case it is
fully specified), and testing of properties exhibited otherwise.
This is preparation for the simulation fuzz test added in a later commit. Since
AddChildrenToWorkSet consumes randomness, there is no way for the simulator to
exactly predict its behavior. By returning the set of made-reconsiderable announcements
instead, the simulator can instead test that it is *a* valid choice, and then
apply it to its own data structures.
For the default number of peers (125), allows each to relay a default
descendant package (up to 25-1=24 can be missing inputs) of small (9
inputs or fewer) transactions out of order.
This limit also gives acceptable bounds for worst case LimitOrphans iterations.
Functional tests aren't changed to check for larger cap because it would
make the runtime too long.
Also deletes the now-unused DEFAULT_MAX_ORPHAN_TRANSACTIONS.
This is largely a reimplementation using boost::multi_index_container.
All the same public methods are available. It has an index by outpoint,
per-peer tracking, peer worksets, etc.
A few differences:
- Limits have changed: instead of a global limit of 100 unique orphans,
we have a maximum number of announcements (which can include duplicate
orphans) and a global memory limit which scales with the number of
peers.
- The maximum announcements limit is 100 to match the original limit,
but this is actually a stricter limit because the announcement count
is not de-duplicated.
- Eviction strategy: when global limits are reached, a per-peer limit
comes into play. While limits are exceeded, we choose the peer whose
“DoS score” (max usage / limit ratio for announcements and memory
limits) is highest and evict announcements by entry time, sorting
non-reconsiderable ones before reconsiderable ones. Since announcements
are unique by (wtxid, peer), as long as 1 announcement remains for a
transaction, it remains in the orphanage.
- This eviction strategy means no peer can influence the eviction of
another peer’s orphans.
- Also, since global limits are a multiple of per-peer limits, as long
as a peer does not exceed its limits, its orphans are protected from
eviction.
- Orphans no longer expire, since older announcements are generally
removed before newer ones.
- GetChildrenFromSamePeer returns the transactions from newest to
oldest.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Move towards a model where TxOrphanage is initialized with limits that
it remembers throughout its lifetime.
Remove the param. Limiting by number of unique orphans will be removed
in a later commit.
Now that -maxorphantx is gone, this does not change the node behavior.
The parameter is only used in tests.
c18bf0bd9be632e54591493a5309e9ed4020f2e5 refactor: cleanup index logging (Sjors Provoost)
Pull request description:
This PR removes the use of `__func__` from index logging, since we have `-logsourcelocations`.
It also improves readability by putting `GetName()` in a more logical place.
Before
> coinstatsindex: best block of the index not found. Please rebuild the index.
After:
> best block of coinstatsindex not found. Please rebuild the index.
I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to split it into its own PR (or get rid of it).
ACKs for top commit:
l0rinc:
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
maflcko:
review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣
Tree-SHA512: 755948371e3ff7a5515b63ce48075631ec7868d69c3c1469176d5be0e8b28e1c071e206ae3f7320f87d8c441f815894acfef61621f05795b5ff6b8a5a3031e3b
node1 (with 24 blocks) causes node0 (with 6 blocks) to silently
reorg. so move the subtest to a point before the 20 blocks are
generated so that node1's state doesn't cause node0 to silently
reorg.
44f3bae300dcafbe53f9b07e6cc22a112833e579 depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE` (Hennadii Stepanov)
Pull request description:
When using CMake policies 3.14 and below, the `export(PACKAGE)` command by default populates the user package registry, which is stored outside the build tree. Setting the `CMAKE_EXPORT_NO_PACKAGE_REGISTRY` variable disables this side effect.
In CMake 3.15 and later, this behavior is disabled by default, and the variable has no effect.
This PR forces `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE` globally, rather than managing it for each dependency package individually rather. It may be reverted once all CMake-based packages have updated to policies 3.15 or newer.
Fixes https://github.com/bitcoin/bitcoin/issues/32938.
ACKs for top commit:
fanquake:
ACK 44f3bae300dcafbe53f9b07e6cc22a112833e579
Tree-SHA512: 0aac398b7182e80185b064d59f81aece4d8477a609fad9cc3fee317da2aff43b66ef7db1efec0135b4f0feaad23b1db664e33bd035fe659712c5b2a9bf2d6fb6
fad191ff48b15832a90c19d560a7c0525c146be3 ci: Avoid cd into build dir (MarcoFalke)
Pull request description:
Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes.
The changes are required for stuff like:
* cmake presets (see https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344031208)
* meta ci tests (like https://github.com/bitcoin/bitcoin/pull/32874)
So remove the `cd` and just make the build dir explicit.
ACKs for top commit:
hebasto:
ACK fad191ff48b15832a90c19d560a7c0525c146be3, I have reviewed the code and it looks OK.
Tree-SHA512: a88a9341445ffe28a0dac3815f235ec8eb0459d10a91a80829fd3184762d3c807d0f68c56243b20c04a6efa5becd8a7fad568f43c2b1e6af1ff8ba07b140ef87
12a6959892cb24b940b3579828f2066651572153 cmake: Drop no longer necessary "cmakeMinimumRequired" object (Hennadii Stepanov)
Pull request description:
The minimum required CMake version is 3.22:6a13a6106e/CMakeLists.txt (L10)
ACKs for top commit:
fanquake:
ACK 12a6959892cb24b940b3579828f2066651572153 - has been unneeded since it was introduced (minimum was already 3.22).
Tree-SHA512: 26f97662bfe52986e19e38dbf4ab8e1e7558bc78c3a65593cbecd1f35887bba7a9f7d8a3d08ccfab8396f41c2334cdad5b0e503999a759cfa158d3bb8d0d14d7
84ef5524d5abaf07cc9970f42e74ee15bd381e3d fix spelling in tor.md docs (stutxo)
Pull request description:
This PR is to fix some spelling mistakes i found of the word occurrences! there are two occurrences of this mistake.
thanks!
ACKs for top commit:
maflcko:
lgtm ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
willcl-ark:
ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
delta1:
ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
Tree-SHA512: 4ba71b772fdc8cf36ada7493d29fb5b312a7a6085099347162eb3495db4de984b0417b7861f2927c617cbd552741356e26688479601bdf7e835c15e097aa28f3
8f766f39df3e312f79f461b2accc2f9c90fa6338 ci: enable -Werror=dev (fanquake)
7b420ca8341a68e196a4f6fbd03104dc27b166ee guix: configure with -Werror=dev (fanquake)
44097ddb19139d07a905d1238fe67e53f8343d54 cmake: enable -Werror=dev in dev-mode preset (fanquake)
Pull request description:
Pass `-Werror=dev` in the CI, Guix and the `dev-mode` preset.
https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-Werror:
> Make developer warnings errors.
> Make warnings that are meant for the author of the CMakeLists.txt files errors. By default this will also turn on deprecated warnings as errors.
Pulled out of #32865.
ACKs for top commit:
Sjors:
re-ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338
hebasto:
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
Tree-SHA512: 0fa321b77d2519b5249d90590664c4e5938ac86209b068658647adf97ab55ea4d54c913aae2f622385fe2f41d7c851cd5d7371905fdad38b66cb124371e16ac7
Expiry is going away in a later commit.
This is only an RPC change. Behavior of the orphanage does not change.
Note that getorphantxs is marked experimental.
a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
94931656b52fddb7c03b29685844d85c008cf6ca cmake: Use newer signature of `qt6_add_lrelease` when available (Hennadii Stepanov)
Pull request description:
See Qt docs here: https://doc.qt.io/qt-6/qtlinguist-cmake-qt-add-lrelease.html.
Fixes https://github.com/bitcoin/bitcoin/issues/32710.
ACKs for top commit:
fanquake:
ACK 94931656b52fddb7c03b29685844d85c008cf6ca
Tree-SHA512: bf0320306967164374499dd0be122473799e830fdff5e070ef13f87af3c14a3b799d90afb423881edd7eea17c13d27af8ced381bbb3cd149353b31b3990dde67
fa894b0f3e13dcc55fd42cec2c56d4aa2115194d log: Properly log warnings with warn loglevel in addrdb (MarcoFalke)
Pull request description:
The logging in addrdb is confusing, because it uses `LogPrintf` (info level) to log warnings.
Fix this by properly using the `warn` level, where needed. Also, drop unused trailing `\n` while touching the lines.
ACKs for top commit:
stickies-v:
ACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d
dergoegge:
utACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d
Tree-SHA512: 96d3823623ea8e1698e8cb541ca97cbab7b2a9934b2f894884171045abbca7be796f07965082e997001c97d06d1e0c4d13b29354eb4fe71c3a2ee680eada5516
- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed
fa8862723c14cdd471bbffc7a4897ece2e6d8a7f fuzz: CheckGlobals in init (MarcoFalke)
fa26bfde988b1940e6b0d21accc60fbc4e45f9b1 test: Avoid resetting mocktime in testing setup (MarcoFalke)
fa6b45fa8ec8248544d22ba8429be8f6df19024a Add SetMockTime for time_point types (MarcoFalke)
Pull request description:
(Tracking issue https://github.com/bitcoin/bitcoin/issues/29018)
During fuzzing, `AppInitParameterInteraction` may actually disable a previously set mocktime. This is confusing and can also cause non-determinism.
Fix this issue, by
* fixing the erroneous `-mocktime` parsing in `AppInitParameterInteraction`.
* adding the missing `SetMockTime` calls to the affected fuzz init functions.
* adding a `CheckGlobals` to the fuzz init, to prevent this issue in the future.
This can be tested by
* Cherry-picking the `CheckGlobals`-commit onto current master and observing a fuzz failure in the touched fuzz targets.
* Reverting the touched fuzz fixups and observing a fuzz failure for each target.
ACKs for top commit:
w0xlt:
ACK fa8862723c
dergoegge:
utACK fa8862723c14cdd471bbffc7a4897ece2e6d8a7f
Tree-SHA512: 5a9400f0467c82fa224713af4cc2b525afbefefc7c3f419077110925ad7af6c7fda3dcd2b50f7facf0ee7df2547c6ac20336906d707adcdfd1d652a9d9a735fe
