This changes the order of transactions within a chunk to be:
1. Topology (parents before children)
2. Individual transaction feerate (high to low)
3. Individual transaction weight (small to large)
4. Random tiebreak (will be changed in a future commit)
To do so, use a heap of topology-ready transactions within
GetLinearization(), sorted by (2), (3), and (4).
This is analogous to the order of chunks within a cluster, which is
unchanged:
1. Topology (chunks after chunks they depend on)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Random tiebreak (will be changed in a future commit)
Whenever a TxGraph::Ref is destroyed, if it by then still appears inside
main-level clusters, wipe the chunk index entries for those clusters, to
prevent having lingering indexes for transactions without Ref.
This is preparation for enabling a callback being passed to MakeTxGraph
to define a fallback order on objects. Once the Ref for a transaction is
gone, it is not possible to invoke the callback anymore. To prevent the
index becoming inconsistent, we need to immediately get rid of the index
entries when the Ref disappears.
This is not a problem, because such destructions necessarily will
trigger a relinearization of the cluster (assuming there are
transactions in it left) before becoming acceptable again, and the chunk
ordering is not observable (through CompareMainOrder, or through the
BlockBuilder interface) until that point. However, the index itself
needs to remain consistent in the mean time, even if not meaningful.
This makes TxGraphImpl::Compact() invoke Cluster::Updated() on all
affected clusters, in case they have internal GraphIndex values stored
that may have become outdated with the renumbering of GraphIndex values
that Compact() caused.
No such GraphIndex values are currently stored, but this will change in
a future commit.
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
pass in a TxGraph::Ref& which is updated to refer to the new transaction
in that graph.
This cleans up the usage somewhat, avoiding the need for dummy Refs in
CTxMemPoolEntry constructor calls, but the motivation is that a future
commit will allow a callback to passed to MakeTxGraph to define a
fallback order on the transaction objects. This does not work when a
Ref is created separately from the CTxMemPoolEntry it ends up living in,
as passing the newly-created Ref to the callback would be UB before it's
emplaced in its final CTxMemPoolEntry.
fa90277d22e1e6662e827e662eb6bda344cdcb20 ci: Use ubuntu-slim for [meta] runners (MarcoFalke)
fa9627af9f89b2e2c04ae564d11d1af148c9682b ci: Rely on cmake --preset toolchain file (MarcoFalke)
fa3f89acaa7ae3332f4bf13aa91fcff44902990c ci: Add check_manifests to ci-windows.py (MarcoFalke)
1111079a16b92bb34ee2f8d4ae3529fbcb3180b2 ci: Add run_tests step to ci-windows.py (MarcoFalke)
fa561682ce4052bed91fac11f8fc872af5132437 ci: [refactor] Add .github/ci-windows.py prepare_tests step (MarcoFalke)
fa3e607c6dfb220684a65638b35ac5a4d3b1fcb7 ci: Print verbose Windows CI build failure (MarcoFalke)
4444808dd3a704ec0f5d97e2c736cc917656c491 ci: [refactor] Add .github/ci-windows.py build step (MarcoFalke)
fabdd4e82342b57e8e7750c5292e9fe0d598105b ci: Refactor Windows CI into script (MarcoFalke)
Pull request description:
Just like all the other CI configs, the Windows one should print a single and readable build failure at the end.
Also, includes a bunch of Windows CI refactors.
ACKs for top commit:
m3dwards:
ACK fa90277d22e1e6662e827e662eb6bda344cdcb20
hebasto:
ACK fa90277d22e1e6662e827e662eb6bda344cdcb20.
willcl-ark:
utACK fa90277d22e1e6662e827e662eb6bda344cdcb20
Tree-SHA512: 00443289e3d8b6d60d1347934d9d4b16098e8c36b6325467e5804b1869714201c4f7e932da3be44392c73e4713a1f52cd8af481894d36c6a281ba7238d43434e
fe0b1513a7c53b8490b81165acf1c7d42297a2ed test: add a test for txgraph staging (Hao Xu)
ef253a9d3d16f62fe39fbeee336b100554ceaff7 test: add block builder tests for txgraph (Hao Xu)
4a1ac31e97c252e5977ddd85109978307d427807 test: add a chunk test for txgraph (Hao Xu)
Pull request description:
Add tests for cluster chunks, including:
- txgraph_chunk_chain test: test chunk implementation for a simple chain style graph .
- txgraph_staging test: test the staging feature for a basic graph.
ACKs for top commit:
instagibbs:
reACK fe0b1513a7c53b8490b81165acf1c7d42297a2ed
sipa:
reACK fe0b1513a7c53b8490b81165acf1c7d42297a2ed
Tree-SHA512: 01010a3b4e4163849df2912d1393be74d26eb199d0544cfbef58a498aca5153463a118f55a2f1cad2995552b74210031e659de8df6b88cbcffdffd2a1b464990
fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false (MarcoFalke)
Pull request description:
Without this doc, there is a risk that the setting will be turned off, see https://github.com/bitcoin/bitcoin/pull/34514.
The reason to disable it is to catch logic bugs, even on trivially copyable types:
```cpp
#include <utility>
void Eat(int&& food) { food = 0; };
int main() {
int food{2};
Eat(std::move(food));
Eat(std::move(food)); // This should err
}
```
ACKs for top commit:
l0rinc:
ACK fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e
hebasto:
ACK fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e.
sedited:
ACK fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e
Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a
fa6801366d76a34f51a7d60be7d3710ed8e722db refactor: [rpc] Remove confusing and brittle integral casts (take 2) (MarcoFalke)
Pull request description:
Second take for cases which I seem to have missed in commit 94ddc2dced5736612e358a3b80f2bc718fbd8161.
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `static_cast<bool>(fCoinBase)`, because `bool{fCoinBase}` only works on modern compilers.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
Sjors:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
sedited:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
fa16b275fa94f66067c94fb8d0854859df6953b8 test: Check that interrupt results in EXIT_SUCCESS (MarcoFalke)
fab7c7f56c1d72178435618c49611a3ecd80b9b8 test: Split large init_stress_test into two smaller functions (MarcoFalke)
Pull request description:
This is a test for https://github.com/bitcoin/bitcoin/pull/34224. The test can be tested by reverting that pull request and observing the test failure.
Also, includes a small test cleanup/refactor.
ACKs for top commit:
janb84:
ACK fa16b275fa94f66067c94fb8d0854859df6953b8
sedited:
ACK fa16b275fa94f66067c94fb8d0854859df6953b8
Tree-SHA512: bb6894316fd4f78b3c0cb299cc93abf7f3f794e0507bf7deda7d86f8f45d60299ec0f027c41a6f909c197a1e5fba44fe269ce4165450b89738b82d8ddf196b80
8c03318387f6b3d1520a539f426b300bae316fc3 consensus/doc: explain `GetValueOut()` precondition (Lőrinc)
82ef92c8d006b3f5c3baaf00e5f8200d289d85d2 consensus/doc: explain unreachable `bad-txns-fee-outofrange` check (Lőrinc)
232a2bce90a96720f5c8d31413f1d14b4c9d90f2 consensus/test: add out-of-range output unit tests for `CTransaction::GetValueOut` (Lőrinc)
aa87aae14f9eee79e3a0fb9c0f5ff3eaa97433e2 consensus/test: add `MoneyRange` unit tests for `CheckTxInputs` (Lőrinc)
Pull request description:
### Problem
Coverage reports indicate that a few consensus related validations aren't exercised in unit-, and some not even in the functional-tests:
Inspired by the coverage reports:
* ["bad-txns-premature-spend-of-coinbase"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L180): Only covered in functional tests
* ["bad-txns-inputvalues-outofrange"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L187): Unreachable in functional tests [1], uncovered in unit tests
* ["bad-txns-in-belowout"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L193): Only covered in functional tests
* ["GetValueOut: value out of range"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/primitives/transaction.cpp.gcov.html#L103) and [total coverage report](https://maflcko.github.io/b-c-cov/total.coverage/src/primitives/transaction.cpp.gcov.html#L103)
Replacing them with explicit throws still passes all unit (and sometimes even functional) tests, confirming those branches are not being exercised, see: https://github.com/l0rinc/bitcoin/pull/112
### Fixes
Add minimal unit test coverage for `Consensus::CheckTxInputs` invalid outcomes for `bad-txns-premature-spend-of-coinbase`, `bad-txns-inputvalues-outofrange`, `bad-txns-in-belowout`.
Add a unit test covering `CTransaction::GetValueOut()` throwing for out of range values.
After the prerequisits are tested, document why `bad-txns-fee-outofrange` is unreachable - while keeping the check in place because it is consensus-critical code.
ACKs for top commit:
maflcko:
lgtm ACK 8c03318387f6b3d1520a539f426b300bae316fc3 🍴
darosior:
utACK 8c03318387f6b3d1520a539f426b300bae316fc3
sedited:
ACK 8c03318387f6b3d1520a539f426b300bae316fc3
Tree-SHA512: 91c65dda99b42d12de99c58b02df0f861203f97d268329a3ecce79bd681fcaf847f508c1d9f2256b2b92a953a94d868cbae647f378def92484681d771722ea27
76dae5d6911b600fafa3a417a740f14b299284f3 cmake: Replace recursive globbing with explicit globbing in folders (Hennadii Stepanov)
88d909257104124bee3fc810f7fa32e5802b92fe cmake: Create subdirectories in build tree in advance (Hennadii Stepanov)
Pull request description:
While reviewing https://github.com/bitcoin/bitcoin/pull/32697, I noticed that symlink creation fails when the target subdirectory does not exist. In such cases, `file(CREATE_LINK ... COPY_ON_ERROR SYMBOLIC)` falls back to copying, which implicitly creates the required path. As a result, a single file is copied instead of symlinked for subdirectories in `test/functional`.
This PR ensures that necessary subdirectories are created in advance, so that subsequent symlink creation does not fail due to missing paths.
For example:
- on the master branch:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 8
-rwxrwxr-x 1 hebasto hebasto 2683 Jul 3 14:11 invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 14:11 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 14:11 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 14:11 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
- with this PR:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 4
lrwxrwxrwx 1 hebasto hebasto 65 Jul 3 13:51 invalid_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 13:51 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 13:51 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 13:51 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
ACKs for top commit:
m3dwards:
ACK 76dae5d6911b600fafa3a417a740f14b299284f3
sedited:
ACK 76dae5d6911b600fafa3a417a740f14b299284f3
janb84:
re ACK 76dae5d6911b600fafa3a417a740f14b299284f3
Tree-SHA512: a720a68752c72390b9452b192b06d09e41cac1080d32cfe3c2caabb65949626771e0709e7193f69677bd24a3c747e368c2323d9c857d4aa97e1890cc463850ed
This is the standard approach and avoids relying on
VCPKG_INSTALLATION_ROOT and -DCMAKE_TOOLCHAIN_FILE= in the ci-windows.py
script.
This makes it easier to run locally.
This is mostly a refactor, except for using the runner workspace (cwd) for:
* fuzz qa-assets
* functional temp prefix
This makes it easier to run the ci-windows.py locally.
7378f27b4fb512567b6152f986f67d9263d08d7a test: run utxo-to-sqlite script test with spk/txid format option combinations (Sebastian Falbesoner)
b30fca7498c93356fbdb8c2ce881aa8e548bae17 contrib: utxo_to_sqlite.py: add options to store txid/spk as BLOBs (Sebastian Falbesoner)
Pull request description:
This PR is a late follow-up to https://github.com/bitcoin/bitcoin/pull/27432, introducing an option for the utxo-to-sqlite script to store the txid/scriptPubKey columns as bytes (= `BLOB` storage class in sqlite, see e.g. https://www.sqlite.org/datatype3.html in sqlite) rather than hex strings. This was proposed in earlier reviews (https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516857024, https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351) and has the obvious advantage of a significantly smaller size of the resulting database (and with that, faster conversion) and the avoidance of hex-to-bytes conversion for further processing of the data [1]. The rationale on why hex strings were chosen back then (and still stays the default, if only for compatibility reasons) is laid out in https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516922824 [2].
The approach taken is introducing new parameters `--spk` and `--txid` which can either have the values "hex", "raw" (for scriptpubkey) and "hex", "raw", "rawle" (for txid). Thanks to ajtowns for providing this suggestion. Happy to take further inputs on naming and thoughts on future extensibility etc.
[1] For a concrete example, I found that having these columns as bytes would be nice while working on a SwiftSync hints generator tool (https://github.com/theStack/swiftsync-hints-gen), which takes the result of the utxo-to-sqlite tool as input.
[2] note that in contrast what I wrote back then, I think there is no ambiguity on byte-string-serialization of txids; they are ultimately just hash results and hence, they should be stored as such, and adding a big/little endian knob wouldn't make much sense. The drawback of not being able to immediately show txid-strings (as one would need to do the bytes-reversal step first, which is not possible in sqlite, see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1165499803) still remains though.
ACKs for top commit:
ajtowns:
ACK 7378f27b4fb512567b6152f986f67d9263d08d7a
w0xlt:
reACK 7378f27b4f
sedited:
ACK 7378f27b4fb512567b6152f986f67d9263d08d7a
Tree-SHA512: 265991a1f00e3d69e06dd9adc34684720affd416042789db2d76226e4b31cf20adc433a74d14140f17739707dee57e6703f72c20bd0f8dd08b6d383d3f28b450
37cc2a2d953c072a236b657bfd7de5167092a47a logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c8d70914d0878a0d7c6a1371dae23c0 logging: Move message formatting to util/log.h (stickies-v)
001f0a428e3aa3f46aad373684beb3282bffb2c0 move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e857f3c58a7ab813eb33b83635101d75 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0347a768daabccdfd76583e6b272dc4 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e9827f8dd23414c7fcf49298420f9fc42 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
stickies-v:
re-ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
optout21:
crACK 37cc2a2d953c072a236b657bfd7de5167092a47a
sedited:
ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
b73a62f667d0220a5700a02e11736684d2214b04 test: Ensure invalid block was processed before checking debug.log (Ava Chow)
Pull request description:
Prior to merging a PR, I run 4 different build configurations and their tests in parallel, and consistently one of those will have `feature_assumevalid.py` fail. The failure is because the `assert_debug_log` context exits before the invalid block is processed, so the lines it is looking for don't appear in the part of the log that it is examining.
This PR should resolve that issue by waiting for `getchaintips` to report that the invalid chain is invalid before exiting the `assert_debug_log` context.
ACKs for top commit:
l0rinc:
tested ACK b73a62f667d0220a5700a02e11736684d2214b04
sedited:
ACK b73a62f667d0220a5700a02e11736684d2214b04
Tree-SHA512: 96409ed55f686961c47949d31569d6be8e424d952883c92a9135a4e8a795047d4e2a86c9d27ef5653d21780717275434b0bca1586059cfd5088cd2c867c7baea
576f8920279820bca0caf2c32362ab9eb6e4ac1a qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
4b9f5beafe9ee77209014093cc5318e037809126 Update Transifex slug for 31.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](46e1288df2/doc/release-process.md).
It is required to open Transifex translations for v31.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/33607.
For reference, see the previous similar PR: https://github.com/bitcoin/bitcoin/pull/33152.
**Note for reviewers:**
To reproduce the diff in the last commit, run:
```
cmake --preset dev-mode
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
stickies-v:
ACK 576f8920279820bca0caf2c32362ab9eb6e4ac1a
Tree-SHA512: 9875831b8ea6ace5b6e47fe10351bc7cdd26b9c27fe275f678c7f33f6df115ce3e956f9d522905b2fda76d82fc0a4135482cfb665c4428ab2bc1164c49cf82f4
633d1831199a2ca6ca31b97312130419328d5479 test: misc interface_ipc_mining.py improvements (Sjors Provoost)
52ccd9215e6700ce3090c63f3a3017a1ae21f98c test: split interface_ipc_mining.py into subtests (Sjors Provoost)
4e49fa2a68846f04cec0beb64c80ba53d3163528 test: add interface_ipc_mining.py (Sjors Provoost)
01a1ae889e5aa71d9ae4a76000265572ffcae99f test: move IPC helpers to ipc_util.py (Sjors Provoost)
Pull request description:
This test has been growing too large, making it difficult to maintain. Especially when multiple pull requests change it.
- move helper functions to `ipc_util.py`
- move mining test to `interface_ipc_mining.py`, keeping only an interface sanity check in `interface_ipc.py`
- split the tests in `interface_ipc_mining.py`
- misc tweaks (to reduce churn in the above commits)
Review hint:
```sh
git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
```
ACKs for top commit:
janb84:
re ACK 633d1831199a2ca6ca31b97312130419328d5479
fjahr:
reACK 633d1831199a2ca6ca31b97312130419328d5479
l0rinc:
code review ACK 633d1831199a2ca6ca31b97312130419328d5479
sedited:
Re-ACK 633d1831199a2ca6ca31b97312130419328d5479
Tree-SHA512: 81bbbec1f03a6ff63068dbefc1bc4768fcd24313d84ba454bb2494fe4a65838dbaeb93ad7f02ed4c9932394f3d24638453bb51ce0e05f561dc613414beda37e4
fa0677d131191d7db9868c4c1b3d780cb6991226 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb39564569e7b09982bec68ae41e45a04f87 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764bbd00fc225df5f4601dd4f687273e0 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e3ce734730c56660a958f9cf9dc7d38 refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec27522959cdf1ad35d54f080aafbfc47 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735281d2cce123dbd59d20c7339b2b4ee 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 fa0677d131191d7db9868c4c1b3d780cb6991226
achow101:
ACK fa0677d131191d7db9868c4c1b3d780cb6991226
janb84:
re ACK fa0677d131191d7db9868c4c1b3d780cb6991226
sipa:
crACK fa0677d131191d7db9868c4c1b3d780cb6991226
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
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
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
