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

💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2894299769)
What I don't understand is why the CentOS task seemingly passed before, but started failing after https://github.com/bitcoin/bitcoin/commit/af65fd1a333011137dafd3df9a51704fd319feb4, even though the task wasn't modified apart from a comment.

📝 maflcko reopened a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)
pls ignore this is just a #32524 playground

💬 maflcko commented on pull request "rev_32343_wip_nomerge_ci":
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2894301545)
(take 2 :sob: )

🤔 hebasto reviewed a pull request: "depends: use "mkdir -p" when installing xproto"
(https://github.com/bitcoin/bitcoin/pull/32568#pullrequestreview-2854071175)
> It looks like the mkdir detection in xproto is broken on Alpine.

Yeah, it seems [so](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15137882165/job/42553939523):
```
checking for a thread-safe mkdir -p... ./install-sh -c -d
```

In that case, why not guide all build scripts with `$($(package)_autoconf) MKDIR_P="mkdir -p"`?

💬 fanquake commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894311549)
> In that case, why not guide all build scripts with $($(package)_autoconf) MKDIR_P="mkdir -p"?

Not sure what you mean, but I'm not planning on changing things globally to work around a single broken package on a single OS.

💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097903049)
I wanted to avoid long-range estimates based on block height, due to `dTxRate` being based on time (number per second), but I guess everything is an estimate anyway and `dTxRate` could be changed to be a number of txs per block if it mattered.

However, I don't see the benefit of your suggestion. If miner-set timestamps are off long-range, this debug stat is the least thing to worry about. Also, the fallback time-based code will have to be kept for IBD-catch-up scenarios.