💬 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.
(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.
(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.
(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.
(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?
(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
(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.
(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.
(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.
(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.
(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.
(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
(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.
(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.
(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.
(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.
(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
...
(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
...
(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
...
💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087500872)
In commit "test: more template verification tests" (c1939c43c3addb17c4316d49580762a1e0ec4504)
Might be nice to split this test up into different methods since most of the checks seem independent. (Could pass in any objects they are sharing in common as arguments)
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087500872)
In commit "test: more template verification tests" (c1939c43c3addb17c4316d49580762a1e0ec4504)
Might be nice to split this test up into different methods since most of the checks seem independent. (Could pass in any objects they are sharing in common as arguments)
💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087339443)
re: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2035713597
> Making this return a `BlockValidationState` means we have to pass it over the interface, which means we can't drop the special handling
Agree it's not good to pass `BlockValidationState` over the IPC interface (and the earlier change I posted didn't do that). I just think it it's best if validation.h/cpp uses BlockValidationState to be internally consistent and return the most information to callers.
> None of th
...
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087339443)
re: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2035713597
> Making this return a `BlockValidationState` means we have to pass it over the interface, which means we can't drop the special handling
Agree it's not good to pass `BlockValidationState` over the IPC interface (and the earlier change I posted didn't do that). I just think it it's best if validation.h/cpp uses BlockValidationState to be internally consistent and return the most information to callers.
> None of th
...