💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2481029377)
done

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472525510)
I split the template block mutation test into a third commit and documented its history in the commit message.

📝 rustaceanrob opened a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753)
`format!` strings may contain variables within the string representation. This is a lint as of a recent `rustc` nightly version.

Pull requests without a rationale and clear improvement may be closed
immediately.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3472571178)
> 1. Log an error that assumevalid is enabled but reindex could not find the specified assumevalid header, print the highest available header in blockindex and inform the user that they can restart with `-assumevalid=0` or `-assumevalid=<best_available_header>`or a fresh IBD, and then shutdown the node.

I agree with this approach, but we would still have to update the `minimumchainwork` logic for reindex, or we ask the user to also update the `minimumchainwork` too.

> 3. generalize validat
...

📝 maflcko opened a pull request: " ci: gha: Set debug_pull_request_number_str annotation "
(https://github.com/bitcoin/bitcoin/pull/33754)
GitHub Actions does not offer any way to determine the pull request number in a machine readable way from the checks API. See https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1503475232.

However, the pull request number can be useful for external tools to act on CI results.

Fix that by using a check run annotation for a single task named `debug_pull_request_number_str`.

This should re-enable the 'CI Failed' labelling mechanism via https://github.com/maflcko/DrahtBot/commit/1
...

💬 maflcko commented on pull request "test: Format strings in `test_runner`":
(https://github.com/bitcoin/bitcoin/pull/33753#issuecomment-3472587205)
Thanks. It makes sense to be clippy-clean and just consistently apply the shorter and easier-to-read variant.

lgtm ACK d305b865c9630646a6cf7d227c08ba4263a379f7

💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481100715)
this seems a bit tedious. Why not just use i64 as a type, which can hold a sign and a value. This way, it won't be necessary to use a bool to hold the sign.

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3472631221)
Concept ACK

Can there ever only be one `waitNext()` call on per `BlockTemplate`? If not, does `interruptWait()` just stop all of them?

💬 Sjors commented on pull request "test: ipc: resolve symlinks in `which capnp`":
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3472661703)
I tested that it doesn't interfere with my macOS setup (`/opt/homebrew/bin/capnp`).

💬 maflcko commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3472734162)
(looks like it worked)

💬 TheCharlatan commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3472793534)
rfm?

💬 Sjors commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3472829721)
> Plan

Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.

💬 laanwj commented on pull request "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO":
(https://github.com/bitcoin/bitcoin/pull/33702#issuecomment-3472856788)
Concept ACK

🤔 stickies-v reviewed a pull request: "zmq: Log bind error at Error level, abort startup on init error"
(https://github.com/bitcoin/bitcoin/pull/33727#pullrequestreview-3403499472)
Concept ACK, the new shutdown behaviour seems much more sane.

💬 stickies-v commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2480875804)
nit: line length (also in .h)

💬 stickies-v commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481230350)
I also don't like this double wrapping, it's error prone. It seems like this is currently necessary because `Create` is actually more like a `MaybeCreate`, with actual creation depending on `gArgs`. A more robust approach imo would be to take `gArgs` out of `Create`, let the caller decide if we actually want to create a `CZMQNotificationInterface` and pass the necessary parameters. With that change, `Create` should always return a value, so we can keep `nullptr` for the failure case?

One appr
...

💬 stickies-v commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481238386)
Instead of disabling the test, we should probably test that invalid arguments shut down the node, as per the new behaviour?

💬 stickies-v commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2480882328)
`Error` and `Debug` are different levels, so this name seems confusing to me. Would simplify to `zmqError` and `zmqDebug`, removing the `level` parameter.

Side note: I don't know much about zmq, but it seems like some of these debug messages should actually be warnings, e.g. when block gets failed to read from disk? Can/should probably be done in a separate pull, though.

💬 stickies-v commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3472925314)
> I agree with this approach, but we would still have to update the `minimumchainwork` logic for reindex, or we ask the user to also update the `minimumchainwork` too.

That seems sensible. I'm currently unsure what the best approach is here, but informing in logging seems like the minimum we can do.

> I'm not sure how important it is for reindex to be able to work without a network connection.

I don't think it is. Reindex being fully offline to me seems like an artifact of our current s
...

🤔 Eunovo reviewed a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3403702771)
Concept ACK https://github.com/bitcoin/bitcoin/pull/33689/commits/435e8b5a55033fbfc6428a612b9826713b3cf57a

The PR looks good already, but I think we can block users from calling `Threadpool::Start()` and `Threadpool::Stop()` inside Worker threads; We can use a thread local variable to identify worker threads and reject the operation.