💬 Sjors commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r2068493498)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716: I find this new function name confusing, adding to the already confusingly named `gbt_force`. Opened #32386 to rename them.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r2068493498)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716: I find this new function name confusing, adding to the already confusingly named `gbt_force`. Opened #32386 to rename them.
🤔 Sjors reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2806530048)
Post merge ACK (did not study the test and fuzz changes). Agree this was a good time to merge.
The changes in 1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 which move logic from RPC to `versionbits.cpp` should make it easier to implement version bit signalling support in Stratum v2 (IPC). And reminded me that I actually need to look into that.
3bd32c20550e69688a4ff02409fb34b9a637b9c4 would have been easier to follow if had been split between one commit that moves and another that modified it (a
...
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2806530048)
Post merge ACK (did not study the test and fuzz changes). Agree this was a good time to merge.
The changes in 1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 which move logic from RPC to `versionbits.cpp` should make it easier to implement version bit signalling support in Stratum v2 (IPC). And reminded me that I actually need to look into that.
3bd32c20550e69688a4ff02409fb34b9a637b9c4 would have been easier to follow if had been split between one commit that moves and another that modified it (a
...
💬 Sjors commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r2068350712)
a679040ec19ef17f3f03988a52207f1c03af701e: I find this intermediate state a bit confusing, but in the final result it looks fine to me: a default value for the `period` instance variable combined with a constructor that overrides it only on test networks
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r2068350712)
a679040ec19ef17f3f03988a52207f1c03af701e: I find this intermediate state a bit confusing, but in the final result it looks fine to me: a default value for the `period` instance variable combined with a constructor that overrides it only on test networks
💬 Kakar21 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841830441)
Concept NACK:
Dropping the 83-byte datacarrier limit makes cheap data-dumping trivial. Even ~1 MB extra OP_RETURN per block means about 52 GB extra chain growth per year, which every node operator has to store. Higher disk and bandwidth costs reduce the number of volunteer nodes and hurt decentralisation. I don’t see a use-case that justifies that trade-off.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841830441)
Concept NACK:
Dropping the 83-byte datacarrier limit makes cheap data-dumping trivial. Even ~1 MB extra OP_RETURN per block means about 52 GB extra chain growth per year, which every node operator has to store. Higher disk and bandwidth costs reduce the number of volunteer nodes and hurt decentralisation. I don’t see a use-case that justifies that trade-off.
💬 TheCharlatan commented on pull request "mining: rename gbt_force and gbt_force_name":
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2841834928)
Can this be a scripted diff?
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2841834928)
Can this be a scripted diff?
💬 ake-khada commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841838411)
Concept NACK.
Wasn't folks running their own nodes the prerogative since like all the time? WTF is this other than direct attack on that premise?
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841838411)
Concept NACK.
Wasn't folks running their own nodes the prerogative since like all the time? WTF is this other than direct attack on that premise?
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841842580)
@LaurentMT
> A given quantity of data stored in a op_return output has a far larger negative externality on the throughput than the same data stored in a witness.
This makes absolutely no sense, I think you're confused about how either OP_RETURN or inscriptions work. A byte is a byte no matter where it's stored in the block.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841842580)
@LaurentMT
> A given quantity of data stored in a op_return output has a far larger negative externality on the throughput than the same data stored in a witness.
This makes absolutely no sense, I think you're confused about how either OP_RETURN or inscriptions work. A byte is a byte no matter where it's stored in the block.
💬 maflcko commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2841846681)
Yeah, it reminds me of https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2284894942, which was a similar lifetime issue. `ConnectionTypeAsString` should be the only call that creates a new string object in this tracepoint here.
I wonder why valgrind only traps and doesn't print more information about the kind of error.
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2841846681)
Yeah, it reminds me of https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2284894942, which was a similar lifetime issue. `ConnectionTypeAsString` should be the only call that creates a new string object in this tracepoint here.
I wonder why valgrind only traps and doesn't print more information about the kind of error.
💬 rebroad commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-2841848904)
There is a bug in the shutdown sequence (null model), so this is not ready to merge yet.
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-2841848904)
There is a bug in the shutdown sequence (null model), so this is not ready to merge yet.
💬 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?
(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
...
(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
(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
(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
(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
...
(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.
(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
...
(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.
(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.
(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
...
(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
...