📝 instagibbs opened a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628)
Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1909039107)
You're right, I suggested the `+ 1min` back when I also thought that the range is in minutes only.

👍 l0rinc approved a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2540272419)
ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1909042923)
It's not a max now (i.e. not a closed range), but a threshold, but the difference may not be important anymore

💬 ryanofsky commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909056496)
re: https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760710

> I made it into a `unsigned int` (I don't particularly like it, but it's not that important).

Thanks, your change makes sense and to be clear I don't like code writing `unsigned int` everywhere either, even though I do think `unsigned int` is a reasonable type for this code to be using. I think more ideally src/flatfile.h would define something like:

```c++
using FilePos = unsigned int;
```

and then code cou
...

👍 ryanofsky approved a pull request: "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2540290494)
Code review ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.

🤔 stickies-v reviewed a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2540319978)
Approach ACK fa6d9a29162fb508385201c926ec745d071086fc. Apologies for the slow re-review, but I like the latest form of this PR much more than the previous one I looked at, nice! I'll complete my full re-review in the next couple of days.

💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1909076664)
nit: `this supports std::string for runtime string` - perhaps worth updating to reflect that it now requires to be wrapped in `RuntimeFormat`?

💬 ryanofsky commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909082218)
In commit "bench: add SaveBlockBench" (86b85bb11f8999eb59e34bd026b0791dc866f2eb)

Could rename SaveBlock to WriteBlock here too

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1909088406)
I was hoping that "runtime string formatting" is clear enough to show the connection to `RuntimeFormat` and then looking at the `RuntimeFormat` constructor, one can see `std::string`. So I didn't change it, but I am happy to take suggestions.

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909093808)
Right, if I need to edit, I'll rename this as well

💬 luke-jr commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2580718388)
That's why I was suggesting pulling a txid from the current mempool

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580767132)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

`uint8_t` and `unsigned char` are the exact same type. If not, the compilation would fail in other places. I don't think it is helpful to create 2432 pull requests to change each instance one-by-one in a separate pull request. If this change i
...

💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580783610)
> As I said in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742 this seems like an optimization that can wait.

Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.

👍 ryanofsky approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2540386980)
Code review ACK 384c73d4fcda4af7c2b4bfb945a248cb93dba47d but it seems like a fuzz check is failing in CI. Since last review, BlockManagerOpts uses DBParams, so no longer needs to duplicate most of its fields, and blockstorage m_opts no longer need to be public so some other changes could be reverted. One other set of changes that were reverted had do to with `m_block_tree_db_in_memory` / `opts.block_tree_db_in_memory;` which seems more correct now.

💬 ryanofsky commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909120127)
In commit "refactor: Remove redundant reindex check" (fd590d5ca1cfb6c2525b7755edb6a3a2cf16c856)

Curious about something: In https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2397922175 you wrote,

> Yes, my eventual goal was to move calling `InitializeChainstate` for the fully validated chainstate into the `ChainstateManager` constructor as well as calling `InitCoinsDB` for it. I wanted to do this in a future pull request if this change is accepted.

So if coins db were intialize
...

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

Looks like indentation unintentionally changed here

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909147382)
Taken, although I moved some of the handling of `expected_ret_code` directly into `is_node_stopped()`.

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909144554)
> I mostly found the previous comments easier to follow because they were more direct and interspersed with relevant code.

Hopefully this is from the latest push can be considered an improvement:
```python
if e.errno not in [ errno.ECONNRESET, # This might happen when the RPC server is in warmup,
# but shut down before the call to getblockcount succeeds.
errno.ETIMEDOUT, # Treat identical to ECONNRESET

...

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909141475)
New version:
```python
# Suppress if cookie file isn't generated yet and no rpcuser or rpcpassword; bitcoind may be starting.
```