💬 glozow commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2381649791)
reACK 940edd6 via range-diff

💬 mzumsande commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2382050086)
> It also seems like the cache resizing has not worked correctly?

Just wanted to mention that I think that the cache resize worked as expected here:
After the snaphot is loaded, the cache of the background chainstate is set to a lower value:
`resized coinstip cache to 22.0 MiB`
because loading from the snapshot chainstate is prioritized.

Then, the cache for the background chainstate is frequently flushed because blocks are downloaded to it and it runs full quickly, resulting in a flush.
...

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2382159543)
Just as a note, it should already be possible to pick the device via an env var, see https://en.cppreference.com/w/cpp/filesystem/temp_directory_path#Notes.

🤔 maflcko reviewed a pull request: "Cover remaining tinyformat usages in CheckFormatSpecifiers"
(https://github.com/bitcoin/bitcoin/pull/30999#pullrequestreview-2336508940)
d49dfafa9aea349ae6741f49ed992b78a88b6839 looks correct, but I am not sure the others.

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780515759)
nit: not sure about changing this back again to accommodate test-only code. Instead of using `-1` for test code, you could set `constexpr INVALID=-1;` and use that.

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780527702)
Why remove this comment? `format("'%1$*3$s %2$-*3$s'", "hi", "w", 12)` is still unsupported and parsed incorrectly at compile time.

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780512926)
in cba51f2e380f0f89f799369b489c2e25e7215221: Not sure about turning the passing one into a runtime check from a compile-time check. Previously it was trivial to compile a single unit (this test) to sanity check the parser, as well as check the compile failures for various errors, by simply looking at the compile output. Also, the code was as close as possible to the real code, serving as documentation of how to use it.

Now, checking the passing cases requires not only compiling, but also link
...

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382352367)
macOS Monterey 12 hardware support: https://support.apple.com/en-us/103260
macOS Big Sur 11 hardware support: https://support.apple.com/en-us/111980

Those differ by about a year. By time this is released, Monterey will work on ten year old machines. So I think it's fine to drop Big Sur (for Qt).

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382371880)
The bump for Ubuntu seems fine as well. There's already the requirement from https://github.com/bitcoin/bitcoin/pull/29987 for all binaries, so going one LTS further for the GUI seems fine.

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382378949)
> Don't forget to update `build-osx.md`.

I don't think the gui requirements are documented anywhere, because for now they were always equal to the daemon requirements? Possibly just saying globally "qt6" is required implies the additional requirement on the OS?

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382414730)
@maflcko I think it's just two changes:

* `brew install qt@5` -> `qt` or `qt@6`
* "For macOS 11 (Big Sur) and 12 (Monterey)": IIUC this doesn't change, except that QT won't work with 11. Not sure how to best document that.

Loading a descriptor wallet with the GUI resulted in an error popup:

> Error: SQLiteDatabase: Failed to configure serialized threading mode: bad parameter or other API misuse

The log also says:

> 2024-09-30T08:08:04Z [qt-init] SQLite Error. Code: 21. Message:
...

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1780645218)
Ok, done

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1780645609)
Sure, done

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1780646045)
Sure, done

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382485661)
Building depends fails for me, maybe something wrong with `qttools_skip_dependencies.patch`?

```
CMake Error at qtbase/cmake/QtTargetHelpers.cmake:171 (target_link_libraries):
Target "linguist" links to:

Qt::PrintSupport

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.

Call Stack (most recent call first):
qtbase/cmake/QtExe
...

👍 EricMujjona approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2336815645)
Efficient code

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382540830)
@Sjors
> Building depends fails for me, maybe something wrong with `qttools_skip_dependencies.patch`?

What's your build platform and the value of the `HOST` variable?

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1780728018)
I wonder if there is any value in premature optimization. I agree the above suggested patch is correct and required. However, just taking the condition variable under the same mutex and keeping the lock while notifying seems easier to enforce with clang thread safety annotations than manual review. The only other place that notifies is already taking the lock on *every* block-connect to `notify_all`, so doing it here *once* more during shutdown shouldn't be noticeable either way.

💬 dergoegge commented on pull request "fuzz: fix bug in p2p_headers_presync harness":
(https://github.com/bitcoin/bitcoin/pull/30980#issuecomment-2382599033)
> According to https://oss-fuzz.com/fuzzer-stats?group_by=by-day&fuzzer=afl_bitcoin-core_p2p_headers_presync&project=bitcoin-core the exec/s is 0 for afl.

This is because oss-fuzz doesn't use our afl++ harness but rather the [afl libfuzzer driver](https://github.com/AFLplusplus/AFLplusplus/tree/stable/utils/aflpp_driver). When using this driver, afl++ will fork after the initialization code is run (https://github.com/AFLplusplus/AFLplusplus/blob/d21fb1a558b25c4f46692fa999c0028dfe0eecc0/utils/
...

💬 dergoegge commented on pull request "fuzz: fix bug in p2p_headers_presync harness":
(https://github.com/bitcoin/bitcoin/pull/30980#discussion_r1780745018)
nit: You could get rid of `total_work` as a variable and just do:

```suggestion
assert(CalculateClaimedHeadersWork(all_headers) < chainman.MinimumChainWork());
```