💬 maflcko commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2097722076)
not sure about forcing an array, when all tests mostly just want to provide a single dummy value.

it would be good to make this optional and then, require one of `output` or `outputs` to be present, or maybe even fallback to `OP_TRUE` if none are given?

👍 luke-jr approved a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-2853787748)
utACK main commit. Not sure why the benchmark is being changed.

💬 luke-jr commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097724115)
Wouldn't it be better to benchmark the less-performant input?

💬 luke-jr commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097711372)
This assumes `size_t` is `unsigned long long` I think? Can we avoid that easily?

💬 luke-jr commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097725346)
How does this avoid optimisations?

💬 hebasto commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#discussion_r2097735642)
Perhaps include a comment explaining why `MKDIRPROG` being added?

💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2097740305)
> Can we maybe check the codepage early at startup (e.g. `SetupEnvironment()`) and fail if it isn't correct? This seems more thorough than any version check.

Thanks! Done.

🤔 hodlinator reviewed a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2853339502)
Code review 9057ab8b80137a89df68a3ed4bc49f9926e0f634

I like how the `AutoFile` change now is broken out into a 2nd commit.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097443271)
nit: Could leave comment justifying scope:
```suggestion
} // Make sure BufferedWriter flushes pending data to file.
```

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097483766)
Why not use this pattern in all fuzz-tests?
```suggestion
assert(auto_file.fclose() == 0);
```

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

Instead of needing to comb through each line in this change and in future modifications after this PR, I think calling a lambda from the destructor on failure is marginally safer, as suggested in https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2288411339 (but with more correct `errno` handling + maybe requiring the lambda be `noexcept`).

Requiring a lambda for the destructor should at the very least be
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097451869)
What do you think about `flatfile.cpp`? It has a bunch of unchecked `fclose`s.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097414194)
nit: Would be nice if `WriteUTXOSnapshot()` took `AutoFile&& afile` to signify a transfer of ownership.

<details><summary>diff</summary>

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -83,7 +83,7 @@ UniValue Wri
...

🤔 luke-jr reviewed a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501#pullrequestreview-2853851501)
Concept NACK, but open to being convinced otherwise. Not seeing any benefit to this over batching. At a minimum, it should keep backward compatibility.

✅ maflcko closed a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501)

💬 maflcko commented on pull request "RPC: removeprunedfunds should take an array of txids":
(https://github.com/bitcoin/bitcoin/pull/32501#issuecomment-2894196322)
Closing for now. I don't think there is any need in opening a pull request that is just a previously closed pull request with:

* the original author stripped,
* no reference to the previously closed pull,
* failing tests,
* no feedback addressed.

Contributions are welcome. Just make sure to run the tests before opening a pull request. Also, pre-existing review feedback would be good to address before opening a pull request.

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2894231304)
Rebased and taken @davidgumberg's great suggestion.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097845078)
Done, thanks

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097865341)
`xor_bytes_reference` randomly generates where the next offset will be, check it by adding:
```C++
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
{
if (!*this) return;
const uint64_t rot_key{m_rotations[key_offset_bytes % SIZE_BYTES]}; // Continue obfuscation from where we left off
std::cout << "key_offset_bytes % SIZE_BYTES = " << (key_offset_bytes % SIZE_BYTES) << std::endl;
```
which reveals:
```
key_offset_bytes
...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097879589)
I don't think so, if this isn't true, we're in trouble - so it has to be the case in prod as well.
The godbold reproducer indicates this line will be eliminated in prod if all calls are demonstrably so already.
I think I had a bug here at one point so added this line to make sure it's easily detectable.