💬 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
...
👍 pinheadmz approved a pull request: "net: replace manual reference counting of CNode with shared_ptr"
(https://github.com/bitcoin/bitcoin/pull/32015#pullrequestreview-2837749970)
ACK 8b93e0894f94883f76b7df5d50a5f1a58544fb6c
Built on macos/arm64, ran all unit and functional tests, reviewed each commit and left a few questions.
Also ran on mainnet for a few hours to watch peers come and go.
This is a great refactor, manually refcounting is bound to be error prone and the patch gives us cleaner code with some performance optimizations. Since `CNode` was mostly handled by raw pointer most of the handling logic didn't have to change to support `std::shared_ptr` inste
...
(https://github.com/bitcoin/bitcoin/pull/32015#pullrequestreview-2837749970)
ACK 8b93e0894f94883f76b7df5d50a5f1a58544fb6c
Built on macos/arm64, ran all unit and functional tests, reviewed each commit and left a few questions.
Also ran on mainnet for a few hours to watch peers come and go.
This is a great refactor, manually refcounting is bound to be error prone and the patch gives us cleaner code with some performance optimizations. Since `CNode` was mostly handled by raw pointer most of the handling logic didn't have to change to support `std::shared_ptr` inste
...
💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087411896)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Why change this from span to vector?
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087411896)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Why change this from span to vector?
💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087412742)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Commit message says deletion is unchanged.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087412742)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Commit message says deletion is unchanged.
💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087433170)
4326f97dc6dccc9e558d99931f8235fc6b6af405
How did this not deadlock before? `FindNode()` called on the next line locks `m_node_mutex`
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087433170)
4326f97dc6dccc9e558d99931f8235fc6b6af405
How did this not deadlock before? `FindNode()` called on the next line locks `m_node_mutex`
💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087458896)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Why did you choose to pass the raw pointer instead of refactoring `ProcessMessages()` to accept the shared_ptr?
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087458896)
4326f97dc6dccc9e558d99931f8235fc6b6af405
Why did you choose to pass the raw pointer instead of refactoring `ProcessMessages()` to accept the shared_ptr?
💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2877805608)
_<ins>Updates</ins>_:
* Addressed @achow101's [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964) by adding an `Assert()` on `CreateWallet()` enforcing only descriptor wallets can be created.
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2877805608)
_<ins>Updates</ins>_:
* Addressed @achow101's [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964) by adding an `Assert()` on `CreateWallet()` enforcing only descriptor wallets can be created.
🤔 musaHaruna reviewed a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2837998074)
Concept ACK [5134492](https://github.com/bitcoin/bitcoin/pull/32378/commits/51344920ebe1772d8dd3bece789a6510d774ab59)
Code review: After reading through [internal-interface-guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines), the new code change adheres to interface guideline and correctly abracts away implementation details to the appropraite files. Naming of function is very descriptive which is good.
I compiled each commit su
...
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2837998074)
Concept ACK [5134492](https://github.com/bitcoin/bitcoin/pull/32378/commits/51344920ebe1772d8dd3bece789a6510d774ab59)
Code review: After reading through [internal-interface-guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines), the new code change adheres to interface guideline and correctly abracts away implementation details to the appropraite files. Naming of function is very descriptive which is good.
I compiled each commit su
...