💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534465728)
we would probably still need the context string to differentiate between the initial `BlockWorkers()` calls vs the final result `WaitFor`. Should re-check if `BOOST_REQUIRE` prints the stack trace; most likely it doesn't and that was the reason behind my past-self added the context string.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534438190)
I'm not sure about including extra dependencies on a low-level class for tiny readability improvements. But I'm not really opposed to this one, so done as suggested.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534442998)
Done as suggested.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534447295)
Done as suggested.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534473189)
Instead of doing it for all the test cases, added another case that exercises the contention.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534474010)
Done as suggested.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534476766)
Done as suggested.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534496697)
> do we need a vector here of can we just consume whatever we just submitted?

Consuming right away would wait for the task to be executed; we want to exercise some concurrency too.

💬 m3dwards commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3542456588)
Not sure I understand the dependence on #33775?

CI is using it's own build, not the guix build right? Also MinGW brings it's own GCC doesn't it?

💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3542515441)
> It's true that you need txdata anyway, but you don't need it immediately. You need it when the pool asks using the RequestTransactionData message.

not really, we also need it for `NewTemplate.merkle_path`

that would be true if there were a `getMerklePath()` method for a given template, in which case we'd be able to do `getHeader()` + `getCoinbase()` + `getMerklePath()`

but with the current interface, `getBlock()` is the only way we can properly build a `NewTemplate` message

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2534658896)
Thanks

💬 waketraindev commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#issuecomment-3542674031)
Concept ACK

nit: Please squash the commits per the contributing guidelines: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Both commits touch `ci/README.md` and cover the same scope.

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2534681621)
Awesome

💬 hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#issuecomment-3542707053)
> nit: Please squash the commits per the contributing guidelines: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

While they do touch adjacent lines, the commits each address separate sub-issues, both with their own links in respective commit message. So I think it's more orderly like this. But open to squash if other reviewers agree.

💬 waketraindev commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2534500280)
Bundling the implementation of `AutoFile::size` here doesn't seem strictly necessary. It looks like you could rely on the length value return by `tell()` and avoid depending on Streams refactoring. That might help keep this PR a bit more focused on the asmap changes without pulling in broader Streams updates or file-size-related changes elsewhere.

💬 instagibbs commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542756169)
> The requirement is for m not to be greater than n, as far as I can see? (via both IsStandard() and Solver())

circa e679ec969c8b22c676ebb10bea1038f6c8f13b33 *creation* was constrained to m-of-3

💬 cobratbq commented on issue "External signer: in case of error shows only "External signer failure"":
(https://github.com/bitcoin/bitcoin/issues/31004#issuecomment-3542769071)
@Sjors you might want to decide whether it's worth changing behavior of how stderr output is displayed. Otherwise, feel free to close. I left it open in case there is interest in following up.

🤔 mzumsande reviewed a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792#pullrequestreview-3473576018)
Code Review ACK https://github.com/bitcoin/bitcoin/commit/52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3542814219)
> Not sure I understand the dependence on #33775?
>
> CI is using it's own build, not the guix build right? Also MinGW brings it's own GCC doesn't it?

The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.

📝 maflcko opened a pull request: " ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG "
(https://github.com/bitcoin/bitcoin/pull/33888)
The sanity check to check the last few merge commit signatures on the main branch was accidentally and silently disabled while moving from cirrus-ci to GHA.

So fix that by re-enabling it.

Also, contains a few other lint cleanup commits.