💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2579834709)
> I also have one last lingering question which is: when does the miner actually construct their coinbase transaction and is that ever compared to the reserved size?

This is entirely up to the pool software. They could call `getblocktemplate` in `proposal` mode (or `checkblock` if #31564 lands).

I think https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084 should be addressed before merging (and so might as well address the other comments now instead of a followup).

📝 TheCharlatan converted_to_draft a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483)
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.

Solve these things by moving the kernel-specific cache size fields to their own struct and moving the
...

✅ Sjors closed a pull request: "getblocktemplate improvements for segwit and sigops"
(https://github.com/bitcoin/bitcoin/pull/27433)

💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-2579855238)
Closing this in favor of two seperate PRs for 9fd56f0ae4b301da38d9e12c57b2ca34941a8d1c and 8a2509e21abfccf64d375b0700034a478ab8aac8. I left 4facb94a481ca121a9117e0992799371bb429da1 as a suggestion for https://github.com/bitcoin/bitcoin/pull/31384/#issuecomment-2579834709.

📝 Sjors opened a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624)
Counting sigops in the witness requires context that CheckBlock() does not have, so it only counts sigops for non-segwit transactions.

It's useful to document, but it should not be a problem.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908583360)
Hmm, you raise a good point. I see two alternatives:
1) The low-code solution: when values get problematic (which is pretty infrequent, and constant), write them as one of follows:

```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{3145728000};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```

or

```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{size_t{3000} << 20};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```


2) instead of adding a `MiBToBytes` function,
...

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908587275)
> I don't think I see a new test case added in the diff. Is it missing?

Snap, they weren't included in my `git diff` because they were added in a new file, and I didn't notice, sorry. I see @TheCharlatan already added a test case in his latest push.

📝 Sjors opened 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.

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

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

👋 TheCharlatan's pull request is ready for review: "kernel: Move kernel-related cache constants to kernel cache"
(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.

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

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

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

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

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

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

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

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