🤔 ismaelsadeeq reviewed a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3390482262)
Concept ACK, thanks for the seperation from the initial index PR.

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470815283)
> nit in [30e5b01](https://github.com/bitcoin/bitcoin/commit/30e5b015c973c1ac375e1eae6649238c6c2e2b28): This seems wrong, as there is no boost usage here?

It is used as in https://github.com/bitcoin/bitcoin/blob/a36b9320300d1fa1e5dbd1b4d493acf3578ac11a/src/policy/truc_policy.cpp#L218 the type of `mempool_ancestors.begin()` is `boost::multi_index::detail::hashed_index_iterator<...>`.

However, the `detail` namespace should be hidden with a Boost-specifc mapping file.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470824383)
hehe, it seems we sent a message at a similar time and missed yours.

re `Interrupt()` removal suggestion:

I find it harder to reason about the current two stages shutdown procedure if we have to call to the same `Stop()` method twice. At that point it would be simpler to just wait for all tasks to finish on the first call and remove the second one. But that seems suboptimal as there is no rush to wait for them at that point of the shutdown sequence.

Also, a benefit of keeping `Interrupt
...

💬 plebhash commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458195100)
I'll give it a try soon

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470835931)
> It is used as in

I understand, but I wonder what value there is in listing the detailed internal boost includes. The `mempool_ancestors` already requires the `txmempool.h` include, which contains all required boost includes. No other module will have to include them to be able to compile.

To me those just seems like bloat for no benefit and the downside of making it harder to change a mempool implementation detail, because headers all over the codebase will have to be adjusted to please
...

👍 hodlinator approved a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3390537459)
re-ACK c62b40d7abd8de1972bacb8c600bbab55a231b20

Changes since my first ACK (https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3168171370):
* Adjusted docs based on my latest feedback.
* Avoids re-converting tx hash in the RPC code.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470845344)
> I don't think we should have a method that would cause the node to crash if Start is called more than once. I disagree and think just calling Stop at the beginning of Start would be safer than the current implementation.

Wouldn't you agree that if your code calls `Start()` twice, you have a design issue? Thread pools are typically started once, maintained for the entire app’s lifecycle, and reused across modules. Mainly because creating and destroying threads isn’t cheap. I think it’s fair
...

💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470851529)
So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added.

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470853688)
You're right.

Reworked.

💬 hebasto commented on pull request "random: scope the environ extern to macOS":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3458248055)
Testing on some other platforms: https://github.com/hebasto/bitcoin-core-nightly/pull/79.

💬 frodrik-froggo commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458279276)
ACK

👍 maflcko approved a pull request: "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`"
(https://github.com/bitcoin/bitcoin/pull/33725#pullrequestreview-3390598306)
review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470891624)
nit in 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0: Same here: Doesn't matter much, but the benefit of including span, when span.h is included, seems limited.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470902222)
> So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added. This follows RAII principles.

Good question. We could easily build an RAII wrapper on top of this. The two approaches aren’t conflicting; it would just be an additional layer over the current code.
Yet, I think the current design smoothly integrates with our existing workflows. We already use the `start`, `interrupt`, and `stop` terms throughout
...

💬 glozow commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3458290468)
If these are Bitcoin Core nodes, is an alternative approach to increase the delay after which we'd send addrs?

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470919585)
Right. But let me leave this change for a follow-up, as it would touch files in `src/crypto`.

💬 davidgumberg commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3458345585)
crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3

Concept ACK on moving the CI script from bash to python and crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/fabe516440c96bb7a6a6902195684d3802d64139 and https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3 as refactors with no behavior change, but those changes seem orthogonal to the PR description and linked issue and I think they would i
...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470959705)
Thanks! Reworked.

💬 SatsAndSports commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458378187)
I've updated the description now, linking to some of the evidence provided by others in this thread, and mentioning the policy that you referenced. I'm not an expert at writing PRs, I hope my updated description is concise and helpful

> @SatsAndSports it would be helpful if you could update the PR description with some of the more specific motivations.

@davidgumberg , thanks for pointing out the README change. I've updated it now too in this PR

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3458382072)
Rebased 78ed83ba337c97798134f4bd0f9307facde3a59d -> a9ec7cdc0a1f6d4a3bd532649e79459074ed9f3b ([kernelInternalLib_20](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_20) -> [kernelInternalLib_21](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_20..kernelInternalLib_21))

* Fixed conflict with #33218