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

💬 Eunovo commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2481025343)
https://github.com/bitcoin/bitcoin/pull/33689/commits/e1eb4cd3a5eb192cd6d9ee5d255688c06ab2089a:

Can we enforce this rule using a thread-local variable?

💬 laanwj commented on pull request "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO":
(https://github.com/bitcoin/bitcoin/pull/33702#discussion_r2481313804)
A copyright header change slipped in here 😄

👍 laanwj approved a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3404168943)
Code review ACK fa2fd0ba1fbd4085df24a2c5472636957db28521

💬 laanwj commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3473021005)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e

Personally, i lost my trust in luke-jr to be responsible with project infrastructure longer ago, when he hijacked the transifex project for Knots (kicking all of the other admins out). Recent developments have done nothing to improve this sentiment.