💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1761796887)
> > And, in fact, we could implement it using the wait_until function too
>
> Yes, I tried this and decided against it, see the description of the PR.

Ah, missed it. Sorry.

> > I’d suggest simply waiting for the entire duration without intermediate checks.
>
> I have also considered this and decided against it because I don't think it's better. I have also described this in the PR description already. Since the polling doesn't seem to lead to a real performance issue I think it's bet
...

✅ achow101 closed an issue: "Raise maximum -dbcache setting"
(https://github.com/bitcoin/bitcoin/issues/28249)

🚀 achow101 merged a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358)

🤔 jonatack reviewed a pull request: "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment"
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2307594542)
Approach ACK 321097a685b67f82c32f53652227b7177fe97a95 on first look

💬 jonatack commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1761770703)
perhaps more clear
```suggestion
//! best header is no longer valid nor guaranteed to be the most-work
```

💬 jonatack commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1761774657)
For references to cs_main, are we still generally preferring to use the scope resolution operator? (you do use it in line 1330)


```suggestion
void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
```

💬 jonatack commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1761767261)
"to a all" -> unclear what this means

💬 achow101 commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2353813095)
ACK 6a1aa510e31e8b77793341473aa5afc9d023a6e3

✅ achow101 closed an issue: "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)"
(https://github.com/bitcoin/bitcoin/issues/20978)

🚀 achow101 merged a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410)

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1761938767)
That makes sense, I didn't consider pipes/fifos.

I agree on this approach given that a bad `m_position` value is only an issue when XOR'ing, and your branch has checks for an unknown `m_position` before any XOR operations. And in general, it seems to me, `ftell`'s are followed by file operations that fail if the `ftell` fails for any reason other than the file being a pipe/fifo (`ESPIPE`).

I think ideally `errno` is checked here for values other than `ESPIPE`, but the current approach is
...

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2353995596)
Rebased after merge of #30286.

💬 jonatack commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1761946005)
`s/for a non-debug build by running build with/by building/`

💬 jonatack commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1761946108)
```suggestion
This can be checked by building the `translate` package with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.
```

(nano-nit, "cmake" is written without the backticks in other documents; could be consistent, also regarding referring to it as "cmake" or "CMake")

💬 achow101 commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2354004288)
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e

💬 jonatack commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1761947070)
```suggestion
print("Re-compile with the -DBUILD_DAEMON=ON build option")
```

🚀 achow101 merged a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436)

💬 petertodd commented on issue "Use IPv4-encoded IPv6 address to get IPv4 node address with port number from DNS seeds":
(https://github.com/bitcoin/bitcoin/issues/30900#issuecomment-2354009797)
NACK

The purpose of the DNS seeds is just to find some bootstrap nodes. More addresses are then learned from those bootstrap nodes, and additional connections are made to them. There's no need for the initial bootstrap nodes to be sampled from the entire set of possible nodes.

💬 furszy commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2354051227)
A bit delayed but here. Sorry.

About your fix, I'm not against it at all. Fully agree that is better than mine. Connecting the signal handler lifetime to the object that it deletes it is what we should be doing.

I think removing the wallet from the GUI early in the process is effective, but it doesn't handle failures well, as we'd need to reload all wallet views from scratch. That said, disabling all wallet actions in the GUI might be a bit tricky to implement with the current code. Still
...

🤔 jonatack reviewed a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2307843032)
(Happy to re-review if you squash the minor comments below into https://github.com/bitcoin/bitcoin/pull/30183.)