💬 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
💬 maflcko commented on pull request "doc: upgrade license to 2025.":
(https://github.com/bitcoin/bitcoin/pull/31611#issuecomment-2580086626)
lgtm ACK b537a2c02a9921235d1ecf8c3c7dc1836ec68131
Looks similar to what was done in commit 1f8450f066724dfbb5c5bc4060843e2f3340ed88 last year.
(https://github.com/bitcoin/bitcoin/pull/31611#issuecomment-2580086626)
lgtm ACK b537a2c02a9921235d1ecf8c3c7dc1836ec68131
Looks similar to what was done in commit 1f8450f066724dfbb5c5bc4060843e2f3340ed88 last year.
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2580086993)
Updated 941a194c944826049f823858cb15c41963e4619a -> 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a ([kernel_cache_sizes_10](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_10) -> [kernel_cache_sizes_11](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_10..kernel_cache_sizes_11))
* Fix UBSan failure by avoiding undefined behaviour by casting to `size_t`.
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2580086993)
Updated 941a194c944826049f823858cb15c41963e4619a -> 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a ([kernel_cache_sizes_10](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_10) -> [kernel_cache_sizes_11](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_10..kernel_cache_sizes_11))
* Fix UBSan failure by avoiding undefined behaviour by casting to `size_t`.
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908758174)
Are there places where this would be re-used? I'd be happy to add something more generic if there is demand for it.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908758174)
Are there places where this would be re-used? I'd be happy to add something more generic if there is demand for it.
💬 hebasto commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1908759779)
No, it doesn't, unfortunately.
NetBSD's [`sha256`](https://man.netbsd.org/sha256.1) is not quite compatible with our depends build syytem.
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1908759779)
No, it doesn't, unfortunately.
NetBSD's [`sha256`](https://man.netbsd.org/sha256.1) is not quite compatible with our depends build syytem.
💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760035)
Done
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760035)
Done
💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760127)
Added it to the assert removal commits, thanks!
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760127)
Added it to the assert removal commits, thanks!
💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760232)
Done
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760232)
Done
💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760480)
I've updated the scripted diff to use consistent Read/Write terminology - dropping the `[To/From]Disk` part which also just seems like noise to me. Thanks for the proposals.
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760480)
I've updated the scripted diff to use consistent Read/Write terminology - dropping the `[To/From]Disk` part which also just seems like noise to me. Thanks for the proposals.