cf5faf73c9 guix: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a (fanquake)
Pull request description:
This includes a commit to fix building LLVM 17 on riscv64, see https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4e26331a5ee87928a16888c36d51e270f0f10f90.
Followup to discussion in https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843313196.
If you don't have riscv64 hardware, this can be tested with the following:
```bash
# observe failure when cross-compiling using our current time-machine
guix time-machine --commit=d5ca4d4fd713a9f7e17e074a1e37dda99bbb09fc -- build --target=riscv64-linux-gnu llvm
....
riscv64-linux-gnu-ld: CMakeFiles/dsymutil.dir/dsymutil.cpp.o: undefined reference to symbol '__atomic_fetch_and_1@@LIBATOMIC_1.0'
riscv64-linux-gnu-ld: /gnu/store/i4ga0pnr1b74bir2bjyp8mcrrbsvk7d3-gcc-cross-riscv64-linux-gnu-11.3.0-lib/riscv64-linux-gnu/lib/libatomic.so.1:
error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
# build success when using the new time-machine
guix time-machine --commit=dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a -- build --target=riscv64-linux-gnu llvm
....
grafting '/gnu/store/7y0j0y8jaz4mjx2nz0y42wdnxxjp6id6-llvm-17.0.6-opt-viewer' -> '/gnu/store/8xvahrrjscbprh6cjj0qp5bm9mm78wwa-llvm-17.0.6-opt-viewer'...
grafting '/gnu/store/bjhw648bz7ijd2p9hgzzdbw1q8hpagk8-llvm-17.0.6' -> '/gnu/store/x50qi8i2ywgpx6azv4k55ms0w5xjxxg5-llvm-17.0.6'...
successfully built /gnu/store/q9xvk8gzzvb4dxfzf6yi5164zd0d1vj2-llvm-17.0.6.drv
```
Also includes at least:
Linux Headers 6.1.67 -> 6.1.80
ACKs for top commit:
TheCharlatan:
ACK cf5faf73c9
hebasto:
ACK cf5faf73c9, tested on x86_64 hardware as described in the PR description.
Tree-SHA512: b49d4f90effeec666b12b5447a24c90315b82675cfc166bc1230ac173134bab6b277fc7e064bbb75e990275165b2b27d88e4ec1cdeea4750541ec6443cb50f41
f65b0f6401 index: Move last_locator_write_time and logging to end of threadsync loop (Fabian Jahr)
Pull request description:
In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit 7878f97bf1 from https://github.com/bitcoin/bitcoin/pull/25494 and was reported by pstratem in https://github.com/bitcoin/bitcoin/pull/26903 with an alternate fix.
ACKs for top commit:
achow101:
ACK f65b0f6401
ryanofsky:
Code review ACK f65b0f6401. Just moved log "Syncing" log line since last commit to avoid having to call now() twice.
furszy:
ACK f65b0f6401
TheCharlatan:
ACK f65b0f6401
Tree-SHA512: afa8f05786318d36346d167ff53ea0b3bc8abdb0ad04465d199dc3eb91e9f837369e24fcb7e24b5757b02d698ec504e61da6ac365eaf006c874fc07a424a7e20
9d9a7458a2 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed2 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d76 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662 assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf9 doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6b validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)
Pull request description:
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.
Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.
Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
ACKs for top commit:
Sjors:
re-ACK 9d9a7458a2
achow101:
ACK 9d9a7458a2
mzumsande:
Tested ACK 9d9a7458a2
maflcko:
ACK 9d9a7458a2🎯
Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
dfcef536d0 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2039
When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.
`FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.
The call stack looks like this:
```
init()
ThreadImport() <-- process fReindex flag
LoadExternalBlockFile()
AcceptBlock()
SaveBlockToDisk()
FindBlockPos()
FlushBlockFile() <-- unnecessary if block is already on disk
```
A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.
ACKs for top commit:
sipa:
utACK dfcef536d0
mzumsande:
re-ACK dfcef536d0
achow101:
ACK dfcef536d0
furszy:
Rebase diff ACK dfcef53.
Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
99afb9d15a refactor: init, simplify index shutdown code (furszy)
0faafb57f8 index: decrease ThreadSync cs_main contention (furszy)
f1469eb454 index: cache last block filter header (furszy)
a6756ecdb2 index: blockfilter, decouple header lookup into its own function (furszy)
331f044e3b index: blockfilter, decouple Write into its own function (furszy)
bcbd7eb8d4 bench: basic block filter index initial sync (furszy)
Pull request description:
Work decoupled from #26966 per request.
The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.
Also, reduces `cs_main` lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.
Testing Note:
To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with `-blockfilterindex` and monitor the logs until the syncing process finish.
Local Benchmark Results:
*Master (c252a0fc0f):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 132,042,516.60 | 7.57 | 0.3% | 7.79 | `BlockFilterIndexSync`
*PR (43a212cfdac6c64e82b601c664443d022f191520):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 126,915,841.60 | 7.88 | 0.6% | 7.51 | `BlockFilterIndexSync`
ACKs for top commit:
Sjors:
re-ACK 99afb9d15a
achow101:
ACK 99afb9d15a
TheCharlatan:
Re-ACK 99afb9d15a
andrewtoth:
ACK 99afb9d15a
Tree-SHA512: 927daadd68f4ee1ca781a89519539b895f5185a76ebaf525fbc246ea8dcf40d44a82def00ac34b188640802844b312270067f1b33e65a2479e06be9169c616de
faecf3a7e6 ci: Bump msan to llvm-18 (MarcoFalke)
Pull request description:
Last one: https://github.com/bitcoin/bitcoin/pull/28476
ACKs for top commit:
fanquake:
ACK faecf3a7e6 - There is now a 18.1.2, but given it doesn't fix the instrumenting in libunwind, we don't need that here. I've tested that both jobs are now working on both arches.
Tree-SHA512: 489c0b343bdc732687131317a570f3efbb18a3f548736d739da90d1a1e784df1dbb208c2da8a2a7740f27f961a841c477487a14c4d59910368f651225f0779b2
2f23987849 test: p2p: check limited peers desirability (depending on best block depth) (Sebastian Falbesoner)
c4a67d396d test: p2p: check disconnect due to lack of desirable service flags (Sebastian Falbesoner)
405ac819af test: p2p: support disconnect waiting for `add_outbound_p2p_connection` (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
5f3a0574c4/src/net_processing.cpp (L3384-L3389)
This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.
In lack of finding a proper file where this test would fit in, I created a new one. Happy to take suggestions there.
ACKs for top commit:
davidgumberg:
reACK 2f23987849
itornaza:
tested ACK 2f23987849
fjahr:
re-utACK 2f23987849
cbergqvist:
re ACK 2f23987849
stratospher:
tested ACK 2f23987. 🚀
Tree-SHA512: cf75d9d4379d0f34fa1e13152e6a8d93cd51b9573466ab3a2fec32dc3e1ac49b174bd1063cae558bc736b111c8a6e7058b1b57a496df56255221bf367d29eb5d
6e873df347 serfloat: improve/simplify tests (Pieter Wuille)
b45f1f5658 serfloat: do not test encode(bits)=bits anymore (Pieter Wuille)
Pull request description:
Closes#28941.
Our current tests for serfloat verify two distinct properties:
1. Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
2. Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.
#28941 seems to show that the second property doesn't always hold, but just for "subnormal" numbers (below $2^{-1021}$). Since we don't care about encoding these numbers, we could exclude such subnormal numbers from the hardware-identical representation test, but this PR goes further and just drops the second property entirely, as I don't think we care about edge-case compatibility with pre-v22.0 code for fee_estimates.dat (the only place it is used).
ACKs for top commit:
glozow:
ACK 6e873df347
fanquake:
ACK 6e873df347 - It's not as much of a priority, but I think we could still backport this.
Tree-SHA512: e18ceee0753a7ee7e999fdfa10b014dc5bb67b6ef79522a0f8c76b889adcfa785772fc26ed7559bcb5a09a9938e243bb54eedd9549bc59080a2c8090155e2267
fae70ba00d ci: Better tidy errors (MarcoFalke)
Pull request description:
Currently tidy errors are not nice, because the user may have to scroll up to see them in a large block of text. See for example (before) https://github.com/bitcoin/bitcoin/runs/19670551485
Fix that by `tee`ing the output to a file and summarizing the errors in the end again. See for example (after): https://github.com/bitcoin/bitcoin/runs/22647850662
ACKs for top commit:
hebasto:
ACK fae70ba00d, logs with errors look cleaner.
TheCharlatan:
ACK fae70ba00d
Tree-SHA512: dcaea557fed40089409d16ce2cbaa8a9cfbf047f601d5daadfee0823b0eed7badc12d803addc0b7b6bb3f1eaf5c787fccb2488475d32c4efd80835f386f761dd
432a542e27 test: fix intermittent failures with test=addrman (Martin Zumsande)
Pull request description:
The `nKey` of the addrman is generated the first time the node is started with an empty `peers.dat`. Therefore, restarting a node or turning it off and on again won't make a previously non-deterministic addrman deterministic.
This could lead to intermittent failures in `feature_asmap.py` and `rpc_net.py`
Fixes#29634
ACKs for top commit:
kevkevinpal:
ACK [432a542](432a542e27)
stratospher:
Tested ACK 432a542e27.
brunoerg:
crACK 432a542e27
0xB10C:
ACK 432a542e27
Tree-SHA512: a8e284baeb0be2df7284b8a2792cb9edc9e2d5b877a3b29ab7277ffdb75b17efa58a4d42576441eb493cd518e7c5ffdb05597b27e42b5001cf1a80e78bb04c83
626f8e398e fuzz: actually test garbage >64b in p2p transport test (Pieter Wuille)
Pull request description:
This fixes an oversight from #28196: in the `p2p_transport_bidirectional_v2` fuzz test, when the desired garbage length is over 64 bytes, the code would actually use garbage length 0. Fix this.
ACKs for top commit:
instagibbs:
ACK 626f8e398e
brunoerg:
crACK 626f8e398e
Tree-SHA512: f6346367adb10464b6c9d20aef43625531d2a4d8110887ad03214b8c1907b83560f2dd5b5415e2180a40b4cd276d51881b32b60c740471b5c6bb218aa19848d8
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/28949
I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.
The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.
ACKs for top commit:
ismaelsadeeq:
Re-ACK 38f70ba6ac👍🏾
glozow:
ACK 38f70ba6ac with some non-blocking nits
murchandamus:
LGTM, code review ACK 38f70ba6ac
Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
64722e4359 ci: Drop `--enable-c++20` option (Hennadii Stepanov)
Pull request description:
This option has ceased to exist since https://github.com/bitcoin/bitcoin/pull/28349.
ACKs for top commit:
maflcko:
ACK 64722e4359
Tree-SHA512: bd392c331f775605615e1b236682269b83a1e6363a4d3f09c4d8d54495cf3d22973a921ebf6b8a9f813ba6c024d3324761f3291aaf7f473995f5eaa4c195bc43
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if the snapshot block was
submitted after loading the snapshot and downloading a few blocks after the
snapshot. In that case ReceivedBlockTransactions() previously would overwrite
the nChainTx value of the submitted snapshot block with a fake value based on
the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx)
check would later fail on the first block after the snapshot. This test was
originally posted by Martin Zumsande <mzumsande@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1974096225
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After the fix, prev->nChainTx is unset instead
of being set to a fake value, so the assert succeeds. This test was originally
posted by maflcko in
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.
This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.
Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.
Add ubsan suppressions for integer overflows in the getchaintxstats RPC.
getchainstatstx line "int nTxDiff = pindex->nChainTx - past_block.nChainTx" can
trigger ubsan integer overflows when assumeutxo snapshots are loaded, from
subtracting unsigned values and assigning the result to a signed int.
The overflow behavior probably exists in current code but is hard to trigger
because it would require calling getchainstatstx at the right time with
specific parameters as background blocks are being downloaded. But the overflow
behavior becomes easier to trigger in the upcoming commit removing fake
nChainTx values, so a suppression needs to be added before then for CI to pass.
getchainstatstx should probably be improved separately in another PR to not
need this suppression, and handle edge cases and missing nChainTx values more
carefully.
636c9862cf ci: Bump `TIDY_LLVM_V` (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.22](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.22), which is compatible with Clang 18.
ACKs for top commit:
fanquake:
ACK 636c9862cf
Tree-SHA512: 78ce89244c5e487dd1be8b4bd2ca6f06d19b04b78289ebc21985110574053545dcce5eb622edf2bede2cf7bb58360170e976d30a4484a127d34dd17b1c604e9c
fa5844f06d Remove unused g++-10 workaround (MarcoFalke)
fa8409e760 build: Bump g++ minimum supported version to 11 (MarcoFalke)
Pull request description:
This drops support for vanilla Ubuntu Focal 20.04 and Debian (Oldstable) Bullseye, compiling from source. Users on those operating systems would have to stick with a pre-compiled release, a previous release branch of Bitcoin Core, upgrade their system, compile their own compiler, or use a non-vanilla PPA or package manager.
Otherwise, g++-11 is offered by common distributions:
* https://packages.ubuntu.com/jammy/g++ (`g++-11`)
* https://packages.debian.org/bookworm/g++ (`g++-12`)
* FreeBSD 12/13 ships with g++ 12
* CentOS-like 9 ships with g++ 11
* OpenSuse Tumbleweed ships with g++ 13 https://software.opensuse.org/package/gcc13-c++ (No idea about OpenSuse Leap)
ACKs for top commit:
TheCharlatan:
ACK fa5844f06d
fanquake:
ACK fa5844f06d
Tree-SHA512: fc72d3a53956a0a4a6475ebf56b5fce76c3c4c793ed8e774327cad2b0f307d2d1c8aeafe2a414a7eb51f8de6d4bb78d30b8f60bf6e383234079851e72015c6e3
5555395c15 lint: Use git --no-pager to print any output in one go (MarcoFalke)
fa5729436c lint: Fix lint-whitespace issues (MarcoFalke)
Pull request description:
The lint check has many issues:
* It uses `COMMIT_RANGE`, which is brittle code, apparently making it harder to run the CI locally, or self-hosted. See https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457739319
* The result depends on `COMMIT_RANGE`, or the number of commits passed to the script, which can cause false negatives or false positives.
* It is based on the diff output, parsing it, and printing it again, which is brittle as well.
* The output does not include line number, making it harder to act on a lint error.
Fix all issues by removing the script and replacing it with a simple call to `git grep -I --line-number ...`.
ACKs for top commit:
TheCharlatan:
Re-ACK 5555395c15
Tree-SHA512: 71ea8b6382af064beb72fb17f21a0ae9e9238c97e7fa43c2ec353fd1dd73a7bbd696ba0f0a9f65d1eff7c86cbf6cc104a992cb5450a3d50f122955e835270065
This includes a commit to fix building LLVM 17 on riscv64, see
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4e26331a5ee87928a16888c36d51e270f0f10f90.
Followup to discussion in
https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843313196.
If you don't have riscv64 hardware, this can be tested with the
following:
```bash
guix time-machine --commit=d5ca4d4fd713a9f7e17e074a1e37dda99bbb09fc -- build --target=riscv64-linux-gnu llvm
....
riscv64-linux-gnu-ld: CMakeFiles/dsymutil.dir/dsymutil.cpp.o: undefined reference to symbol '__atomic_fetch_and_1@@LIBATOMIC_1.0'
riscv64-linux-gnu-ld: /gnu/store/i4ga0pnr1b74bir2bjyp8mcrrbsvk7d3-gcc-cross-riscv64-linux-gnu-11.3.0-lib/riscv64-linux-gnu/lib/libatomic.so.1:
error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
guix time-machine --commit=dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a -- build --target=riscv64-linux-gnu llvm
....
grafting '/gnu/store/7y0j0y8jaz4mjx2nz0y42wdnxxjp6id6-llvm-17.0.6-opt-viewer' -> '/gnu/store/8xvahrrjscbprh6cjj0qp5bm9mm78wwa-llvm-17.0.6-opt-viewer'...
grafting '/gnu/store/bjhw648bz7ijd2p9hgzzdbw1q8hpagk8-llvm-17.0.6' -> '/gnu/store/x50qi8i2ywgpx6azv4k55ms0w5xjxxg5-llvm-17.0.6'...
successfully built /gnu/store/q9xvk8gzzvb4dxfzf6yi5164zd0d1vj2-llvm-17.0.6.drv
```
bf264e0598 test: check_mempool_result negative feerate (kevkevin)
Pull request description:
Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result`
Asserts "Amount out of range" error message and `-3` error code
Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250)
ACKs for top commit:
maflcko:
lgtm ACK bf264e0598
brunoerg:
nice, utACK bf264e0598
davidgumberg:
Looks great, ACK bf264e0598
Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
10d56530e0 guix: temporarily disable powerpcle taget (fanquake)
001412a4d2 guix: use GCC 12.3.0 (fanquake)
ce54330cf6 ci: use Debian Bookworm (GCC 12) for ARM ci job (fanquake)
0da6451c58 ci: use Debian Bookworm (GCC 12) for win64 job (fanquake)
Pull request description:
Switch to using [GCC `12.3.0`](https://gcc.gnu.org/gcc-12/) to build release binaries.
Temporarily disables the `powerpc64le-linux-gnu` target due to non-determinism issues when building across `aarch64` and `x86_64`. Trying to fix the non-determinism was going to require trying to selectively disable optimization flags, which is already not ideal (and didn't fix all issues), and the migration to GCC 12 as our release compiler is now the blocker for multiple other (c++20 and similar) changes, so leaving this blocked on the `powerpc64le` binaries does not seem like a good tradeoff.
ACKs for top commit:
TheCharlatan:
ACK 10d56530e0
Tree-SHA512: 401bbaaf2b72c795a06a24875ffd666151b41bae8f45bda10526ff4f6b59782704246afc6585f6b849021cbff8a7b861961d139bffe45536aaaeb3952b72ae57
0831b54dfc test: simplify test_runner.py (tdb3)
Pull request description:
Implements the simplifications to test_runner.py proposed by sipa in PR #23995.
Remove the num_running variable as it can be implied by the length of the jobs list.
Remove the i variable as it can be implied by the length of the test_results list.
Instead of counting results to determine if finished, make the queue object itself
responsible (by looking at running jobs and jobs left).
ACKs for top commit:
mzumsande:
re-ACK 0831b54
davidgumberg:
reACK 0831b54dfc
marcofleon:
re-ACK 0831b54dfc
Tree-SHA512: e5473e68d49cd779b29d97635329283ae7195412cb1e92461675715ca7eedb6519a1a93ba28d40ca6f015d270f7bcd3e77cef279d9cd655155ab7805b49638f1
c70e4fc9a3 netbase: remove unnecessary log message (Matthew Zipkin)
Pull request description:
This is a follow-up to #27375 that removes a spammy non-debug-level log message we don't need.
See https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1994498888
ACKs for top commit:
fanquake:
ACK c70e4fc9a3 - thanks. Merging this now because it's swamping non-debug logs. i.e:
Tree-SHA512: 837682860abdf740fce5dc88c8599e066660cf16b4cab1473391eb51ad538ae52d236ecd3543df866e2a2165870397a8bf21ad9f5125ed8212a3fe207d615553
1ec6684b08 test: Add test for loadtxoutset when headers are not synced (Fabian Jahr)
2bc1ecfaa9 test: Remove unnecessary sync_blocks in assumeutxo tests (Fabian Jahr)
Pull request description:
It adds a test for the change to `loadtxoutset` made in #29345. Before that change the test doesn't fail right away but times out after 10 minutes.
Also removes a `sync_blocks` call that didn't seem to do anything valuable.
ACKs for top commit:
achow101:
ACK 1ec6684b08
pablomartin4btc:
tACK 1ec6684b08
BrandonOdiwuor:
ACK 1ec6684b08
theStack:
ACK 1ec6684b08
Tree-SHA512: 1337decdf91e4a4f7213fcf8ace1d705e5c1422e0ac3872a59b5be9c33e743850cb8f5f7474750a534952eefd5cfe43fe85a54efb9bc0e47515136a2903676e5
28287cfbe1 test: add script compression coverage for not-on-curve P2PK outputs (Sebastian Falbesoner)
Pull request description:
This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](44b05bf3fe/src/pubkey.cpp (L297-L302)), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't be recovered (see also call-site of `IsToPubKey`):
44b05bf3fe/src/compressor.cpp (L49-L50)
Likewise, for a compressed script of an uncompressed P2PK script (i.e. compression ids 4 and 5) where the x coordinate is not on the curve, decompression fails:
44b05bf3fe/src/compressor.cpp (L122-L129)
Note that the term "compression" is used here in two different meanings (though they are related), which might be a little confusing. The encoding of a pubkey can either be compressed (33-bytes with 0x02/0x03 prefixes) or uncompressed (65-bytes with 0x04 prefix). On the other hand there is also compression for whole output scripts, which is used for storing scriptPubKeys in the UTXO set in a compact way (and also for the `dumptxoutset` result, accordingly). P2PK output scripts with uncompressed pubkeys get compressed by storing only the x-coordinate and the sign as a prefix (0x04 = even, 0x05 = odd). Was diving deeper into the subject while working on https://github.com/bitcoin/bitcoin/pull/27432, where the script decompression of uncompressed P2PK needed special handling (see also https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108798536).
Trivia: as of now (block 801066), there are 13 uncompressed P2PK outputs in the UTXO set with a pubkey not on the curve (which obviously means they are unspendable).
ACKs for top commit:
achow101:
ACK 28287cfbe1
tdb3:
ACK for 28287cfbe1.
cbergqvist:
ACK 28287cf!
marcofleon:
Nicely done, ACK 28287cfbe1. Built the PR branch, ran the unit and functional tests, everything passed.
Tree-SHA512: 777b6c3065654fbfa1ce94926f4cadb91a9ca9dc4dd4af6008ad77bd1da5416f156ad0dfa880d26faab2e168bf9b27e0a068abc9a2be2534d82bee61ee055c65
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
07cd510ffe [refactor] consolidate invalid MempoolAcceptResult processing (glozow)
9353aa4066 [refactor] consolidate valid MempoolAcceptResult processing (glozow)
Pull request description:
Every time we try to `ProcessTransaction` (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in `m_recent_rejects` so we don't try to download/validate it again.
There are 2 current places and at least 1 future place where we need to process `MempoolAcceptResult`:
- In the `ProcessMessage` logic after receiving and validating a tx
- In `ProcessOrphanTx` where we retry orphans
- With #28970, after processing a package of transactions, we should do these updates for each tx in the package.
Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR.
ACKs for top commit:
instagibbs:
ACK 07cd510ffe
achow101:
ACK 07cd510ffe
dergoegge:
Code review ACK 07cd510ffe
TheCharlatan:
ACK 07cd510ffe
Tree-SHA512: c4e74cb65e4f52882fca52e6682efa5dcf1562d98418454e09be256ffd026caae642a90aa5b9cccaae214be240d6f4be9d87b516953b2ee69a655f16ea569ed9
