💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589387132)
added a blank line between the paragraph and the table in `## Compiler` and moved `Python` to the `## Optional` table. thanks for the help everyone.
I think `Linux Kernel` should be moved to optional as well then but since it was in the required table before and I'm not sure if I should change more stuff after receiving acks I just left it as is now.
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589387132)
added a blank line between the paragraph and the table in `## Compiler` and moved `Python` to the `## Optional` table. thanks for the help everyone.
I think `Linux Kernel` should be moved to optional as well then but since it was in the required table before and I'm not sure if I should change more stuff after receiving acks I just left it as is now.
💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2589399613)
> @hebasto: Did you get a reply in the Developer Community issue?
Not really. I only received an email confirming the creation of a new issue, along with the link I posted above.
> I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist") Thanks!
It's strange, but the link I received doesn't work for me either.
cc @CaseyCarter
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2589399613)
> @hebasto: Did you get a reply in the Developer Community issue?
Not really. I only received an email confirming the creation of a new issue, along with the link I posted above.
> I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist") Thanks!
It's strange, but the link I received doesn't work for me either.
cc @CaseyCarter
💬 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