💬 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`...

💬 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
...

💬 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.

💬 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.

💬 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.

💬 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?

💬 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?

💬 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

💬 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.

💬 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`.

💬 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.

💬 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.

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(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!

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(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.

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760615)
Done, also added TODOs to the commit (+ message) to obviate that they're temporary

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760710)
I usually went in the other direction to make sure we know the sizes exactly.
Since `size_t` doesn't really serialize (e.g. in `fileout << GetParams().MessageStart() << blockundo_size` -> `call to 'Serialize' is ambiguous`) - but I made it into a `unsigned int` (I don't particularly like it, but it's not that important).

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760793)
Done, went with `unsigned int`

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908761002)
I've reverted the comments that were used to group functionality (write header ... write [undo[block), but I've deleted the ones that were just repeating code. Especially after extracting the constant to names values, they don't add any value.