💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396412336)
Rebased.

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1789888213)
Sorry, I don't think my last comment on this was very clear. Lines L109-L112 are moved up from below, and `cache_sizes.block_tree_db` is set. So I think L127 should be:

```suggestion
.block_tree_db_cache_size = cache_sizes.block_tree_db,
```

unless I'm missing something?

💬 fanquake commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2396436910)
> But did loading of the snapshot actually complete eventually

It did. I haven't had time to (re-)test anything here further. I don't think I have the full logs to share any more, but I will try and recreate them.

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1789897798)
semi-related: I wonder if Windows can be bumped to Windows 10 either way. If long EOL, and ancient versions of Windows are documented to be supported, people may use them and open up themselves to security issues. I think it could be useful to only list supported (receiving security updates) operating systems here.

Could be done here, or in a separate pull.

⚠️ fanquake opened an issue: "lint: markdown check linting build outputs"
(https://github.com/bitcoin/bitcoin/issues/31044)
Running the linter locally I see this output:
```bash
DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter
<snip>
One or more markdown links are broken.

Relative links are preferred (but not required) as jumping to file works natively within Emacs.

Markdown link errors found:
[Err ] ./depends/work/build/x86_64-apple-darwin/qt/6.7.2-b49980c3ba7/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Tes
...

💬 fanquake commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#discussion_r1789919608)
> I wonder if TEST_RUNNER_TIMEOUT_FACTOR * 30 can be re-used here,

Sure, updated to use this. I haven't run a s390x build for a while, but I'd assume it would fail here. Started one in any case.

💬 maflcko commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2396496103)
review ACK 56aad83307e46983a397236bd0959e634207f83e

No change to the new timeout value of 1200 in this force push, but making it configurable, in case someone needs that, and also in symmetry with the functional tests.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396504362)
> It'd be good if [fba4df9](https://github.com/bitcoin/bitcoin/commit/fba4df9c200af0be5e2ed4ecc98cb2c0cb0b33ab) explained why macOS `12.x` is needed, because Qt 6 claims to support macOS `11.x` through to `14.x`: https://doc.qt.io/qt-6/macos.html.

Qt 6 uses `@available` directives in Objective-C++ code, which causes unresolved `__isPlatformVersionAtLeast` symbols during cross-compilation. A [patch](https://github.com/bitcoin/bitcoin/blob/dae1d48c546c2e1daadbf982420ba3d8feda2a9f/depends/patche
...

💬 maflcko commented on issue "lint: markdown check linting build outputs":
(https://github.com/bitcoin/bitcoin/issues/31044#issuecomment-2396511868)
> simultaneously claim that things are broken, but that all checks passed?

Some lint checks print output on success, so that may be confusing. I think the only relevant information for errors is the presence of `⚠️`, which should exactly correlate with a non-zero exit status. Could you check `echo $?`?

The mlc issue is tracked in https://www.github.com/becheran/mlc/pull/96

💬 TheCharlatan commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2396514254)
Going to address the left-over comments in a follow-up.

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2396520302)
Updated 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 60ade81516d42d275c1143f49f47e142d32c45fc ([blockmanDB_1](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_1) -> [blockmanDB_2](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_1..blockmanDB_2))

* Addressed @josibake's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1789888213), reusing a option value in `bitcoin-chainstate`
* Exchanged
...

💬 maflcko commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1789946568)
> @maflcko I'm puzzled why the `test each commit` job didn't catch this. No test coverage? Shouldn't it use `-Werror`?

Yeah, probably, that should be enabled. Ref: https://github.com/bitcoin/bitcoin/actions/runs/11091169274/job/30814507260#step:6:493

```
/home/runner/work/bitcoin/bitcoin/src/util/chaintype.cpp:47:13: warning: enumeration value 'TESTNET4' not handled in switch [-Wswitch]
47 | switch (chain) {
| ^~~~~
1 warning generated.

📝 maflcko opened a pull request: " ci: Add missing -DWERROR=ON to test-each-commit "
(https://github.com/bitcoin/bitcoin/pull/31045)
Found by in Sjors in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610 (Thanks!)

Also, includes an unrelated commit to simplify the ci scripts, by assuming nproc exists on macos as well. (Having more than one commit is also required to trigger the `test-each-commit` task)

💬 maflcko commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1789957118)
Fixed in https://github.com/bitcoin/bitcoin/pull/31045

⚠️ maflcko reopened an issue: "Intermittent timeout in tsan feature_init.py"
(https://github.com/bitcoin/bitcoin/issues/30586)
This happens intermittently with something like https://cirrus-ci.com/task/6520727065591808?logs=ci#L3416

```
Remaining jobs: [feature_init.py]
...............................................................................................................................................................................................................................................................................................................................................................
...

💬 maflcko commented on issue "Intermittent timeout in tsan feature_init.py":
(https://github.com/bitcoin/bitcoin/issues/30586#issuecomment-2396552702)
https://cirrus-ci.com/task/6689351046791168?logs=ci#L4759

💬 maflcko commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1789972151)
I think this patch is still incomplete. This will just turn the timeout into an early error. Obviously a fast error is better than a long timeout, but it would be better to exclude the test from the non-fuzz-only builds in the CI and mention in the pull request description that `require_build_for_fuzzing` targets must be excluded manually in non-fuzz-only builds.

An alternative would be to not register it, as explained by Cory.

📝 TheCharlatan opened a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046)
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.

The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.

💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2396584951)
Copy and pasting the command from the docs doesn't work:

```
$ git apply << "EOF" ...
error: corrupt patch at line 15
```

⚠️ fanquake opened an issue: "build: RFC Coverage build type"
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.

It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.

There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...