💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906691328)
nit: Not that it matters for performance, but for documentation, it would be good to limit cs_main to the only call that needs it.

```suggestion

const CBlockIndex& tip{*WITH_LOCK(chainman.GetMutex(), return CHECK_NONFATAL(chainman.ActiveChain().Tip()))};
```

(same above, ...)

💬 maflcko commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2577032254)
Thanks for reproducing! It would be good to share the backtrace. I am still wondering if this is a GUI issue or a wallet issue.

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2577149570)
It is already part of https://github.com/bitcoin/bitcoin/pull/31519/commits and I am not sure if it is relevant enough to be split up

👍 hodlinator approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2536582644)
re-ACK 352391c2cf1a45231ae92ca92d2415b3786ab9ad

Confirmed through range-diff to only be comment changes following my [previous feedback](https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2533921553).

💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2577159948)
If other reviewers think it's relevant enough we can simplify that draft PR of yours to focus on Spans.

👍 hodlinator approved a pull request: "fuzz: Abort when global PRNG is used before SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31548#pullrequestreview-2536596842)
ACK fa7267022e8992f3db8eb7c8e81563c29294c19b

Good that you also replaced the defunct comment in *fuzz.cpp* with a useful one.

Could extend this to also catch too early uses of randomness when they occur in `GetRandBytes`, but `g_rng_temp_path_init` makes that a bit cumbersome.

#### Tested

Removing `g_used_g_prng = false` in `g_rng_temp_path_init` - triggered expected assert.

Reverted fadd568931a2d21e0f80e1efaf2281f5164fa20e as suggested - again triggering expected assert.

💬 hodlinator commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1906858617)
Just curious - why `inline` rather than `static`?

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2577307694)
I made a note in #31283 to look into both suggestions.

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2577312996)
I didn't know about #30941. Let me know if there's anything there that you'd like me to add here.

📝 maflcko opened a pull request: " test: Remove --noshutdown flag, Tidy startup failures"
(https://github.com/bitcoin/bitcoin/pull/31620)
The `--noshutdown` flag is brittle, confusing, and redundant:

* Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with `--pdbonfailure`. If there was a use case to replicate `--pdbonfailure` without starting pdb, a dedicated flag could be added for that use case.
* It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perfo
...

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1906960908)
This test is supposed to illustrate the highest timestamp at which you trigger the timewarp error, and then the next one shows the first timestamp where you don't.

Lines 185-187 illustrate what happens if a miner just uses their clock, which doesn't have to be an edge case.

The difference would probably be more clear if I picked a higher number for future.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1906965234)
With #31003 this would be logged every second.

💬 hodlinator commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2577338490)
3% speedup is less to write home about but still good. Having a less constrained ([Sjors](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2575279833)) -dbcache setting (30GB) would lead to less XOR read/writes.

12.3% speedup ([l0rinc](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)) could at least in part be explained by the 1GB setting, leading to more XOR-operations.

An optimization that provides a bigger win for constrained devices should be welcomed.

👍 Sjors approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2536792956)
ACK 7452638559e8ccbe00db5ef5a53cb21ffe27d337

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1906977462)
You lost a line here:

```md
- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
```

💬 maflcko commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2577351760)
I've created https://github.com/bitcoin/bitcoin/pull/31620 to reduce the long, redundant and confusing error message observed originally a bit.

But the two changes are unrelated otherwise.

lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

💬 maflcko commented on pull request "test: Remove --noshutdown flag, Tidy startup failures":
(https://github.com/bitcoin/bitcoin/pull/31620#issuecomment-2577352102)
For testing the third commit different ways are possible. For example:

```
$ rm -rf ./releases
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
```

Then, observing a shorter error message.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906980852)
For the web developer in me that's obvious :-)

But I'll add `.json` to make it more clear this doesn't apply to the other format.

💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1906984997)
I think anything is fine that documents that this function is local to this module.

💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577366143)
> Could extend this to also catch too early uses of randomness when they occur in `GetRandBytes`, but `g_rng_temp_path_init` makes that a bit cumbersome.

That is what this pull is trying to do. Did I miss something?