👍 hebasto approved a pull request: "random: clarify where the environ extern is needed"
(https://github.com/bitcoin/bitcoin/pull/33714#pullrequestreview-3384482126)
ACK f24ef2b4e01884ac1dea07ebbc7e7d0aa2c09541.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2466434070)
Yes, nice find!

⚠️ torkelrogstad opened an issue: "Failure to bind to ZMQ addresses is swallowed in debug logs"
(https://github.com/bitcoin/bitcoin/issues/33715)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

When failing to bind to specified ZMQ addresses due to them already being in use, the error is completely swallowed up when not running with debug logs.

### Expected behaviour

I'd expect one of two things to happen:

1. Bitcoin Core exiting with a non-zero status code and a clear message saying what happened (I'd prefer this)
2. An error message in the logs, that is visible without havi
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2466463098)
Interesting how I didn't get it but I think this is related to https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2466310562. The check was wrong so it was playing with not-fully-valid keys.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3452454600)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2466310562. Thanks, @marcofleon.

💬 ismaelsadeeq commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#issuecomment-3452529222)
C.I is falling after a rebase due to merge of https://github.com/bitcoin/bitcoin/pull/33210
I have to update `FeeEstimatorTestingSetup`, I did that locally, but will not push, because I am working some minor changes.

https://github.com/bitcoin/bitcoin/pull/33218 is not affected by the planned change at all.

💬 achow101 commented on pull request "Permit Combiner to strip bip32_deriv information":
(https://github.com/bitcoin/bitcoin/pull/30341#issuecomment-3452555077)
Approach NACK

I don't think it's up to the combiner to strip this information. If a signer wants to preserve their own privacy, then they should remove it prior to sending the PSBT to any of their counterparties, or the combiner. I think it would be better to have the strip functionality be in `walletprocesspsbt`, and signers can pass their PSBT back through that if they want to strip the derivation paths.

💬 achow101 commented on pull request "doc: corrected lockunspent rpc quoting":
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-3452574855)
ACK 7e93e2925981c78c17a3deb2f6e6a16fa56730c4

💬 achow101 commented on issue "RFC: "Insufficient review" tag for closed PRs":
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-3452587730)
I've added the label.

💬 achow101 commented on issue "psbt: set global_xpubs (at least for multisig descriptors)":
(https://github.com/bitcoin/bitcoin/issues/27583#issuecomment-3452590310)
This is more reasonable to do now that legacy wallets are no longer supported.

💬 achow101 commented on issue "add ability to remove imported public keys/addresses (or even privkeys)":
(https://github.com/bitcoin/bitcoin/issues/23765#issuecomment-3452606713)
With the removal of the legacy wallet, this would be reasonable to do for watch-only wallets only. However, we should not have anything that can remove private keys from a wallet since that can lead to a loss of funds.

Additionally, with descriptor wallets, a watch-only wallet without the imported descriptor (for the address in question) can be created by using `listdescriptors`, removing the descriptor from the result, and `importdescriptors` into a new watch-only wallet.

✅ waketraindev closed an issue: "qt: rpc console, filtered commands replaced with '(…)' may execute unintended actions when recalled from history"
(https://github.com/bitcoin-core/gui/issues/907)

👍 ryanofsky approved a pull request: "validation: invalid block handling followups"
(https://github.com/bitcoin/bitcoin/pull/32843#pullrequestreview-3384601373)
Code review ACK e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c. Thanks for these followups, I think they should make it easier to remember what this code is trying to do.

💬 ryanofsky commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2466535451)
In commit "doc: Improve doc for candidate blocks" (e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c)

Note for reviewers: best description I've seen for why it's not currently safe to assume `m_best_header` is consistent with CBlockIndexWorkComparator ordering is here https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2129780775

💬 ryanofsky commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2466511097)
In commit "doc: Improve doc for candidate blocks" (e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c)

Would maybe s/nChainWork/only nChainWork/ since using nChainWork shouldn't be surprising here, but *only* using nChainWork might be.

💬 ryanofsky commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2466449213)
In commit "validation: Don't repeat work in InvalidateBlock" (30b190eebab9df9273586e0a4b6462847883d9d1)

I tend to disagree and think the code is clearer with this change than without it. Without it, intent is not possible to discern. The code is doing the same work twice without an indication of why it is doing that. With this change, behavior actually makes sense and intent is clear.

I do agree code could be reorganized better and made less fragile, but I don't see the new code here as be
...

💬 ryanofsky commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2466566211)
In commit "doc: Improve doc for candidate blocks" (e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c)

To be complete, this could also mention that when there is a tie, this will set `m_best_header` to whichever block happens to come first in `m_block_index` `std::unordered_map` iteration order. This ordering is consistent with the ordering used in `RecalculateBestHeader`, but inconsistent with the ordering used in `AddToBlockIndex`, which is based on the order headers are received.

I'm not sure how
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3452644870)
There was one more case that I forgot to count in. We should consider the split chain support when verifying the result (`desc_spkms`). Force-pushed fixing it ([6ced804](https://github.com/bitcoin/bitcoin/commit/6ced804488d9eca4b3da8daa4e8c9145109b3384) to [6436ad7](https://github.com/bitcoin/bitcoin/commit/6436ad7f2f93774cb719c89965800b51f546420c)).

💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2466642167)
Done

💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2466642445)
Done