🤔 Sjors reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2533761935)
Code reviewed ededdd13f3263df08f7878aa2514d3796436cbfb, almost there.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905127347)
ededdd13f3263df08f7878aa2514d3796436cbfb: maybe replace this line with:

```md
This specifies how many weight units to reserve for
the coinbase transaction.

Upgrade note: the default of `-maxcoinbaseweight`
ensures backward compatibility for users who did
not override `-blockmaxweight`. If you previously
configured `-blockmaxweight=4000000` then you can
safely set `-maxcoinbaseweight=4000` to maintain
existing behavior.
```

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905371266)
76a95522b322c1a8ab1594ca8bf5ac99ab379c46: to keep log lines short, maybe say `(ex. coinbase)`. Maybe also move it left: `CreateNewBlock(): block weight (ex. coinbase): ...`

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905367911)
6cc658075e1be59ef49a060b36921538acf0edb1 nit: current naming convention uses snake case. Maybe just use `max_block_weight`.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905363548)
354f41f5c76f5f43cbfdddb63660cc77c4a76989: I think this should be `MAX_BLOCK_WEIGHT - coinbase_max_additional_weight`, because later on this fuzzer adds a small coinbase transaction.

(can probably just hardcode something like `- 4000`)

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905381347)
76a95522b322c1a8ab1594ca8bf5ac99ab379c46 nit: `(excluding coinbase)`, since the description already says "transactions"

💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#issuecomment-2575214683)
@luke-jr do you mean that every client that wants to call `checkblock $block_hex` or `getblocktemplate '{"mode": "proposal", "data": "$block_hex"}'` for a weak block, probably implements its own proof-of-work check? That's probably true, since it most likely needs the ability to parse the block anyway in order to inspect the coinbase. Compared to that calculating proof-of-work and comparing against a target is trivial.

What would an extension look like to pass `check_pow` and `target` argumen
...

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2575241273)
Thanks for the reviews @hodlinator, just cleaning up your nits:

Updated 54bdaaefbc683a61f3051dd90bbaf69de5d6cac5 -> 224bfeb07f7561470451c95dc0df1db959bc187e ([kernel_cache_sizes_6](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_6) -> [kernel_cache_sizes_7](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_6..kernel_cache_sizes_7))

* Addressed @hodlinator's [comment](https://githu
...

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1905431196)
Should probably fix this typo in commit it was introduced (a5742b7c09f1b4204abf4a1992018fb7be2fef27), instead of in final commit (224bfeb07f7561470451c95dc0df1db959bc187e).

💬 Sjors commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2575279833)
I'm running a benchmark to compare this PR (898a07e2ab3e5b653ddadc76f2d04d625f35607c, rebased) against master @ 433412fd8478923dfdb20044f74c5d1e19fa8dd8. Will update this comment.

AMD Ryzen 7950x machine with Ubuntu 24.10, with one local network peer and a gigabit internet connection.

```
bitcoind -dbcache=30000 -stopatheight=878000 -blocksdir=/magnetic/.bitcoin -addnode=local-network
```

Before: .. hours .. minutes
After: .. hours and .. minutes

Time includes .. minutes to flush
...

💬 ldionne commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575469949)
Our documentation is misleading. We shouldn't mention `_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING` and friends the way we do it -- you're never intended to set these macros yourself. Instead, you must pass them at CMake configuration time as `-DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY;etc"`.

The difference is that passing them at CMake configuration time (when building libc++) will result in these macros being baked into the library headers (lo
...

📝 l0rinc opened a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616)
The line has been present since the beginning.
This PR simply removed the trailing whitespace and capitalizes the line for consistency.

Example logs before the change:
```
2025-01-07T11:58:33Z Verification progress: 99%
2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions)
2025-01-07T11:58:33Z block index 31892ms
2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode
```

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2575493948)
Updated 224bfeb07f7561470451c95dc0df1db959bc187e -> 9f61eb074438f85ed1a23fb3da94e4c68855d3fb ([kernel_cache_sizes_7](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_7) -> [kernel_cache_sizes_8](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_7..kernel_cache_sizes_8))

* Fixed comment in the correct commit now.

🤔 hodlinator reviewed a pull request: "refactor: cache block[undo] serialized size for consecutive calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2534307845)
Concept ACK cba4378072a6bd43f3e37a7d5e662d3041681566

Removes repetitive calls to `GetSerializeSize()`. And avoids calling `.tell()`.

Nothing blocking.


### PR summary

Repetitive mention of #31539.


### Commit history

Tested splitting up 96c9b578a6b85fac9977cdf25bf2e52d04645bd4 into one which refactors existing code (225d294070fc403e03028b9fab714bd9dc4f3307), and a second commit (5f43ff3f1928e6aaca0edb4f108a03d66c8dac44) which renames the file and adds `SaveBlockBench()`. Keep
...

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905565819)
nit:
### Terminology

To me read/write and save/load are distinct pairs, so `ReadBlock`/`SaveBlock` seems off. Would have preferred `WriteBlockUndo` and `WriteBlock`.

I liked the explicitness of `ToDisk`/`FromDisk` for cases where that is actually what it's doing, makes code more self-documenting, even if slightly verbose. I see the change was prompted by https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2509562827.

Could the scripted diff commit message at least include
...

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905437524)
nit:
```suggestion
static CBlock CreateTestBlock()
```

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905481382)
nit: Could add missing newline before EOF.

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905456335)
nit: Newer code shouldn't need to explicitly log `__func__` as we have `-logsourcelocations`.
```suggestion
LogError("OpenUndoFile failed");
```

💬 l0rinc commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905586719)
Thanks @hodlinator!
The terminology was requested by @ryanofsky and @TheCharlatan - I don't have strong feelings about them either way. What do other reviewers thing?

💬 vasild commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575517874)
Alright, changed that to `-DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR"` when compiling libc++.