💬 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:

...

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2382647693)
Force pushed some trivial style-only test-only fixups.

💬 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_r1780781699)
I don't think this is an improvement, I find it less clear about the distinction between the compile-time format string and the run-time string parameters, so I'm gong to leave this as is for now.

💬 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_r1780784357)
That seems unrelated to this PR though?

💬 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_r1780791324)
Looks like this is already covered in #30999, resolving this.

💬 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_r1780792138)
Resolving since it has its own PR now.

💬 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_r1780793262)
I suppose we don't, but that seems unrelated to this PR so going to leave that as is.

💬 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_r1780796251)
Addressed in #30999, resolving.