🤔 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());
```

💬 dergoegge commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2382608414)
How about using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` for these checks? Then we wouldn't have to maintain a patch in the docs.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2382630708)
Force push: auto-squashed the fixup commit.

> In this PR, natpmp=1 controls both PCP and NAT-PMP.

Yes, after initial discussion we decided to add no new command line, or UI options. PCP is regarded a newer version of NATPMP (which is strictly true) that has some extra mapping features as well as IPv6 support.

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1780764321)
I think `BOOST_TEST_CONTEXT` would simplify this whole block quite nicely, but it's out of scope for this PR so I'm going not going to push that here. I'll update l184 with a `strprintf` as per your suggestion.

<details>
<summary>git diff on 2d4f82d590</summary>

```diff
diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
index a6c4a3613d..e70d9aa826 100644
--- a/src/test/txrequest_tests.cpp
+++ b/src/test/txrequest_tests.cpp
@@ -164,7 +164,6 @@ public:

...