💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814887974)
No I think this one is just "belt" -- if you look at lines 146-148 above, we're not always modifying the "child" transaction here, so sometimes it'll be a duplicate of something we already have.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814895952)
Yep you're right. I swapped the order of those two commits to avoid this problem, and I also added a test for package rbf with priority (which fails on https://github.com/bitcoin/bitcoin/commit/1c33e133db5f876f2ba6eeb28877a5dcf3f6ac04, but passes by the end of this branch).

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2435184924)
Could remove "To run release binaries on macOS, [Monterey 12](https://support.apple.com/en-us/103260) or later is required." from the pull description, now that macOS 13 is required and this is no longer relevant?

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2435190097)
> Could remove "To run release binaries on macOS, [Monterey 12](https://support.apple.com/en-us/103260) or later is required." from the pull description, now that macOS 13 is required and this is no longer relevant?

Removed.

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

🚀 fanquake merged a pull request: "build: replace custom `MAC_OSX` macro with existing `__APPLE__`"
(https://github.com/bitcoin/bitcoin/pull/29450)

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