💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2193184904)
Another rebase screwup, tried again... that should also fix the windows CI failure

🤔 janb84 reviewed a pull request: "test: less ambiguous error if bitcoind is missing"
(https://github.com/bitcoin/bitcoin/pull/32921#pullrequestreview-2998712380)
ACK 83bb41455715a9e05320ba791987204031626c10

PR changes error when no binaries are found that are necessary for the functional test(s). Old error gave the impression that previous binaries needed to be downloaded, new error (only) correctly state that the binaries are missing and gives an hint that one forgot to compile. This more correct and I agree with the change.

- code review ✅
- tested on Apple Silicon without #32922 (nix shell)

<details>

Master:

```shell
$ build/test
...

💬 maflcko commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049916511)
Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.

That is, old wallets will (obviously) continue to write their `CLIENT_VERSION` to the `version` record. Also, any new wallet will continue to write a value of the current `CLIENT_VERSION` to it, just like on the current development branch (this is unchanged as well). The only change is tha
...

💬 achow101 commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049933892)
> Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.

Sorry, I was responding to @ryanofsky's comment at the same time since they were very similar.

I think that may be workable.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193207335)
Done. I didn't really know the best name for this initially. I like `TxidOrWtxid` a bit better.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193209465)
Took this suggestion 🤝

💬 achow101 commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3049952776)
Reordered as suggested

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193216368)
I agree that there isn't a need for branching on wtxid/txid and that having the comment there is enough. Just to be sure, @glozow if you prefer to keep the branching, let me know and I can revert.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193217740)
Good catch, same for `ForgetTxHash`.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193219736)
Good looks

💬 luke-jr commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3050037676)
crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193283389)
Good point, I will add a test for this.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193284376)
Yes, I will look into implementing the separate size limit.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193285093)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193289150)
I'm not sure if that name would be accurate, because this boolean is set to `true` even when we are trying to create a v3 transaction, in which case we are technically not excluding version 3.

💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3050069477)
> maybe I'm missing something but even without the calc_flags_and_header=false flag, this means that [the condition](https://github.com/bitcoin/bitcoin/blob/a8bff38236ac988f82dcfa85438946cfe0d3afe3/src/validation.cpp#L2074) cannot hit true and RecalculateBestHeader() in InvalidChainFound() cannot be called right?

Yes, that's true, good point! To phrase it in my words, the condition that checks whether `m_best_header` needs update will not be fulfilled, because `m_best_header` was already succ
...

🤔 luke-jr requested changes to a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2998927241)
Concept ACK, but I'm not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it's not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353160)
I do agree, just seemed more "idiomatic" to handle the wallet_name variable with the `has_value()`, but yeah, not very practical in the end. I've removed the `MaybeArg` extension commit making the `wallet_name` to hold a pointer in the `GetWalletNameFromJSONRPCRequest` overload.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353278)
As in [previous](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500) comment, already removed the commit containing also this change. I've analysed the [discussion](https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104) around the string copy in the original implementation of `MaybeArg` and even if we could go around it and "fix"/ adding it for the string specific case/ condition (~`return std::optional<std::string_view>{params[i].get_str_ref()}`?) perhaps makes
...

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353506)
True, not really needed there, but resolved the `getdescriptoractivity` RPC issue using `std::ranges::all_of)` instead, as you suggested.