fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce test: Fix typo (MarcoFalke)
fac932698f6539e9ac4a13df51194bcb60d5d933 ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke)
Pull request description:
This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e61759f689615a304254fed33cda7f895e.
Otherwise, the CI will fail:
```
node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running.
```
(https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605)
Also, include a random second commit, so that the CI task is run in this pull.
ACKs for top commit:
l0rinc:
ACK fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce
willcl-ark:
ACK fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce
Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
fa6af856341384e4a84c5674e66fe7c1f13dd73c refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke)
fa692974ac2d69e01091f03625cd8a227e310065 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke)
Pull request description:
The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB).
Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows)
This can be tested by a command that fails valgrind before the change and passes after:
```
echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
ACKs for top commit:
achow101:
ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
l0rinc:
lightly tested code review ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
sedited:
ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
408d5b12e80151feded31c2a5509e2dc5f15edf3 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4f3049b0e742499b762f7c13bb006cc test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee324d37ba1dd7c2c4e27e40e0508bedc test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d0324b65121935b7c8a285c6421fe74a6 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af02d850a1f9257f18610cfb9de9cb828 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb9b318bf1fa00a3499f514c5ebe9bb6 test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315774f0ed65da633ba99899fd0dad740 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa22d253b9367c11b0a198bd4244ad2f threadpool: active-wait during shutdown (furszy)
e88d2744301a434064714f0a21e1395d41ac3984 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363fb85f5487a19ace82aa0d12d5fab450 threadpool: guard against Start-Stop race (furszy)
9ff1e82e7dbdf31ddf1c534853da4581a1f41bd5 test: cleanup, block threads via semaphore instead of shared_future (l0rinc)
Pull request description:
A few follow-ups to #33689, includes:
1) `ThreadPool` active-wait during shutdown:
Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.
2) Decouple `HasReason` from the unit test framework machinery
This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.
3) Include response body in non-JSON HTTP error messages
Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.
ACKs for top commit:
maflcko:
review ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3 🕗
achow101:
ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3
hodlinator:
re-ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3
Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
Avoid providing the entire unit test framework dependency to tests that only
require access to the HasReason utility class.
E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp,
and script_parse_tests.cpp only require access to HasReason and nothing else.
Move the operator<< overloads used by BOOST_CHECK_* out of the
unit test machinery test/setup_common, into test/util/common.h.
And replace the individual per-type ToString() overloads with
a single concept-constrained template that covers any type
exposing a ToString() method. This is important to not add
uint256.h and transaction_identifier.h dependencies to the
shared test/util/common.h file.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Instead of waiting for the workers to finish processing tasks, help
them actively inside Stop().
This speeds up the JSON-RPC and REST server shutdown procedure,
and results in a faster node shutdown when many requests remain unhandled
e5f0613503b6973dbc886eba8e999f208d84853b net processing: Check if we are in ibd before processing block for txdownloadman (sedited)
ce8b692897f6aacbe936fe2220e85f23cd83cbf2 Add functional test exercising tx downloadman recently confirmed filter (Lőrinc)
Pull request description:
Calculating the rolling bloom filters for the txorphanage takes some CPU time from the scheduler thread. This can be observed for example in [this flamegraph](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-172/20066462508/mainnet-default-instrumented-base-flamegraph.svg?x=920203898521&y=780), where handling the filter takes about 2.6% of total time (and most of the scheduler thread's time).
During ibd the entries in the tx download bloom filter are just continuously rolled over and aren't consumed, since no mempool entries are created by incoming transactions from peers during ibd. The mempool does accept transactions via RPC, or the wallet at the time, however these don't interact with the orphanage and the txdownloadman, because adding anything to those is guarded by IsInitialBlockDownload() checks as well.
We're usually latching ibd to false a few blocks before catching up to the tip, so this should also not significantly degrade the performance of the filter once fully caught up.
ACKs for top commit:
l0rinc:
ACK e5f0613503b6973dbc886eba8e999f208d84853b
instagibbs:
ACK e5f0613503b6973dbc886eba8e999f208d84853b
fjahr:
Code review ACK e5f0613503b6973dbc886eba8e999f208d84853b
Tree-SHA512: d667e677f5723c438cdf5b34f0f9c1ade7cc1b2e98530c23f14384514daa38217c4e7c3b756194b6831b590a487449c4514b52bf0fb461ae8083061722824270
33fbaed310a6a37d41d26af8fb34308d088d72c8 policy: don't CheckEphemeralSpends on reorg (Greg Sanders)
Pull request description:
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion. On reorg, this could cause each subsequent "generation" to be rejected. These rejected transactions may contain a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relayed them prior to the reorg.
ACKs for top commit:
darosior:
re-ACK 33fbaed310a
ismaelsadeeq:
reACK 33fbaed310a6a37d41d26af8fb34308d088d72c8
sedited:
ACK 33fbaed310a6a37d41d26af8fb34308d088d72c8
Tree-SHA512: cf0a6945066e9f5f8f9a847394c2c1225facf475a8aa4bc811b436513eff79c0a720d4ad21ba6b0f1cc4dfdd61cf46acb148333ac592b2ee252953732326ad1d
744d47fcee0d32a71154292699bfdecf954a6065 clusterlin: adopt trained cost model (feature) (Pieter Wuille)
4eefdfc5b7d0b86a523683de2a90da910b77a106 clusterlin: rescale costs (preparation) (Pieter Wuille)
ecc9a84f854e5b77dfc8876cf7c9b8d0f3de89d0 clusterlin: use 'cost' terminology instead of 'iters' (refactor) (Pieter Wuille)
9e7129df2962f7c52d07c14a56398bb285cac084 clusterlin: introduce CostModel class (preparation) (Pieter Wuille)
Pull request description:
Part of #30289, replaces earlier #34138.
This introduces a more accurate cost model for SFL, to control how much CPU time is spent inside the algorithm for clusters that cannot be linearized perfectly within a reasonable amount of time.
The goal is having a metric for the amount of work performed, so that txmempool can impose limits on that work: a lower bound that is always performed (unless optimality is reached before that point, of course), and an upper bound to limit the latency and total CPU time spent on this. There are conflicting design goals here:
* On the one hand, it seems ideal if this metric is closely correlated to actual CPU time, because otherwise the limits become inaccurate.
* On the other hand, it seems a nightmare to have the metric be platform/system dependent, as it makes network-wide reasoning nearly impossible. It's expected that slower systems take longer to do the same thing; this holds for everything, and we don't need to compensate for this.
There are multiple solutions to this:
* One extreme is just measuring the time. This is very accurate, but extremely platform dependent, and also non-deterministic due to random scheduling/cache effects.
* The other extreme is using a very abstract metric like counting how many times certain loops/function inside the algorithm run. That is what is implemented in master right now, just counting the sum of the numbers of transactions updated across all `UpdateChunks()` calls. It however necessarily fails to account for significant portions of runtime spent elsewhere, resulting in a rather wide range of "ns per cost" values.
* This PR takes a middle ground, counting many function calls / branches / loops, with weights that were determined through benchmarking on an average on a number of systems.
Specifically, the cost model was obtained by:
* For a variety of machines:
* Running a fixed collection of ~385000 clusters found through random generation and fuzzing, optimizing for difficulty of linearization.
* Linearize each 1000-5000 times, with different random seeds. Sometimes without input linearization, sometimes with a bad one.
* Gather cycle counts for each of the operations included in this cost model, broken down by their parameters.
* Correct the data by subtracting the runtime of obtaining the cycle count.
* Drop the 5% top and bottom samples from each cycle count dataset, and compute the average of the remaining samples.
* For each operation, fit a least-squares linear function approximation through the samples.
* Rescale all machine expressions to make their total time match, as we only care about relative cost of each operation.
* Take the per-operation average of operation expressions across all machines, to construct expressions for an average machine.
* Approximate the result with integer coefficients.
The benchmarks were performed by `l0rinc <pap.lorinc@gmail.com>` and myself, on AMD Ryzen 5950X, AMD Ryzen 7995WX, AMD Ryzen 9980X, Apple M4 Max, Intel Core i5-12500H, Intel Core Ultra 7 155H, Intel N150 (Umbrel), Intel Core i7-7700, Intel Core i9-9900K, Intel Haswell (VPS, virtualized), Intel Xeon E5-2637, ARM Cortex-A76 (Raspberry Pi 5), ARM Cortex-A72 (Raspberry Pi 4).
Based on final benchmarking, the "acceptable" iteration count (which is the minimum spent on every cluster) is to 75000 units, which corresponds to roughly 50 μs on Ryzen 5950X and similar modern desktop hardware.
ACKs for top commit:
instagibbs:
ACK 744d47fcee0d32a71154292699bfdecf954a6065
murchandamus:
reACK 744d47fcee0d32a71154292699bfdecf954a6065
Tree-SHA512: 5cb37a6bdd930389937c435f910410c3581e53ce609b9b594a8dc89601e6fca6e6e26216e961acfe9540581f889c14bf289b6a08438a2d7adafd696fc81ff517
3281824ecfa72c4f69ab69c94003b7f5a82c7265 fuzz: prevent invalid `FRESH` entries and surface `BatchWrite` errors (Lőrinc)
780f460635af86b91c4215e761b6895be762ed3e fuzz: avoid invalid `AddCoin` overwrites (Lőrinc)
d7e0d510f2bf2981e92e3b323aeba1c845377950 fuzz: make `AddCoins` query view for overwrites (Lőrinc)
b8fa6f0f701f04cffca6a085337b508381016649 util: introduce `TrySub` to prevent unsigned underflow (Lőrinc)
Pull request description:
### Problem
This is an alternative approach to #34647, fixes#34645.
### Fix
First, add `CheckedSub` and use it for decrements of `m_dirty_count` and `cachedCoinsUsage`, so unsigned underflows turn into immediate failures instead of silently wrapping and only failing later.
<details><summary>Assertion `j <= i' failed.</summary>
```bash
util/overflow.h:44 T CheckedSub(const T, const U) [T = unsigned long, U = bool]: Assertion `j <= i' failed.
==72817== ERROR: libFuzzer: deadly signal
#0 0x556e9225eab5 in __sanitizer_print_stack_trace (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x191dab5) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#1 0x556e921acafc in fuzzer::PrintStackTrace() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186bafc) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#2 0x556e92191bb7 in fuzzer::Fuzzer::CrashCallback() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1850bb7) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#3 0x7164cfc458cf (/lib/x86_64-linux-gnu/libc.so.6+0x458cf) (BuildId: ae7440bbdce614e0e79280c3b2e45b1df44e639c)
#4 0x7164cfca49bb in __pthread_kill_implementation nptl/pthread_kill.c:43:17
#5 0x7164cfca49bb in __pthread_kill_internal nptl/pthread_kill.c:89:10
#6 0x7164cfca49bb in pthread_kill nptl/pthread_kill.c💯10
#7 0x7164cfc4579d in raise signal/../sysdeps/posix/raise.c:26:13
#8 0x7164cfc288cc in abort stdlib/abort.c:73:3
#9 0x556e92f9d591 in assertion_fail(std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.cpp:41:5
#10 0x556e9250daf0 in bool&& inline_assertion_check<false, bool>(bool&&, std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.h:90:13
#11 0x556e9250daf0 in unsigned long CheckedSub<unsigned long, bool>(unsigned long, bool) /mnt/my_storage/bitcoin/src/util/overflow.h:44:5
#12 0x556e9250daf0 in CoinsViewCacheCursor::NextAndMaybeErase(std::pair<COutPoint const, CCoinsCacheEntry>&) /mnt/my_storage/bitcoin/src/coins.h:282:25
#13 0x556e92507eb2 in (anonymous namespace)::MutationGuardCoinsViewCache::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:90:75
#14 0x556e92c17a2b in CCoinsViewCache::Flush(bool) /mnt/my_storage/bitcoin/src/coins.cpp:282:11
#15 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1::operator()() const /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:135:34
#16 0x556e924fb732 in unsigned long CallOneOf<TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11>(FuzzedDataProvider&, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11) /mnt/my_storage/bitcoin/src/test/fuzz/util.h:42:27
#17 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:114:9
#18 0x556e92503b0c in coins_view_overlay_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:404:5
#19 0x556e92bcb7a5 in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/lib/gcc/x86_64-linux-gnu/15/../../../../include/c++/15/bits/std_function.h:593:9
#20 0x556e92bcb7a5 in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:88:5
#21 0x556e92bcb7a5 in LLVMFuzzerTestOneInput /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:216:5
#22 0x556e9219318f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x185218f) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#23 0x556e92192799 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1851799) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#24 0x556e92194139 in fuzzer::Fuzzer::MutateAndTestOne() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853139) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#25 0x556e92194c95 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853c95) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#26 0x556e92181255 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1840255) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#27 0x556e921ad696 in main (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186c696) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#28 0x7164cfc2a577 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#29 0x7164cfc2a63a in __libc_start_main csu/../csu/libc-start.c:360:3
#30 0x556e921757e4 in _start (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x18347e4) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
NOTE: libFuzzer has rudimentary signal handlers.
Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 2 PersAutoDict-CopyPart- DE: "\005\000"-; base unit: ecb626aff8724f0fdde38a0a6965718f2096d474
artifact_prefix='/tmp/fuzz_artifacts/'; Test unit written to /tmp/fuzz_artifacts/crash-1d19026c1a23f08bfe693fd684a56ce51187c6e5
./build_fuzz/bin/fuzz /tmp/fuzz_corpus/coins_view_overlay -max_total_time=3600 -rss_limit_mb=2560 -artifact_prefix=/tmp/fuzz_artifacts/ >fuzz-16.log 2>&1
```
</details>
The coins view fuzz targets can call `AddCoin`/`AddCoins` and construct `BatchWrite` cursors in ways that violate `CCoinsViewCache` caller contracts. These invalid states can trigger `BatchWrite` `std::logic_error` and can desync dirty-entry accounting (caught by `Assume(m_dirty_count == 0)` currently).
Make the fuzzer avoid generating invalid states instead of catching and resetting:
* Derive `AddCoin`’s `possible_overwrite` from `PeekCoin`, so `possible_overwrite=false` is only used when the outpoint is absent - similarly to 67c0d1798e/src/test/fuzz/coinscache_sim.cpp (L312-L317)
- Only use `AddCoins(check=false)` when we have confirmed the txid has no unspent outputs; otherwise fall back to `check=true` so `AddCoins` determines overwrites via the view.
- When constructing a `CoinsViewCacheCursor`, avoid setting `FRESH` when the parent already has an unspent coin, and ensure `FRESH` implies `DIRTY`.
### Fuzzing
The original error could be reproduced in ~10 minutes using `coins_view_overlay`. I ran the `coins_view`, `coins_view_db`, `coins_view_overlay`, and `coinscache_sim` fuzzers for this PR overnight and they didn't fail anymore.
ACKs for top commit:
achow101:
ACK 3281824ecfa72c4f69ab69c94003b7f5a82c7265
sipa:
ACK 3281824ecfa72c4f69ab69c94003b7f5a82c7265. Ran the 4 relevant fuzz tests for ~1 CPU day each. Will run more overnight.
andrewtoth:
ACK 3281824ecfa72c4f69ab69c94003b7f5a82c7265
Tree-SHA512: b8155e8d21740eb7800e373c27a8a1457eb84468c24af879bac5a1ed251ade2aec99c34a350a31f2ebb74e41bb7380bf20214d38d14fe23310a43282d2434fb7
50cf6838e6aa51e0d712cbc1e13d686253bc8fe0 wallet: rpc: manpage: fix example missing `fee_rate` argument (SomberNight)
Pull request description:
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index. The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
ACKs for top commit:
svanstaa:
tACK 50cf6838e6
kannapoix:
Tested ACK 50cf6838e6aa51e0d712cbc1e13d686253bc8fe0
achow101:
ACK 50cf6838e6aa51e0d712cbc1e13d686253bc8fe0
rkrux:
tACK 50cf6838e6aa51e0d712cbc1e13d686253bc8fe0
theStack:
Tested ACK 50cf6838e6aa51e0d712cbc1e13d686253bc8fe0
Tree-SHA512: 499701729038cd863b612698098a73ce995589fc5ab08a2962f8edf1ff3cb3de6f7090e04722ca13ba7707a566fa3750ae549b6ad55750a3d01127eb6b94a79f
Stop() has two windows where Start() could cause troubles:
1) m_workers is temporarily empty while workers are being joined,
this creates a window where Start() could slip through and reset
m_interrupt to false, preventing the old workers from exiting and
causing a deadlock.
2) Start() could be called after workers are joined but before the
empty() sanity check on m_work_queue, causing a crash.
Fix both races by keeping m_interrupt set for the entire duration
of Stop(), so any concurrent Start() call is rejected until all
workers have exited.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
e8f8b74a46aa075bf6c74c104fd572cc89d3b53b test: index, improve txospenderindex_initial_sync() test code (furszy)
ac3bea07cdceac9e316448a9a5f190848156efd5 test: improve rpc_gettxspendingprevout.py code (furszy)
Pull request description:
Fixes#34637.
Was reviewing #34637 and, while reading the new txospender index
test code for the first time, found it could use some cleanups. Finding
stuff in there is harder than it should be due to the amount of dup code.
The first commit cleans up `rpc_gettxspendingprevout.py` by introducing
helper functions to avoid repeating the same dicts everywhere, using
for-loops instead of duplicating the same checks for each node, and
renaming variables to better reflect what they actually represent.
The second commit reorganizes `txospenderindex_initial_sync()`
moving index initialization after the test setup phase, since the index
doesn't participate in it anyway. It adds a post-sync check to catch
cases where `Sync()` aborted prematurely.
Note:
This is just a pre-work for deeper index changes I'm cooking.
ACKs for top commit:
achow101:
ACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b
sedited:
Re-ACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b
w0xlt:
reACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b
Tree-SHA512: 3f7026712ab20a43f376afa28c683dcd5daec8ed1bbf1c36d7ec6bbf231f468d4de74efae4aa8295ff3afb83986286ccaf31c03b34e45fc9971652f064791ed0
This parametrizes the cost model for the SFL algorithm with another
class. Right now, the behavior of that class matches the naive cost
model so far, but it will be replaced with a more advanced on in a
future commit.
The reason for abstracting this out is that it makes benchmarking for
creating such cost models easy, by instantiating the cost model class
with one that tracks time.
The index is now initialized after the setup phase (chain generation
and txs creation), since it doesn't participate on it at all.
This improves readability and splits setup from what we actually
want to check.
This also adds a check after Sync() to verify the index best block hash
matches the tip, so we know it fully synced before checking the
processed data. This will help catching errors as Sync() could have
aborted prematurely.
As a happy side effect, the SyncWithValidationInterfaceQueue() call at
the end of the test is no longer needed and has been removed.
fcaec2544b32226fd5357a88506fe080058d25bc doc: release note for IPC cooldown and interrupt (Sjors Provoost)
1e82fa498cf4881466f0539146c101242b9dc30d mining: add interrupt() (Sjors Provoost)
a11297a9048e0d910915e1a37b2be467c057a78d mining: add cooldown argument to createNewBlock() (Sjors Provoost)
Pull request description:
As reported in #33994, connected mining clients will receive a flood of new templates if the node is still going through IBD or catching up on the last 24 hours. This PR fixes that using an _optional_ cooldown mechanism, only applied to `createNewBlock()`.
First, cooldown waits for IBD. Then, as the tip keeps moving forward, it waits a few seconds to see if the tip updated. If so, it restarts the timer and waits again. The trade-offs for this mechanism are explained below.
Because this PR changes `createNewBlock()` from a method that returns quickly to one that can block for minutes, we rely on #34568 to fix a bug in our `.capnp` definition, adding the missing `context` to `createNewBlock` (and `checkBlock`).
The second commit then adds an `interrupt()` method so that clients can cleanly disconnect.
---
## Rationale
The cooldown argument is optional, and not used by internal non-IPC code, for two reasons:
1. The mechanism wreaks havoc on the functional test suite, which would require very careful mock time handling to work around. But that's pointless, because only IPC clients need it.
2. It needs to be optional for IPC clients too, because in some situations, like a signet with only one miner, waiting for IBD can mean being stuck forever.
The reason it's only applied to `createNewBlock()` is that this is the first method called by clients; `waitNext()` is a method on the interface returned by `createNewBlock()`, at which point the cooldown is done.
After IBD, we wait N seconds if the header is N blocks ahead of the tip, with a minimum of 3 and a maximum of 20 seconds. The minimum waiting time is short enough that it shouldn't be annoying or confusing for someone manually starting up a client. While the maximum should be harmless if it happens spuriously (which it shouldn't).
If the minimum wait is too short, clients get a burst of templates, as observed in the original issue. We can't entirely rule this out without a lot of additional complexity (like scanning our own log file for heuristics). This PR should make it a lot less likely, and thanks to the IBD wait also limit it to one day worth of blocks (`-maxtipage`).
Some test runs on an M4 MacBook Pro, where I had a node catch up on the last few days worth of blocks:
<img width="872" height="972" alt="Schermafbeelding 2026-02-04 om 18 21 17" src="https://github.com/user-attachments/assets/7902a0f2-0e0b-4604-9688-cec2da073261" />
As the chart shows, sometimes it takes longer than 3 seconds. But it turns out that in all those cases there were quite a few headers ahead of the tip. It also demonstrates that it's important to first wait for IBD, because it's less likely a random tip update takes longer than 20 seconds.
- modified sv2-apps: https://github.com/Sjors/sv2-apps/tree/2026/02/cooldown
- test script: https://gist.github.com/Sjors/feb6122c97acc2b9e6d66b168614609c#file-run_mainnet_pool_loop-zsh
- chart script: https://gist.github.com/Sjors/feb6122c97acc2b9e6d66b168614609c#file-tip_interval_charts-py
ACKs for top commit:
ryanofsky:
Code review ACK fcaec2544b32226fd5357a88506fe080058d25bc. Only changes since last review were removing two cooldown arguments from the mining IPC test to simplify it
enirox001:
ACK fcaec2544b
Tree-SHA512: 08b75470f7c5c80a583a2fdb918fad145e7d5377309e5c599f67fc0d0e3139d09881067ba50c74114f117e69da17ee50666838259491691c031b1feaf050853f
a9e59f7d955f995078b3e0bf3b527c03c74fef8d rpc: add optimal result to getmempoolinfo (Greg Sanders)
a3fb3dd55c2326452a5085add220bd3682052352 mempool: log if we detect a non-optimal mempool (Greg Sanders)
Pull request description:
Post-SFL #34023 I don't think we expect the mempool to be unordered for long periods of time. If we consider it likely to be a serious regression in production, it would be useful to expose the fact that the mempool is not known to be optimal.
1. do a MEMPOOL log after any `DoWork()` returns false, meaning non-optimal
2. expose it via getmempoolinfo, by calling `DoWork(0)`, which does nothing but return known-optimality
I'm not wedded to either approach, I just think something is better than nothing for the next release.
ACKs for top commit:
ajtowns:
ACK a9e59f7d955f995078b3e0bf3b527c03c74fef8d
ismaelsadeeq:
reACK a9e59f7d955f995078b3e0bf3b527c03c74fef8d [c89b93b95..a9e59f7d95](c89b93b958..a9e59f7d95) fixed typo, added more logging for block/reorg additions to mempool, and fixed brittle test case.
sedited:
ACK a9e59f7d955f995078b3e0bf3b527c03c74fef8d
sipa:
ACK a9e59f7d955f995078b3e0bf3b527c03c74fef8d
Tree-SHA512: 1560ad21cc1606df7279c102f35f61d4555c0ac920f02208b2a6eb89b14d7e22befb6d7f510a00a9074c2f9931f32e9af86bcea3a8dd9a1d947b0398c84666dd
Modify fuzzer logic to avoid setting `FRESH` for an outpoint that already exists unspent in the parent view, and ensure `FRESH` implies `DIRTY`.
This keeps cursor invariants realistic and lets `BatchWrite` failures expose real bugs without resetting state.
The coins view fuzzer can call `AddCoin` with `possible_overwrite=false` for an outpoint that already exists unspent in the view, which violates the `AddCoin` caller contract.
Derive `possible_overwrite` from `PeekCoin` so `possible_overwrite=false` is only used when the outpoint is absent.
This matches the approach used by the `coinscache_sim` fuzzer, which derives the overwrite flag from simulated state.
In validation, `AddCoins(check_for_overwrite=false)` is only used after BIP30 has already ensured the transaction does not overwrite any unspent outputs in the UTXO view.
The coins view fuzz target can call `AddCoins` with arbitrary txids, so using the `check_for_overwrite=false` fast path on non-coinbase transactions may violate the `AddCoin` caller contract and trigger logic errors.
Only use `check_for_overwrite=false` when we have first confirmed that none of the outputs are currently unspent.
Otherwise, fall back to `check_for_overwrite=true` so `AddCoins` determines overwrites via the view.
Introduce `TrySub(T&, U)` which subtracts an unsigned integral `U` from an unsigned integral `T`, returning `false` on underflow.
Use with `Assume(TrySub(...))` at coins cache accounting decrement sites so invariant violations fail immediately rather than silently wrapping.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
This avoids wasting work on calculating bloom filters that aren't
consumed during ibd and continuously re-calculated as now blocks get
validated.
Also update the functional test to document that transactions would now
be requested again once out of IBD.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
4b53cbd69220c1c786bb23a72c0b26a6f78a38f7 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15cb3269015523e6fab8ae9241f4181a1 miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fbc497b657025afe48d0dfbf103ee120 descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86e2298ba6cdd79d55c9d6e23e5136a5 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c5e99e3df1c20d5c0ae8278a714b9f8 test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
sipa:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
darosior:
Other than that, Approach ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
fd06157d1465d93b960e8be6e8e419295abde9a1 test: Add coverage for restarted node without any block sync (Fabian Jahr)
3d7ab7ecb7dfcdfb8aaa45869388887b948841c8 rpc, test: Address feedback from #29668 (Fabian Jahr)
312919c9dd5dba7da20317604e1638bdc5010f14 test: Indices can not start based on block data without undo data (Fabian Jahr)
a9a3b29dd687b4c355e131fefc145e8e48b48b17 index: Check availability of undo data for indices (Fabian Jahr)
881ab4fc82fe3cf36b227cf1ba704448df160745 support multiple block status checks in CheckBlockDataAvailability (furszy)
Pull request description:
Currently, we check that `BLOCK_HAVE_DATA` is available for all blocks an index needs to sync during startup. However, for `coinstatsindex` and `blockfilterindex` we also need the undo data for these blocks. If that data is missing in the blocks, we are currently still starting to sync each of these indices and then crash later when we encounter the missing data.
This PR adds explicit knowledge of which block data is needed for each index and then checks its availability during startup before initializing the sync process on them.
This also addresses a few open comments from #29668 in the last commit.
ACKs for top commit:
achow101:
ACK fd06157d1465d93b960e8be6e8e419295abde9a1
furszy:
utACK fd06157d1465d93b960e8be6e8e419295abde9a1
sedited:
Re-ACK fd06157d1465d93b960e8be6e8e419295abde9a1
Tree-SHA512: e2ed81c93372b02daa8ddf2819df4164f96d92de05b1d48855410ecac78d5fcd9612d7f0e63a9d57d7e75a0b46e1bea278e43ea87f2693af0220d1f9c600e416
Both waitTipChanged() and createNewBlock() can take a long time to
return. Add a way for clients to interrupt them.
The new m_interrupt_mining is safely accessed with a lock on
m_tip_block_mutex, but it has no guard annotation. A more thorough
solution is discussed here:
https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2743566474
At startup, if the needs to catch up, connected mining clients will
receive a flood of new templates as new blocks are connected.
Fix this by adding a cooldown argument to createNewBlock(). When set
to true, block template creation is briefly paused while the best
header chain is ahead of the tip.
This wait only happens when the best header extends the current tip,
to ignore competing branches.
Additionally, cooldown waits for isInitialBlockDownload() to latch to
false, which happens when there is less than a day of blocks left to sync.
When cooldown is false createNewBlock() returns immediately. The argument
is optional, because many tests are negatively impacted by this
mechanism, and single miner signets could end up stuck if no block
was mined for a day.
The getblocktemplate RPC also opts out, because it would add a delay
to each call.
Fixes#33994
We expect this to be rare in practice, and to not be the
usual state of the mempool. If we we detect non-optimal
ordering after a DoWork() invocation, allow this to be
observed in MEMPOOL logs.
f700609e8ada3b48fd45ec19979cd72d943d47a6 doc: Release notes for mining IPC interface bump (Ryan Ofsky)
9453c153612ae9b30308362048099bc53afcde6f ipc mining: break compatibility with existing clients (version bump) (Sjors Provoost)
70de5cc2d205672743379f2e1a94290ee8b4b84b ipc mining: pass missing context to BlockTemplate methods (incompatible schema change) (Sjors Provoost)
2278f017afad4d2c570527b15df776ee64fc1ee2 ipc mining: remove deprecated methods (incompatible schema change) (Ryan Ofsky)
c6638fa7c5e97f9fd7a5ea8feb29f8caeac788bd ipc mining: provide default option values (incompatible schema change) (Ryan Ofsky)
a4603ac77412790b6498ab1750017e31353740bf ipc mining: declare constants for default field values (Ryan Ofsky)
ff995b50cf9e1ea521f3cf546339f05d10b79a4d ipc test: add workaround to block_reserved_weight exception test (Ryan Ofsky)
b970cdf20fce43fb58dde1cbf713e97ff21d7a2e test framework: expand expected_stderr, expected_ret_code options (Ryan Ofsky)
df53a3e5ec8781833c29682ff9e459fca489fa7b rpc refactor: stop using deprecated getCoinbaseCommitment method (Ryan Ofsky)
Pull request description:
This PR increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:
- Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
- Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.
More details about these changes are in the commit messages.
ACKs for top commit:
Sjors:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
enirox001:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
ismaelsadeeq:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
sedited:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
Tree-SHA512: 0901886af00214c138643b33cec21647de5671dfff2021afe06d78dfd970664a844cde9a1e28f685bb27edccaf6e0c3f2d1e6bb4164bde6b84f42955946e366d
0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a Minimize mempool lock, sync txo spender index only when and if needed (sstone)
3d82ec5bdd019cf1c048c41fe44faa855fcb8b53 Add a "tx output spender" index (sstone)
Pull request description:
This PR adds a new "tx output spender" index, which allows users to query which tx spent a given outpoint with the `gettxspendingprevout` RPC call that was added by https://github.com/bitcoin/bitcoin/pull/24408.
Such an index would be extremely useful for Lightning, and probably for most layer-2 protocols that rely on chains of unpublished transactions.
UPDATE: this PR is ready for review and issues have been addressed:
- using a watch-only wallet instead would not work if there is a significant number of outpoints to watch (see https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1276595646)
- this PR does not require `-txindex` anymore
We use a composite key with 2 parts (suggested by romanz): hash(spent outpoint) and tx position, with an empty value. Average composite key size is 15 bytes.
The spending tx can optionally be returned by `gettxspendingprevout` (even it `-txindex is not set`).
ACKs for top commit:
hodlinator:
re-ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
sedited:
Re-ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
fjahr:
ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
w0xlt:
reACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
Tree-SHA512: 95c2c313ef4086e7d5bf1cf1a3c7b91cfe2bb1a0dcb4c9d3aa8a6e5bfde66aaca48d85a1f1251a780523c3e4356ec8a97fe6f5c7145bc6ccb6f820b26716ae01
cae6d895f8a8cf5f57e05519536fda5d62b10841 fuzz: add target for CoinsViewOverlay (Andrew Toth)
86eda88c8e486eb1db724e60948f71349d050e1d fuzz: move backend mutating block to end of coins_view (Andrew Toth)
89824fb27b228a12d5c2f63106c2a4d793e73107 fuzz: pass coins_view_cache to TestCoinsView in coins_view (Andrew Toth)
73e99a59665551243d6dbe03a0e9baa9cab046b9 coins: don't mutate main cache when connecting block (Andrew Toth)
67c0d1798e6147f48d4bafc2c9e5ff30f2a62340 coins: introduce CoinsViewOverlay (Andrew Toth)
69b01af0eb9017a6ae7ca3134c9dcf89e74dbfa8 coins: add PeekCoin() (Andrew Toth)
Pull request description:
This is a slightly modified version of the first few commits of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
When accessing coins via the `CCoinsViewCache`, methods like `GetCoin` can call `FetchCoin` which actually mutate `cacheCoins` internally to cache entries when they are pulled from the backing db. This is generally a performance improvement for single threaded access patterns, but it precludes us from accessing entries in a `CCoinsViewCache` from multiple threads without a lock.
Another aspect is that when we use the resettable `CCoinsViewCache` view backed by the main cache for use in `ConnectBlock()`, we will insert entries into the main cache even if the block is determined to be invalid. This is not the biggest concern, since an invalid block requires proof-of-work. But, an attacker could craft multiple invalid blocks to fill the main cache. This would make us `Flush` the cache more often than necessary. Obviously this would be very expensive to do on mainnet.
Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads coins without mutating the underlying cache via `FetchCoin()`.
Add `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`.
This is the foundation for async input fetching, where worker threads must not mutate shared state.
ACKs for top commit:
l0rinc:
ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
sipa:
reACK cae6d895f8a8cf5f57e05519536fda5d62b10841
sedited:
Re-ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
willcl-ark:
ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
vasild:
Cursory ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
ryanofsky:
Code review ACK cae6d895f8a8cf5f57e05519536fda5d62b10841. PR is basically back to the form I had acked the first time, implementing `PeekCoin()` by calling `GetCoin()`. This is not ideal because `PeekCoin()` is not supposed to modify caches and `GetCoin()` does that, but it at least avoids problems of the subsequent approach tried where `GetCoin()` calls `PeekCoin` and would result in bugs when subclasses implement `GetCoin` forgetting to override `PeekCoin`. Hopefully #34124 can clean all of this by making relevant methods pure virtual.
Tree-SHA512: a81a98e60ca9e47454933ad879840cc226cb3b841bc36a4b746c34b350e07c546cdb5ddc55ec1ff66cf65d1ec503d22201d3dc12d4e82a8f4d386ccc52ba6441
afea2af1391314be0e5caaa0125c884da2405316 net: reduce log level for PCP/NAT-PMP NOT_AUTHORIZED failures (ANAVHEOBA)
Pull request description:
Cherry-picks (and tweaks) a commit from #34117 which the ANAVHEOBA
did not follow up with when changes were requested.
The tweak here is to log once at `LogWarning`, so that users have a chance
to spot misconfiguration.
----
Users running on home networks with routers that don't support PCP (Port
Control Protocol) or NAT-PMP port mapping receive frequent warning-level
log messages every few minutes:
"pcp: Mapping failed with result NOT_AUTHORIZED (code 2)"
This is expected behavior for many consumer routers that have PCP
disabled by default, not an actionable error.
Add explicit constants for the NOT_AUTHORIZED result code (value 2)
for both NAT-PMP and PCP protocols. Log the first NOT_AUTHORIZED
failure at warning level for visibility, then downgrade subsequent
occurrences to LogDebug to avoid log noise. Other failure types
continue to warn unconditionally.
Fixes#34114
ACKs for top commit:
achow101:
ACK afea2af1391314be0e5caaa0125c884da2405316
sedited:
ACK afea2af1391314be0e5caaa0125c884da2405316
Tree-SHA512: 43dad9f3cca0ef9b82446045a3ccd90418cd5595c9965e938d9d539bbba863dde6b4a358adbee56f8d97d6efbf947eb5ddbbaf426faefcf3b1e36e4c8edb0d94
2a1d0db7994eb2aa8527944f62161b6b8af682da doc: Mention private broadcast RPCs in release notes (Andrew Toth)
c3378be10b0a90e81b46e53eb85c41eb8caabac5 test: Cover abortprivatebroadcast in p2p_private_broadcast (Andrew Toth)
557260ca14ac5fb4732f4ce0692a2bf364bb5238 rpc: Add abortprivatebroadcast (Andrew Toth)
15dff452eb61ae9e2fd7b48c957e795c4c397443 test: Cover getprivatebroadcastinfo in p2p_private_broadcast (Andrew Toth)
996f20c18af02281034c51af4b2766d8f4d37a2c rpc: Add getprivatebroadcastinfo (Andrew Toth)
5e64982541f301773156a87988c60ca7797a3f06 net: Add PrivateBroadcast::GetBroadcastInfo (Andrew Toth)
573bb542be80b63b1713a0b76bedaa5e37c3783f net: Store recipient node address in private broadcast (Andrew Toth)
Pull request description:
Follow up from #29415
Sending a transaction via private broadcast does not have any way for a user to track the status of the transaction before it gets returned by another peer. The default logs have been removed as well in #34267. Nor is there any way to abort a transaction once it has been added to the private broadcast queue.
This adds two new RPCs:
- `getprivatebroadastinfo` returns information about what transactions are in the private broadcast queue, including all the peers' addresses we have chosen and timestamps.
- `abortprivatebroadcast` stops broadcasting a transaction in the private broadcast queue.
ACKs for top commit:
nervana21:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
achow101:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
l0rinc:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
danielabrozzoni:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
sedited:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
Tree-SHA512: cc8682d0be68a57b42bea6e3d091da2b80995d9e6d3b98644cb120a05c2b48a97c2e211173289b758c4f4e23f1d1a1f9be528a9b8c6644f71d1dd0ae5f673326
