💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052664896)
nit: maybe the following would be simpler?
```c++
for (const auto& [k, v] : m_map) {
out += k + ": " + v + "\r\n";
}
```

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052668744)
Yes, that is the gist of it. Will add a commit at the end to add some documentation in the header.

🤔 shahsb reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2781713855)
Concept ACK

💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818997734)
@sipa Addressed

👍 ryanofsky approved a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2781756291)
Code review ACK 6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97. Main change is expansion of test to cover restoring from the backup.

IIUC, PR description seems helpful for describing part of the problem, but contains inaccuracies and would be good to revise to avoid being misleading. Here are things I might add or correct:

1. PR describes there being a problems with relative paths, but not all relative paths have a problem, and absolute paths have problems too. Things only currently work correct
...

🤔 janb84 reviewed a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2781803719)
Re ACK [e3d7533](https://github.com/bitcoin/bitcoin/pull/31640/commits/e3d7533ac954516d87a09b230c2898f5c9b1e213)

Changes since last ACK:
- code cleanup

💬 darosior commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052733269)
I think it would be clearer to take a vector of spent `Coin`s directly? Or even a span?

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052746710)
So i looked into this and i don't think there is a clean way of doing it that would help here. What this function is doing is really a hack, creating a block assembler for a specific tip and mining a block with `prev` that does not correspond to it. However one thing is the `nSequence` doesn't change from one block to another. I added here for clarity but i now think it's unnecessarily confusing, so i removed it.

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2781869977)
Code review ACK 9e0fede6a6d608ff307b48b9a326c6d467b410cf. Main change since last review were a lot of `feature_framework_startup_failures.py` cleanups. I do think the drop or squash commit 9e0fede6a6d608ff307b48b9a326c6d467b410cf is an improvement so would vote for squash, but no strong opinion

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052772059)
In commit "Drop or Squash? - Break out startup failures into distinct classes" (9e0fede6a6d608ff307b48b9a326c6d467b410cf)

I think could also simplify the `sys.modules` code and write `globals()[class_name](__file__).main()`

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052783821)
It's only related to the coinbase nSequence/nLockTime change insofar as those change the txid of the coinbase transactions and therefore of all transactions in the test (and all other tests, but this test is one of those tests that implicitly relied on specific txid values).

This test is exercising the error returned by `CheckConflictTopology`, which iterates through the set of conflicts: https://github.com/bitcoin/bitcoin/blob/d91a746815e4428c738f1a096a950292cbdb5469/src/txmempool.cpp#L1277
...

⚠️ Aria-saeid opened an issue: "Better planning"
(https://github.com/bitcoin/bitcoin/issues/32319)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Frequent rotation in the loop

### Expected behaviour

Restoring my information
1125111712317

### Steps to reproduce

I propose the solution and according to the principle
1125111712317

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Other

### What version of Bitcoin Core are you using?

match masters

### Operating system and version

windowes10

### Mac
...

✅ willcl-ark closed an issue: "Better planning"
(https://github.com/bitcoin/bitcoin/issues/32319)

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052793660)
I added the comment as you requested above (https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2018964740), hopefully this makes it clearer.

I don't think there should be a startup flag since i don't see a reason for disabling it: this should not affect miners in any way.

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2781901294)
Code review ACK 067acb5907b5b135cff702da13a1f64a02d64110. No changes since previous review other than rebasing

💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2052789582)
In commit "rpc: add optional blockhash to waitfornewblock" (0625f26b09084fcbd4daa48b3456f54eff6b22de)

Do we think "Abort if RPC came out of warmup too early." comment is still accurate? I've having trouble understanding what it could mean and I don't think there are cases where this aborted even before this change. Would probably suggest dropping this comment or trying to make it clearer.

Also could replace /** with a normal comment above since /** normally indicates a doxygen comment.

💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2052792825)
In commit "rpc: unhide waitfor{block,newblock,blockheight}" (067acb5907b5b135cff702da13a1f64a02d64110)

Maybe s/takes/now takes/ to be clear this is a change

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052795436)
Added a comment.

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052795779)
Added a comment.

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052796088)
Leaving unresolved as it may be useful to other reviewers.