💬 l0rinc commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2576755745)
@jonatack, we could likely achieve something similar by adding a different hash for every example instead - the users should be able to extrapolate.

@luke-jr, I agree that the users should see a working example - I got this "random" txid from the source, see the commit message.

💬 luke-jr commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2576774599)
An old txid will only work with txindex enabled, though

💬 maflcko commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2576896113)
Wouldn't it be better to add the missing includes? Next to just removing bdb, of course.

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906669275)
nit: The function does not accept nullptr, so it would be good to document it properly.

```suggestion
void NextEmptyBlockIndex(const CBlockIndex& tip, const Consensus::Params& consensusParams, CBlockIndex& next_index)
{
CBlockHeader next_header{};
next_header.hashPrevBlock = tip.GetBlockHash();
```

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906667136)
Not sure about adding a file to the git repo whose worst case size would be 8GB (possibly for testnet5). Given that you only need the headers, it could make sense to mine a minimal chain and only commit the nonces. An alternative would be to use the main chain, which doesn't have to be changed again in the future.

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906681208)
The comment is wrong, because cs_main isn't needed to call a getter that returns a const reference to a const blob.

So it would be better to move the call out of cs_main to avoid that impression, and to remove the comment.

Also, instead of `chainman.GetParams().GetConsensus()`, you can simply call `chainman.GetConsensus()`.

(This comment applies to the whole diff)

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906669739)
nit: I don't think the comment (and maintenance overhead) should be added.

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906705481)
I don't think this is right. The `.json` suffix is missing? (It is not possible to return a "field" in the other formats)

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