📝 Sjors converted_to_draft a pull request: "miner: always treat SegWit as active"
(https://github.com/bitcoin/bitcoin/pull/31625)
The `getblocktemplate` RPC would check if SegWit has activated yet at the tip. This has been the case for more than seven years.
(https://github.com/bitcoin/bitcoin/pull/31625)
The `getblocktemplate` RPC would check if SegWit has activated yet at the tip. This has been the case for more than seven years.
💬 hebasto commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934)
> > The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
>
> That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
>
> > Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
>
> It seems kinda unlikely they'd do that if the option was incompati
...
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934)
> > The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
>
> That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
>
> > Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
>
> It seems kinda unlikely they'd do that if the option was incompati
...
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914459164)
dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I'm puzzled by the need to `deepcopy` here, but if I just do `block.vtx = txs` then in the next call to `mine`, without passing any transactions in, `txs` will have the coinbase of the previous block in it.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914459164)
dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I'm puzzled by the need to `deepcopy` here, but if I just do `block.vtx = txs` then in the next call to `mine`, without passing any transactions in, `txs` will have the coinbase of the previous block in it.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914465891)
Well the linter forced me to change the `txs` default to `None`, which "solved" the issue.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914465891)
Well the linter forced me to change the `txs` default to `None`, which "solved" the issue.
💬 hebasto commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2589365958)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2589365958)
Concept ACK.
💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914467060)
I guess you figured it out, given the CI failure https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2589364296 ?
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914467060)
I guess you figured it out, given the CI failure https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2589364296 ?
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914473845)
thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.
weird that it displays correctly for me but not for you though (also tested with `gh-markdown-preview` locally).
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914473845)
thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.
weird that it displays correctly for me but not for you though (also tested with `gh-markdown-preview` locally).
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1914478423)
> Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?
As far as I understand it is legal to shift e.g. `int32_t{1} << 31` to get the minimum `int32_t` value. But I think in this case we should prohibit this quirk of the underlying bit representation and use `std::numeric_limits<T>::digits`.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1914478423)
> Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?
As far as I understand it is legal to shift e.g. `int32_t{1} << 31` to get the minimum `int32_t` value. But I think in this case we should prohibit this quirk of the underlying bit representation and use `std::numeric_limits<T>::digits`.
💬 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