✅ pinheadmz closed an issue: "Improve/simplify node sync for pruned nodes"
(https://github.com/bitcoin/bitcoin/issues/30224)
(https://github.com/bitcoin/bitcoin/issues/30224)
💬 pinheadmz commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30224#issuecomment-2147819544)
> When running a pruned node it means it will keep only last X amount of GB predefined in the configuration, so why is it necessary to download the whole blockchain if only a tiny part will be kept at the end ?
Because a fully validating node must have the entire UTXO set to continue validate new blocks, and that UTXO set can only be computed by processing the entire blockchain. The "tiny part" of block data the prune node does keep on disk is only there in case the blockchain is rewound (rev
...
(https://github.com/bitcoin/bitcoin/issues/30224#issuecomment-2147819544)
> When running a pruned node it means it will keep only last X amount of GB predefined in the configuration, so why is it necessary to download the whole blockchain if only a tiny part will be kept at the end ?
Because a fully validating node must have the entire UTXO set to continue validate new blocks, and that UTXO set can only be computed by processing the entire blockchain. The "tiny part" of block data the prune node does keep on disk is only there in case the blockchain is rewound (rev
...
👍 ismaelsadeeq approved a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2096759851)
Code Review ACK 39d135e79f3f0c40dfd8fad2c53723d533cd19b4 🚀
I've tested this locally
c17550bc3a6 ensures that the target weight is calculated correctly, at most we get 3 off.
b2f0a9f8b07 fails without the first commit.
c17550bc3a6 fixes the issue of respecting fee rate when target weight is provided, as large tx now has correct fee rate.
My comments are not blockers, just suggestions for improvement, can come in a follow-up, but happy to reACK when fixed.
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2096759851)
Code Review ACK 39d135e79f3f0c40dfd8fad2c53723d533cd19b4 🚀
I've tested this locally
c17550bc3a6 ensures that the target weight is calculated correctly, at most we get 3 off.
b2f0a9f8b07 fails without the first commit.
c17550bc3a6 fixes the issue of respecting fee rate when target weight is provided, as large tx now has correct fee rate.
My comments are not blockers, just suggestions for improvement, can come in a follow-up, but happy to reACK when fixed.
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2147839858)
I'm not sure, but it seems like a potentially good thing to me that `last_block_processed` and BESTBLOCK are two distinct concepts.
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).
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.
It seems like the first
...
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2147839858)
I'm not sure, but it seems like a potentially good thing to me that `last_block_processed` and BESTBLOCK are two distinct concepts.
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).
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.
It seems like the first
...
💬 maflcko commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147845035)
If you want to help bring assumeutxo forward, you can help reviewing the outstanding pull requests. A lot of work has already been done, some work remains, but creating this issue will not help bring it forward. If you want to contribute otherwise to assumeutxo, you can read the previous discussions and contribute to them, if there is anything that has been missed.
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147845035)
If you want to help bring assumeutxo forward, you can help reviewing the outstanding pull requests. A lot of work has already been done, some work remains, but creating this issue will not help bring it forward. If you want to contribute otherwise to assumeutxo, you can read the previous discussions and contribute to them, if there is anything that has been missed.
💬 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
...
(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
...
(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.
(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"'`