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

💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2819162272)
@furszy has uncovered that it seems like migrating legacy to descriptors also solves the problem. Since removal of the legacy wallet is close, I will look into this a bit further and probably turn this PR into just a test.

📝 achow101 converted_to_draft a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)
A bug in 0.21.x and 22.x resulted in some wallets storing invalid derivation paths that are the concatenation of two derivation paths. This corruption follows a rigid pattern and can be easily detected and repaired.

The bug that resulted in such wallets was fixed in #23304 but wallets that already ran into it would still have bad data stored. This PR should fix such wallets.

Fixes #29109

💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819163619)
reACK e3d7533ac954516d87a09b230c2898f5c9b1e213

👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2781932567)
Code review ACK 341b2a99e646246a9e4d3aa283b2b59442cfad27. Only change since last review is making various improvements to comments and rebasing.

Would be nice to update doc/dependencies.md here since #32293 is no longer doing that but not important.

👍 ryanofsky approved a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2781952277)
Code review ACK 0ac19a98f329fe8952f394509a7a29775ab805b0

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2052821662)
In commit "doc: warn that CheckBlock() underestimates sigops" (0ac19a98f329fe8952f394509a7a29775ab805b0)

Might be good to say why this check is useful if it's not a problem for it to be inaccurate.

Might also be good if PR description mentioned motivation for the change. This all looks ok but it's a bit confusing because it's not clear what the context is.

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2819200264)
Do people think i should change the value of the coinbase transaction used in the fuzz test here as well? I previously kept it along with the PR introducing validation for those fields, but since i change this in unit tests already it might make sense to do it here.

💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819202269)
utACK e3d7533ac954516d87a09b230c2898f5c9b1e213

🚀 glozow merged a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640)