Bitcoin Core Github
45 subscribers
118K links
Download Telegram
💬 fanquake commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2397119816)
Looks like this is working: https://cirrus-ci.com/task/5848920612405248?logs=ci#L2234
```bash
[14:18:46.371] 126/135 Test #130: spend_tests .......................... Passed 19.83 sec
[14:19:14.504] 127/135 Test #110: txvalidationcache_tests .............. Passed 122.32 sec
[14:19:15.437] 128/135 Test #8: bench_sanity_check_high_priority ..... Passed 318.61 sec
[14:19:24.864] 129/135 Test #76: random_tests ......................... Passed 208.03 sec
[14:19:25.959] 130/135 T
...
💬 fanquake commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2397123023)
It works with `llvm@18`, so it looks like this could be an incompatibility with LLVM 19 (19.1.1) and Apples `ld`.
💬 maflcko commented on pull request "ci: Add missing -DWERROR=ON to test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/31045#issuecomment-2397143969)
The CI logs:

* Before https://github.com/bitcoin/bitcoin/actions/runs/11091169274/job/30814507260#step:6:353:

```
Treat compiler warnings as errors ..... OFF
```

* This pull https://github.com/bitcoin/bitcoin/actions/runs/11213662779/job/31167158663?pr=31045#step:6:350:

```
Treat compiler warnings as errors ..... ON
💬 theuni commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2397158180)
Concept ACK.
💬 maflcko commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2397181620)
> Looks like this is working: https://cirrus-ci.com/task/5848920612405248?logs=ci#L2234

Though, this is msan, which seems just like a slow machine, because out of 10 runs, it sometimes takes longer: https://cirrus-ci.com/task/6180976303276032?logs=ci#L2223

Once https://github.com/bitcoin/bitcoin/issues/30852 is fixed, the results should be hopefully both faster and more consistent.
⚠️ Sjors opened an issue: "macOS 13.7 depends build can't find qt"
(https://github.com/bitcoin/bitcoin/issues/31050)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

```
cd depends
make NO_BDB=1
...
copying packages: boost libevent qt qrencode sqlite miniupnpc zeromq
to: /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0

cd ..
cmake -B build --toolchain depends/x86_64-apple-darwin22.6.0/toolchain.cmake
```

This fails, see below

(building BDB hasn't work for me for a while, but that's going away anyway)

### Expected behaviour

T
...
💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2397189214)
Will investigate and provide more details later.
🤔 instagibbs reviewed a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2352223628)
> Pushed to https://github.com/bitcoin/bitcoin/commit/ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9 to replace assert_approx with assert_equal (using setmocktime)

good call, approx always sets off alarm bells as a test reader :)

we have a chance to outright reject verbosity>2 for extensibility before a release, but I can't think of what other things you'd possible want that can't be given in the 3 modes already included.

concept ACK
💬 instagibbs commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790399506)
let's name it the same as in the CPP code? `ORPHAN_TX_EXPIRE_TIME`
💬 theuni commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2397218483)
> Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.

Could you please explain why this is the case? Ninja seems like an odd requirement here.
💬 l0rinc commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2397229720)
Finished benchmarking with the default 2 mb file size vs 4, 8, 64 and 128 mb.
This time it's full IBD with real peers until 800k blocks on a HDD.

<img src="https://github.com/user-attachments/assets/17d1bb75-c85c-4f82-82f7-d4028bbcb88a">

<details>
<summary>Benchmarks</summary>

```bash
hyperfine --runs 1 --export-json /mnt/ibd_DBFILESIZE.json --parameter-list DBFILESIZE 2,4,8,64,128 --prepare 'rm -rf /mnt/BitcoinData/*' './build/src/bitcoind -datadir=/mnt/BitcoinData -stopat
...
💬 maflcko commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1790444004)
This may also work around the CI false positive
💬 theuni commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2397251484)
I could also possibly supply a beefy dedicated threadripper machine in an MIT datacenter (which should have local-ish mirrors for apt and etc). When comparing costs, I think it makes sense to think of buying hardware vs renting as well.
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2351684992)
first pass light code review ACK 2b21aebd9754c503bac12d1dbf566b3aa27451e8

Mostly trying to understand what the code is doing!


comments are just nits
💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790355464)
nit maybe calling `mapping` a container will be better.
```suggestion
*@param mapping A container, such that mapping[i] gives the position in the new DepGraph
```
💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790049084)
nit: This loop can be modified to be more readable by using using descriptive variable names and some helpful comment on what the inner loops are doing.
```suggestion
for (ClusterIndex i = 0; i < depgraph.TxCount(); ++i) {
// Initialize the set of ancestors with just the current transaction itself.
SetType ancestor_union = SetType::Singleton(i);
// Iteratively add parents and parents of parents to the ancestor set until there is no more ancestor t
...
📝 fanquake opened a pull request: "test: remove unused code from `script_tests`"
(https://github.com/bitcoin/bitcoin/pull/31051)
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 | CMutableTransact
...
🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2352291145)
Remaining (non nitty) things I think to be addressed:
1) https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2391768053
2) https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1787010637

`git range-diff master ce205abe10ebccfdbea60adf4c568a8ba61390c3 ff1f2dc055efe92b3202387c66a12948134eced2`
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1790442628)
correct_tx appears in both for the txid-indexed version
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2352323652)
li