💬 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
...

👍 ryanofsky approved a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749#pullrequestreview-3405061095)
Code review ACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9

I was wondering if this should use `.parent.resolve().parent` instead of `.resolve().parent.parent` but I could imagine resolving first being better if someone has a bin directory with symlinks to installs in different prefixes (like nix or some virtual environments have). There probably isn't a perfect answer but this seems like an improvement.

💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473896041)
> I have a slight tendency to the first option because some people seem to like the convenience of this feature. What do you think?

I think I like both equally, and it feels like whichever default you pick shouldn't matter too much because it will change again soon when asmap data is embedded? (I'm assuming when asmap data is embedded, the default will be to use embedded data unless you specify -noasmap or a filename.)

🚀 fanquake merged a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749)

⚠️ Sjors opened an issue: "Header-only support for waitNext()"
(https://github.com/bitcoin/bitcoin/issues/33756)
I'd like to expand the `waitNext()` Mining IPC function to optionally (via `BlockWaitOptions`) return a new template as soon as we have a header with enough proof-of-work, pending block validation.

https://github.com/bitcoin/bitcoin/blob/3cd4263bf66408760ec3b4ce6ccedcc87e0d53de/src/interfaces/mining.h#L63-L74

I suspect there's little benefit if compact block reconstruction succeeds, and IIUC we hold cs_main during this process anyway so there's no way to generate a template.

But if we need a
...

💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-3473947623)
Possibly relevant: #33756

💬 Sjors commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3473950791)
Do you think #33756 would be compatible with this?

💬 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_r2482073588)
Applied the patch, thanks

💬 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_r2482074480)
Done

💬 ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482074676)
re: 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 check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.

I feel like it's not that confusing once you notice it's declared as outside reference (`bool& interrupt_wait`)
...

💬 Fi3 commented on issue "Header-only support for waitNext()":
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3473971497)
it make sense for the TP to optimistically create templates as soon as there is a possible new block since this will reduce the time that a miner waste on stale jobs. Is kinda safe since the TP can stop following the optimistic path when 2 or more consecutive invalid blocks are received. If we leave it like it is now the miner will keep mine on the old job until the block template do not provide a new job.

So worst case we waste a bunch of hundreds milliseconds mining invalid jobs, that we are
...

💬 0xB10C commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473985400)
Note that the @plebhash comments were actually remove from the archive (as it just pulls the GitHub API. Gone from the GitHub API means gone from the latest version of the archive and mirroring) and might thus be missing from the mirroring in the next rebuild..

See https://github.com/bitcoin-data/github-metadata-backup-bitcoin-bitcoin/commit/9ca6ff3e35249f8ccb710de7cee6ff13e028f0ee#diff-a4bfd29817b2ed302dbaf2c9bd4514ace8aa1ffeacf02046400d93b526b8ab02L847

💬 glozow commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/33196#discussion_r2482100129)
The first few sentences seem largely redundant with the rest of the docs (see few paragraphs of "Security" section, and "Secure authentication". However, the bit about whitelisting RPC commands for specific users seems helpful.

📝 l0rinc opened a pull request: "refactor: make script Solver's often-unused solutions parameter optional"
(https://github.com/bitcoin/bitcoin/pull/33757)
### Summary
Many callsites only need to determine the script type and don't use the parsed solutions.
Previously, these callers still incurred the cost of allocating and populating a `std::vector<std::vector<unsigned char>>`.

### Fix
Changing `Solver` signature from reference to nullable pointer parameter with `nullptr` default allows calling the method for its result only. Note that we can't call `std::optional` here because that would copy the values but we need the result to be provided
...

💬 l0rinc commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482103559)
I don't like the reuses, seems like a workaround instead of a fix, pushed an alternative to https://github.com/bitcoin/bitcoin/pull/33757, added @Raimo33 as a coauthor