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

I don't actually see why this check (introduced in #21009) is necessary, and think it would be better to drop. Looking at NeedsRedownload() definition it seems like it will already return false if chainstates are empty, so it should be ok to skip.

If this check can be dropped it looks like a number of other simplifications are possible: BlockManager::m_opts would not need to be public, s
...

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