💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2435213571)
> Add the [#31042 (comment)](https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428995399) requirement at the same time?

Rebased on https://github.com/bitcoin/bitcoin/pull/31042.

Please review https://github.com/bitcoin/bitcoin/pull/31042 and https://github.com/bitcoin/bitcoin/pull/31147 first.

💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2435214195)
> > It is definitely broken in #30997.
>
> Shouldn't #30997 be based on this PR then (or at least mention in the PR description it's expected to be broken)?

[Done](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2435213571).

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2435222694)
@fanquake pushed

✅ maflcko closed an issue: "CI failure: `Error: Directory '/tmp/ccache_dir' must be created in advance.`"
(https://github.com/bitcoin/bitcoin/issues/31108)

💬 maflcko commented on issue "CI failure: `Error: Directory '/tmp/ccache_dir' must be created in advance.`":
(https://github.com/bitcoin/bitcoin/issues/31108#issuecomment-2435224696)
Done in https://github.com/bitcoin/bitcoin/pull/31146. Let's continue the discussion there.

💬 fanquake commented on pull request "cmake, qt, test: Remove problematic code":
(https://github.com/bitcoin/bitcoin/pull/31147#issuecomment-2435227027)
> As a side effect, this PR fixes https://github.com/bitcoin-core/gui/issues/842.

Tested that this fixes building on Tumbleweed. `-DWITH_QRENCODE=OFF` is required, but that seems to be another issue, probably with the `libqrencode` package on that distro.

I take it this code has been dead/broken since it was introduced? 975d67369b8f3a33a21fd7618c299c0ec138292c

💬 hebasto commented on pull request "cmake, qt, test: Remove problematic code":
(https://github.com/bitcoin/bitcoin/pull/31147#issuecomment-2435232893)
> I take it this code has been dead/broken since it was introduced? [975d673](https://github.com/bitcoin/bitcoin/commit/975d67369b8f3a33a21fd7618c299c0ec138292c)

Correct.

👍 fanquake approved a pull request: "cmake, qt, test: Remove problematic code"
(https://github.com/bitcoin/bitcoin/pull/31147#pullrequestreview-2392428144)
ACK fb46d57d4e7263495c007f4a499a349bff2b21e0

💬 laanwj commented on pull request "run_command: Close non-std fds when execing slave processes":
(https://github.com/bitcoin/bitcoin/pull/30756#issuecomment-2435244592)
Concept ACK. Closing unnecessary fds before starting external commands is good practice.

> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it

Pretty sure i brought up this [exact concern](https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2054078250) prior to the cpp-subprocess cleanup, but was assured it wasn't removed. Appa
...

💬 hebasto commented on issue "How to compile the GUI on opensuse tumbleweed with cmake?":
(https://github.com/bitcoin-core/gui/issues/842#issuecomment-2435264454)
> Even though the package is available, looks like it didn't work there either.

The `qrencode-devel` package should be installed.

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2435278993)
The main issue this PR addresses is that currently, no man pages are generated if a single binary is missing. The goal is to skip the missing binary and generate man pages for the others, as was done in the previous shell script.

If I understand correctly, you want to ensure no information is lost in the man pages, right? In that case, I can simply remove the following line:
```py
42 BINARIES.remove(relpath)
```
This way, the existing binaries will have their man pages generated,
...

👍 stickies-v approved a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2385195309)
ACK 303661871debadee5f67bd7e4cd0cccc85344ef2

I think reducing the special (and imo unexpected) treatment of packages of transactions without unconfirmed parents by this PR is an improvement. I looked at the call hierarchy of `IsChildWithParents()`, and the changes look safe to me, but I am not super familiar with this part of the code.

💬 stickies-v commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1810721390)
Orthogonal to the changes here, but related and I think it would be good to add this assert + doc to `IsChildWithParentsTree()` too, if you force-push.

<details>
<summary>git diff on 92ed1bd586</summary>

```diff
diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp
index 38f26a9c4b..e34f94cbb1 100644
--- a/src/policy/packages.cpp
+++ b/src/policy/packages.cpp
@@ -138,6 +138,7 @@ bool IsChildWithParentsTree(const Package& package)
{
if (!IsChildWithParents(package)) r
...

💬 stickies-v commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1810732642)
I think I agree with instagibbs that adding complexity on top should be avoided if we can, and it seems to me like modifying `IsChildWithParents()` to better align with the `Package` definition (i.e. child and all unconfirmed parents, as per `AcceptPackage()`) is preferable for long-term robustness, over special-casing a transaction that has 0 unconfirmed parents as is currently the case.

I looked at the callstacks that make use of `IsChildWithParents()`, and since the scope is fairly limited
...

💬 laanwj commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2435308803)
Kind of. The problem is that the command will still finish successfully in case of an incomplete build (or build configuration), while the before a release it's critical to update all the pages.

This is why i suggested to add this as an option eg `--skip-missing-binaries`.

> This way, the existing binaries will have their man pages generated, and all the other man pages will still be referenced at the bottom, even if the binaries are missing.

That is an improvement, but doesn't entirely
...

💬 Sjors commented on pull request "miner: Reorg Testnet4 minimum difficulty blocks":
(https://github.com/bitcoin/bitcoin/pull/31117#issuecomment-2435316425)
cc @Emzy

💬 1440000bytes commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2435336204)
> Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed here), some of which directly affected our software (RCE in 2015, OOM in 2020).

and CVE-2017-1000494 in 2018?

Concept ACK

Lint error: https://github.com/bitcoin/bitcoin/pull/31130/checks?check_run_id=31973806317

💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2435337573)
`/Users/sjors/dev` is a symlink to `/Volumes/SSD/Dev/`. I'll try a build without using a symlink.

💬 dergoegge commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2435345680)
Looks like switching from Assume to assert in BitSet has a noticeable impact on the `Linearize*` bench tests: https://corecheck.dev/bitcoin/bitcoin/pulls/31093

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1815036443)
maybe
"If you want to open a port automatically, consider using the `-natpmp` option instead, which uses PCP or NAT-PMP depending on router support."
(in the GUI, the option is still called NATPMP as well, so no need to mention that seperately i think)