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

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908515569)
It seems wrong to me to attempt using the RPC connection in `stop_node()` when we known that `self.rpc_connected == False`. New version has:
```Python
if self.rpc_connected:
# Call stop-RPC...
elif force:
# Kill etc...
else:
raise RuntimeError(f"Cannot call stop-RPC as we don't have an RPC connection to process {self.process.pid}.")
```

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909135121)
Went for option 3, making clear in the log message just above that a WARNING is expected.

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908463115)
Thanks for pointing out the race condition! It is not so likely if we get here because of RPC connection timeout, but still possible.

I tried adding a second `.kill()` after the first one and it does not raise an exception, and no exception is [documented](https://docs.python.org/3/library/subprocess.html#subprocess.Popen.kill).

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909136687)
Added *test/functional/feature_framework_rpc_failure_details.py*.