💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632132631)
As far as I can tell GetOSRand is unchanged in this PR. Why move below the `class RNGState` declaration?

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632131052)
Nit: Why not rand_exp_duration instead? It is one character shorter and `exp` reads more clearly to me as exponential than `expo`?

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632133956)
```suggestion
* If allow_deterministic is true, and MakeDeterministic has been called before, output
* from the deterministic PRNG.
```

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632130266)
Is the change from milliseconds to microseconds intentional?

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632133792)
This confused me because MixExtract is a deterministic function based on hasher, m_counter, and m_state. Unless I am mistaken the non-determinism comes from the calling function determining on the value of ret. Why not address strengthen ret value in the calling function rather than here?

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632135242)
Does `allow_deterministic=false` and `m_deterministic_prng.has_value()=true` imply that something has gone wrong? Should we log an error or take some action or are there cases where this is expected behavior?

💬 BenWestgate commented on issue "Pruning keeps getting reenabled":
(https://github.com/bitcoin-core/gui/issues/709#issuecomment-2156327310)
Hi hebasto, a work around would be to put a bitcoin.conf file in `/Volumes/Drive/Bitcoin` with `prune=0` BEFORE mounting your external drive, that way if it hasn't been mounted Bitcoin Core will not try to prune. This file will be buried and ignored after your external drive mounts.

Another work around would be adding `-prune=0` to whatever method you are launching Bitcoin, be that command line or a .desktop file.

The file that remembers where your custom datadir is `$HOME/.config/Bitcoin/
...

💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#discussion_r1632201184)
This is still needed. You didn't change the fuzz test runner invocation, only the functional test runner invocation.

💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#discussion_r1632203401)
You will have to remove this diff hunk, or apply the same approach to the fuzz test runner, as you did to the functional test runner.

💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2156377203)
> Passing `--exclude "rpc_bind.py --ipv6"` excludes not only the specified test but also `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback`.

This is the existing behavior on current master and this pull request. I don't think it was ever supported. Though, I guess this means this pull request isn't needed, then.

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204359)
Not sure about exposing this. This is an expert-only setting. Anyone changing it needs to understand exactly what they are doing, so they might as well modify the `ci/test/00_setup_env_native_asan.sh` config file manually.

I'd prefer to keep the `sed` approach as-is. If you want to change it, it may be better to do in a separate pull request. This way, the focus here is kept on only moving the task from one infra to the other.

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204875)
In theory, only the fuzz binary compilation could be excluded, because there is a separate task for the fuzz tests under asan. But either seems fine.

💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632205551)
Looks like this imported a broken wine 9 release candidate from `debian:trixie`?

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1632207682)
https://github.com/bitcoin/bitcoin/blob/5cc2ae4ff287c6fef10b2e634e35353588f9ea3d/contrib/verify-binaries/verify.py#L511-L515

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2156393347)
> tACK [8135632](https://github.com/bitcoin/bitcoin/commit/8135632c33f4bad8434403b5807c48d7dba3b3d7)
>
> Still one open comment regarding specifying bitcoincore.org, but happy to reACK if that gets changed, and it's not a blocker for me.

I wrote a better error message that gives a "Did you mean: <closest_match>" when the platform typed isn't in the SHA256SUMS. Avoiding the domain issue and being significantly more helpful. LMK if you like it enough to add to this PR.

💬 maflcko commented on pull request "ci: Native Windows CI job cleanup":
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2156398733)
ACK 0d3ef83433805d3f367130fd5bd227a8ed5a7ccd

💬 maflcko commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319)
> I'm not sure if I'm doing something wrong or if there is indeed a bug where the divergent chain is not rewound. I will continue investigating.

This sounds like a bug. Just to clarify, the active chain is stuck at height `298` and the background chain continues to sync past `399`?

💬 maflcko commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2156407545)
29996 doesn't seem optional, but possibly a blocker, see https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632217106)
I tested this myself, and the reason seems to be that absent a `HEAD` request, a 404.html would be downloaded as the targz, which then fails the shasum check.

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156409463)
> Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,

This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.