💬 krishpranav commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2604597095)
Hi, is it still open? i am looking to work on some issue.

💬 krishpranav commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2604598213)
Hi, is it still open? i am looking forward to contribute.

👍 TheCharlatan approved a pull request: "test: Check that reindex with prune wipes blk files"
(https://github.com/bitcoin/bitcoin/pull/31696#pullrequestreview-2564480924)
ACK fa975d1075668e3dc3366122463d6900e6d94d0c

💬 kevkevinpal commented on pull request "lint: Call more checks from test_runner":
(https://github.com/bitcoin/bitcoin/pull/31653#discussion_r1923651316)
yea should be fine just thought I'd point it out, I think it is fine as is

💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2604618817)
re-ACK ea53b468e2650dc6e5d6d996cc899846a5ffe901

Just addressed minor review comments.

💬 kevkevinpal commented on pull request "lint: Call more checks from test_runner":
(https://github.com/bitcoin/bitcoin/pull/31653#discussion_r1923657439)
I guess it doesnt add a new line but modify's what is already there
adding `Checking commit range`

example from #31690
```
[20:29:08.514] + echo
[20:29:08.514]
[20:29:08.514] + git log --no-merges --oneline HEAD~..HEAD
[20:29:08.516] 2db6923332 [doc] Amend notes on benchmarking
```

either way looks good to me!

💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2604630509)
Concept ACK

What's keeping this in draft status?

🤔 Sjors reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2564262259)
Concept ACK

Mostly happy with feb96d8c1c4dfcdf087250a55f893aed0c768398

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923553764)
936fe5899f9086f99a9a7f05584f467add2bc45b: should or could? IIUC if we migrate a pre-segwit wallet, then the `IsMine()` check below will throw out `P2WPKH` and `P2SH-P2WPKH`. But the migrated wallet will watch those addresses anyway, because it uses a `combo()` descriptor.

If so, it would be good to note that.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923515986)
9c0d5856d2065119232bb4ca651391ed829a99d4: maybe add a comment here that coins sent to invalid addresses won't show up in the destination wallet.

You can also add:

```py
assert_equal(wallet.getbalances()["watchonly"]["trusted"], 0)
```

And maybe:

```py
assert_equal(len(wallet.listtransactions(include_watchonly=True)), 1)
```

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923632724)
ca0564e02c72ea57d89ec37f0f4948e6a5d0096e: you're renaming `it` to `mit` for consistency with above? Not sure if it's helpful though.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923574532)
936fe5899f9086f99a9a7f05584f467add2bc45b: it seems worth noting here that we'll add some p2sh-of-p2sh scripts, but that those will get filtered, so we don't bother with a `script.IsPayToScriptHash()` check.

Ditto for the segwit and multisig checks that are now removed, I think it's worth mentioning that are intentionally added here for simplicity, and then filtered.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923639342)
ca0564e02c72ea57d89ec37f0f4948e6a5d0096e: I think it's worth preserving some of this comment, if only to explain to future readers why we went for the `GetCandidateScriptPubKeys() | CanProvide` approach.

Perhaps in `CanProvide`?

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923583439)
7d199d062d8794cde46631e61f2c116b9e37f573: why is it safe to not abort the migration over this?

Also, won't this hit the `assert(spks.size() == 0);` added by the next commit?

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923653059)
e41f47597d86022d5a9947ee0b08151d4336cffc: Is this only intended for users who called `importmulti` on a legacy wallet? Or could those scripts have ended up in legacy wallets in other ways?

If the latter is the case, do you mean that only modern node versions with miniscript support can sign them? Or could pre-miniscript nodes spend them as well? Might be worth illustrating with an older node (< v25?), if the `miniscript ` wallet here can be loaded by that older version.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1923657743)
e41f47597d86022d5a9947ee0b08151d4336cffc: "containing scripts matching miniscript"?

💬 Sjors commented on pull request "wallet: migration, don't create spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2604652741)
Is this superseeded by #31495 or still relevant?

💬 Sjors commented on pull request "wallet: migration, don't create spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2604656730)
Concept ACK

Can this go in parallel with #31495 or better to wait for it?

👍 theStack approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2564548427)
re-ACK ea53b468e2650dc6e5d6d996cc899846a5ffe901

Verified that all review comments have been adressed since my previous ACK.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2604687152)
@fjahr I'm trying to use some real Mainnet blocks here instead. Block 861848 for example, has a lot of taproot inputs and could be good for measuring batch-validation impact on a taproot-heavy block.
cc @l0rinc