💬 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.
(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.
(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.
(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.
(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`).
(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.
(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
(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.
(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
(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.
(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`.
(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?
(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"'`
(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`.
(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.
(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
...
(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.
(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
...
(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
...
✅ fanquake closed a pull request: "Update and rename bitcoinkernel.cpp to bitcoinkernel.cppx"
(https://github.com/bitcoin/bitcoin/pull/30225)
(https://github.com/bitcoin/bitcoin/pull/30225)
📝 fanquake locked 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
...
(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
...