💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519746515)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)

I'm a little confused by "may move the witness reserved value" in this comment. I would think a future soft fork could restrict this value but not move it anywhere. BIP 141 says the coinbase witness has to be a single 32-byte so it doesn't seem like there is more flexibility here.

🤔 w0xlt reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3455640771)
reACK https://github.com/bitcoin/bitcoin/pull/32606/commits/32bfd6136134e7194ea0b56ec08498f66829fc40

💬 w0xlt commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2519911724)
Maybe add a comment for clarification?

```suggestion
// Accept unsolicited cmpctblock *only* from peers we selected as high-bandwidth
// "to" (hb_to). Peers that merely requested HB *from* us (hb_from) don't qualify.
// This matches BIP152's "unsolicited in high-bandwidth mode" intent.
if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
```

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2519946934)
please update the code comments as well

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#issuecomment-3524102355)
big concept ACK, some tests and comments likely still need updating, but can't wait for this to be finally gone, thanks for taking care of it.

For the record this is a continuation of https://github.com/bitcoin/bitcoin/pull/33042:
> Note
CCoinsView::BatchWrite (and transitively CCoinsViewCache::Flush & CCoinsViewCache::Sync) were intentionally not changed here. While all implementations return true, the base CCoinsView::BatchWrite returns false. Changing this would cause coins_view tests to
...

💬 w0xlt commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3524119756)
Adding a new field in the result called `feerate_unit` (string) to specify the unit used (`sat/vB` or `BTC/kvB`) may address this issue.
The current approach looks good to me, though.

💬 hebasto commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2519975141)
> I think the `_$<CONFIG>` part is unnecessary, because the value of the `LOCATION` property is already config dependent.

Are you sure about that? I tested your suggestion, and it returns the path to the “Release” plugin when building with `--config Debug`.

> `$<PATH:GET_*,...>` requires CMake version 3.24.

I think it's safe, as the version of CMake shipped with Visual Studio was [updated to 3.24.1](https://learn.microsoft.com/en-us/cpp/overview/what-s-new-for-visual-cpp-in-visual-studi
...

🤔 hebasto reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455760913)
My Guix build:
```
aarch64
eacb251187a66b61d420c134b48b4081cac704cfaef3965bc10f61f2dedbe9b1 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/SHA256SUMS.part
245e7592e13a90600f6ccff66442e98027d9f36b868942c88a117c05625e603e guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu-debug.tar.gz
083a468e350320244bfb83b46f2015d28a31ab69845fd5f605e31742c9665e27 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu.tar.gz
6031b50181edff
...

👍 ryanofsky approved a pull request: "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-3455789823)
Code review ACK b03feedadb411e15967659be83940d73ce977848. Assuming #33819 goes ahead, there should be less need for this change from the mining interface perspective. But not adding OP_0 seems like more sensible default behavior and all the changes here seem nice to make the test code and BlockAssembler code more explicit and clear about what they are doing.

🤔 hebasto reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455802540)
nit: a typo in commit 9af749f4a455d4b64080e33c707204dee5369a2d message:

"qdusviewer" --> "qdbusviewer".

💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524198301)
> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.

I don't think it is particularly important that reindex be able to complete offline. As @hodlinator pointed out:

> Not the cleanest perhaps. But we have 2 main usages of `-reindex` AFAIK:
>
> * Regular node-runners which run into some kind of disk corruption issue - they should typically have peers available to sync headers from.
> * Developers doing benchmarks/other work who either have suf
...

💬 pablomartin4btc commented on pull request "Added test coverage for qt gui#901 console history filter":
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520038416)
so we don't have to update it every time...
```suggestion
// Copyright (c) 2016-present The Bitcoin Core developers
```

📝 stickies-v opened a pull request: "kernel: allow null data_directory"
(https://github.com/bitcoin/bitcoin/pull/33867)
An empty path may be represented with a `nullptr`. For example, `std::string_view::data()` may return nullptr.

Removes the `BITCOINKERNEL_ARG_NONNULL` attribute for data_directory, and instead handles such null arguments in the implementation.

Also documents how `BITCOINKERNEL_ARG_NONNULL` should be used.

Follow-up to https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265

💬 waketraindev commented on pull request "Added test coverage for qt gui#901 console history filter":
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520040168)
Ah will do, didn't think you meant present literally

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2520053036)
I thought of eliminating the worst case here (when `pa == pb`, where it has to do the comparison for every byte) with something like:
```C++
return pa != pb &&
std::tie(pa->nChainWork, pb->nSequenceId, pb)
< std::tie(pb->nChainWork, pa->nSequenceId, pa);
```

But the benchmarks indicate that it's not really faster.

<details><summary>benchmark with pointer duplicates</summary>

```C++
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software lice
...

💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524231657)
> 4\. try to connect that block, which will result in the entire chain up to that block being connected by the `msghand` thread. This could block the thread hours, in which no other peer would be processed, so existing peers will get disconnected (ping timeout), while net thread will still continually make new connections (that never complete the version handshake because we are too busy connecting blocks).

The easiest solution to this seems to be to use the ThreadPool from [33689](https://gi
...

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520079581)
does this apply to `Flush` as well?

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520070772)
is this still the right abstraction?

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2520101987)
Could you please address them, now that you had to [rebase](https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3510573430) anyway?

💬 hebasto commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2520213223)
I cannot find this change upstream, but it is correct. Otherwise, the `xcb_syslibs` test fails with the following errors:
```
xcb_auth.c:(.text+0x81): undefined reference to `XauGetBestAuthByAddr'
...
xcb_auth.c:(.text+0x292): undefined reference to `XauDisposeAuth'
```