💬 luke-jr commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#issuecomment-2575161578)
What would use the "weak block" check that doesn't need the ability to do it independently from proposals anyway?

While the existing GBT BIPs are Final, the protocol was designed to allow for extensions. If there's a use case for this feature, doing it that way seems fine

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