💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193091714)
A more descriptive name may be `exclude_version3`.

👍 maflcko approved a pull request: "test: use notarized v28.2 binaries and fix macOS detection"
(https://github.com/bitcoin/bitcoin/pull/32922#pullrequestreview-2998590151)
lgtm

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2193110095)
```suggestion
HOST can be set to any of the `host-platform-triplet`s from
```

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2193116490)
>I switched to args.host, see https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192923546

I don't think you did. The line still looks wrong and is missing `args.host`.

💬 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-3049818044)
> I wonder if this can be uncoupled from CLIENT_VERSION, because the coupling is confusing and makes testing harder:

Definitely, however I'd like to be able to reuse the existing "version" record which mostly has the desired behavior since v24.0. The detection only works because of older versions overwriting the field with their CLIENT_VERSION. Introducing a new field now means that we cannot detect when an upgraded wallet was opened in any version older than v30.0 (assuming this is merged fo
...

🤔 janb84 reviewed a pull request: "test: Turn rpcauth.py test into functional test"
(https://github.com/bitcoin/bitcoin/pull/32881#pullrequestreview-2998635782)
LGTM ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62

PR turns rpcauth.py test to functional test, this looks like a continuation of the migration efforts of https://github.com/bitcoin/bitcoin/pull/32697.
Seems like a logical step and it simplifies the test/cmake setup.

- build & tested ✅
- code reviewed ✅

```shell
rpc_whitelist.py | ✓ Passed | 2 s
tool_rpcauth.py | ✓ Passed | 0 s
tool_signet_miner.py
...

💬 achow101 commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3049886134)
> I believe that the PR title and more importantly the PR description should be updated to remove the stuff related to watch-only as that code removal was done in another PR. The last paragraph of the description can be kept.

Updated

📝 theStack opened a pull request: "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)"
(https://github.com/bitcoin/bitcoin/pull/32924)
Currently in our tests, all ECDSA signatures passing verification have sizes of 69 bytes and above (that's the DER-encoded size, i.e. counted without the sighash flag byte) [1]. This PR adds test coverage for the minimum-sized valid case of 8 bytes, by taking an interesting testnet transaction that I stumbled upon:
https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433
Note that this is a very obscure construction that only works because the public k
...

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