💬 polespinasa commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2879033872)
> Perhaps we can make the second argument of `generateblock` optional, and if there is no set of txs provided then mine the mempool

That could be a smart approach. But I would always take the mempool or also put an optional argument to mine the mempool so it is not one or the other.

> I don't think we should be duplicating the effort implementing this on more than one RPC. Then every new mining test feature needs to be duplicated (or triplicated for `generatetodescriptor`/`generatetomany`)
...

📝 maflcko opened a pull request: "refactor: Remove UB in prevector reverse iterators"
(https://github.com/bitcoin/bitcoin/pull/32490)
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:

When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.

... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j
...

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2879192945)
I still found the comment a bit confusing: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086713176

Maybe it's better to also move the `GetTip()` implementation to the miner, so we call it from the WaitTipChanged implementation. See https://github.com/Sjors/bitcoin/commits/2025/05/interface/

If you then squash the last commit into 51344920ebe1772d8dd3bece789a6510d774ab59, the result is a smaller change to the `waitTipChanged` body, see 2dae1cb16e3e32aa477713b88adcfbb6251cc779.
...

👍 maflcko approved a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2839190115)
nice, lgtm

💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088333853)
style nit: Could adjust the indent manually, while touching this, so that the `ser_writedata*` are aligned? Also, personally I prefer to use `uint8_t(a)` over the verbose static cast for integral types. See https://github.com/bitcoin/bitcoin/blob/f9d8910539a2f7c4542457e08cd2bdd2dcb9cd08/doc/developer-notes.md#L111

But just style nits, up to you.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088367575)
Good point, otherwise it leaves `bitcoin.exe` behind because it's not inside the `daemon` subdirectory.

I'll add this to my Windows test:

```
diff --git a/share/setup.nsi.in b/share/setup.nsi.in
index 270468ad7b..2160cc05a1 100644
--- a/share/setup.nsi.in
+++ b/share/setup.nsi.in
@@ -130,6 +130,7 @@ done${UNSECTION_ID}:

# Uninstaller sections
Section /o -un.Main UNSEC0000
+ Delete /REBOOTOK $INSTDIR\@BITCOIN_WRAPPER_NAME@@EXEEXT@
Delete /REBOOTOK $INSTDIR\@BITCOIN_GUI_
...

💬 delta1 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879319514)
@gmaxwell drahtbot has categorized your comment as nack instead of ack

👍 TheCharlatan approved a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2839318576)
ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088424348)
Works like a charm.

I also briefly tested `bitcoin.exe gui`, `node` and `rpc`.

📝 fanquake opened a pull request: "build: document why we check for `std::system`"
(https://github.com/bitcoin/bitcoin/pull/32491)
It's probably debatable if we support targets like iOS, but for now, document why we are checking for this standard library feature.

Trying to use `std::system` for a `aarch64-darwin-ios` target results in:
```bash
test.cpp:7:10: error: 'system' is unavailable: not available on iOS
7 | std::system("some_command");
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_stdlib.h:203:6: note: 'system' has been explicitly marked unavailable here
203 |
...

✅ fanquake closed an issue: "Mistake in `feature_pruning.py` functional test?"
(https://github.com/bitcoin/bitcoin/issues/32249)

💬 fanquake commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2879402851)
Closed via #32312.

💬 fanquake commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2879403090)
Backported to 29.x in #32292.

💬 fjahr commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2879442591)
re-ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088470479)
Yes, that is it. I don't think this will actually matter in the grand scheme. I think I'll drop it again.

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088486315)
Pushed the compiler issue on a separate branch here: https://github.com/TheCharlatan/bitcoin/actions/runs/15016860100/job/42196724477#step:10:198 .

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088492832)
> edit: I see this is already (partially) suggested and addressed in https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2078426205, I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span

I did implement a concept initially, but did not find it really that helpful, since the two usable template variants are concretely defined anyway.

💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088471060)
Nit: Will it be removed for sure? If there's no clear consensus on removal, maybe is worth deleting the last part of the sentence. I've seen a lot of controversy bc of that the last two days.

🤔 polespinasa reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2839410351)
code review ACK 35bcd8eed3

left one small comment to avoid further discussions...

💬 maflcko commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879517285)
> drahtbot has categorized your comment as nack instead of ack

<details><summary>off-topic reply</summary>

When a comment contains both `ack` and `nack` (in uppercase), the bot just picks one. Obviously, it is off-topic to discuss here, but I think there is a misunderstanding what the goal of the summary comment by the bot is. The goal is *not* to have everyone "register" their "vote". It is simply a best-effort overview to have a clickable link to every reviewer's most recent review comme
...