💬 kevkevinpal 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-2576244717)
ACK [1ea7e45](https://github.com/bitcoin/bitcoin/pull/31462/commits/1ea7e45a1f445d32a2b690d52befb2e63418653b)

💬 mzumsande commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1906056946)
I really wasn't sure what the best solution is when I opened the issue and still am not, but these are my thoughts:

Yes, there are two reasons why we might not apply assumevalid during a reindex currently:
1.) The best header has not enough chainwork
2.) The assumevalid block is not in our block index

> I could change this to always skip script checks during a reindex, but IIUC having a block on disk doesn't mean that it was connected during the previous IBD.

I think the question is
...

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906077380)
Something like this?

```c++
bool TxDownloadManagerImpl::DoOrphanThingRenameMe(const std::vector<Txid>& unique_parents, const Wtxid& wtxid, NodeId nodeid, std::chrono::microseconds now)
{
if (auto delay{OrphanResolutionCandidate(nodeid, wtxid, unique_parents.size())}) {
const auto& info = m_peer_info.at(nodeid).m_connection_info;
for (const auto& parent_txid : unique_parents) {
m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, n
...

💬 maflcko commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2576295985)
> With qt, how do I create more transactions while the wallet is loading? The GUI blocks for me until the wallet is loaded.

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?

💬 jonatack commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2576310502)
One idea could be to update `RPCExamples::ToDescriptionString` to take a `std::optional<string>` arg that would enable optionally printing something like the following, instead of just "Examples:"

```
Examples (replace the txid with a valid value):
> bitcoin-cli gettransaction "1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d"
```

👍 ryanofsky approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2535388835)
Code review ACK 182945e946ac6a92daad012d6579e5441b9641cc. I think a number of improvements are possible here (see comments below) but it does make sense to move block database initialization out of `src/node/chainstate.cpp` and into the `BlockManager` class, and move its options out of `ChainstateManager::Options` and into `BlockManager::Options`.

On constructor initialization question I do think it's often useful if lightweight construction with delayed initialization or lazy initialization
...

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

It seems far from ideal for bitcoin-chainstate and other kernel library users (not to mention our own test code) to need to hardcode the "blocks/index" path and to manually copy the cache size from one struct to another. I think a better API would allow these option fields to be unset and internally use a `Flatten` method similar to the ones in `validation.cpp` and `txmempool.cpp` to set ap
...

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