🤔 tdb3 reviewed a pull request: "rpc: fix mintime field testnet4, expand timewarp attack test"
(https://github.com/bitcoin/bitcoin/pull/31600#pullrequestreview-2535775301)
Concept ACK

Planning to circle back and want to make sure #30941 isn't applicable anymore before closing.

💬 tdb3 commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1906362322)
Could re-use `block` instead of doing another copy?

💬 tdb3 commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1906338612)
`future - MAX_TIMEWARP - 1` is just `t`, right? (`future = t + MAX_TIMEWARP + 1`).
Would this be the same logical case as in lines 185-187?

💬 tdb3 commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1906299046)
If retouching: `Witout` --> `Without`

💬 luke-jr commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2576711226)
As an example, it might be nice if it "just worked" when the user tries it. Maybe just pick a random txid out of the current mempool?

💬 luke-jr commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2576715764)
Why would this be NetBSD-specific? Shouldn't GCC 14 have the same warning/error on every platform?

👍 luke-jr approved a pull request: "test: expect that files may disappear from /proc/PID/fd/"
(https://github.com/bitcoin/bitcoin/pull/31614#pullrequestreview-2535965972)
crACK

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