💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2183756175)
Rebased and fixed the previous CI error. But now CI is showing a different error which is unrelated to my current change.

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2183768740)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768

> [04e51ba](https://github.com/bitcoin/bitcoin/commit/04e51ba1110e13598e0b11a35ce5abf4a1789f53): now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.

Good idea. Latest push now integrates the release notes from the other PR and describes the changes as a whole

🤔 ryanofsky reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2984719522)
Thanks for the reviews!

Updated 705791cd436f237fe9bbac2cf52d63ab4b2a41c7 -> 05de8bc1735accc494291e22da718bebab163547 ([`pr/libexec.7`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.7) -> [`pr/libexec.8`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.7..pr/libexec.8)) updating commit message to match release description, and implementing Sjors `bitcoin` help suggestion https://github.com/bitcoin/bitcoin/p
...

✅ achow101 closed a pull request: "descriptors: Allow `H` as a hardened indicator"
(https://github.com/bitcoin/bitcoin/pull/32788)

💬 achow101 commented on pull request "descriptors: Allow `H` as a hardened indicator":
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-3033825918)
https://github.com/bitcoin/bips/pull/1888 was merged so this is no longer relevant.

💬 achow101 commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-3033841061)
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702

🚀 achow101 merged a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307)

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183868035)
Thanks for this suggestion, I've implemented this approach in https://github.com/bitcoin/bitcoin/pull/32273/commits/55e60bd9c76ea74a108cd5c7275e26554b822b4c, and I've reused `migrate_and_get_rpc()` in the other tests added here (except [`test_failed_migration_cleanup_relative_path()`](https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L1038) where it's not feasible)

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183873334)
As suggested by @furszy [above](https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274) I moved this logic to `migrate_and_get_rpc()` in https://github.com/bitcoin/bitcoin/commit/55e60bd9c76ea74a108cd5c7275e26554b822b4c:

https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L125-L147

And I've reused `migrate_and_get_rpc()` in the tests added in this PR.

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3033891866)
> I don't think belt and suspenders checks are a great idea in general, but even when they are appropriate I think they need to be labeled with "// This condition is never expected to trigger" or similar comments to avoid confusing people reading the code and trying understand why the checks are there.

OK, in lieu of an assert/assume, I've gone to the simplified form suggested, and I've also used the lambda+ternary over if+else form as you suggested. I've also taken advantage of the fact that
...

💬 achow101 commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3033949761)
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12

✅ achow101 closed an issue: "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix [AssertionError: Estimated fee (0.00923427) out of range]"
(https://github.com/bitcoin/bitcoin/issues/32461)

🚀 achow101 merged a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463)

💬 furszy commented on pull request "fees: prevent redundant estimates flushes":
(https://github.com/bitcoin/bitcoin/pull/32748#discussion_r2183951988)
could write this in a single line instead.
```c++
if (WITH_LOCK(m_cs_fee_estimator, return !MaxUsableEstimate())) return;
```

Still, it would be nice to explain why `MaxUsableEstimate` returning 0 means "nothing to do". It is not immediately clear to me without checking the `MaxUsableEstimate` internals.

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2183963160)
```suggestion
assert {"height": 13, "hash": block.hash, "branchlen": 1, "status": "headers-only"} in self.nodes[1].getchaintips()
```

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2183963930)
nit:

```suggestion
node.bumpmocktime(200 * 60 * 2)
```

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183965661)
In 146b133697f07c8dda004d60087505b2e1ec14b9 "test: Migration of a wallet ending in `/`"

Could add a check for the expected backup path.

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183964782)
In e7fec273593d825e0e7dbec46a026aacc58c479f "test: Migration fail recovery w/ `../` in path"

I think `failed_watchonly` is the wrong wallet name. Probably best to do `f"{wallet_name}_watchonly"`

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183965792)
In 9fa5480fc4c46fa11345c45fbe4dd21c127e3e05 "test: Migration of a wallet ending in `../`"

Could add a check for the expected backup path.

💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3034004122)
Maybe a dumb question, but why not relax the other side here? I don't know what the new criteria would be, but in an ideal world, nodes should not be sending block announcements for blocks they have identified as invalid just to avoid upsetting their peers, so maybe the constraints set by `PeerManagerImpl::SendMessages()` are too rigid?

https://github.com/bitcoin/bitcoin/blob/e3f416dbf7633b2fb19c933e5508bd231cc7e9cf/src/net_processing.cpp#L5865-L5873