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

💬 TheCharlatan commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2481363335)
Why did you think this single test was not enough? Is there additional clarity or coverage from the additional checks?

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3473040563)
Pushed, thanks for the review, you had a few good points, added you as coauthor.

> the gain of using the spaceship operator (which comes at the cost of readability) is marginal.

the spaceship might not be very familiar to us yet, but it's arguably simpler, having fewer moving parts, and some compilers prefer that over the manual comparisons.

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2481401944)
> Goal is to keep the same synchronization mechanisms we had in the http server code

What is the reason for that, isn't the goal to have a reusable `ThreadPool`?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2481400847)
k, thanks, please resolve the comment

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2481442031)
Where would we be getting negative numbers from?
If we care about the value being in a certain range, we could validate that instead:
```C++
assert(num_workers > 0 && num_workers <= MAX_WORKER_COUNT);
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2481430614)
Can you point me to the test that fails please? I have removed it, ran the tests added in this PR and they all passed.