💬 achow101 commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#issuecomment-2452600410)
ACK d7fd766feb2f579bdba0e778bacdeb13103e8282

✅ achow101 closed an issue: "Gracefully handle dropped UPnP support "
(https://github.com/bitcoin-core/gui/issues/843)

🚀 achow101 merged a pull request: "init: warn, don't error, when '-upnp' is set"
(https://github.com/bitcoin/bitcoin/pull/31198)

🚀 achow101 merged a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139)

🤔 brunoerg reviewed a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2410910457)
crACK 87532fe55856efc063cf81244800da37a015ba75

code looks great, but I didn't test in practice yet.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826323008)
In this case, a new state is indeed created in `HasNonStandardInput`, but either RVO or an implicit move is performed not a copy?

I think, in general, returning a value is preferred over using out parameters?
same was also recommended here https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c-functions-and-methods

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2452643916)
> Also, the witness stack array inside TxToUniv is a VARR that could be reserved with a parameter resolved at runtime.

Thanks, I've added this in the latest push.

🤔 hodlinator reviewed a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2410955172)
Concept ACK c9baf41372b06e08976a3daf459468d3f6ad28ea


Tested with and without PR changes:
```
> cmake -B build --preset vs2022-static
```
Confirmed that QR image is not shown when displaying receive address without the PR. 👍

Only found a download path for the QREncode lib in *depends/packages/qrencode.mk*, **could not spot** how the new *cmake/module/FindQRencode.cmake*
...

💬 davidgumberg commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2452673979)
> * Everything in an `if constexpr` is still looked at by the compiler and any code issues should be detected.

Didn't think of this, that makes sense to me. I also may have misunderstood or misrepresented the use-case/concern I tried to describe, but it's not my own and I don't have any view on it, so it seems silly that I even mentioned it.

I still ACK https://github.com/bitcoin/bitcoin/pull/31191/commits/fafbf8acf419d5e2ca307e5804099361ca7471af for fixing the regression measured in #311
...

💬 darosior commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2452697413)
> One thing i wonder though: do we have to explicitly remove the setting from settings.json after this warning, to prevent it happening every startup? Warning the user once is enough.

I agree we should still fix the GUI. However i'm not sure how best to achieve that. Also if we do it it would be nice to make it extensible so next time we remove a startup option we don't have to go through this again.

One hack would be to make the `ArgsManager` passed to `AppInitParameterInteraction` non-`c
...

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452765858)
> it is indeed missing

Oh interesting! Well, I guess not in this important for this PR, but perhaps it should be explicit. Especially so people don't get those evil red lines

💬 laanwj commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2452911222)
> Another way could be to never persist hidden args in the settings, and we always keep startup options we remove as hidden args for a couple versions?

Agree that a consistent way would be better.

i'm not sure about re-using hidden. Could we maybe have a "REMOVED" flag on arguments specifically for this "don't complain on load, but don't persist" behavior? Could keep this around for a long time after removal, potentially, even after removing the warning code, to make sure users never run i
...

💬 hebasto commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2452927225)
> Only found a download path for the QREncode lib in _depends/packages/qrencode.mk_, **could not spot** how the new _cmake/module/FindQRencode.cmake_ script hooks into Depends after the PkgConfig check fails? 🤔

Depends are not involved when building on Windows.

The [toolchain file provided by `vcpkg`](https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/CMakePresets.json#L32) sets the [`CMAKE_FIND_ROOT_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_F
...

⚠️ maflcko reopened an issue: "Gracefully handle dropped UPnP support "
(https://github.com/bitcoin-core/gui/issues/843)
https://github.com/bitcoin/bitcoin/pull/31130 dropped UPnP support. It's now recommended that
users use PCP (with NATPMP fallback).

But this is not explained in a friendly manner:

<img width="496" alt="error" src="https://github.com/user-attachments/assets/d35891ac-82e0-4f61-8e9b-671aaa1d0d75">

A user would have to figure out that they need to manually edit settings.json or delete it and redo all their settings.

We should probably automatically delete it from settings.json. And the
...

💬 maflcko commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2452961465)
I don't think this was fixed fully for the gui

👍 i-am-yuvi approved a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2411278339)
Tested ACK ad5c012157c5f261503022cfa22d7124bfda5765

> Add a method `CNetMessage::GetMemoryUsage` and use this for accounting of the size of the process receive queue instead of the raw message size (like we already do for the send buffer and `CSerializedNetMsg`).
>
> This ensures that allocation and deserialization overhead is better taken into account.
>
> On average, this counts about ~100 extra bytes per packet on x86_64:
>
> ```
> 2024-10-27T09:50:12Z [net] 24 bytes -> 112 bytes
...

✅ TheBlueMatt closed an issue: "Support validating a PoW-free block over over RPC"
(https://github.com/bitcoin/bitcoin/issues/31136)

💬 TheBlueMatt commented on issue "Support validating a PoW-free block over over RPC":
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055656)
Ah, thanks. I always forget about `getblocktemplate`'s options (and the BIP isn't always useful as IIRC only part of it was ever implemented).

💬 TheBlueMatt commented on issue "Support validating a PoW-free block over over RPC":
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947)
It'd be nice to have this in the mining protocol.

💬 tdb3 commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-2453085686)
> > > Does bitcoind consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.
>
> I've added a release notes for this

The release note helps a lot. To err on the side of caution, it seems appropriate to include a `deprecatedrpc=` option, to enable a period of deprecation for users.