💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481753366)
Also related to the discussion [here](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366).

💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2481763116)
> As far as I can tell, CTransaction's hashes are computed and cached on construction

No, I mean `uint256 hash = header.GetHash()` is only needed for the debug log - let me try separating all calculations that are only needed for debugging. Given that we're clearing `header` and `txn_available` I understand why the split wasn't done before. But we can still use `block` for the hash and `vtx_missing` for the missing, so I think it should be possible to split the two.
That's a more invasive ch
...

💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481787433)
I agree that `zmqDebug`/`zmqError` would be ideal, however `zmqError` is currently used to log at the `Debug` level - changing it to log at `Error` level while keeping the type signature the same seems likely to cause misuse.

💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481808137)
MAX_REST_HEADERS_RESULTS is a small number, so I don't think we need any architecture-specific and special PTRDIFF handling and can just use fixed size integral values.

Also, to walk backwards, I am sure you can just use `pprev` and not need to create a new method.

💬 instagibbs commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473633999)
If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.

🚀 fanquake merged a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753)

💬 fanquake commented on pull request "test: ipc: resolve symlinks in `which capnp`":
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3473673447)
cc @ryanofsky

🤔 Sjors reviewed a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3404815806)
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

I left some suggestions for extra clarity.

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481810673)
Is this checking that the response to `waitNext()` is a null template?

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481802226)
nit: you could move these helper methods out of the test:

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..7a447eba92 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -82,10 +82,20 @@ class IPCInterfaceTest(BitcoinTestFramework):
coinbase_data = BytesIO((await block_template.result.getCoinbaseTx(ctx)).result)
tx = CTransaction()
tx.deserialize(coinbase_data)
retur
...

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500)
It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line can check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.

💬 TheCharlatan commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473751931)
> If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.

https://docs.counterparty.io/docs/advanced/protocol/#transactions and https://github.com/CounterpartyXCP/counterparty-core/blob/master/counterparty-core/counterpartycore/lib/api/composer.py#L304 should give an idea. AFAICT the real pubkey goes last.

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473753273)
Github completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473764682)
@ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I've simply gotten used to how things were.

> The basic problem here is that this option's default value (ip_asn.map) and default behavi
...

💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3473793738)
[DrahtBot conflict list](https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3190433503) was updated, no more cluster memool conflict, ready for review again!

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3473794785)
I wrote:

> The Template Provider I implemented takes all sorts of stuff from the coinbase in order to produce its NewTemplate message

This is a continuing source of confusion, see e.g. https://github.com/Sjors/sv2-tp/pull/55#pullrequestreview-3404172536

Orthogonal to this PR, but I plan to add some getters to the interface later.

🤔 Crypt-iQ reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3404930775)
> treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily

I think yes, except for -blocksonly nodes, where on receipt we wouldn't punish or anything, but we would ideally drop the message?

> change our behaviour when sending GETBLOCKTXN to also request any transactions that were in o
...

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481900868)
Haven't tested. I think this will cause extra log messages if:
- receive `HEADERS` from LB peer=0
- send `GETDATA` to peer=0
- receive `CMPCTBLOCK` from HB peer=1, it reconstructs, clearing download state for all peers for this block
- receive `CMPCTBLOCK` from LB peer=0

Might confuse a user, but nothing is actually wrong here?

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481892837)
Agree that it doesn't make sense to process, we should drop it asap. Haven't tested either of the below things:

`ProcessHeadersMessage` will trigger a `GETDATA` and if the block turns out to be invalid, our peer who sent us the `CMPCTBLOCK` won't respond and will get disconnected for stalling. If our peer is modified to relay these in the first place to all peers, they could isolate themselves from -blocksonly nodes (assuming they aren't a mining node and don't validate what they send out). T
...

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3473833357)
For my benchmarking needs `reindex-chainstate` suffices, especially now that I know about this corner-case.

> Log an error that assumevalid is enabled but reindex could not find the specified assumevalid header

Now that https://github.com/bitcoin/bitcoin/pull/33336 was merge we're getting part of that message already which should already help a lot in this situation.

Thanks for taking care of this issue, agree with the concerns of @stickies-v, but I personally would either keep `reindex
...