💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877536457)
@instagibbs Yes and no, depending on the meaning of standardness I guess. The worrisome transactions *are* non-standard (in the sense that they are not relayed/accepted), but are standard in the `IsStandard()` sense, and thus not immediately rejected before script execution even begins.

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087404190)
yea I tried doing that but then I was getting less coverage for some reason not sure if I just need to let the fuzzer run longer

Without `ConsumeTransaction`
```
#657589 REDUCE cov: 813 ft: 3922 corp: 309/188Kb lim: 4096 exec/s: 8323 rss: 359Mb L: 941/3853 MS: 1 EraseBytes-
```

With `ConsumeTransaction`
```
#14901 REDUCE cov: 1461 ft: 8651 corp: 536/449Kb lim: 4096 exec/s: 3725 rss: 350Mb L: 677/4093 MS: 1 EraseBytes-
```

I can try running the fuzz test for a while and see if it
...

💬 vicjuma commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877558632)
Maybe `-rpcwallet=<wallet>` will come in handy in this situation where you would have to use a different wallet for these situations to prevent the conflicts.

💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087412829)
Done.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087415174)
> the best block locator could be a little out of date and this is maybe not ideal but fine?

I'm not sure that actually could happen, but I think that's the worse that can happen, and I think that's fine. It should only be a couple of blocks which would be fast to rescan.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087427505)
I think I will leave this as is.

💬 instagibbs commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087432483)
`scriptsig-size` failures are being hit for non-witness cases, which I think would stop caching?

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087432939)
Done

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087434556)
Changed to `WriteBestBlock()`.

> I also wonder if we can consolidate the seperate updates in blockConnected, deleting SetLastBlockProcessedInMem at the top, and moving it to the bottom here like:

It's at the top because there's some stuff in `AddToWallet` which need the last block processed in memory to be updated to the current block.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087435824)
Done as suggested.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087436170)
Done as suggested.

💬 achow101 commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877610128)
> The idea was to show results for descriptors that don't have all the private keys but do have at least one.

I think that's fine to show a result, but I don't think we should show any result if there are no private keys at all.

💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2087482127)
True. I guess I could squash everything into one commit _doc: Improve dependencies.md_ and list everything in commit description? wdyt.

💬 maflcko commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2087486313)
Yeah, seems fine to squash

💬 hodlinator commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2087492189)
I'd keep the removal of the Linux Kernel and addition of self-compilation info as separate commits and merge the other 3 into 1 - leaving 3 remaining commits. But squashing all is fine too.

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087495200)
I did that earlier but got a failure and I thought it was related with migration and rolled it back without checking it properly. I'll add it back.

🤔 rkrux reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2837868769)
This partial review is for the previous diff at e2e9c30a4d04bd8173c3386c5b97dbac227e810e. I started looking into it today and need to spend some more time in internalising the consequences of this diff but at the moment I'm inclining towards a concept ack.

Left few questions below.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087487486)
Now that we have this `WriteBestBlock` function, something similar happens within `BackupWallet` as well: https://github.com/bitcoin/bitcoin/blob/8309a9747a8df96517970841b3648937d05939a3/src/wallet/wallet.cpp#L3267-L3274

Do you think we can leverage `WriteBestBlock` over there? I mention because the pattern is quite similar and I would prefer to have the same function being called.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087491546)
This comment is specific to the concept of scanning for wallet transactions and then updating the last block processed both in memory and disk:

> Additionally, after rescanning on wallet loading, instead of writing
the locator for the current chain tip, write the locator for the last
block that the rescan had scanned.

There are couple other usages of `ScanForWalletTransactions` in `rescanblockchain` RPC and conditionally in `importdescriptors`. I guess we don't want to update the block l
...

👍 ryanofsky approved a pull request: "Add checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31981#pullrequestreview-2837626300)
Code review ACK c1939c43c3addb17c4316d49580762a1e0ec4504. Since last review changes were improving error handling adding many comments, and making TestBlockValidity a standalone method again accepting a chainstate.

I left another suggestion (and a diff) below to make TestBlockValidity return a BlockValidationState instead of a bool and strings, which I think would be better, but feel free to keep current approach if you prefer.

Might also be nice to link to the review club discussion in th
...