💬 maflcko commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550)
Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550)
Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.
💬 0xB10C commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2589450691)
Note that the reserved weight is not only used for the coinbase transaction. The block header takes up 320 WU (80 bytes * 4) and the `var_int` for the number of transactions takes up either 4 WU (1 byte `var_int` for up to 252 txns in the block) or 12 WU (3 byte `var_int` for up to 65535 txns). For most mainnet blocks this is an additional 332 WU.
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2589450691)
Note that the reserved weight is not only used for the coinbase transaction. The block header takes up 320 WU (80 bytes * 4) and the `var_int` for the number of transactions takes up either 4 WU (1 byte `var_int` for up to 252 txns in the block) or 12 WU (3 byte `var_int` for up to 65535 txns). For most mainnet blocks this is an additional 332 WU.
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760)
> Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.
tbh. I don't understand what "version used" really means in this context, looked at a few linked PR's but they went over my head...
but anyways I think this is **out of scope for this PR** / could be addressed in another PR. I only wanted
...
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760)
> Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.
tbh. I don't understand what "version used" really means in this context, looked at a few linked PR's but they went over my head...
but anyways I think this is **out of scope for this PR** / could be addressed in another PR. I only wanted
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2589466471)
@0xB10C pointed out the header always uses 80 bytes and there's few other unavoidable bytes in the block. So it sounds like there should be a minimum value for `-coinbasereservedweight`.
Additionally perhaps it should be renamed to just `-reservedweight` with a clarification in the document comment of how much of that goes to stuff other than the coinbase.
Alternatively we could make `-coinbasereservedweight` refer _only_ to the coinbase, but internally account for these other bytes. That'
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2589466471)
@0xB10C pointed out the header always uses 80 bytes and there's few other unavoidable bytes in the block. So it sounds like there should be a minimum value for `-coinbasereservedweight`.
Additionally perhaps it should be renamed to just `-reservedweight` with a clarification in the document comment of how much of that goes to stuff other than the coinbase.
Alternatively we could make `-coinbasereservedweight` refer _only_ to the coinbase, but internally account for these other bytes. That'
...
💬 0xB10C commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2589468549)
From https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2589450691
> Note that the reserved weight is NOT only used for the coinbase transaction. The block header takes up 320 WU (80 bytes * 4) and the `var_int` for the number of transactions takes up either 4 WU (1 byte `var_int` for up to 252 txns in the block) or 12 WU (3 byte `var_int` for up to 65535 txns). For most mainnet blocks this is an additional 332 WU. See also https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-t
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2589468549)
From https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2589450691
> Note that the reserved weight is NOT only used for the coinbase transaction. The block header takes up 320 WU (80 bytes * 4) and the `var_int` for the number of transactions takes up either 4 WU (1 byte `var_int` for up to 252 txns in the block) or 12 WU (3 byte `var_int` for up to 65535 txns). For most mainnet blocks this is an additional 332 WU. See also https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-t
...
💬 maflcko commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589477304)
"Version used" means the version used in Bitcoin Core's depends system, which compiles the version and statically links it into the release binaries.
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589477304)
"Version used" means the version used in Bitcoin Core's depends system, which compiles the version and statically links it into the release binaries.
✅ Sjors closed a pull request: "miner: always treat SegWit as active"
(https://github.com/bitcoin/bitcoin/pull/31625)
(https://github.com/bitcoin/bitcoin/pull/31625)
💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2589489281)
It would also seem more consistent to drop the `unexpected-witness` check entirely, rather than just change the miner code. That would dramatically increase the scope of this PR though. Leaving up for grabs for now.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2589489281)
It would also seem more consistent to drop the `unexpected-witness` check entirely, rather than just change the miner code. That would dramatically increase the scope of this PR though. Leaving up for grabs for now.
💬 hebasto commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2589491670)
> secp is the only C code we have. So arguably there's no need for `APPEND_CFLAGS` at all when `SECP256K1_APPEND_CFLAGS` can just be used directly, no?
Correct. It is solely about maintaining consistency with the other `APPEND_*` flags. I believe the following
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic"
```
is better than
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DSECP256K1_APPEND
...
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2589491670)
> secp is the only C code we have. So arguably there's no need for `APPEND_CFLAGS` at all when `SECP256K1_APPEND_CFLAGS` can just be used directly, no?
Correct. It is solely about maintaining consistency with the other `APPEND_*` flags. I believe the following
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic"
```
is better than
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DSECP256K1_APPEND
...
👍 theStack approved a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2549426467)
re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2549426467)
re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
👍 brunoerg approved a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2549447462)
code review ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2549447462)
code review ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914669735)
I was wondering if these are stored as part of the block index, or exposed via RPC. Otherwise changing their numeric values shouldn't matter.
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914669735)
I was wondering if these are stored as part of the block index, or exposed via RPC. Otherwise changing their numeric values shouldn't matter.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914673933)
Indeed seems better to keep this. Perhaps mention the last mainnet checkpoint height, since `MinimumChainWork()` is far above that.
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914673933)
Indeed seems better to keep this. Perhaps mention the last mainnet checkpoint height, since `MinimumChainWork()` is far above that.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914677090)
Judging from the change to `ipc_test.cpp` below it seems that eventually the values might be sent over IPC. At that point we have to fix their integer values. Might as well do it now? cc @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914677090)
Judging from the change to `ipc_test.cpp` below it seems that eventually the values might be sent over IPC. At that point we have to fix their integer values. Might as well do it now? cc @ryanofsky
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914687073)
This is a nice improvement. I've been running my node with `-nocheckpoints=1` for years, so now it will just throw a warning rather than abort launch. (which I tested)
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914687073)
This is a nice improvement. I've been running my node with `-nocheckpoints=1` for years, so now it will just throw a warning rather than abort launch. (which I tested)
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2589713954)
Concept ACK
If we go ahead with this, right after the v29 branch-off would seem like a good time.
I plan to expand the alternate mainnet chain in #31583 to 11111+ blocks and have ~p2p_dos_header_tree.py` use that instead. That would make it easier to revert the checkpoints even after testnet3 is completely dropped.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2589713954)
Concept ACK
If we go ahead with this, right after the v29 branch-off would seem like a good time.
I plan to expand the alternate mainnet chain in #31583 to 11111+ blocks and have ~p2p_dos_header_tree.py` use that instead. That would make it easier to revert the checkpoints even after testnet3 is completely dropped.
💬 hebasto commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2589737954)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined. But I'm not sure who's supposed to define that? I suspect that adding it to our cppflags _might_ be the correct thing to do, but who knows what side-effects it would have.
Defining `_NETBSD_SOURCE` works, but I agree that it is not an optimal solution, especially when the `_XOPEN_SOURCE` is already defined.
> Also, it seems these were masked by the fact that we use `-Wno-
...
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2589737954)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined. But I'm not sure who's supposed to define that? I suspect that adding it to our cppflags _might_ be the correct thing to do, but who knows what side-effects it would have.
Defining `_NETBSD_SOURCE` works, but I agree that it is not an optimal solution, especially when the `_XOPEN_SOURCE` is already defined.
> Also, it seems these were masked by the fact that we use `-Wno-
...
💬 kevkevinpal commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589756582)
light code review ACK [2ed161c](https://github.com/bitcoin/bitcoin/pull/31646/commits/2ed161c5ce648cb66ec3d2941b02d68b6ca4c390)
I think it makes sense that our test suite should not attempt to make outbound connections this is a welcomed change
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589756582)
light code review ACK [2ed161c](https://github.com/bitcoin/bitcoin/pull/31646/commits/2ed161c5ce648cb66ec3d2941b02d68b6ca4c390)
I think it makes sense that our test suite should not attempt to make outbound connections this is a welcomed change
🤔 marcofleon reviewed a pull request: "fuzz: Abort when global PRNG is used before SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31548#pullrequestreview-2549561974)
ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
iiuc this PR adds a check to make sure that no randomness is used before `SeedRandomStateForTest(SeedRand::ZEROS)` occurs, vs what `check_globals` does which is just checking if seeding happened at any point in the fuzz test.
Tested by moving `SeedRandomStateForTest(SeedRand::ZEROS)` to after test setup in `utxo_total_supply` and saw the assertion failure.
```
/src/test/util/random.cpp:45 SeedRandomStateForTest: Assertion `!g_used_g_prng' fail
...
(https://github.com/bitcoin/bitcoin/pull/31548#pullrequestreview-2549561974)
ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
iiuc this PR adds a check to make sure that no randomness is used before `SeedRandomStateForTest(SeedRand::ZEROS)` occurs, vs what `check_globals` does which is just checking if seeding happened at any point in the fuzz test.
Tested by moving `SeedRandomStateForTest(SeedRand::ZEROS)` to after test setup in `utxo_total_supply` and saw the assertion failure.
```
/src/test/util/random.cpp:45 SeedRandomStateForTest: Assertion `!g_used_g_prng' fail
...
💬 Sjors commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2589771910)
Tested on my self-hosted Cirrus setup, seems fine: https://cirrus-ci.com/task/6297483418009600
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2589771910)
Tested on my self-hosted Cirrus setup, seems fine: https://cirrus-ci.com/task/6297483418009600