💬 stickies-v commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091385844)
Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091385844)
Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
👍 hebasto approved a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2844036012)
ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2844036012)
ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091377191)
Why use an exception? Wouldn’t exiting with an error code suffice?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091377191)
Why use an exception? Wouldn’t exiting with an error code suffice?
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091379846)
GHA [docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#example-of-adding-a-system-path) do not suggest `-Encoding utf8`. Is it necessary?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091379846)
GHA [docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#example-of-adding-a-system-path) do not suggest `-Encoding utf8`. Is it necessary?
💬 maflcko commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2884160071)
lgtm ACK e62423d6f1514b022155edb5bc930cecc4236731 🛄
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK e62423d6f1514b02
...
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2884160071)
lgtm ACK e62423d6f1514b022155edb5bc930cecc4236731 🛄
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK e62423d6f1514b02
...
💬 theuni commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2884164753)
Concept ACK. Neat :)
> Maybe this is not an issue for the PR, but it would be good to make clear what types of corruption BlockTreeStore can and can't detect and what types of corruption it can recover from. If it can do simple things to detect corruption like adding checksums, or to prevent it like writing to temporary files and renaming them in place, those could be good to consider.
Yeah, I think this is the heart of it. I'm onboard for a new impl outside of leveldb, but before getting
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2884164753)
Concept ACK. Neat :)
> Maybe this is not an issue for the PR, but it would be good to make clear what types of corruption BlockTreeStore can and can't detect and what types of corruption it can recover from. If it can do simple things to detect corruption like adding checksums, or to prevent it like writing to temporary files and renaming them in place, those could be good to consider.
Yeah, I think this is the heart of it. I'm onboard for a new impl outside of leveldb, but before getting
...
💬 furszy commented on pull request "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now":
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884172976)
> > What if instead of excluding the entire test file, we only exclude the specific failing test case?
>
> Your diff would exclude the test case for all CI runs (even CI runs not using valgrind).
That would be easy to fix by just providing another environment variable for Valgrind runs. But np, it was just an idea to keep the test running while the issue is being investigated.
> I think longer term it would be better to just fix it.
Hard to disagree.
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884172976)
> > What if instead of excluding the entire test file, we only exclude the specific failing test case?
>
> Your diff would exclude the test case for all CI runs (even CI runs not using valgrind).
That would be easy to fix by just providing another environment variable for Valgrind runs. But np, it was just an idea to keep the test running while the issue is being investigated.
> I think longer term it would be better to just fix it.
Hard to disagree.
💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091413180)
> Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
It *will* be negative and is known to be, otherwise there would be no suppression needed in `master` right now.
Also, it is perfectly fine and intended to serialize any `int8_t` value (even a negative one) as `uint8_t`, because the two int types are the same width. The only requirement is that t
...
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091413180)
> Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
It *will* be negative and is known to be, otherwise there would be no suppression needed in `master` right now.
Also, it is perfectly fine and intended to serialize any `int8_t` value (even a negative one) as `uint8_t`, because the two int types are the same width. The only requirement is that t
...
🤔 rkrux reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2843270318)
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
I like how this PR brings the best/last block processed updates both in memory & disk in unison, making it easier to reason about the wallet code ultimately. The detailed commit messages aids review.
I have verified that the location of such changes makes it possible for the following wallet RPCs to correctly handle best block related memory & disk updates:
```
createwallet
loadwallet
restorewallet
unloadwallet
backupwallet
```
I feel
...
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2843270318)
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
I like how this PR brings the best/last block processed updates both in memory & disk in unison, making it easier to reason about the wallet code ultimately. The detailed commit messages aids review.
I have verified that the location of such changes makes it possible for the following wallet RPCs to correctly handle best block related memory & disk updates:
```
createwallet
loadwallet
restorewallet
unloadwallet
backupwallet
```
I feel
...
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963)
Within `AttachChain` itself, there is another opportunity to use `setLastBlockProcessedInMem` here:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L3137-L3143
The `cs_wallet` is held at the start of this function.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963)
Within `AttachChain` itself, there is another opportunity to use `setLastBlockProcessedInMem` here:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L3137-L3143
The `cs_wallet` is held at the start of this function.
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091164443)
Though these `m_attaching_chain` updates were done in `AttachChain` function, it looks quite cleaner now to get rid of this low level boolean from wallet instance, makes it easier to wrap head around `AttachChain`.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091164443)
Though these `m_attaching_chain` updates were done in `AttachChain` function, it looks quite cleaner now to get rid of this low level boolean from wallet instance, makes it easier to wrap head around `AttachChain`.
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091225516)
`as it may be different in case the case of a reorg.`
Redundancy if retouched.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091225516)
`as it may be different in case the case of a reorg.`
Redundancy if retouched.
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090936254)
Thanks, this makes sense to me.
Limiting the writing of best block to disk mainly to cases[^1] that are actively involved in processing the new blocks seems logical. Since these cases will be updating the best block periodically, it's prudent to not interfere by doing the same via other user driven RPCs that work on different criterion as highlighted in the above comments.
> This is because they are expected to be used during normal operation when the wallet will still be handling incoming
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090936254)
Thanks, this makes sense to me.
Limiting the writing of best block to disk mainly to cases[^1] that are actively involved in processing the new blocks seems logical. Since these cases will be updating the best block periodically, it's prudent to not interfere by doing the same via other user driven RPCs that work on different criterion as highlighted in the above comments.
> This is because they are expected to be used during normal operation when the wallet will still be handling incoming
...
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091415690)
```diff
- assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824.
+ assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
```
Nit: The correct balance is there, can be asserted.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091415690)
```diff
- assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824.
+ assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
```
Nit: The correct balance is there, can be asserted.
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700)
> self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
IMO the test log should be updated to mention this improvement also.
That the wallet transactions are not missed anymore in case of unclean shutdown.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700)
> self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
IMO the test log should be updated to mention this improvement also.
That the wallet transactions are not missed anymore in case of unclean shutdown.
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090981396)
I verified that `ScanForWalletTransactions` does update the best block locator on disk intermittently:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L1875-L1883
But I don't see it updating `m_last_block_processed`, instead it reads it. Even if `m_last_block_processed` comes out to be same as the best block locator at the end in the `AttachChain` case, I find updating `m_last_block_processed` and best block locator here after rescan bet
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090981396)
I verified that `ScanForWalletTransactions` does update the best block locator on disk intermittently:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L1875-L1883
But I don't see it updating `m_last_block_processed`, instead it reads it. Even if `m_last_block_processed` comes out to be same as the best block locator at the end in the `AttachChain` case, I find updating `m_last_block_processed` and best block locator here after rescan bet
...
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091293082)
> since the chainstate was not flushed
I now realise that this refers to the node specific chainstate.
Having seeing the removal of chain state flushed function in the wallet, I initially thought it referred to that (which doesn't exist anymore).
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091293082)
> since the chainstate was not flushed
I now realise that this refers to the node specific chainstate.
Having seeing the removal of chain state flushed function in the wallet, I initially thought it referred to that (which doesn't exist anymore).
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091402902)
> the wallet should record the new best block.
I assume there is no way for us to verify this before the crash and after tip invalidation. The wallet info returns value based on the one that's in memory: `m_last_block_processed `
> wallet.getwalletinfo()['lastprocessedblock']['hash']
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091402902)
> the wallet should record the new best block.
I assume there is no way for us to verify this before the crash and after tip invalidation. The wallet info returns value based on the one that's in memory: `m_last_block_processed `
> wallet.getwalletinfo()['lastprocessedblock']['hash']
💬 Sjors commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884193908)
Just to add another thought here: it seems that recently fee bumps were a big source of failed compact block propagation. By restricting the extra pool to just those, and perhaps expanding its default size, we might get more value of it, with less DoS downside.
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884193908)
Just to add another thought here: it seems that recently fee bumps were a big source of failed compact block propagation. By restricting the extra pool to just those, and perhaps expanding its default size, we might get more value of it, with less DoS downside.
💬 theuni commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2884199007)
> When looking at the new file alone, I see several oportunities for improvement: Drop the `f` prefix, use default member initializers, use `std::exchange`. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.
Indeed. I have some follow-up work that introduces a `GrantableSemaphore` and rewrites `CountingSemaphoreGrant` to be correct-by-construction (where a successfully created gran
...
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2884199007)
> When looking at the new file alone, I see several oportunities for improvement: Drop the `f` prefix, use default member initializers, use `std::exchange`. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.
Indeed. I have some follow-up work that introduces a `GrantableSemaphore` and rewrites `CountingSemaphoreGrant` to be correct-by-construction (where a successfully created gran
...