💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622933175)
typo: instead

💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1623132784)
Why call `PruneBlockIndexCandidates` twice instead of moving it here?

💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622932918)
typo: replace

💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632134959)
If I am understanding this logic correctly, if `m_deterministic_prng.has_value()` is true then MixExtract in SeedStrengthen will never use m_counter or m_state to do MixExtract?

Strengthen is going to make the output of this call non-deterministic anyways, so what value does `allow_deterministic=true` as an argument provide in MixExtract? Shouldn't this be false?

💬 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