💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102112756)
Note that this here is in a bench test where we operate on a real file and an error to close it means something really went wrong badly, e.g. a disk IO error. Such a condition should stop the test with a failure.

In contrast, in fuzz tests some of the "files" we operate on are fuzzed files which mean they are not really backed on disk and their operations are mocked to sometimes return an error, e.g. to simulate a broken disk. Those mocked errors are to be expected and should not make the tes
...

🤔 Sjors reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2860429843)
Some more questions about 35db4f2dcfc3435e10935581ffa447ffe219cc1e.

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2102121605)
In 35db4f2dcfc3435e10935581ffa447ffe219cc1e "descriptor: Add MuSigPubkeyProvider": could avoid old school `for i`, make it slightly shorter and have the intention be more clear:

```diff
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -691,9 +691,9 @@ public:
std::string ToString(StringType type=StringType::PUBLIC) const override
{
std::string out = "musig(";
- for (size_t i = 0; i < m_participants.size(); ++i) {
- const auto& pub
...

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2102052043)
In 35db4f2dcfc3435e10935581ffa447ffe219cc1e "descriptor: Add MuSigPubkeyProvider": no tests break if I delete this loop. What's the typical use case for `Clone()`? Maybe `Check()` in `descriptor_tests.cpp` should do this for all valid descriptors?

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2102178194)
In 35db4f2dcfc3435e10935581ffa447ffe219cc1e "descriptor: Add MuSigPubkeyProvider": just to check my own understanding, by ranged participants you mean the case of `musig(alice/*, bob)`, i.e. derivation before aggregation?

I assume that if the participants are ranged, then you can't also have a range after derivation, e.g. `musig(alice/*, bob)/*` isn't allowed. Where do we check this?

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102187863)
Fuzzed `FILE*`, which comes from `fuzzed_file_provider.open()` is expected to have a failing `fclose()`. Should not fail the test for that, I replied there: https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102112756

💬 hebasto commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2900712416)
Concept ACK.

💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2900733155)
> What is the longer term plan here?

The initial plan was to maintain our own code independently of the upstream repository. However, there have been a few requests to upstream our changes. At this point, I still believe we shouldn't concern ourselves with upstream and should instead take full advantage of C++20.

💬 fanquake commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2900742217)
Guix Build:
```bash
d9e24e61f22de8b9d83cbbec4830bdb9192ab55ac90aba125a53ec9fba719d67 guix-build-6b4bcc162345/output/aarch64-linux-gnu/SHA256SUMS.part
9780961f5ce0c23db0f6ec4b9930728344a3cb4706e4a36dff2364f12905acf8 guix-build-6b4bcc162345/output/aarch64-linux-gnu/bitcoin-6b4bcc162345-aarch64-linux-gnu-debug.tar.gz
2f281a8e266bab0631e40a738db21a4f4865aa2e91e9dfadc2d208b05d5a2f8b guix-build-6b4bcc162345/output/aarch64-linux-gnu/bitcoin-6b4bcc162345-aarch64-linux-gnu.tar.gz
1639f2ab6e353d5a
...

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2900799235)
`9057ab8b80...c7b68dceb2`: address suggestions

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102277623)
Done, patch taken. Thanks!

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102278565)
Done.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102279134)
Yes. Added one more commit to handle those.

🚀 fanquake merged a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32292)

👍 fanquake approved a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2860814033)
ACK 6b4bcc16234575108bb691c15c3532198d9bf98a

✅ fanquake closed an issue: "Use of deprecated CryptAcquireContext in random.cpp"
(https://github.com/bitcoin/bitcoin/issues/32391)

🚀 fanquake merged a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400)

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102300215)
> This throw would trigger the Assume() in ~AutoFile() to fail, no?

Yes. Added a `fclose()` call before the `throw` to address that.

I will consider the lambda in the destructor again. From the `fclose()` callers, I counted:

* 7 that ignore the `fclose()` return value
* 7 that log message + take a different code path
* 4 throw

Taking a different code path upon `fclose()` failure would be tricky when that is called from the `AutoFile` destructor. It would require code like:

```cp
...

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102306083)
> The only tool we need is msbuild.exe which is what my original version did?

And we do use it—along with the VCToolsVersion environment variable—but only to determine whether the vcpkg binary cache needs to be invalidated. CMake is smart enough to inspect the build environment and locate the required tools on its own.

We might be able to use other indicators to invalidate the cache at the right time. Perhaps the top commit hash of vcpkg?

💬 TheCharlatan commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2900897714)
Concept ACK