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

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906989778)
I thought about generating a fake testnet4 with empty blocks, but that wouldn't be much smaller. However if testnet5 turns out to have large initial blocks, we can always do that.

Headers are not enough, because I need the tip to update. Unless you want to revive #9483 :-)