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

💬 hebasto commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542936019)
> Yeah, if it is pulled in through LIEF, it could make sense to strip the unmaintained dependency from LIEF upstream.

FWIW:
```
# guix graph --path python-lief python-py-cpuinfo
python-lief@0.17.1
python-pydantic@2.10.4
python-pytest-benchmark@5.1.0
python-py-cpuinfo@9.0.0
```

(guix checked out at a different commit though)

🤔 l0rinc requested changes to a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3473454632)
Left a few more comments to help with moving this forward.
I like the new structure more and after the tests and the restructuring are finished, I would like to spend some more time reviewing the `ThreadPool` as well - but I want to make sure we have a solid foundation first.

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534665307)
Can you please apply this to the other cases as well?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534663536)
Can you please apply this to the other cases as well?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534655978)
Continuing the thread from https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477534828:

```patch
#include <common/system.h>
+#include <test/util/setup_common.h>
#include <util/string.h>
```

```suggestion
BOOST_CHECK_EXCEPTION(threadPool.Submit([]{ return false; }), std::runtime_error, HasReason{"No active workers; cannot accept new tasks"});
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534675487)
Continuing the discussion in https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477876362, could we randomize this on the call-site, so that we can exercise cases when we have other than 3 workers, i.e. in the test it would be something like:
```C++
const auto num_tasks{1 + m_rng.randrange<size_t>(20)};
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534738205)
The original failure looks like this:
> unknown location:0: fatal error: in "threadpool_tests/submit_tasks_complete_successfully": std::runtime_error: Timeout waiting for: test1 task, task index 0
>
>test/threadpool_tests.cpp:77: last checkpoint: "submit_tasks_complete_successfully" test entry

If you want the extra method to not mask the failure stack, we can make it a macro, like we did with e.g. https://github.com/bitcoin/bitcoin/blob/cc5dda1de333cf7aa10e2237ee2c9221f705dbd9/src/test/headers
...

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534746780)
I still see init + reserve + loop + emplace in most tests, could we apply to the rest as well?