💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2147869048)
> The last block processed is the block that CWallet in-memory state has been synced to (particularly CWalletTx::m_state which includes mempool / abandoned information not serialized to disk).

But none of that state is relevant to the last block processed. Any state related to blocks (confirmed or conflicted) is written to disk.

> The BESTBLOCK record is the last block whose data has been flushed to disk, that the wallet should begin syncing from next time it is reloaded.

The point is t
...

💬 ryanofsky commented on pull request "wallet: Avoid potentially writing incorrect best block locator":
(https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2147877812)
> I'm now wondering whether the wallet should even be doing anything on ChainStateFlushed since that doesn't seem like it should have any bearing on what the wallet knows about.

Notifications are sent in order, so it does have some meaning. It means wallet is in-between blockConnected events and synchronized up to some block, and that the node has written some data to disk recently, so the wallet may want write its data to disk, too.

I think the only problem with the ChainStateFlushed noti
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2147885391)
> It doesn't make sense to me that we wouldn't store the block the wallet's state has been synced to

You may be right this is the better approach. But I think the previous approach does make some sense, too. You might not want to write to the wallet database each time a block is connected if the block doesn't contain any relevant transactions, especially during reindexing.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626264025)
Done, let me know if this is better.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626264244)
Added.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626264403)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626265719)
Yes; but on GCC/stdlibc++, `std::popcount` just uses `__builtin_popcount`, which in benchmarks seems to perform worse than the code here. The I've updated the comment to reflect that (it was written before the C++20 switch which introduced `std::popcount`).

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626266141)
Done. Credit to @glozow.

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1626267484)
figured as much based on my investigation thanks

💬 laanwj commented on pull request "refactor: remove unused `CKey::Negate` method":
(https://github.com/bitcoin/bitcoin/pull/30218#issuecomment-2147894731)
ACK 8801e319d51209fe3a3b06e2aab5f96ceead290d
There are no API consistency arguments to keep this, negating isn't a conventional thing for keys.

💬 sipa commented on pull request "refactor: remove unused `CKey::Negate` method":
(https://github.com/bitcoin/bitcoin/pull/30218#issuecomment-2147912758)
ACK 8801e319d51209fe3a3b06e2aab5f96ceead290d

🤔 tdb3 reviewed a pull request: "test: Added test coverage to listsinceblock rpc"
(https://github.com/bitcoin/bitcoin/pull/30195#pullrequestreview-2096517820)
Approach ACK

Thanks for adding test coverage.
Exercised test's ability to fail by commenting out the `rename` calls (failed as expected with no exception being raised).

Built and ran all unit/functional tests. All passed.

💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626078619)
nit: The comment could be updated to match the current action, e.g. `# Restoring block file`.

💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626100437)
Is this send necessary for this test? The test seems to work with or without it, and `listsinceblock` would return an empty array (rather than throw) if there were no transactions to show, right?

Maybe I'm missing something?

💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626084163)
nit: Could phrase this a little clearer:
`'Test the RPC error "Can't read block from disk"'`

💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626076767)
nit: The comment could be updated to match the current action, e.g. `# Renaming the block file to induce unsuccessful block read`.

💬 ffrediani commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147939514)
I wish to contribute this way, having a proper and structured discussion about a topic which is still an issue. If you want to suggest other ways to contribute it is fine, but I hope you are not making it a must on the way I must contribute.
Creating this issue contributes to raise awareness to the issue (which currently exists), to discuss possible solution and have a solution implemented for it.

💬 ffrediani commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30224#issuecomment-2147949569)
@pinheadmz if there are alternatives ways and strategies why just stay with what is there right now and not improve it for these scenarios ?

Seems some developers in this project don't like to have issues opened they can't or are unwilling to work on and arbitrarily close them. I still disagree with this autocratic behaviour and that it should be discussed elsewhere. Seems more a personal preference of some rather than how it really is.

This is a feature request and as such has a place in
...

💬 maflcko commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147949594)
Can you explain how your discussion is different from the assumeutxo discussions already happening? I don't think it makes sense to have duplicate and redundant issues.

📝 aaservice opened a pull request: "Update and rename bitcoinkernel.cpp to bitcoinkernel.cppx"
(https://github.com/bitcoin/bitcoin/pull/30225)
Q

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that i
...