fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
fa43897c1d doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac5415466 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a0 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada838014 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3cee test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)
Pull request description:
There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.
Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.
Fix both issues in a series of commits, to:
* refactor the single unit test to call higher-level functions
* Make `ProcessMessage` private again
* Use references instead of implicit non-null pointers, mostly in a scripted-diff
ACKs for top commit:
optout21:
reACK fa43897c1d
ajtowns:
ACK fa43897c1d
Crypt-iQ:
crACK fa43897c1d
achow101:
ACK fa43897c1d
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
d79249d279 ci: add chimera Linux LTO CI job (fanquake)
Pull request description:
Adds a CI config based on using [Chimera Linux](https://chimera-linux.org/). This might be interesting for any of the following:
* Chimera is based on LLVM & musl libc - we test both of these in isolation, but not together.
* No GNU components. I don't think we have an existing Linux CI job that doesn't have a gcc/stdlibc++ install. This exercises the depends logic for a fully LLVM/Clang/lld only build, including building the native tools (related to #33902).
* We don't currently have a job with LTO enabled (here using CMakes `CMAKE_INTERPROCEDURAL_OPTIMIZATION`, which is `-flto=thin` for LLVM/Clang). I think this is worth having generally (we do use LTO in some other places, like oss-fuzz). If runtime is too much of an issue, then it could also be dropped. (Chimera itself is also compiled with LTO).
QT in depends doesn't build (#32744), so is excluded for now.
Chimera has pointed out at least a few quirks, i.e #34390, #34408 and https://github.com/bitcoin/bitcoin/pull/29963#discussion_r2707922298.
ACKs for top commit:
maflcko:
lgtm ACK d79249d279
hebasto:
ACK d79249d279.
Tree-SHA512: 1174a7462bf2e7433a2c27a6cf398e94b05db42bb414629c71cf9f9a297ca269e173ae1b7517b30510b494b4397f918eef706d3c75c4286767c5557aeb6db4c7
b65a3d8009 iwyu: Fix patch to prefer `<cstdint>` (Hennadii Stepanov)
Pull request description:
The goal of the [patch](https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_iwyu.patch) is to suggest C++ headers rather than their C counterparts. However, for fixed width integer types, the patched IWYU currently suggests `<cinttypes>` where `<cstdint>` is sufficient.
This PR fixes this behavior.
ACKs for top commit:
maflcko:
lgtm ACK b65a3d8009
furszy:
utACK b65a3d8009
willcl-ark:
utACK b65a3d8009
Tree-SHA512: 695efdd44b92a642401738572e49c8b6591aa4463d387107fdf3d2f7c9c4b39f4097cb82413752caf9e8890dcca7246a894e562a1dd17023b05a7e455705beac
PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.
CoinsResult has a cleaner interface with separate fields
for both these amounts.
2 behavioural changes:
1. no more default assert error if effective value is unset
- previously PreSelectedInputs::Insert() called
COutput::GetEffectiveValue() which assert failed
if the optional was unset.
- now we don't default assert anymore.
* in GUI/getAvailableBalance better not to assert.
* SelectCoins's preselected inputs always contain a
feerate, so effective amount should be set.
explicitly added an assertion to ensure this.
2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
populate CoinsResult's coins map. it's discarded later.
faa4ab113c ci: Drop valgrind fuzz from GHA matrix (MarcoFalke)
Pull request description:
The valgrind fuzz task is problematic, because:
* It is redundant with the msan fuzz task, which has std lib hardening enabled, so often UB is diagnosed before it even happens in the valgrind task.
* All issues so far found by the valgrind fuzz task were also found by the hardened msan fuzz task.
* All other issues were false-positives, which are hard to debug, and confusing and tedious to work around.
I don't think there is any value in asking pull request authors to debug valgrind false-positives that they triggered by accident. So remove the task for now.
I know that there are some devs, who like to keep the task, but if the task is kept, it should come with clear instructions on how to deal with false-postives in pull requests.
I am not proposing to remove the config itself, and I am happy to continue maintaining it, like it was done before. However, as of now, running it in the GHA matrix is of negative or questionable benefit.
ACKs for top commit:
l0rinc:
ACK faa4ab113c
fanquake:
ACK faa4ab113c - hopefully we can revisit re-adding soon. To be clear, I don't agree with the rationale from #34304, or the initial changes there. The case here, and the fact that it is causing disruption in this repo, is more pressing.
Tree-SHA512: 59272f4b5b01c3b8ee6078ea635441f17776d4d8923f1adacdabdbb00bd2eb0234b30dc5b27938e29f8e79b3c3bebed5f339ae36c2c8fb17ea9d3a2884bee986
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
returns the total amount (if SFFO), otherwise the effective amount.
previously, this was the logic in calculating
PreSelectedInputs::total_amount when PreSelectedInputs::Insert()
was called.
return optional to force callers to explicitly handle the case
when effective amount optional is not set.
coins.size() would be the number of the OutputType keys in the map.
whereas Size() would return total number of COutput objects when
flattening the map.
before #25806, set<COutput> was used and would not
contain same COutputs in the set.
now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.
so preserve previous behaviour by making sure values
in the set are also distinct
e67a676df9 fix: uptime RPC returns 0 on first call (Lőrinc)
Pull request description:
### Problem
#34328 switched uptime to use monotonic time, but `g_startup_time` was a function-local static in `GetUptime()`, meaning it was initialized on first call rather than at program start.
This caused the first uptime RPC to always return 0.
### Fix
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first `uptime()` call returns actual elapsed time.
### Reproducer
Revert the fix and run the test or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
./build/bin/bitcoind -regtest -daemon
sleep 10
./build/bin/bitcoin-cli -regtest uptime
./build/bin/bitcoin-cli -regtest stop
```
<details>
<summary>Before (uptime is initialized on first call)</summary>
```bash
Bitcoin Core starting
0
Bitcoin Core stopping
```
</details>
<details>
<summary>After (first uptime call is in-line with sleep)</summary>
```bash
Bitcoin Core starting
10
Bitcoin Core stopping
```
</details>
----
Fixes#34423, added reporter as coauthor.
ACKs for top commit:
maflcko:
lgtm ACK e67a676df9
optout21:
crACK e67a676df9
carloantinarella:
Tested ACK e67a676df9
achow101:
ACK e67a676df9
musaHaruna:
Tested ACK [e67a676](e67a676df9)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
54d0393058 FUZZ: Test that BnB finds best solution (Murch)
Pull request description:
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
ACKs for top commit:
achow101:
ACK 54d0393058
brunoerg:
ACK 54d0393058
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
322c4ec442 build: replace WERROR with CMAKE_COMPILE_WARNING_AS_ERROR (fanquake)
Pull request description:
`CMAKE_COMPILE_WARNING_AS_ERROR` was added to CMake in 3.24.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
Potential alternative to #33297. Closes#33284.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
ACKs for top commit:
purpleKarrot:
re-ACK 322c4ec442
maflcko:
review ACK 322c4ec442📩
hebasto:
re-ACK 322c4ec442.
Tree-SHA512: 1e9b41e87568c1021aecf9414cfc70a3215d1663a13adb1f32493f2dba18978b53888520775d1aa2945224b0404ef83bda0837dddf5f8a089cafb940b856598c
f2b8acc0ed remove glozow from trusted keys (glozow)
Pull request description:
As planned!
Also bump trusted-git-root to 88a7294356, which is the last merge signed with this key.
ACKs for top commit:
achow101:
ACK f2b8acc0ed
stickies-v:
ACK f2b8acc0ed
Tree-SHA512: e68025c519ab003ad464783325502c1a6a3f560c0708a9383332cb0971be2039f462fc95f85373bb075191c165ca8e3904312a66e41b52d1b99dd2a8a837be9a
-Werror is added to the previous releases job, given it runs on Ubuntu
22.04, which uses an older CMake.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
CMAKE_COMPILE_WARNING_AS_ERROR was added to CMake in 3.24.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
Co-authored-by: willcl-ark <will8clark@gmail.com>
a50d0b6720 build: don't pass on boost dependency to kernel consumers (Cory Fields)
Pull request description:
This is unnecessary now that the kernel now exports a (boost-less) API.
Noticed while slimming down boost dependencies in #34495.
ACKs for top commit:
stickies-v:
ACK a50d0b6720
hebasto:
ACK a50d0b6720, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from https://github.com/bitcoin/bitcoin/pull/34143 and building with depends.
Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
1f8f7d477a Change BlockRequestAllowed() to take ref (optout)
Pull request description:
As [suggested here](https://github.com/bitcoin/bitcoin/pull/34416#discussion_r2745302958), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent.
The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class.
Related to #34440.
ACKs for top commit:
maflcko:
review ACK 1f8f7d477a🎐
l0rinc:
tested ACK 1f8f7d477a
stickies-v:
ACK 1f8f7d477a
Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19
42ee31e80c doc: fix broken bpftrace installation link (jayvaliya)
Pull request description:
The bpftrace project has moved from the `iovisor` organization to its own
`bpftrace` organization on GitHub. The old installation documentation link
(`https://github.com/iovisor/bpftrace/blob/master/INSTALL.md`) is now broken (404).
The project also restructured their documentation - installation instructions
are now in the README.md under the "Quick Start" section rather than a
separate INSTALL.md file.
ACKs for top commit:
maflcko:
lgtm ACK 42ee31e80c
Tree-SHA512: 5320e747b77fe9bb347a98396364f3942214b6d3ee69707f62452df0aaaa2f6cfd58fc3b6155f64c4acb00f1a4dba8a751618805e75b823454c73ac93877ad4f
The bpftrace project moved from iovisor/bpftrace to bpftrace/bpftraceand
removed the separate INSTALL.md file. Installation instructionsare now
in the README.md Quick Start section.
BnB’s solution is the input set with the lowest waste score, excluding
any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to
ensure that BnB succeeds.
db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
fad9dd1a88 test: kernel test fixups (MarcoFalke)
fabb58d42d test: Use clang-tidy named args for create_chainman (MarcoFalke)
fa51594c5c refactor: Small style fixups in src/kernel/bitcoinkernel.cpp (MarcoFalke)
Pull request description:
Just some small style and test fixups after https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3420542946
ACKs for top commit:
stickies-v:
re-ACK fad9dd1a88
frankomosh:
Code Review ACK fad9dd1a88. All changes are sound refactoring with no functional issues. Nice improvements to readability (named args in create_chainman, span.data(), range checks now properly require non-empty).
Tree-SHA512: 0a92e871b4db75a590acad39672594625e402895bc0d36635d36ec2fe8dce7cc2c5cb6ebf2a92bc14617d94648b84bffb95ff783cea71bd91ac4a9871ef5dbef
fa33acec89 Revert "valgrind: add suppression for bug 472219" (MarcoFalke)
Pull request description:
Now that several Alpine releases are out, it seems fine to remove this suppression.
This should work since Alpine 3.19: https://pkgs.alpinelinux.org/packages?name=valgrind&branch=v3.19&repo=&arch=aarch64, which just dropped out of `main` support (https://alpinelinux.org/releases/)
ACKs for top commit:
fanquake:
ACK fa33acec89
Tree-SHA512: 2a58475d20f8214c012b77efb7fe145e810a1506736341f8debea74dabd31499899a968bc2d4a88008b2a3878b4fe5e48fa14c83a2e943a91943bf0e1e159b6f
4dfb6eef70 test: Add DERSIG tests to script_tests (billymcbip)
884978f389 test: Fix a STRICTENC test in script_tests (billymcbip)
527e8ca7b5 test: Remove outdated comment in script_tests (billymcbip)
Pull request description:
1. Remove a comment referencing a file that no longer exists in the codebase: `script_invalid.json`.
2. Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in `CHECKMULTISIG`. Use `OP_1` for the second signature instead of `OP_0`.
3. Copy existing `STRICTENC` tests and change the flag to `DERSIG`. `DERSIG` is a consensus flag (unlike `STRICTENC`), so it'd be good to have dedicated test cases.
`script_tests` pass on my end.
ACKs for top commit:
darosior:
reACK 4dfb6eef70
sipa:
ACK 4dfb6eef70
Tree-SHA512: aea4aa5199530804561e9f597f69d6cffd7a40d4830919f9371fe97e4d04d8f10e8ed29b91e65e007a3f6e3a38e2881f88dff25831e741ad7592a12ed02b801a
The goal of the patch is to suggest C++ headers rather than their C
counterparts. However, for fixed width integer types, the patched IWYU
currently suggests `<cinttypes>` where `<cstdint>` is sufficient.
This change fixes this behavior.
02b5f6078d fees: make flushes log debug only (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the flushing log to use debug-level logging under the estimatefee category. It also ensures the log consistently includes only the full file path.
The motivation behind this is that the "Flushed fee estimates to fee_estimates.dat." logs can become noisy; it's done after one hour, so hiding it in the debug estimatefee category seems reasonable.
---
I left the logs when the file is not found as info because that should only occur when you start a fresh node, change datadir, or explicitly delete the file
ACKs for top commit:
achow101:
ACK 02b5f6078d
furszy:
utACK 02b5f6078d
l0rinc:
Lightly tested ACK 02b5f6078d
sipa:
utACK 02b5f6078d
Tree-SHA512: 3e4b822caa23db9b30f1bb8990b36f35dcfcd82dbfb27c319463447da1df988eded84c47d5319ad637c822bdd04f0a727a176da3632c40d786332b6a5aaa6d89
bbbb78a4f2 ci: Print verbose build error message in test-each-commit (MarcoFalke)
2222dadabb ci: [refactor] Allow overwriting check option in run helper (MarcoFalke)
Pull request description:
Currently, the build error in the test-each-commit task is not too nice. E.g. https://github.com/bitcoin/bitcoin/actions/runs/21509735101/job/61973587699#step:8:10464:
```
...
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidation_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidationcache_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_block_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstatemanager_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_flush_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_test_fixture.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/db_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselector_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselection_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/feebumper_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/group_outputs_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/ismine_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/psbt_wallet_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/scriptpubkeyman_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/spend_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_rpc_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_transaction_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletdb_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletload_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/ipc/test/ipc_tests.cpp.o
[ 77%] Linking CXX executable ../../bin/test_bitcoin
[ 87%] Built target test_bitcoin
gmake: *** [Makefile:146: all] Error 2
Command '['cmake', '--build', 'ci_build', '-j', '4']' returned non-zero exit status 2.
error: cannot rebase: Your index contains uncommitted changes.
warning: execution failed: git merge --no-commit origin/master && python3 ./.github/ci-test-each-commit-exec.py && git reset --hard
and made changes to the index and/or the working tree.
You can fix the problem, and then run
git rebase --continue
Error: Process completed with exit code 1.
```
Fix it by just using the same approach that the other CI tasks are using:
01651324f4/ci/test/03_test_script.sh (L143-L146)
ACKs for top commit:
willcl-ark:
ACK bbbb78a4f2
Tree-SHA512: ae54ee62f53e060ed42f76ca59daf7a017bd12495e171efed03d1f5dda969db4f7e2e2c9ed7a178ff5fa9a5baa55ab6a7b30b3ab6b81d5279fe937006ac8228f
7528d18796 ci: show more verbose ccache stats (will)
580e9eefe3 ci: bump CCACHE_MAXSIZE to 2G (will)
Pull request description:
Currently some CI jobs don't have great ccache hitrates which we should try to improve: https://willcl-ark.github.io/bitcoin-core-ci-stats/graph/ccache/
- bump ccache maxsize to 2GB in all jobs. We have 150GB shared cache to use, so this should be OK at maximum of 36GB total (current jobset).
- print more verbose ccache stats in the CI logs
The idea is that increasing the cache size to > 2x needed size should eliminate any cache thrashing which might be taking place on master builds when we save the cache. Additionally, larger caches result in more hits in general.
ACKs for top commit:
maflcko:
lgtm ACK 7528d18796
Tree-SHA512: ae00a05159e3f38d24aebc50a2576c5f11241b1196058a4ca2f5f78909795b891bef20cdc4412f512a2ba09cc9ec65afa5132ac0509b54845dd84f933528500c
d511adb664 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d06 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d6 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)
Pull request description:
This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.
Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.
ACKs for top commit:
achow101:
ACK d511adb664
ryanofsky:
Code review ACK d511adb664. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
sedited:
ACK d511adb664
Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba
Currently std::span::subspan is called without checking the size first.
This is UB, unless the std lib is hardened.
With a hardened stdlib, the program aborts:
> include/c++/v1/span:512: libc++ Hardening assertion __offset <= size()
> failed: span<T>::subspan(offset, count): offset out of range
Fix the UB and the abort by using the implementation from DataStream,
which throws when hitting end-of-data.
This commit should not change any behavior, because the UB is currently
unreachable. Also, the newly added throw should properly be caught by
any code that calls any streams function.
