💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908602680)
Sorry, I'm misunderstanding you but I think for a run-time invocation of `MiBToBytes()` with `mib==-1` this assertion `Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21);` would not be hit, which is different to your hard-coded static_assert expression.
But I agree that @ryanofsky's right-shifting the max approach preferable (and also what I suggested in my inlined approach in https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907267762)
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908602680)
Sorry, I'm misunderstanding you but I think for a run-time invocation of `MiBToBytes()` with `mib==-1` this assertion `Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21);` would not be hit, which is different to your hard-coded static_assert expression.
But I agree that @ryanofsky's right-shifting the max approach preferable (and also what I suggested in my inlined approach in https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907267762)
💬 maflcko commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579902580)
Not sure about this. If someone goes back in the main chain to an earlier block to mine on top of it for testing (or fun), this could lead to the mined block being rejected from the own node. Conceptually, there is an argument to be self-consistent, but I am not sure how to weight that against removing the lines of code.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579902580)
Not sure about this. If someone goes back in the main chain to an earlier block to mine on top of it for testing (or fun), this could lead to the mined block being rejected from the own node. Conceptually, there is an argument to be self-consistent, but I am not sure how to weight that against removing the lines of code.
👋 TheCharlatan's pull request is ready for review: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483)
(https://github.com/bitcoin/bitcoin/pull/31483)
💬 maflcko commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2579907094)
Just the duration, edited my comment.
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2579907094)
Just the duration, edited my comment.
💬 maflcko commented on pull request "tracing: Rename the `MIN` macro to `_TRACEPOINT_TEST_MIN` in log_raw_p2p_msgs":
(https://github.com/bitcoin/bitcoin/pull/31623#issuecomment-2579942859)
I did not test with https://github.com/bitcoin/bitcoin/issues/31418, but the change would be harmless in the worst case anyway.
review ACK f93f0c93961bbce413101c2a92300a7a29277506 🔶
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV
...
(https://github.com/bitcoin/bitcoin/pull/31623#issuecomment-2579942859)
I did not test with https://github.com/bitcoin/bitcoin/issues/31418, but the change would be harmless in the worst case anyway.
review ACK f93f0c93961bbce413101c2a92300a7a29277506 🔶
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV
...
💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579984745)
> the mined block being rejected from the own node
Currently `getblocktemplate` has a guard on mainnet against `miner.isInitialBlockDownload()`, which among other things checks `MinimumChainWork`. So without patching the code you can't generate a pre-segwit block using `getblocktemplate`.
But if you patched that check away, why would a SegWit block be rejected?
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579984745)
> the mined block being rejected from the own node
Currently `getblocktemplate` has a guard on mainnet against `miner.isInitialBlockDownload()`, which among other things checks `MinimumChainWork`. So without patching the code you can't generate a pre-segwit block using `getblocktemplate`.
But if you patched that check away, why would a SegWit block be rejected?
💬 maflcko commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2579985449)
Jammy and Bullseye don't provide the alias/redirect from pkg-config to pkgconf, but I guess their version of pkgconf should be good enough? Otherwise, it may be better to wait until they are unsupported.
If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2579985449)
Jammy and Bullseye don't provide the alias/redirect from pkg-config to pkgconf, but I guess their version of pkgconf should be good enough? Otherwise, it may be better to wait until they are unsupported.
If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
💬 maflcko commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579988889)
> But if you patched that check away, why would a SegWit block be rejected?
Witness data is not allowed per consensus rules before segwit activation.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2579988889)
> But if you patched that check away, why would a SegWit block be rejected?
Witness data is not allowed per consensus rules before segwit activation.
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2580005706)
Thank you for the reviews @ryanofsky and @stickies-v , sorry for the many pushes here, pushed a stale branch by mistake.
Updated 578654c686e394d08bac7c3bcddbfd90b46eaa62 -> 941a194c944826049f823858cb15c41963e4619a ([kernel_cache_sizes_9](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_9) -> [kernel_cache_sizes_10](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_10), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_9..kernel_cache_siz
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2580005706)
Thank you for the reviews @ryanofsky and @stickies-v , sorry for the many pushes here, pushed a stale branch by mistake.
Updated 578654c686e394d08bac7c3bcddbfd90b46eaa62 -> 941a194c944826049f823858cb15c41963e4619a ([kernel_cache_sizes_9](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_9) -> [kernel_cache_sizes_10](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_10), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_9..kernel_cache_siz
...
💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2580020621)
> Jammy and Bullseye don't provide the alias/redirect from pkg-config to pkgconf, but I guess their version of pkgconf should be good enough? Otherwise, it may be better to wait until they are unsupported.
The Debian package for `pkg-config` is still there, and depends on `pkgconf`... if only it were as lightweight as merely aliasing.
> If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
Thank you for
...
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2580020621)
> Jammy and Bullseye don't provide the alias/redirect from pkg-config to pkgconf, but I guess their version of pkgconf should be good enough? Otherwise, it may be better to wait until they are unsupported.
The Debian package for `pkg-config` is still there, and depends on `pkgconf`... if only it were as lightweight as merely aliasing.
> If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
Thank you for
...
📝 hebasto opened a pull request: "depends: Use base system's `sha256sum` utility"
(https://github.com/bitcoin/bitcoin/pull/31626)
On FreeBSD, the `shasum` utility is provided by the [`perl5`](https://ports.freebsd.org/cgi/ports.cgi?query=%5Eperl5&stype=all&sektion=all) port, which is not part of the base system and must be [installed](https://github.com/hebasto/bitcoin-core-nightly/blob/0e3518579a02e6c79bafc67817136220579a0d5c/.github/workflows/freebsd.yml#L104) separately. Note that this requirement is currently not documented in [`depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md).
T
...
(https://github.com/bitcoin/bitcoin/pull/31626)
On FreeBSD, the `shasum` utility is provided by the [`perl5`](https://ports.freebsd.org/cgi/ports.cgi?query=%5Eperl5&stype=all&sektion=all) port, which is not part of the base system and must be [installed](https://github.com/hebasto/bitcoin-core-nightly/blob/0e3518579a02e6c79bafc67817136220579a0d5c/.github/workflows/freebsd.yml#L104) separately. Note that this requirement is currently not documented in [`depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md).
T
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580026256)
> > I tried testing the asan job, via `time MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh`, on a Fedora dev box, and it fails with the following output.
>
> I guess checking that we're in a docker container (that hopefully doesn't have other services that does anything network related) is the only option here. Only fail in docker, otherwise just print, if possible.
None of the CI tasks use fedora, so the only way to run the native_asan task on Fedora
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580026256)
> > I tried testing the asan job, via `time MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh`, on a Fedora dev box, and it fails with the following output.
>
> I guess checking that we're in a docker container (that hopefully doesn't have other services that does anything network related) is the only option here. Only fail in docker, otherwise just print, if possible.
None of the CI tasks use fedora, so the only way to run the native_asan task on Fedora
...
💬 adlai commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1908698211)
This change appears to remove `pkg-config` without including its replacement, `pkgconf`...
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1908698211)
This change appears to remove `pkg-config` without including its replacement, `pkgconf`...
💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2580036366)
Ok, so someone could roll back their node to before SegWit activation, patch `getblocktemplate` to skip the `IsInitialBlockDownload()` check (or remove the `MinimumChainWork` and `max_tip_age` checks from from the latter).
If they then call `getblocktemplate` and if they have any SegWit transactions in their mempool, it would generate a template with SegWit transactions and a commitment. Since it verifies the template at the end, the RPC call would fail with `unexpected-witness`.
They coul
...
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2580036366)
Ok, so someone could roll back their node to before SegWit activation, patch `getblocktemplate` to skip the `IsInitialBlockDownload()` check (or remove the `MinimumChainWork` and `max_tip_age` checks from from the latter).
If they then call `getblocktemplate` and if they have any SegWit transactions in their mempool, it would generate a template with SegWit transactions and a commitment. Since it verifies the template at the end, the RPC call would fail with `unexpected-witness`.
They coul
...
💬 maflcko commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2580048274)
> Ok, so someone could roll back their node to before SegWit activation, patch `getblocktemplate` to skip the `IsInitialBlockDownload()` check (or remove the `MinimumChainWork` and `max_tip_age` checks from from the latter).
I don't think a patch is needed. ibd will latch, so in theory one could use that. I am not claiming this is a valid use case, just that internal self-consistency was the reason why this hasn't been done previously, IIRC.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2580048274)
> Ok, so someone could roll back their node to before SegWit activation, patch `getblocktemplate` to skip the `IsInitialBlockDownload()` check (or remove the `MinimumChainWork` and `max_tip_age` checks from from the latter).
I don't think a patch is needed. ibd will latch, so in theory one could use that. I am not claiming this is a valid use case, just that internal self-consistency was the reason why this hasn't been done previously, IIRC.
💬 fjahr commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2580054912)
Re-ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce
Confirmed only change was a rebase.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2580054912)
Re-ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce
Confirmed only change was a rebase.
💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580057259)
Note that this isn't Fedora specific. The exact same issue happens on Ubuntu.
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580057259)
Note that this isn't Fedora specific. The exact same issue happens on Ubuntu.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580061968)
Maybe the test changes can be split up from the ci changes, given that this is still WIP?
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2580061968)
Maybe the test changes can be split up from the ci changes, given that this is still WIP?
💬 l0rinc commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1908734678)
Doesn't this apply to [NetBSD](https://github.com/bitcoin/bitcoin/blob/7b06ffce9c50110b475c722918c55a14402346a5/depends/builders/netbsd.mk#L1) as well?
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1908734678)
Doesn't this apply to [NetBSD](https://github.com/bitcoin/bitcoin/blob/7b06ffce9c50110b475c722918c55a14402346a5/depends/builders/netbsd.mk#L1) as well?
💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2580080649)
I haven't tested this, and it looks harmless and is easy to revert, if needed. So lgtm ACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2580080649)
I haven't tested this, and it looks harmless and is easy to revert, if needed. So lgtm ACK e87429a2d0f23eb59526d335844fa5ff5b50b21f