💬 janb84 commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2683205888)
Tested ACK [073a017](https://github.com/bitcoin/bitcoin/pull/31943/commits/073a017016e62c7d52562aa61d942024379c110d)

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2683212787)
Trying to build this locally on Apple Silicon macOS fails:

```
cmake -B build -DENABLE_IPC=ON
...
CMake Error at src/ipc/CMakeLists.txt:14 (add_subdirectory):
add_subdirectory given source "libmultiprocess" which is not an existing
directory.


CMake Error at src/ipc/CMakeLists.txt:16 (target_link_libraries):
Cannot specify link libraries for target "multiprocess" which is not built
by this project.
```

Same error as on CI in #30975 https://github.com/bitcoin/bitcoin/a
...

💬 murchandamus commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2683229560)
> I find the use of "selection" twice in the sentence doesn't add much clarity. Now with the added assertions, I feel like the tittle fits the test however I'm open to a better title.
>

Fair enough, maybe use "Test that a solution is found when some combinations would exceed weight"

💬 l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#issuecomment-2683231437)
@andrewtoth, can we save the first few `memusage:` related commits from here at least?
Do we need any change to them or can we just extract to a new PR?

💬 maflcko commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2683242368)
lgtm ACK 073a017016e62c7d52562aa61d942024379c110d

📝 m3dwards opened a pull request: "doc: update fuzz instructions when on macOS"
(https://github.com/bitcoin/bitcoin/pull/31954)
Fixes: #31049

Updates the instructions for fuzzing on macOS to use `lld` instead of `ld`.

Tested instructions on M1 Mac running 14.6.1

💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2683288260)
Hopefully most GUI users at this point have the setting enabled without editing `bitcoin.conf`.

> It may be too risky to try to fix it here in this PR.

Probably yes. Though the following would add it to the list of overridden options:

```diff
diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
index 5782a57a26..54ea8f9ee4 100644
--- a/src/qt/optionsmodel.cpp
+++ b/src/qt/optionsmodel.cpp
@@ -229,6 +229,13 @@ bool OptionsModel::Init(bilingual_str& error)
}

...

💬 dergoegge commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2683350393)
Limited the stack size limit to the GHA macos jobs (as per https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2653979844)

📝 maflcko opened a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955)
In Python the meaning of `args or argsn` is that `argsn` is fully ignored when `args` is a list with at least one element. However, the RPC server accepts mixed positional and named args in the same RPC.

Fix the debug log by always printing both.

Can be tested via `--tracerpc` on a call that uses named args mixed with positional args.

💬 darosior commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2683385857)
Concept ACK. WIll test soon.

💬 hanis12345 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/758a93d6215c2fa4799741d721e610a8a7214c34#r153001421)
[ ]

> [](url)

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2683474021)
@murchandamus
> 3) Test that a solution is found when some combinations would exceed the allowed weight.

That is not inaccurate and better, although coin-grinder does more than that.. I see 7 solutions that are less than max allowed weight `10,000`, however the solution returned by coin-grinder is of weight `7,344`. Another solution 9×2 ₿ and 17×0.33 ₿ is of weight `7616` which is _not_ returned although still less than the allowed weight.

I think this would be a stronger statement:

...

💬 jonatack commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2683479174)
Concept ACK

🤔 hodlinator reviewed a pull request: "init: Handle dropped UPnP support more gracefully"
(https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2641238999)
Concept ACK 64de0a22dbd2eb72ff96d6dddb59abb2e22938b2

#### Concept

Good to help users migrate automatically, hopefully *decreasing the number of nodes that drop off the network* due to UPnP-support being dropped in favor of more modern port forwarding protocols.

#### PR description / commit message

I think the prevention of nodes dropping off the network-aspect could be called out.

> If this solution is acceptable i will add a functional test.

This should be implemented or dropp
...

💬 hodlinator commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1969969186)
Noticed there is some parameter interaction soft setting `-natpmp` to `false` when using a `-proxy` or !`-listen`. But the soft set here will not be allowed to re-enable it. :+1:

💬 hodlinator commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1970653732)
Not sure if this comment is needed, the log message inside of the code block is enough IMO. Suggestions if kept:
```suggestion
// We dropped UPnP support but kept the arg as hidden for now to display a friendlier error to user who has the
```

💬 hodlinator commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1969880047)
Could disambiguate between `false`/`nullptr` both for review and the compilers type check:
```suggestion
if (common::FindKey(settings.rw_settings, "natpmp") == nullptr) {
```
<details><summary>(...)</summary>

I know this is not performance critical, but could avoid duplicate lookups:
```C++
auto [_, inserted] = settings.rw_settings.try_emplace("natpmp", *upnp);
if (inserted) {
LogWarning(R"(Added "natpmp": %s to settings.json to replace obsolete "upnp" setting)", upnp-
...

💬 luke-jr commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2683554828)
Concept NACK, just makes the code less straightforward and forces re-calculation later if needed. Harmless to leave it, and make use (eg, in #31283 and other feature I haven't PR'd yet)

💬 luke-jr commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1970725031)
Probably best not to have this be a positional parameter.

💬 yancyribbens commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1970739081)
The comment ` // j cannot be 0 here;` implies this should be `j != 0` instead of `j > 0`. Since it's unsigned `j != 0` is also correct.