π¬ maflcko commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#issuecomment-2841616263)
lgtm re-ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
Only change are some minor format cleanups for the error messages.
(https://github.com/bitcoin/bitcoin/pull/32383#issuecomment-2841616263)
lgtm re-ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
Only change are some minor format cleanups for the error messages.
π¬ stickies-v commented on pull request "refactor: validation: mark CheckBlockIndex as const":
(https://github.com/bitcoin/bitcoin/pull/32364#issuecomment-2841672078)
Force-pushed to deduplicate the `GetAll()` overloads by adding a `GetAllImpl()` templated helper, addressing @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/32364#pullrequestreview-2804523333).
> I wrote this simpler approach here [TheCharlatan@fe13fa0](https://github.com/TheCharlatan/bitcoin/commit/fe13fa082e3d6befa6d5332428584570ead608ab) after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial t
...
(https://github.com/bitcoin/bitcoin/pull/32364#issuecomment-2841672078)
Force-pushed to deduplicate the `GetAll()` overloads by adding a `GetAllImpl()` templated helper, addressing @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/32364#pullrequestreview-2804523333).
> I wrote this simpler approach here [TheCharlatan@fe13fa0](https://github.com/TheCharlatan/bitcoin/commit/fe13fa082e3d6befa6d5332428584570ead608ab) after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial t
...
π¬ snakezhu commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841722467)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841722467)
Concept ACK.
π¬ LaurentMT commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841723105)
@Sjors
> There is no difference in actual resource consumption, block weight is just an accounting trick.
I have to disagree here. The weight of data stored in blocks has a direct impact on the throughput of the system, a property that is as critical for the operations of the system, if not more, than others properties like the size of the UTXO set.
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
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841723105)
@Sjors
> There is no difference in actual resource consumption, block weight is just an accounting trick.
I have to disagree here. The weight of data stored in blocks has a direct impact on the throughput of the system, a property that is as critical for the operations of the system, if not more, than others properties like the size of the UTXO set.
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
...
π¬ l0rinc commented on issue "Stop shipping ARM32 builds for releases?":
(https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-2841724415)
As mentioned before, 32 bit support isn't tested as thoroughly as 64 (e.g. https://github.com/bitcoin/bitcoin/pull/28531/files#r2036954366), it would simplify a few scenarios if we didn't have walk on eggshells when doing storage size calculations (e.g. https://github.com/bitcoin/bitcoin/blob/master/src/util/byte_units.h#L15)
(https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-2841724415)
As mentioned before, 32 bit support isn't tested as thoroughly as 64 (e.g. https://github.com/bitcoin/bitcoin/pull/28531/files#r2036954366), it would simplify a few scenarios if we didn't have walk on eggshells when doing storage size calculations (e.g. https://github.com/bitcoin/bitcoin/blob/master/src/util/byte_units.h#L15)
π¬ scgbckbone commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841741416)
Concept ACK
Better to just increase default size and keep config ops. If some people'd like to pretend that it is not already happenig - so be it
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841741416)
Concept ACK
Better to just increase default size and keep config ops. If some people'd like to pretend that it is not already happenig - so be it
π Sjors opened a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386)
The term "force" is ambiguous and not used in [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes) where there ! rule prefix is introduced.
E.g. this code is hard to read:
```cpp
if (!gbt_force) {
s.insert(s.begin(), '!');
```
Additionally, #29039 renamed `gbt_vb_name` to `gbt_force_name` which might, at least for me, further increased the confusion.
This is a pure (variable rename) refactor and does not change behavior.
(https://github.com/bitcoin/bitcoin/pull/32386)
The term "force" is ambiguous and not used in [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes) where there ! rule prefix is introduced.
E.g. this code is hard to read:
```cpp
if (!gbt_force) {
s.insert(s.begin(), '!');
```
Additionally, #29039 renamed `gbt_vb_name` to `gbt_force_name` which might, at least for me, further increased the confusion.
This is a pure (variable rename) refactor and does not change behavior.
π¬ willcl-ark commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2841768158)
I am able to replicate this on NixOS
<details>
<summary>Details</summary>
```log
bitcoin on ξ master [$?β‘] via β³ v3.30.5 via π v3.12.7 via βοΈ impure (nix-shell-env)
β― just build -DWITH_USDT=ON
configure running
configure complete
build running
build complete
bitcoin on ξ master [$?β‘] via β³ v3.30.5 via π v3.12.7 via βοΈ impure (nix-shell-env) took 41s
β― sudo -E PYTHONPATH="/nix/store/nql0w8in8bj1i3f0xgpxr40hx2gy2lgg-bcc-0.31.0/lib/python3.12/site-packages:/nix/store/bymayrdmvq8ahkhrlcw2akjs
...
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2841768158)
I am able to replicate this on NixOS
<details>
<summary>Details</summary>
```log
bitcoin on ξ master [$?β‘] via β³ v3.30.5 via π v3.12.7 via βοΈ impure (nix-shell-env)
β― just build -DWITH_USDT=ON
configure running
configure complete
build running
build complete
bitcoin on ξ master [$?β‘] via β³ v3.30.5 via π v3.12.7 via βοΈ impure (nix-shell-env) took 41s
β― sudo -E PYTHONPATH="/nix/store/nql0w8in8bj1i3f0xgpxr40hx2gy2lgg-bcc-0.31.0/lib/python3.12/site-packages:/nix/store/bymayrdmvq8ahkhrlcw2akjs
...
π¬ stevenroose commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2841771631)
ACK
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2841771631)
ACK
π¬ Ali2kCom commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841782583)
Concept NACK. Removing OP_RETURN makes it more difficult to run full nodes and, as a result, gradually centralizes the network. This is not an issue where a small group of developers can implement changes that affect Bitcoin's core assumptions.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841782583)
Concept NACK. Removing OP_RETURN makes it more difficult to run full nodes and, as a result, gradually centralizes the network. This is not an issue where a small group of developers can implement changes that affect Bitcoin's core assumptions.
π¬ stevenroose commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841784481)
Concept ACK. Implementation looks ok, can't comment on functional test coverage.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2841784481)
Concept ACK. Implementation looks ok, can't comment on functional test coverage.
π¬ 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.