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

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2453089942)
ACK 51c2394eb8158113e73659f4ae65c237813bd5d2

🤔 tdb3 reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2411707754)
Approach ACK

The update suggested in https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1810430953 seems to increase clarity.

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2453108430)
Thanks for the hints @maflcko, I was under the impression that big-endian tests were run automatically.

### Fix

It seem that that `std::rotr` doesn't take endianness into account, so the fix basically looks like:
```C++
size_t key_rotation = 8 * key_offset;
if constexpr (std::endian::native == std::endian::big) key_rotation *= -1;
return std::rotr(key, key_rotation);
```
I've emulated the s390x behavior locally like this:

<details>
<summary>Details</summary>

```bash
brew inst
...

👍 tdb3 approved a pull request: "test: Fix intermittent issue in wallet_backwards_compatibility.py"
(https://github.com/bitcoin/bitcoin/pull/29982#pullrequestreview-2411713750)
ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4

With a similar delay (150ms) induced with https://github.com/Randy808/bitcoin/commit/f39144d808975c016c1a94022a2c4251950afb7d,
Saw test failure without the `sync_mempools()` line added in this PR. After adding the line, saw consistent passing (20x runs).

🤔 hodlinator reviewed a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2411714494)
Thanks for the review @tdb3!

Implemented feedback from that review, made the code a bit more Pythonic and improved comments slightly.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774806)
Tried using `update()`, but it resulted in things like:
```
AssertionError: [node 0] Unable to connect to bitcoind after 10s (ignored errors: {'m': 40, 'i': 120, 's': 120, 'n': 80, 'g': 40, '_': 40, 'c': 40, 'r': 40, 'e': 80, 'd': 40, 't': 40, 'a': 40, 'l': 40}, latest error: ValueError('No RPC credentials'))
```
i.e. it was iterating over the string and incrementing an entry for each unique character, which seems to match the [docs](https://docs.python.org/3/library/collections.html#collect
...

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826772195)
(Same issue with `update()`).

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774077)
Had to add a commit (2a0c0e410e9f32dbf7af7229457889b09553c438) that refurbishes `assert_raises_message()`, but test is much cleaner now!