The current `prevector` size of 28 bytes (chosen to fill the `sizeof(CScript)` aligned size) was introduced in 2015 (https://github.com/bitcoin/bitcoin/pull/6914) before SegWit and TapRoot.
However, the increasingly common `P2WSH` and `P2TR` scripts are both 34 bytes, and are forced to use heap (re)allocation rather than efficient inline storage.
The core trade-off of this change is to eliminate heap allocations for common 34-36 byte scripts at the cost of increasing the base memory footprint of all `CScript` objects by 8 bytes (while still respecting peak memory usage defined by `-dbcache`).
Increasing the `prevector` size allows these scripts to be stored inline, avoiding extra heap allocations, reducing potential memory fragmentation, and improving performance during cache flushes. Massif analysis confirms a lower stable memory usage after flushing, suggesting the elimination of heap allocations outweighs the larger base size for common workloads.
Due to memory alignment, increasing the `prevector` size to 36 bytes doesn't change the overall `sizeof(CScript)` compared to an increase to 34 bytes, allowing us to include `P2PK` scripts as well at no additional memory cost.
Performance benchmarks for AssumeUTXO load and flush show:
* Small dbcache (450MB): ~1-3% performance improvement (despite more frequent flushes)
* Large dbcache (4500MB): ~6-8% performance improvement due to fewer heap allocations (and basically the number of flushes)
* Very large dbcache (4500MB): ~5-6% performance improvement due to fewer heap allocations (and memory limit not being reached, so there's no memory penalty)
Full IBD and reindex-chainstate with larger `dbcache` values also show an overall ~3-4% speedup.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Verifies that script types are correctly allocated using prevector's direct or indirect storage based on their size:
Direct allocated script types (size ≤ 28 bytes):
* OP_RETURN (small)
* P2WPKH
* P2SH
* P2PKH
Indirect allocated script types (size > 28 bytes):
* P2WSH
* P2TR
* P2PK
* MULTISIG (small)
This test provides a baseline for verifying changes to prevector's inline capacity.
The `CHECK_SCRIPT_STATIC_SIZE` and `CHECK_SCRIPT_DYNAMIC_SIZE` macros were added to differentiate the two cases - while preserving the correct source code line in case of failure.
1cb23997033c395d9ecd7bf2f54787b134485f41 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f618b04e0140ec2c50c6e95c2a2e2e4 p2p: rename GetAddresses -> GetAddressesUnsafe (Daniela Brozzoni)
Pull request description:
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.
Additionally, better reflect in the documentation that the two methods should be used in different contexts.
Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
l0rinc:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
luisschwab:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
brunoerg:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
theStack:
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
mzumsande:
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
face8123fdc10549676c6679ee3225c178a7f30c log: [refactor] Use info level for init logs (MarcoFalke)
fa183761cb09d916ed2a3bbab71b80c5c7942a30 log: Remove function name from init logs (MarcoFalke)
Pull request description:
Many of the normal, and expected init logs, which are run once after startup use the deprecated alias of `LogInfo`.
Fix that by using `LogInfo` directly, which is a refactor, except for a few log lines that also have `__func__` removed.
(Also remove the unused trailing `\n` char while touching those logs)
ACKs for top commit:
stickies-v:
re-ACK face8123fdc10549676c6679ee3225c178a7f30c
fanquake:
ACK face8123fdc10549676c6679ee3225c178a7f30c
Tree-SHA512: 28c296129c9a31dff04f529c53db75057eae8a73fc7419e2f3068963dbb7b7fb9a457b2653f9120361fdb06ac97d1ee2be815c09ac659780dff01d7cd29f8480
fa1a14a13a15ecfb7587a94ee86b4ace7c819519 fuzz: Reset chainman state in process_message(s) targets (MarcoFalke)
fa9a3de09b4c6eef40f1073f09c9a0bd1858adf2 fuzz: DisableNextWrite (MarcoFalke)
aeeeeec9f749dddeaf8eaa357b69cd45ed3dd76c fuzz: Reset dirty connman state in process_message(s) targets (MarcoFalke)
fa11eea4059a608f591db4469c07a341fd33a031 fuzz: Avoid non-determinism in process_message(s) target (PeerMan) (MarcoFalke)
Pull request description:
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
### Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism re-appear.
ACKs for top commit:
marcofleon:
ReACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
dergoegge:
reACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
Tree-SHA512: 37b5b6dbdde6a39b4f83dc31e92cffb4a62a4b8f5befbf17029d943d0b2fd506f4a0833570dcdbf79a90b42af9caca44e98e838b03213d6bc1c3ecb70a6bb135
06ab3a394ade1e0d4fdb1fef636ff0d6bad71948 tests: speed up coins_tests by parallelizing (Anthony Towns)
Pull request description:
Updates the cmake logic to generate a separate test for each BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp into three separate suites so that they can be run in parallel. Also updates the convention enforced by test/lint/lint-tests.py.
ACKs for top commit:
l0rinc:
reACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
maflcko:
lgtm ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
achow101:
ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
Tree-SHA512: 940d9aa31dab60d1000b5f57d8dc4b2c5b4045c7e5c979ac407aba39f2285d53bc00c5e4d7bf2247551fd7e1c8681144e11fc8c005a874282c4c59bd362fb467
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
Updates the cmake logic to generate a separate test for each
BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp
into three separate suites so that they can be run in parallel. Also
updates the convention enforced by test/lint/lint-tests.py.
31c4e77a256c15c221dcc278883f6d3be55660b4 test: fix ReadTopologicalSet unsigned integer overflow (ismaelsadeeq)
Pull request description:
This PR is a simple fix for a potential unsigned integer overflow in ReadTopologicalSet.
We obtain the value of `mask` from fuzz input, which can be the maximum representable value.
Adding 1 to it would then cause an overflow.
The fix skips the addition when the read value is already the maximum.
See https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569 for more context
ACKs for top commit:
maflcko:
lgtm ACK 31c4e77a256c15c221dcc278883f6d3be55660b4
Tree-SHA512: f58d7907f66a0de0ed8d4b1cad6a4971f65925a99f3c030537c21c4d84126b643257c65865242caf7d445b9cbb7a71a1816a9f870ab7520625c4c16cd41979cb
96da68a38fa295d2414685739c41b8626e198d27 qa: functional test a transaction running into the legacy sigop limit (Antoine Poinsot)
367147954d16c961bbd28c361abf27b4cb665f10 qa: unit test standardness of inputs packed with legacy sigops (Antoine Poinsot)
5863315e33ba9b75a1e5189ee3da3d7311bbf193 policy: make pathological transactions packed with legacy sigops non-standard. (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should make transactions that are not valid
according to the new rules non-standard first because it would otherwise be a trivial DoS to
potentially unupgraded miners after the soft fork activates.
ML post: https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u
ACKs for top commit:
instagibbs:
reACK 96da68a38f
maflcko:
review ACK 96da68a38fa295d2414685739c41b8626e198d27 🚋
achow101:
ACK 96da68a38fa295d2414685739c41b8626e198d27
glozow:
light code review ACK 96da68a38fa, looks correct to me
Tree-SHA512: 106ffe62e48952affa31c5894a404a17a3b4ea8971815828166fba89069f757366129f7807205e8c6558beb75c6f67d8f9a41000be2f8cf95be3b1a02d87bfe9
This is required in the process_message(s) fuzz targets to avoid leaking
the next write time from one run to the next. Also, disable it
completely because it is not needed and due to leveldb-internal
non-determinism.
The PeerManager has several members, such as the FastRandomContext,
which need to be reset before every run to avoid leaking state from one
run into the next.
Also, style fixups in p2p_handshake.cpp, where this code is copied from.
This is meant to focus the usages to narrow the scope of the obfuscation optimization.
`Obfuscation::Xor` is mostly a move.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Mechanical refactor of the low-level "xor" wording to signal the intent instead of the implementation used.
The renames are ordered by heaviest-hitting substitutions first, and were constructed such that after each replacement the code is still compilable.
-BEGIN VERIFY SCRIPT-
sed -i \
-e 's/\bGetObfuscateKey\b/GetObfuscation/g' \
-e 's/\bxor_key\b/obfuscation/g' \
-e 's/\bxor_pat\b/obfuscation/g' \
-e 's/\bm_xor_key\b/m_obfuscation/g' \
-e 's/\bm_xor\b/m_obfuscation/g' \
-e 's/\bobfuscate_key\b/m_obfuscation/g' \
-e 's/\bOBFUSCATE_KEY_KEY\b/OBFUSCATION_KEY_KEY/g' \
-e 's/\bSetXor(/SetObfuscation(/g' \
-e 's/\bdata_xor\b/obfuscation/g' \
-e 's/\bCreateObfuscateKey\b/CreateObfuscation/g' \
-e 's/\bobfuscate key\b/obfuscation key/g' \
$(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
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>
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.
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
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
d7fca5c171f450621112457ea6f7c99b2e21d354 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de65026c46f70cb91e3cb2213c336be0 clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed18a0a3defbed33f6bd355929f67b48 clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29b7e6a1e03de9697a6bdf6fbe944ef7 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33e6fe63c4f791de7aacfb912059e394 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d2401d0d7b5c3466d66a4b09e51ccb0 clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac390835d3d9c82d197f58e992ccc584f clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f8d928b3893e2910ce0c400a6fd1928 clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef9cff6fa63dc15adbe22e68f76aac58 clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e10a61af80e3bca1e1ae04de978d5c0 clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee704b4a83d56135ed10cf2adf4b3c18af clusterlin tests: count SimpleCandidateFinder iterations better (Pieter Wuille)
Pull request description:
Part of the cluster mempool project: #30289
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ought to find that in a test specific to `SimpleCandidateFinder` rather than as a side-effect of testing `SearchCandidateFinder`. Split this functionality out into a new `clusterlin_simple_finder`.
* `clusterlin_linearize`: establishes the correctness of `Linearize` by comparing against both `SimpleLinearize` and literally every valid linearization for the cluster. Again, if `SimpleLinearize` works correctly, then this comparison with all valid linearizations is redundant, and if it isn't we should find it in a test for `SimpleLinearize`. Do so by splitting off that functionality into `clusterlin_simple_linearize`.
After that, a few general improvements to the affected tests are made (comparing with linearizations and subsets read from the fuzz input, plus a performance improvement).
ACKs for top commit:
marcofleon:
Re ACK d7fca5c171f450621112457ea6f7c99b2e21d354
ismaelsadeeq:
re-ACK d7fca5c171f450621112457ea6f7c99b2e21d354
monlovesmango:
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
8cc3ac6c2328873e57c31f237d194c5f22b6748a validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5469e6476dc87f54b8ac25074603f0c test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
TheCharlatan:
Re-ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
The std::source_location conveniently stores the file name, line number,
and function name of a source code location. We switch to using it instead
of the __func__ identifier and the __FILE__ and __LINE__ macros.
BufferedLog is changed to have a std::source_location member, replacing the
source_file, source_line, and logging_function members. As a result,
MemUsage no longer explicitly counts source_file or logging_function as the
std::source_location memory usage is included in the MallocUsage call.
This also changes the behavior of -logsourcelocations as std::source_location
includes the entire function signature. Because of this, the functional test
feature_config_args.py must be changed to no longer include the function
signature as the function signature can differ across platforms.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
LogRateLimiter will be used to keep track of source locations and our
current time-based logging window. It contains an unordered_map and a
m_suppressions_active bool to track source locations. The map is keyed
by std::source_location, so a custom Hash function (SourceLocationHasher)
and custom KeyEqual function (SourceLocationEqual) is provided.
SourceLocationHasher uses CSipHasher(0,0) under the hood to get a
uniform distribution.
A public Reset method is provided so that a scheduler (e.g. the
"b-scheduler" thread) can periodically reset LogRateLimiter's state when
the time window has elapsed.
The LogRateLimiter::Consume method checks if we have enough available
bytes in our rate limiting budget to log an additional string. It
returns a Status enum that denotes the rate limiting status and can
be used by the caller to emit a warning, skip logging, etc.
The Status enum has three states:
- UNSUPPRESSED (logging was successful)
- NEWLY_SUPPRESSED (logging was succcesful, next log will be suppressed)
- STILL_SUPPRESSED (logging was unsuccessful)
LogLimitStats counts the available bytes left for logging per source
location for the current logging window. It does not track actual source
locations; it is used as a value in m_source_locations.
Also exposes a SuppressionsActive() method so the logger can use
that in a later commit to prefix [*] to logs whenenever suppressions
are active.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
We mark ~DebugLogHelper as noexcept(false) to be able to catch the
exception it throws. This lets us use it in test in combination with
BOOST_CHECK_THROW and BOOST_CHECK_NO_THROW to check that certain log
messages are (not) logged.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
