💬 Specter2100 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841852343)
Concept NACK. But if the proposal is changed to allow "choosing" a size between 0 and 10,000 bytes instead of "removing" the feature, then I would ACK.
Rather than "removing" the individual choice that has existed until now, wouldn’t it be better to keep it as a "choice" like it is currently?

💬 laanwj commented on issue "Stop shipping ARM32 builds for releases?":
(https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-2841854963)
> IIRC, Raspberry Pi OS only started shipping 64-bit OSes a couple of years ago. For a while, you could buy a Pi 3 or 4 which had 64-bit hardware but the available Raspberry Pi OS image was only 32-bit. I would imagine that several of these machines are probably still in use. It also seems like the 32-bit images are still prominently shown on their [website](https://www.raspberrypi.com/software/operating-systems/) so I think it may be a bit premature to stop shipping 32-bit?

Agree, there's no h
...

📝 ryanofsky opened a pull request: "[DRAFT] ipc: add windows support"
(https://github.com/bitcoin/bitcoin/pull/32387)
This PR is a draft. I wrote most of the code that should be needed to support windows, but am still debugging various issues.

This is meant to resolve https://github.com/bitcoin-core/libmultiprocess/issues/53 and https://github.com/bitcoin-core/libmultiprocess/issues/114

💬 ryanofsky commented on pull request "[DRAFT] ipc: add windows support":
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2841866036)
Updated 9df5a838aa9c020adf8d024393749d75bd932ec2 -> 87432b6a4325e09a13c912d77b386daa3832b34d ([`pr/ipc-win.1`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.1) -> [`pr/ipc-win.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.1..pr/ipc-win.2)) fixing accidentally disabled tests

💬 NukeThemAII commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841867764)
Time to move to Kaspa.org $KAS

💬 smbpunt commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841868177)
Concept NACK

Removing the OP_RETURN size limit risks excessive blockchain growth, increasing storage and bandwidth costs for node operators. This could reduce the number of volunteer nodes, threatening Bitcoin's decentralization—a core element that must be taken very seriously. I see no use-case justifying this trade-off.

Additionally, marking some comments as duplicates and hiding them is concerning. Some comments added valuable perspective and weren’t copy/paste. Suppressing discussion o
...

💬 Sjors commented on pull request "mining: rename gbt_force and gbt_force_name":
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2841871642)
@TheCharlatan converted to a scripted diff plus a documentation commit.

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2068601939)
I ran the following on Linux for `a1fe87eb668e8a1eeeef335951547882b5fb52d8`:

```bash
cmake --preset=libfuzzer && cmake --build build_fuzz && \
FUZZ=coin_grinder build_fuzz/bin/fuzz -runs=500000 && \
FUZZ=coin_grinder_is_optimal build_fuzz/bin/fuzz -runs=500000 && \
FUZZ=coincontrol build_fuzz/bin/fuzz -runs=500000 && \
FUZZ=coins_deserialize build_fuzz/bin/fuzz -runs=500000 && \
FUZZ=coins_view build_fuzz/bin/fuzz -runs=500000 && \
FUZZ=coinscache_sim build_fuzz/bin/fuzz -r
...

👍 laanwj approved a pull request: "util: Remove `fsbridge::get_filesystem_error_message()`"
(https://github.com/bitcoin/bitcoin/pull/32383#pullrequestreview-2806981828)
Code review ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

This was sure some horrible code, i'm confused what it even does (calling `MultiByteToWideChar` *twice* then do another pass of `std::codecvt_utf8_utf16` seems way overkill). Glad to replace it with a standardized method.

💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-2841921930)
Rebased after #29039.

👍 rkrux approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2807002075)
tACK b6101b1849fbc2ec510c56cfae39d7de244a50cc

I found this pull more involved because of multiple moving pieces and I _think_ I have covered all the areas that I could. Hence, sharing below my own notes of the review that hopefully would be helpful for anyone else reviewing the PR (& also my future self).

I see the main changes are along 3 lines:
1. Addition of atypical (non default/all) sighash type in the PSBT (Input).
2. Checking of the sighash type in the PSBT against the one provide
...

💬 davidgumberg commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068639507)
non blocking nit, I think we can use `e.what()` in place of `e.code().message()`

💬 laanwj commented on pull request "[PoC] Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2068645615)
Is this necessary with the manifest?

Also we already have
```
SetConsoleCP(CP_UTF8);
SetConsoleOutputCP(CP_UTF8);
```
in `void SetupEnvironment()`.

💬 davidgumberg commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#issuecomment-2841953785)
untested crACK https://github.com/bitcoin/bitcoin/pull/32383/commits/97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

This code made sense when using boost, but I think `std::filesystem` implementations are responsible for and better equipped to handle platform specific character width issues.

💬 maflcko commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068662162)
> non blocking nit, I think we can use `e.what()` in place of `e.code().message()`

This will redundantly include the filenames again, or it may not, so it seems worse?

👍 laanwj approved a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2807061907)
Code review ACK 7e80030a952a06101d5755032ebb1ff15823815e

i have not compared this to upstream but the change in themselves LGTM.

💬 maflcko commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2068672564)
> Am I missing anything important here?

Yes, the preset doesn't check for those. You'll have to run with the integer sanitizer to be able to detect integer sanitizer issues.

The testing steps are also explained in the comment: https://github.com/bitcoin/bitcoin/pull/28865#issue-1991140953

💬 hebasto commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068672604)
UTF-8 paths included in `e.what()` are not displayed correctly on Windows.

💬 francisco-alonso commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842014836)
This PR must not be merged. I seriously question whether the fact that we can even entertain proposals like this means Bitcoin is truly decentralized. BTC was designed as a peer-to-peer transaction system and it has nothing to do with embedding garbage into transactions.

Core concepts must be untouchable; from that foundation, any proposal is welcome.

💬 LaurentMT commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842018949)
@Sjors
Sorry but it seems that we're talking past each other. Are you telling me that 1 byte of data stored in the witness is weighted like 1 byte of data stored in an op_return output? If it's not the case then I don't see how we can disagree that 1 byte of data stored in an op_return output is more damaging to the throughput of the system than 1 byte of data stored in the witness. The fact that the user pays more fees to the miner doesn't change that.