💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910076808)
> I think the `weight` and `sigops` results of `getblocktemplate` are for individual transactions in the block template.

Ah that would make sense, since I tried to add those totals in an early version of #27433.

💬 maflcko commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582264739)
Not sure how useful it is to derive speed improvements from measurements where the variance is about half as large as the difference itself. Not claiming this is the case here, but if you measure from the public network, you could very well just measure the bandwidth of the picked nodes (completely unrelated to this pull).

It is fine if you want to do those measurements locally for fun, but putting them in the pull request title and description doesn't seem ideal. It would be better to focus
...

👍 Sjors approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2541983102)
ACK 7ab1344eb13228998bcead7328d5cc0a905971d4

Just some feedback on the release notes.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910102771)
7ab1344eb13228998bcead7328d5cc0a905971d4

`-blockmaxweight` was never `4,000,000` by default

Maybe change this and the next two lines to:

```md
When the node constructs a new block, e.g. for a `getblocktemplate` RPC call,
it doesn't generate the coinbase transaction. Instead it reserves `4,000` weight
units (WU) for it. Before this pull request, the default for `-blockmaxweight`
was `3,996,000`, which effectively added another `4,000` weight units (WU) to
this reservation, leading t
...

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910120119)
I think think this wording is a bit too vague, how about:

```md
Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
`-coinbasereservedweight` to `4,000` to maintain the previous behaviour.
```

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910123643)
`-coinbasereservedweight`

💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582274342)
Usually the variance is a lot lower (see previous measurements), but these are just my benchmarks (I want them to be as close to reality as possible, that's why I'm repeating them to have some predictability), I would appreciate if you could provide independent measurements that you find more stable.

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1910139282)
Agree it's a bit verbose right now. Especially like the first suggestion in the diff.

👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2542035711)
re-ACK 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a

Mixing of MiB and bytes in constants is tolerable, and at least the units are documented.

New unit tests look good to me.

---

Nit: commit message in 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a contains 3 ways of describing the number of bits:
- "memory was allocated on 32bit platforms."
- "maximum size of a 32 bit unsigned"
- "in behaviour on 32-bit systems."

💬 maflcko commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2582320880)
Not sure if it was ever mentioned publicly where a link exists.

Obviously, if you want to reproduce the inconsistency, it would be easier in regtest, and the change may theoretically break someone's test deployment.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910175994)
Taken, thanks.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910176196)
fixed

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910177236)
fixed

💬 0xB10C 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-2582351026)
tested ACK f93f0c93961bbce413101c2a92300a7a29277506

📝 hodlinator opened a pull request: "net: Disconnect message follow-ups to #28521"
(https://github.com/bitcoin/bitcoin/pull/31633)
- Add missing calls to `DisconnectMsg()` - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
- Specify context when stopping nodes - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890780754
- Bring back log message when resetting socket in case new entrypoints are added - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890795074
- Use `DisconnectMsg()` in `CConnman` as well - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791797716

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2582401336)
re-ACK dba211a9ceafc57a953efc757adfe09c0f3fdb14

👍 danielabrozzoni approved a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616#pullrequestreview-2542141582)
tACK e04be3731f4921cd51d25b1d6210eace7600fea4

🚀 fanquake merged a pull request: "doc: upgrade license to 2025."
(https://github.com/bitcoin/bitcoin/pull/31611)

💬 maflcko commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#issuecomment-2582444004)
review ACK 286de9749612565fd8d42f08f66c8426faf8a1f7 📋

<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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 286de9749612
...

👍 danielabrozzoni approved a pull request: "test: importdescriptors is not available for non-descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/31609#pullrequestreview-2542174643)
My understanding of the legacy removal PRs is limited, but it appears that the importdescriptor function will still include the check for legacy wallets.

utACK f9dc45680fa958bc89335fa50f46052e6bb29b37