💬 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

💬 davidgumberg commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3474017856)
lgtm ack https://github.com/bitcoin/bitcoin/commit/fa9d0f994b45a94e3f26c01e395c58ff59f47f43

https://github.com/bitcoin/bitcoin/actions/runs/18971691417/job/54180745411?pr=33754#step:2:11

💬 Raimo33 commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482120044)
what about the other commits? @l0rinc.
there are some intresting zero-cost optimizations. maybe not worth for a PR alone, but could be included as small refactorings in bigger PRs.

💬 Raimo33 commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3474029652)
Code Review ACK 2726abd896817c0dfd5b171a643bad6af015d070

I proposed the same in this private commit: 3ea121d49c01935f74673cbb825ed608179e43d5
would like a bit more clarity on the benchmarks referenced. are we talking about `CCoinsCaching`?