💬 hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534494637)
Thanks! Pushed new version which is hopefully slightly more accurate.

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

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

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