fa43897c1d14549e7af0d9f912e765875b634c39 doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b87a5441c90d513d2022f4c4d9678cb scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac54154660438db6a601584fa91f87bc09395b2 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e0db44875f8728c9e0b6a05d2145e75 refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a01c421523ec5d012c6aafb006011788 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada8380148c1266f2cc1ddb0f65f42651c82a62 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3ceed4eb58732c2f6f748277772a8a1c36 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 fa43897c1d14549e7af0d9f912e765875b634c39
ajtowns:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Crypt-iQ:
crACK fa43897c1d14549e7af0d9f912e765875b634c39
achow101:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
Bundle all function's outputs inside the util::Result returned object.
Reasons for the refactoring:
- The 'change_pos' ref argument has been a source of bugs in the past.
- The 'fee' ref argument is currently only set when the transaction creation process succeeds.
48161f6a0503d7dde693ef544f0d3285c8b93adc wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1759472b004ce03c217cf4a5e32262c wallet: remove PreSelectedInputs (stratospher)
7819da2c1643e9ca892f0fc97ffc2003ac265dac walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f179c5637b6c5f2077a1c5223ea357e1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01404cc0b63b277878d0f2f988a1daba wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e39d200c5e49c736a281d3db180c716a wallet: ensure COutput added in set are unique (stratospher)
fefa3be782eaf3e2fbff3ed8772fb91f2134ac0d 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. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: 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 48161f6a0503d7dde693ef544f0d3285c8b93adc
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
In feature_assumevalid.py, we check that a modified block 102 is invalid
by asserting a message in the debug.log. However, this can
intermittently fail as exiting the assert_debug_log can occur before the
block has actually been validated, thus causing the test to fail as the
validation error message is not present in the chunk of the debug.log
being examined.
We can wait for the block to make an invalid chain tip to ensure that the log
line will be present.
Split the Mining interface test into focused subtests.
Keep the initial tip-change pre-mine check in run_mining_interface_test.
As a result run_block_template_test no longer has newblockref.
Split Mining interface tests into interface_ipc_mining.py and keep
interface_ipc.py for echo + simple inspectors.
Register the new test in test_runner.py.
The setup code around "Create Mining proxy object" is duplicated
in the new test file, but the simple insector checks below it
are not moved.
d79249d2799e7f450d5708ca3366461f5f2069bc 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 d79249d2799e7f450d5708ca3366461f5f2069bc
hebasto:
ACK d79249d2799e7f450d5708ca3366461f5f2069bc.
Tree-SHA512: 1174a7462bf2e7433a2c27a6cf398e94b05db42bb414629c71cf9f9a297ca269e173ae1b7517b30510b494b4397f918eef706d3c75c4286767c5557aeb6db4c7
b65a3d80093b992a2574d5762a8cd03ce2eb4412 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 b65a3d80093b992a2574d5762a8cd03ce2eb4412
furszy:
utACK b65a3d80093b992a2574d5762a8cd03ce2eb4412
willcl-ark:
utACK b65a3d80093b992a2574d5762a8cd03ce2eb4412
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.
faa4ab113cc9e300b3b8dce0c774d0a33a555883 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 faa4ab113cc9e300b3b8dce0c774d0a33a555883
fanquake:
ACK faa4ab113cc9e300b3b8dce0c774d0a33a555883 - 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 makes it easier to:
* Run the exact command of any CI type and step locally
* Re-Run older CI tasks on GHA and using the latest merged config.
(.github/ci-windows.py is merged with master on re-runs, but
.github/workflows/ci.yml is NOT)
Also, writing it in Python has benefits:
* Any developer (even non-Windows ones) can read and modify the script.
* Python is already required for tests, so no new dependency is needed.
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
e67a676df9af5ece5307438ae1b4ddb0730e3482 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 e67a676df9af5ece5307438ae1b4ddb0730e3482
optout21:
crACK e67a676df9af5ece5307438ae1b4ddb0730e3482
carloantinarella:
Tested ACK e67a676df9af5ece5307438ae1b4ddb0730e3482
achow101:
ACK e67a676df9af5ece5307438ae1b4ddb0730e3482
musaHaruna:
Tested ACK [e67a676](e67a676df9)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
54d039305823f67688ec9116757d8244f84badc6 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 54d039305823f67688ec9116757d8244f84badc6
brunoerg:
ACK 54d039305823f67688ec9116757d8244f84badc6
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
322c4ec4422a2e03fc81e61b1315b1245ba47d2f 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 322c4ec4422a2e03fc81e61b1315b1245ba47d2f
maflcko:
review ACK 322c4ec4422a2e03fc81e61b1315b1245ba47d2f 📩
hebasto:
re-ACK 322c4ec4422a2e03fc81e61b1315b1245ba47d2f.
Tree-SHA512: 1e9b41e87568c1021aecf9414cfc70a3215d1663a13adb1f32493f2dba18978b53888520775d1aa2945224b0404ef83bda0837dddf5f8a089cafb940b856598c
f2b8acc0edb6f1acf58cd3d7423d6ad60e8d90c8 remove glozow from trusted keys (glozow)
Pull request description:
As planned!
Also bump trusted-git-root to 88a7294356e75bbaa136c9427c64e239f7c6fd40, which is the last merge signed with this key.
ACKs for top commit:
achow101:
ACK f2b8acc0edb6f1acf58cd3d7423d6ad60e8d90c8
stickies-v:
ACK f2b8acc0edb6f1acf58cd3d7423d6ad60e8d90c8
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>
a50d0b6720f300987d2b3d82f4fb3a2336259887 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 a50d0b6720f300987d2b3d82f4fb3a2336259887
hebasto:
ACK a50d0b6720f300987d2b3d82f4fb3a2336259887, 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
1f8f7d477ae0d33bd96f7936889c17bd40805fb9 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 1f8f7d477ae0d33bd96f7936889c17bd40805fb9 🎐
l0rinc:
tested ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9
stickies-v:
ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9
Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19
Member variables of `CConnman::m_private_broadcast` (introduced in
https://github.com/bitcoin/bitcoin/pull/29415) could influence the tests
which creates non-determinism if the same instance of `CConnman` is used
for repeated test iterations.
So, reset the state of `CConnman::m_private_broadcast` from
`ConnmanTestMsg::Reset()`. Currently this affects the fuzz tests
`process_message` and `process_messages`.
Reported in
https://github.com/bitcoin/bitcoin/issues/34476#issuecomment-3849088794
42ee31e80c99bdb4d6affdc9dc22a0f3d5da7b59 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 42ee31e80c99bdb4d6affdc9dc22a0f3d5da7b59
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.
