💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452535019)
> @jasonribble `template<std::integral I>` is the same as `template<typename I>`, with the added restriction that `I` must be a type that satisfies the `std::integral` concept. Since `int` does satisfy that concept, `I` can be instantiated as `int`.

Magical. Thank you 🙏

💬 TheCharlatan commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452537469)
> But, I don't see #include <concepts>.

If you check the IWYU logs in the "tidy" CI job you will see that it is indeed missing:
```
[19:28:01.280] /ci_container_base/src/policy/feerate.h should add these lines:
[19:28:01.280] #include <concepts> // for integral
```
This report is generated on each CI run, but the missing includes are not enforced. This can be confusing, so best to always check the CI output.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826266792)
Not too sure about the `return removed == 1`

The assume is there to make sure that if the code happens to be wrong, it is catched on testing. In the current approach, if the code moving data from `delayed_set` to `m_local_set` was broken somehow, this method will return that data was actually removed. If were to return `removed == 1` in the aforementioned case, it would return false, and the caller's logic may break.

I don't think any of these cases is likely to happen given the assume wil
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826275867)
I just realized this is also the case for the ancestors. Went relaying with ancestors, we should also check that the ancestor we are trying to relay is part of the reconciliation set (i.e. that it has not been previously flooded)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826278558)
This is only called if there are in mempol parents though, the ordering doesn't matter much otherwise

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2452563866)
Rebased to make CI green, plus covered (and masked as resolved) some outstanding comments:

https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818553463
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818555798
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818577080
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818641775
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818703437
https://github.com/bitcoin/bitcoin/pull/30116#discussion
...

💬 instagibbs commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2452572531)
my guess is from #10146 this commit ada0caa165905b50db351a56ec124518c922085a looks like a covert fix for `submitblock` with a completely empty block. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L1041 shows that it will call:

`UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex` which would result in UB, before any PoW checks are made.

💬 achow101 commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2452594931)
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0

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