💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258663429)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124

In commit "wallet: Write new descriptor's cache in AddWalletDescriptor" (fcf7dfe08519aefd193d12958070bfc5ea52f090)

Can confirm if I revert this change I see a `Wallet corrupted (-4)
` error from `self.online.restorewallet("imports_watchonly", res["exported_file"])` running the test with `Error: Unable to expand wallet descriptor from cache` in the logs

💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258660270)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101952111

In commit "wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()" (1170ade3a6d1fbb50ee2a83b906bb83560b0484d)

Can confirm without this change the test fails as expected with `self.funds.sendtoaddress(online_wallet.getnewaddress(), 10)` throwing "No addresses available"

💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260742888)
In commit "test: Test for exportwatchonlywallet" (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)

Does this need a sync_mempools like the other case below to make sure offline wallet receives the funds?

Also would be nice here and other places to replace 0, 1 with self.online.index and self.offline.index

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3164996064)
with [8bc4035](https://github.com/bitcoin/bitcoin/commit/8bc4035570acfe512c756c6de30d6440d322d514) it has been rebased and the fuzzing CI error has been addressed

🤔 glozow reviewed a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33074#pullrequestreview-3097984714)
ACK b9e637bd0ee4d1da5f587ec33cbed9ee28c07daf

Compared the backport, didn't test

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165029728)
> it has internal noise higher than the savings here

Sure, but I'm just running them a lot of times, the trends are obvious this way.

> will run the same for the full test suite

I have measured running the tests with the 3 setups with 1-6x the nproc for parallelism.
The 3 solutions all scale well beyond the number of cpus (I'm now running the same until 8x on a different platform to confirm the findings).

<details>
<summary>Details</summary>

```
COMMITS="d767503b6a2618e0c99407
...

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165069155)
> Which indicates to me that the new solution scales a bit better - but since we haven't even hit the bottom yet (surprisingly), I'll post those in a follow-up.

Thanks for the benchmarks, but honestly, I can't see that the new solution scales better. You only did 3 runs for each point, and the error bars overlap with the other points each time. Also the hyperfine output has warnings about outliers. I guess you'd have to run at least 100 times for each data point (not saying you should :sweat_
...

💬 pythcoiner commented on pull request "wallet: support bip388 policy with external signer":
(https://github.com/bitcoin/bitcoin/pull/33008#issuecomment-3165080503)
> At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.

In the range of devices supporting miniscript, only Ledger requires the software to store the Proof Of Registration, other devices stores it internally(SpecterDiy, BitBox, Coldcard, Jade) or not at all (Krux).

💬 maflcko commented on pull request "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/33136#issuecomment-3165130053)
(trivial rebase)

💬 w0xlt commented on issue "spendable is true for UTXO of private key disabled wallet":
(https://github.com/bitcoin/bitcoin/issues/33110#issuecomment-3165217540)
Considering that the legacy wallet can no longer be loaded, will this field always be true?

💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3165272701)
(trivial rebase)

💬 w0xlt commented on pull request "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL":
(https://github.com/bitcoin/bitcoin/pull/33021#discussion_r2261145849)
Brace initialization will convert `size_t` to `int64_t`

```suggestion
constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES{10_MiB};
```

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3165392837)
> it could debug-assert that capacity is even

Added, thanks for the hint

👍 ryanofsky approved a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3097866742)
Code review ACK dd1a7ab428a7c7f87b2a02580aca0d593c2de68a. Thanks for the updates! I left a few more suggestions, but nothing important.

I think it might be good to link directly to your optimization commit d76d7531dfcc7ef2d104b8977a2239cb1fc89119 in the PR description since it helps explain the changes here and bring them together.

On style changes in the CScript methods I think they are an improvement overall, though personally I'd try to avoid front/back methods and magic numbers and wri
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261132664)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255448800

> You still think it needs additional explanations?

It seems ok, but maybe consider adding your explanation to the commit message: "The `BOOST_CHECK_EQUAL` to `BOOST_CHECK` change is just to test the new helper method in a simpler way, now that we don't have to use `Solver`."

I wouldn't have asked the question if I had known what the intention behind the solver change was, so this wasn't obvious to me.

Also I do
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260808130)
In commit "refactor: rename `GetSigOpCount` to `CountSigOps`" (006a428613710dde24866cf4f6efd7b68b6ee294)

This sentence should be prefixed with "When enforcing the `MAX_BLOCK_SIGOPS_COST` limit" to be correct since it doesn't apply in other cases. (Problem is fixed in the next commit but the change belongs this commit. Sorry my previous feedback was a bit jumbled and I mixed the commits together.)

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260854463)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (6bf5dce801e0da9d372d42d2c0650c283ec787f5)

I still think you should consider simplifying these lines to:

```c++
nSigOps += CountP2SHSigOps(tx.vin[i].scriptSig, coin.out.scriptPubKey);
```

and simplifying CheckSigopsBIP54 to:

```c++
sigops += txin.scriptSig.CountSigOps(/*fAccurate=*/true);
sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
sigops += CountP2SHSigOps(txin.scriptSig, prev_txo.scriptPubKey);
`
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261095050)
In commit "refactor: make sure `OP_CHECKSIGADD` isn't considered valid (legacy) sigop" (a2cce66f11e635924c34b27b253214be8815b0bc)

Thanks, maybe also replace "(legacy)" in commit description with "pre-tapscript" or "base"

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261177012)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254732750

To clarify I agree code is doing the right thing and the CScript class shouldn't be looking inside the keys to check the format. I just think short comments could be helpful to prevent misunderstandings. Maybe:

- `//! Detect P2PK script with a compressed public key. Doesn't check the 0x02/0x03 key prefix.`
- `//! Detect P2PK script with a uncompressed public key. Doesn't check the 0x04 key prefix.`

Feel free to le
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260876336)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255358039

Thanks. I think I just dislike reviewing changes before I know what purpose / motivation of the change is. I know you can often guess motivations after the fact, but it is usually helpful imo to see them stated explicitly.