💬 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
...
💬 sipa commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2884199732)
I think a file structure of `$DATADIR/blocks/[${(HEIGHT//2016)*2016}]/$HEIGHT-$HASH.dat` would be a nice color for the bikeshed. That would mean typically 2016 block files per directory (if no branches appear), organized nearly per retarget period.
As for putting block and undo data in the same file, I'm unsure. Undo data to me feels more like a validation-level thing, while block data is more a storage-level thing.
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2884199732)
I think a file structure of `$DATADIR/blocks/[${(HEIGHT//2016)*2016}]/$HEIGHT-$HASH.dat` would be a nice color for the bikeshed. That would mean typically 2016 block files per directory (if no branches appear), organized nearly per retarget period.
As for putting block and undo data in the same file, I'm unsure. Undo data to me feels more like a validation-level thing, while block data is more a storage-level thing.
👍 hebasto approved a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2844135431)
ACK e62423d6f1514b022155edb5bc930cecc4236731.
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2844135431)
ACK e62423d6f1514b022155edb5bc930cecc4236731.
💬 hebasto commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091436527)
nit: From https://github.com/bitcoin/bitcoin/pull/30997#issue-2554496391:
> Starting with Qt 6.5.0, the `libxcb-cursor0` package is required to be installed at runtime.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091436527)
nit: From https://github.com/bitcoin/bitcoin/pull/30997#issue-2554496391:
> Starting with Qt 6.5.0, the `libxcb-cursor0` package is required to be installed at runtime.
💬 hebasto commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091435084)
nit:
```suggestion
| CMake | [link](https://cmake.org/download/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) |
```
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091435084)
nit:
```suggestion
| CMake | [link](https://cmake.org/download/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) |
```
📝 instagibbs opened a pull request: "functional test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage"
(https://github.com/bitcoin/bitcoin/pull/32516)
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
I didn't assert the exact order of pruning, but can if desired.
(https://github.com/bitcoin/bitcoin/pull/32516)
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
I didn't assert the exact order of pruning, but can if desired.
💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2884220162)
My Guix build:
```
aarch64
14f6c52455cfd4f871e97271938e906137d0a0eac8adb381cf22775e71e9662a guix-build-8f4fed7ec700/output/aarch64-linux-gnu/SHA256SUMS.part
793ebdb24d8956a0829c59074e3a5748b0914aa34f076956a6d0b4ef09274187 guix-build-8f4fed7ec700/output/aarch64-linux-gnu/bitcoin-8f4fed7ec700-aarch64-linux-gnu-debug.tar.gz
6bc3f991fce9e2f3dbd9e8aafd3f5a3957f92fdef478c1b95af2e5badfaddc30 guix-build-8f4fed7ec700/output/aarch64-linux-gnu/bitcoin-8f4fed7ec700-aarch64-linux-gnu.tar.gz
e475ede2
...
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2884220162)
My Guix build:
```
aarch64
14f6c52455cfd4f871e97271938e906137d0a0eac8adb381cf22775e71e9662a guix-build-8f4fed7ec700/output/aarch64-linux-gnu/SHA256SUMS.part
793ebdb24d8956a0829c59074e3a5748b0914aa34f076956a6d0b4ef09274187 guix-build-8f4fed7ec700/output/aarch64-linux-gnu/bitcoin-8f4fed7ec700-aarch64-linux-gnu-debug.tar.gz
6bc3f991fce9e2f3dbd9e8aafd3f5a3957f92fdef478c1b95af2e5badfaddc30 guix-build-8f4fed7ec700/output/aarch64-linux-gnu/bitcoin-8f4fed7ec700-aarch64-linux-gnu.tar.gz
e475ede2
...
💬 instagibbs commented on pull request "functional test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2091440961)
MiniWallet doesn't appear to support more than 49 utxos at a time? Unclear to me what's going on so I did this instead.
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2091440961)
MiniWallet doesn't appear to support more than 49 utxos at a time? Unclear to me what's going on so I did this instead.
💬 furszy commented on issue "test: wallet_reorgsrestore.py failure under valgrind":
(https://github.com/bitcoin/bitcoin/issues/32493#issuecomment-2884223781)
We could try expanding the shutdown procedure to ensure the app's lock can be acquired before returning.
This would guarantee that the process can be re-initiated at any time afterward. Thinking of something like: https://github.com/furszy/bitcoin-core/commit/c6c9723f523c74615d784788b51e3365c5bee7c0
If this fails, it would mean the lock file is not being released at all after the process is killed (so far, we only can guess that during Valgrind runs, the lock release process might be taking lon
...
(https://github.com/bitcoin/bitcoin/issues/32493#issuecomment-2884223781)
We could try expanding the shutdown procedure to ensure the app's lock can be acquired before returning.
This would guarantee that the process can be re-initiated at any time afterward. Thinking of something like: https://github.com/furszy/bitcoin-core/commit/c6c9723f523c74615d784788b51e3365c5bee7c0
If this fails, it would mean the lock file is not being released at all after the process is killed (so far, we only can guess that during Valgrind runs, the lock release process might be taking lon
...
✅ fanquake closed an issue: "test: wallet_reorgsrestore.py failure under valgrind"
(https://github.com/bitcoin/bitcoin/issues/32493)
(https://github.com/bitcoin/bitcoin/issues/32493)
🚀 fanquake merged a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507)
(https://github.com/bitcoin/bitcoin/pull/32507)
💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091455024)
Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091455024)
Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
💬 szarka commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884240017)
> I have to imagine that especially with policy changes happening, this vector is useful (yes, in the absence of attackers). If you don't support 1p1c yet, you'll currently be throwing the `TX_RECONSIDERABLE` parent and `TX_MISSING_INPUTS` child in there. Same if you don't support TRUC yet. I think I had a comment somewhere about returning false for `TX_CONSENSUS` errors.
>
> @0xB10C was collecting data on how often we use this for compact block relay reconstruction I think? Correct me if I'm
...
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884240017)
> I have to imagine that especially with policy changes happening, this vector is useful (yes, in the absence of attackers). If you don't support 1p1c yet, you'll currently be throwing the `TX_RECONSIDERABLE` parent and `TX_MISSING_INPUTS` child in there. Same if you don't support TRUC yet. I think I had a comment somewhere about returning false for `TX_CONSENSUS` errors.
>
> @0xB10C was collecting data on how often we use this for compact block relay reconstruction I think? Correct me if I'm
...