💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964)
In e66e9ee9f19413c93d8523afdb8f9f3e07b818ec "wallet, refactor: Remove Legacy warnings and errors"

I'd prefer to at least preserve enforcement that the wallet being created is a descriptor wallet. Perhaps we could add an `Assert()` or `Assume()` for the flag.

💬 maflcko commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2877505784)
lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c

💬 rkrux 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-2877510396)
> If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors.

I have not checked this PR yet but the original intention came from this issue: https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475

The idea was to show results for descriptors that don't have _all_ the private keys but do have at least one. This is not for the case when the descriptor doesn't have _any_ private key.

I do like how th
...

💬 instagibbs commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877519744)
@darosior just noting the tests are specifically written to not worry about standardness, so at least these would have to be augmented to cover the cases this PR is ostensibly supporting

🤔 stratospher reviewed a pull request: "test: Fix feature_pruning test after nTime typo fix"
(https://github.com/bitcoin/bitcoin/pull/32312#pullrequestreview-2837725653)
tested ACK 2aa63d5. verified that `nTime` is being incremented now.

agree with @naiyoma - this appears a "block being requested is valid/invalid in node1 problem" and not a "which node to connect to problem".

- node2 and node5 have identical chain tips (aside from differing prune heights: node2 at 1246, node5 at 774).
- on master: node2 requests block 1245 from node1. but in node1, that block is marked BLOCK_FAILED_VALID, so the request is ignored. I see this log in node1:
`ignoring requ
...

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