💬 ryanofsky commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1906052767)
In commit "kernel: Move block tree db open to block manager" (182945e946ac6a92daad012d6579e5441b9641cc)

I don't think this line should be dropped. It seems like potentially slow loading steps do happen before this `CompleteChainstateInitialization` call, so it's good to check if a shutdown was requested before proceeding.

💬 ryanofsky commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1906073567)
In commit "kernel: Move block tree db open to block manager" (182945e946ac6a92daad012d6579e5441b9641cc)

All of these new fields are just duplicating the fields of the existing `DBParams struct`, except for adding a `block_tree` to the name. Would be nice to replace them all with a single `DBParams block_db_params` field.

💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2576433431)
> Would it be possible to start the GUI with `-server=1` and then use one of the RPC threads to create more blocks while the GUI is blocked?

Good point, that worked! I could reproduce the crash in the GUI that way (with sleeps in `AttachChain()` and `blockConnected()` to avoid races) - I can confirm that it's fixed by the patch in the OP, whereas I could still reproduce it with #30221, so that PR apparently doesn't fix it.

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2535622350)
Code review re ACK 223e8bcb9990834a5c9ffcd8a79f617a6f2ecfc1

🤔 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, ...)