This combines the clusterlin_add_dependency and clusterlin_cluster_serialization
fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts
from an empty DepGraph and performs a arbitrary number of AddTransaction,
AddDependencies, and RemoveTransactions operations on it, and compares the
resulting state with a naive reimplementation.
This commits introduces support in DepGraph for the transaction positions to be
non-continuous. Specifically, it adds:
* DepGraph::RemoveTransactions which removes 0 or more positions from a DepGraph.
* DepGraph::Positions() to get a set of which positions are in use.
* DepGraph::PositionRange() to get the highest used position in a DepGraph + 1.
In addition, it extends the DepGraphFormatter format to support holes in a
compatible way (it serializes non-holey DepGraphs identically to the old code,
and deserializes them the same way)
This changes DepGraph::AddDependency into DepGraph::AddDependencies, which takes
in a single child, but a set of parent transactions, making them all dependencies
at once.
This is important for performance. N transactions can have O(N^2) parents combined,
so constructing a full DepGraph using just AddDependency (which is O(N) on its own)
could take O(N^3) time, while doing the same with AddDependencies (also O(N) on its
own) only takes O(N^2).
Notably, this matters for DepGraphFormatter::Unser, which goes from O(N^3) to O(N^2).
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
This does not change the serialization format.
It turns out that it is unnecessary to keep track of the order of transactions
in the so-far reconstructed DepGraph to decide how far from the end to insert
a new transaction.
This commit does not change the serialization format. Its purpose is making a
few changes already in order to reduce the diff size of the later commit that
introduces support for holes in DepGraph.
The previous approach was to immediately construct a transaction as soon as its
feerate was known in a preliminary position, and then undo that, and place it
in the correct position once the position information is known (such that a
deserialization error in between would not result in an inconsistent state).
The new approach is to delay the actual transaction creation until all its
information is known, avoiding the need to undo and redo. This requires a
different means of determining whether dependencies are redundant, but that has
the advantage that a later commit can apply all dependencies at once, reducing
the complexity of deserialization.
A fuzz test already relies on these operations, and a future commit will need
the same logic too. Therefore, abstract them out into proper member functions,
with proper testing.
This has been unused since #29648.
Noticed while running a newer version of clang-tidy (19.1.1):
```bash
[127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp
bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors]
126 | CMutableTransaction tx2 = tx;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message);
512 warnings generated.
```
56aad83307 ci: set a ctest timeout of 1200 (20 minutes) (fanquake)
Pull request description:
This should be long enough (with headroom) for our longest running tests, which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` [under TSAN](https://cirrus-ci.com/task/6321297691508736?logs=ci#L2520):
```bash
tests ................................ Passed 795.20 sec
```
[and MSAN](https://cirrus-ci.com/task/4913922807955456?logs=ci#L2226):
```bash
tests ................................ Passed 658.48 sec
```
This will also prevent the current issue we are seeing of `ctest` running until it reaches the CI timeout, see #30969.
We still need to figure out what underlying issue is causing the tests to (sometimes) run for so long, but in the mean time, this will stop `ctest` wasting our CI CPU. It should also make it more clear in the logs, exactly which test is the one that is hitting the timeout.
ACKs for top commit:
maflcko:
review ACK 56aad83307
tdb3:
re ACK 56aad83307
Tree-SHA512: 43c0dc12b8b12b1d9804751a9816935e2abbe962b451e12a268f2d2c430bc568b83995dbc405f100b596dfb0f1e9f65b78074de98916592d3ae4ebc2126e3a6c
This should be long enough (with headroom) for our longest running tests,
which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` under TSAN:
```bash
tests ................................ Passed 795.20 sec
```
and MSAN:
```bash
tests ................................ Passed 658.48 sec
```
This will also prevent the current issue we are seeing of `ctest`
running until it reaches the CI timeout, see #30969.
However, we still need to figure out what underlying issue is causing
the tests to (sometimes) run for so long, but in the mean time, this
will stop `ctest` wasting our CI CPU.
80761afced qt6: Handle different signatures of `QANEF::nativeEventFilter` (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
For more details, please refer to 3b38c73c7f.
No behaviour change.
ACKs for top commit:
maflcko:
lgtm ACK 80761afced
promag:
Code review ACK 80761afced.
Tree-SHA512: a265e1c33cc7da37003bb0e6fd40950acb5e948ca9ec63a59a79c5e2a1894334f48d5565539c91d4d777b48a589366958df1498eaa6935e3b7fb534493adb51a
deacf3c7cd cmake: Avoid hardcoding Qt's major version in Find module (Hennadii Stepanov)
Pull request description:
This PR facilitates future migration to Qt 6 and is a prerequisite for https://github.com/bitcoin/bitcoin/pull/30997.
No behaviour change.
ACKs for top commit:
l0rinc:
utACK deacf3c7cd
promag:
Code review ACK deacf3c7cd.
maflcko:
lgtm ACK deacf3c7cd
Tree-SHA512: 6991f30e9cf6a7103bfe91f8958246b17360210bf695ab620ca1c8b59565bf6192cc366036cf54f860ccc4d782b9c28899970978de79392c88a45ef149c06a79
fee4cba484 gui: Fix proxy details display in Options Dialog (pablomartin4btc)
Pull request description:
Currently, setting up a proxy (whether SOCKS5 or Tor) with an IPv6 address works correctly via the command line or configuration file in both `bitcoind` and `bitcoin-qt` (also from the UI the ipv6 address gets saved properly in `settings.json`). However, the UI does not reflect this properly, which can create confusion. Since some ISPs and VPNs still experience issues with IPv6, users may mistakenly think there is a problem with Bitcoin Core, when in fact the proxy setup is functioning as expected.
So this PR ensures that the proxy IP is displayed correctly in the UI when using an IPv6 address.
No functionality impact; changes only affect UI display.
<details>
<summary>Click her to see <b>before</b> and <b>after</b> screenshots.</summary>
- Before:
- After:
</details>
---
<details>
<summary>Test instructions</summary>
(Ubuntu 22.04)
1. Start ssh service on localhost.
`ssh -D [::1]:1080 -f -C -q -N localhost`
2. Check that the service is up and running.
```
ps aux | grep ssh
pepe 2860289 0.0 0.0 20456 5576 ? Ss 06:59 0:00 ssh -D [::1]:1080 -f -C -q -N localhost
```
3. Check with `bitcoind` if it works correctly.
`bitcoind -onlynet=ipv6 -proxy=[::1]:1080`
4. Check for established connections.
```
netstat -natl |grep 1080
tcp6 0 0 ::1:1080 :::* LISTEN
tcp6 0 0 ::1:47610 ::1:1080 ESTABLISHED
tcp6 0 0 ::1:1080 ::1:47610 ESTABLISHED
tcp6 0 0 ::1:1080 ::1:47606 TIME_WAIT
```
```./build/src/bitcoin-cli getpeerinfo
[
{
"id": 0,
"addr": "[2a01:4f9:4a:2a07::2]:8333",
"addrbind": "[::1]:47638",
"network": "ipv6",
...
```
5. Stop `bitcoind` and run `bitcoin-qt` adding the corresponding configuration in `settings.json`.
```
{
"onlynet": "ipv6",
"proxy": "[::1]:1080",
}
```
6. Open the Peers window to check available connections or run `getpeerinfo` on the rpc-console window.
7. Same can be done for Tor setting up `tor` service (I'll add instructions later) and configuring on its default port 9050 and forcing `"onlynet": "onion"` to verify easily the net traffic.
</details>
---
Thanks jarolrod and vasild for your help on validating ipv6 was not broken.
ACKs for top commit:
vasild:
ACK fee4cba484
promag:
Code review ACK fee4cba484.
hebasto:
ACK fee4cba484, I have reviewed the code and it looks OK.
Tree-SHA512: 4be9052569ccb1e17ce94fb15691debf0651fa172ed1a83d60696d10f20d469b19d70a979b65322951f5783cd7582d55b39b669edb588e20404d8d10e767c49a
98c1536852 test: add getorphantxs tests (tdb3)
93f48fceb7 test: add tx_in_orphanage() (tdb3)
34a9c10e8c rpc: add getorphantxs (tdb3)
f511ff3654 refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2 refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852
hodlinator:
re-ACK 98c1536852
danielabrozzoni:
ACK 98c1536852
pablomartin4btc:
tACK 98c1536852
itornaza:
reACK 98c1536852
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
5be34bacf6 qt: Fix linking when configured with `-DENABLE_WALLET=OFF` (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
When building with Qt 6 in my dev branch, I encountered a linker error when configured with `-DENABLE_WALLET=OFF`:
```
$ cmake -B build -DENABLE_WALLET=OFF -DBUILD_GUI=ON
$ cmake --build build -t bitcoin-qt
<snip>
[100%] Linking CXX executable bitcoin-qt
/usr/bin/ld: libbitcoinqt.a(rpcconsole.cpp.o): in function `QtPrivate::MetaObjectForType<WalletModel const*, void>::metaObjectFunction(QtPrivate::QMetaTypeInterface const*)':
/usr/include/x86_64-linux-gnu/qt6/QtCore/qmetatype.h:903:(.text._ZN9QtPrivate17MetaObjectForTypeIPK11WalletModelvE18metaObjectFunctionEPKNS_18QMetaTypeInterfaceE[_ZN9QtPrivate17MetaObjectForTypeIPK11WalletModelvE18metaObjectFunctionEPKNS_18QMetaTypeInterfaceE]+0x2b): undefined reference to `WalletModel::staticMetaObject'
/usr/bin/ld: libbitcoinqt.a(rpcconsole.cpp.o): in function `QMetaTypeIdQObject<WalletModel const*, 8>::qt_metatype_id()':
/usr/include/x86_64-linux-gnu/qt6/QtCore/qmetatype.h:1313:(.text._ZZN9QtPrivate16QMetaTypeForTypeIPK11WalletModelE17getLegacyRegisterEvENUlvE_4_FUNEv[_ZZN9QtPrivate16QMetaTypeForTypeIPK11WalletModelE17getLegacyRegisterEvENUlvE_4_FUNEv]+0x53): undefined reference to `WalletModel::staticMetaObject'
collect2: error: ld returned 1 exit status
gmake[3]: *** [src/qt/CMakeFiles/bitcoin-qt.dir/build.make:154: src/qt/bitcoin-qt] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:2107: src/qt/CMakeFiles/bitcoin-qt.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:2114: src/qt/CMakeFiles/bitcoin-qt.dir/rule] Error 2
gmake: *** [Makefile:998: bitcoin-qt] Error 2
```
This PR resolves the issue.
ACKs for top commit:
promag:
ACK 5be34bacf6, all other changes in 33657e1c958146312e4c68765a92871920401396 are not required, makes the code harder to read.
pablomartin4btc:
tACK 5be34bacf6
Tree-SHA512: d10da665384e6539b8e9106dc905ba30e9e57cd4603fc7e5eb893fc899f55f26889c570687fa5daf55c6fc5bf4fcfcfcd9b70822cfe637f31f9151bb653b7941
5625840c11 qt6, test: Handle deprecated `QVERIFY_EXCEPTION_THROWN` (Hennadii Stepanov)
cb750b4b40 qt6, test: Use `qWarning()` instead of `QWARN()` macro (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
---
This PR can be tested on macOS using the first commit from https://github.com/bitcoin/bitcoin/pull/30997 and Homebrew's `qt` package.
ACKs for top commit:
promag:
Code review ACK 5625840c11.
Sjors:
tACK 5625840c11
Tree-SHA512: e7307eaf0027c6addc9481ba91ed31b81554ffb0d2ba77938e68915c9d490a7962e55a330f97ea31d49bbfb30f92773c3af3afc867a4215d00752405d7e3bb6d
27709f51ee docs: Add instructions on how to self-sign bitcoin-core binaries for macOS (Chris Stewart)
Pull request description:
Related to #15774
This PR adds instructions to the release notes to tell users how to self sign bitcoin core binaries so they are executable on macOS.
Tested on
```
Darwin Chriss-MacBook-Pro.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6031 arm64
```
These commands do not appear to require 'phoning home'. I tested these commands when disconnected from a network connection and things worked.
ACKs for top commit:
andrewtoth:
reACK 27709f51ee
achow101:
ACK 27709f51ee
Tree-SHA512: db19c61577bb774420a2506d3f06bc0193116117f09ebd2d022a4524e8ca32d2cf9277a2997744ddfe8844600a569176e194aafc252dd31b48fc6e74db3c74d0
a7498cc7e2 Fix bug in p2p_headers_presync harness (marcofleon)
Pull request description:
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
https://github.com/bitcoin/bitcoin/pull/30918 followup
ACKs for top commit:
dergoegge:
utACK a7498cc7e2
instagibbs:
ACK a7498cc7e2
glozow:
makes sense, utACK a7498cc7e2
mzumsande:
ACK a7498cc7e2
Tree-SHA512: b95f25dcf7ace220e30f1d72f50d85ee18777467927c0cc1ed8582b390cb7185ffc0e2f127309eb083044fb41f5a13fce5ebb15b7952718a899bafff26921be8
fa2b7d8d6b Remove redundant unterminated-logprintf tidy check (MarcoFalke)
bbbb2e43ee log: Enforce trailing newline, Remove redundant m_started_new_line (MarcoFalke)
Pull request description:
There are many problems around missing a trailing newline while logging:
* All log lines are currently terminated by a trailing newline. This means any runtime code trying to handle a "missing" newline is currently dead code.
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmaintained (no one updated it for the new log function names), probably harder to maintain than normal C++ code (because it requires clang AST matcher knowledge), brittle (it can fail to detect issues at any time, if it goes out-of-sync, or be explicitly disabled via `NOLINT`), and annoying for devs (it is slow and intricate to run locally and thus only effectively run on CI or via the CI scripts).
Fix all issues by enforcing the trailing newline in logs directly in the code. Then remove all the other stuff.
This refactor does not change behavior.
ACKs for top commit:
stickies-v:
re-ACK fa2b7d8d6b
achow101:
ACK fa2b7d8d6b
ryanofsky:
Code review ACK fa2b7d8d6b. Just comment and test cleanup since last review
Tree-SHA512: 10ed420f6c2fdb0f491d6c880be8dd2e8beef628f510adebadf4c3849d9f5e28906519d5cbaeb295f4c7c1b07c4c88a9905b3cfe30fee3a2c91ac9fd24ae6755
fd38711217 ci: make CI job fail when check-deps.sh script fails (Ryan Ofsky)
d51edecddc common: move pcp.cpp and netif.cpp files from util to common library since they depend on netaddress.cpp (Ryan Ofsky)
Pull request description:
Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by _fanquake_ in https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097.
Also make CI fail when the `check-deps.sh` script fails. Previously it would output errors but not cause the job to fail (which was not intended).
ACKs for top commit:
Sjors:
utACK fd38711217
laanwj:
Untested ACK fd38711217
achow101:
ACK fd38711217
tdb3:
ACK fd38711217
Tree-SHA512: 06316e68617ded7d96d540c9934b08cf9fbba5ff5e7f54d7a0c0a9087a26bf8adc97e9e8c39a2bfd3da34e27f3652b1531ec6136a2c69393ae0b26585abadb6b
a1576edab3 test: add missing sync to feature_fee_estimation.py (Martin Zumsande)
Pull request description:
This fixes a race:
- In the `test_estimate_dat_is_flushed_periodically` subtest, node 0 is isolated and creates 10 blocks (no sync).
- In `clear_estimates` the nodes are reconnected (but we don't wait for them to sync!)
- In the `sanity_check_rbf_estimates` subtest, node 1 generates another block and syncs with the other nodes. The sync fails if the generated block is at the same height as the tip of node 0.
Fix this by adding a sync to `clear_estimates`.
Fixes#30990Fixes#30640
ACKs for top commit:
maflcko:
lgtm ACK a1576edab3
tdb3:
code review ACK a1576edab3
Tree-SHA512: 608ba619cacb4ff3a1ea934e03286f18c96afeebf06439334d40bff72025bd7bcc2c1093dae1824b30a37d3ac3ea569bc3118c33c0ca51610592aa1b4f420840
CMake parses some paths from the spec of the C compiler, assuming it
will be the linker, resulting in the link to end up with
`-L/usr/lib/gcc/x86_64-w64-mingw32/12-win32` on debian bookworm if both
-win32 and -posix variants are installed, and -win32 is the default
alternative.
This results in the wrong C++ library being linked, missing
std::threads::hardware_concurrency and other threading functions.
To fix this, use the -posix variant of gcc as well when available. This
fixes a regression compared to autotools, where this scenario worked.
Previously the check-deps.sh would write information about unexpected
dependencies to stderr, but return exit code 0, so the error would be ignored
by CI. Now it will return code 1 and cause CI to fail if unexpected
dependencies are detected.
