💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-3049612125)
I think all feedback has been addressed @mzumsande @sipa @furszy @TheCharlatan @maflcko

Please let me know if there's anything else pending or if anything doesn't make sense.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2192977836)
> That _is_ indeed confusing - I have tried to unify it as much as I could based on whether it's important that the key is stored as bytes or not. Let me know if it's still confusing or if I forgot anything.

I'd say the `Obfuscation` constructor that takes an uint64_t should probably be removed, because it is unused outside of tests. Also, it exposes an implementation detail, which could even be confusing in the context of endianness. It is only used to set a value of `0` (or all-zero bytes).
...

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2193048005)
Though, generally, I also wonder if the rename from xor to obfuscation is really needed or meaningful. I think it is clear that the xor approach isn't trivial to change without writing data-migration/upgrade code. Also, there hopefully isn't a reason to having to ever change it in the future. Also, there are still plenty of places that refer to xor (`MEMPOOL_DUMP_VERSION_NO_XOR_KEY`, ...).

I'd say just leaving the naming as-is will be beneficial, because:

* The diff is smaller and more foc
...

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193052705)
Instead of magic numbers, try using `TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR`

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193078705)
Also, there is a separate size limit for TRUC children - I don't think that's been implemented yet?

🤔 glozow reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-2998494185)
Nice, concept ACK! Saw that these tests fail on #31936, which is helpful for showing its issues. Ultimately, I think the wallet commits should be introduced before the RPC ones.

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193049601)
We should also check that we can't fund a v2 transaction when everything is v3 unconfirmed

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193065536)
Comment seems inaccurate: it mentions a set but this only allows for 1 transaction. Should also mention (1) that this is used to stop us from creating another unconfirmed child and (2) this is specifically the in mempool-sibling, as there can be multiple siblings but only 1 in mempool (unless there was a reorg).

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