blockstorage: flush dirty undo files during state flush#103
Draft
blockstorage: flush dirty undo files during state flush#103
Conversation
This was introduced by commit ab9edbd. It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here. This commit fixes the situation. Also in this commit: - Fix script_tests to adjust to the corrected error message - Fix p2p_invalid_tx functional test to produce the desired error message
Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used. It returns `false` if there is at least one pubkey in the descriptor for which the provider does not have a private key. ToPrivateString() behaviour will change in the following commits to only return `false` if no priv keys could be found for the pub keys in the descriptor. HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining if a descriptor is 'watchonly'. Co-authored-by: rkrux <rkrux.connect@gmail.com>
ToPrivateString() behaviour will be modified in the following commits. In order to keep the scope of this PR limited to the RPC behaviour, this commit updates wallet migration to use 'Descriptor::HavePrivateKeys()' in place of 'Descriptor::ToPrivateString()' to determine watchonly descriptors. A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the watchonly migration wallet.
This commit modifies the Pubkey providers to return the public string if private data is not available. This is setup for a future commit to make Descriptor::ToPrivateString return strings with missing private key information. Co-authored-by: rkrux <rkrux.connect@gmail.com>
- Refactor Descriptor::ToPrivateString() to allow descriptors with missing private keys to be printed. Useful in descriptors with multiple keys e.g tr() etc. - The existing behaviour of listdescriptors is preserved as much as possible, if no private keys are availablle ToPrivateString will return false
Co-authored-by: rkrux <rkrux.connect@gmail.com>
The addrman field is already a reference. However, some tests would benefit from the reference being re-seatable, so that they do not have to create a full Connman each time.
This makes IWYU suggest `<cassert>` over `<assert.h>`.
- Add test for multiple transactions to same address - Add test for invalid address format error
… or by move-construction fa64d84 refactor: Enforce readability-avoid-const-params-in-decls (MarcoFalke) faf0c2d refactor: Avoid copies by using const references or by move-construction (MarcoFalke) Pull request description: Top level `const` in declarations is problematic for many reasons: * It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value. * In constructors it prevents move construction. * It can incorrectly imply some data is const, like in an imaginary example `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are *not* const. * The compiler ignores the `const` from the declaration in the implementation. * It isn't used consistently anyway, not even on the same line. Fix some issues by: * Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy. * Using move-construction to avoid a copy * Applying `readability-avoid-const-params-in-decls` via clang-tidy ACKs for top commit: l0rinc: diff reACK fa64d84 hebasto: ACK fa64d84, I have reviewed the code and it looks OK. sedited: ACK fa64d84 Tree-SHA512: 293c000b4ebf8fdcc75259eb0283a2e4e7892c73facfb5c3182464d6cb6a868b7f4a6682d664426bf2edecd665cf839d790bef0bae43a8c3bf1ddfdd3d068d38
… them as errors a5a8c41 ci, iwyu: Fix warnings in `src/kernel` and treat them as errors (Hennadii Stepanov) Pull request description: Now seems like a good time to update the includes in `src/kernel`. ACKs for top commit: maflcko: review ACK a5a8c41 🍱 purpleKarrot: ACK a5a8c41 sedited: ACK a5a8c41 Tree-SHA512: ba401b27b03dee66d52d0b348972268e162506c4bafa40f408349173b68c40a11f20ca24f46c98945515e1d5c84f740d6e6784f7e4c799df46ab816cf5d11483
Also, util/string and util/strencodings
This can be reviewed with the git option --color-moved=dimmed-zebra
…ve from libkernel faf07bd doc: Fix typo found by LLM (MarcoFalke) faf6667 refactor: [move-only] Merge core_io module (MarcoFalke) fa6947f kernel: Remove unused core_read.cpp from kernel (MarcoFalke) Pull request description: Currently the core_io module is split across two translation units. This will confuse code readers and tooling about the real state of the module. Fix that by merging the module and removing the mapping workarounds. Also, remove the module from the kernel lib, because it is not used there: The kernel does not use any json or string parsing or formatting. ACKs for top commit: hebasto: re-ACK faf07bd, only rebased since my recent [review](bitcoin#34296 (review)). sedited: Re-ACK faf07bd stickies-v: ACK faf07bd Tree-SHA512: 3f5d91f1a4cb86dfe329b28ff31e93d65f2f0659a6f6f2de22ca6fb65056fb818ae369ef0ad773d4f5b92f63891a7a9450246377d8e14c34bc43f3deee0554cb
`IWYU pragma: export` enforces the transitive inclusion of the headers, which undermines the purpose of IWYU. The remained cases seem useful and could be considered separately: - `<cassert>` in `util/check.h` - `<filesystem>` in `util/fs.h` - `<chrono>` in `util/time.h`
This project uses angle brackets instead of quotes for project-specific headers. Setting MainIncludeChar enables clang-format to automatically detect the main header, so it can be kept as the top group of includes. For example, without this change, the below command would demote <signet.h> from being the main header. With this change, the order is preserved. `clang-format -i src/signet.cpp`
…_IF evaluation a7b5814 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu) Pull request description: This was introduced by commit ab9edbd. It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here. This commit fixes the situation. EDIT: Note this turns out to be a dupe of the abandoned bitcoin#30359 . ACKs for top commit: billymcbip: tACK a7b5814 achow101: ACK a7b5814 darosior: utACK a7b5814 sedited: ACK a7b5814 Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
…ng tests for getreceivedbyaddress d45ec3f test: Add getreceivedbyaddress coverage to wallet_listreceivedby (b-l-u-e) Pull request description: This PR adds comprehensive functional test coverage for the `getreceivedbyaddress` RPC method. ACKs for top commit: maflcko: lgtm ACK d45ec3f fjahr: reACK d45ec3f achow101: ACK d45ec3f rkrux: lgtm ACK d45ec3f Tree-SHA512: e7f024297c18b2e11da108d9588bbf96089dce24e2fdee255dbd2f754f21ec63cb3cefa6d92b621b5ab66d18fe29523b87d14ceba38a83afa4c85eb5944a0fb3
0dafc0d clang-format: use AngleBracket for main includes (stickies-v) Pull request description: This project uses angle brackets instead of quotes for project-specific headers. Setting [`MainIncludeChar`](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#mainincludechar) enables `clang-format` to automatically detect the main header, so it can be kept as the top group of includes. For example, without this change, `clang-format` would demote `<signet.h>` from being the main header in `src/signet.cpp`. With this change, the order is preserved. On 5e49f5d: ``` % clang-format src/signet.cpp | head -n 15 // Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <consensus/merkle.h> #include <consensus/params.h> #include <consensus/validation.h> #include <logging.h> #include <primitives/block.h> #include <primitives/transaction.h> #include <script/interpreter.h> #include <script/script.h> #include <signet.h> #include <streams.h> #include <uint256.h> ``` With this PR: ``` % clang-format src/signet.cpp | head -n 10 // Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <signet.h> #include <consensus/merkle.h> #include <consensus/params.h> #include <consensus/validation.h> #include <logging.h> ``` Note: `AngleBracket` `requires clang-format 19`, and will cause older versions (including our current minimum llvm version `17`) to fail ACKs for top commit: maflcko: review ACK 0dafc0d sedited: Nice, ACK 0dafc0d hebasto: ACK 0dafc0d, tested on Ubuntu 25.10. Tree-SHA512: c0876f505ec188f76e435af0731c411c66266b83e4c08528d0637263abcd84b3968ee6fbfa72630192f1a0cd2728af873d3d6c32f93ab8b228222fad16f232be
03f363d doc: Document IWYU workaround (Hennadii Stepanov) Pull request description: This PR addresses the following comments: - bitcoin#34079 (comment): > it would be good to reduce and report this bug upstream. Otherwise, wide-spread use of iwyu in this code-base seems risky. - bitcoin#34079 (comment): > Would have been good if it was documented, rather than adding undocumented workarounds for buggy tools. ACKs for top commit: maflcko: lgtm ACK 03f363d sedited: ACK 03f363d Tree-SHA512: 160a963c07f853995c8b4741a6ccca1d8431a576c760fca082116cebde4d133f7c8ec51f09e8f85f54428f86bad2635e1bd708177eecf71feb0bf1489f1e2b3e
…U usage d938947 doc: Add "Using IWYU" to Developer Notes (Hennadii Stepanov) e1a90bc iwyu: Do not export `crypto/hex_base.h` header (Hennadii Stepanov) 19a2edd iwyu: Do not export C++ headers in most cases (Hennadii Stepanov) Pull request description: First two commits address comments from discussion in bitcoin#33779: - bitcoin#33779 (comment) - bitcoin#33779 (comment) The last commit adds a new section to the Developer Notes to document IWYU usage. ACKs for top commit: maflcko: re-ACK d938947 🚀 Tree-SHA512: 8d6c63e9d2fd190815211d80e654cb7379d16b6611b8851444f49bbbaa0fbc93557675fbcc558afd9a1cdf643570fba5eff9c1aecb5530f978797387b10b9a11
9482f00 chore: Update outdated GitHub Actions versions (Padraic Slattery) Pull request description: This PR updates outdated GitHub Action versions to ensure compatibility and improve functionality. The following changes are made to the GitHub Actions: - `actions/upload-artifact` updated from v4 to v6 - `actions/cache` updated from v4 to v5 - `actions/download-artifact` updated from v5 to v7 The updates are necessary to support newer environments and features, and ensure consistent behavior across different workflows. The changes will be tested in the CI pipeline of the pull request. ACKs for top commit: fanquake: ACK 9482f00 Tree-SHA512: 248e79162c5b2748e1a367d87a360d62eb961c24b4f8060bb932ef99a79ef10cab3e65175c092226c90140f31686fb9424911e6609729cb186b304b598a9af44
de509c6 iwyu: Add missed line to IWYU patch (Hennadii Stepanov) Pull request description: This PR makes IWYU suggest `<cassert>` over `<assert.h>`. Fixes bitcoin#34237. ACKs for top commit: maflcko: lgtm ACK de509c6 Tree-SHA512: edba91eaf36992f684be2920f5da8c13a25ba6d79b879b92193e2af106cd454a64d7c4cf9dabc25675490df9edbccff1fd54c9f393e984a3a7a628b1c65f6c53
…essage(s) fuzz targets fabf8d1 fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke) fac7fed refactor: Use std::reference_wrapper<AddrMan> in Connman (MarcoFalke) Pull request description: *Found and reported by Crypt-iQ (thanks!)* Currently the process_message(s) fuzz targets do not have any meaningful `SendMessages` code coverage. This is not ideal. Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future. ### Historic context for this regression The regression was introduced in commit fa11eea, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace `g_setup->m_node.peerman->SendMessages(&p2p_node);` with `peerman->SendMessages(&p2p_node);`. This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used. A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure. Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman. So fix all issues by: * Allowing the addrman reference in connman to be re-seatable * Clearing all stale objects, before creating new objects, and then using references to the new objects in all code ACKs for top commit: Crypt-iQ: crACK fabf8d1 frankomosh: ACK fabf8d1 marcofleon: code review ACK fabf8d1 sedited: ACK fabf8d1 Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
…urn descriptors with private key information when wallet contains descriptors missing any key 9c7e477 test: Test listdescs with priv works even with missing priv keys (Novo) ed945a6 walletrpc: reject listdes with priv key on w-only wallets (Novo) 9e5e982 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo) 5c4db25 descriptor: refactor ToPrivateString for providers (Novo) 2dc74e3 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo) e842eb9 descriptors: add HavePrivateKeys() (Novo) Pull request description: _TLDR: Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_ In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([bitcoin#32078 (comment)](bitcoin#32078 (comment))). This change makes it possible to do so. This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys. ### Notes - The new behaviour is applied to all descriptors including miniscript descriptors - `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour bitcoin#24361 (comment) - Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet. ### Relevant PRs bitcoin#24361 bitcoin#32186 ### Testing Functional tests were added to test the new behaviour EDIT **`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error** ACKs for top commit: Sjors: ACK 9c7e477 achow101: ACK 9c7e477 w0xlt: reACK bitcoin@9c7e477 with minor nits rkrux: re-ACK 9c7e477 Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
Add unit and functional coverage for a situation where undo is written to an older `rev?????.dat` after the block cursor has advanced to a newer `blk?????.dat`. Replace `rev00000.dat` with a directory and verify that forcing a flush does not try to flush the older undo file.
Undo data can be written to older `rev?????.dat` files after the block cursor has advanced, for example during IBD when blocks are received out of order. `FlushChainstateBlockFile` previously only flushed the current block file, allowing `WriteBlockIndexDB` to persist `BLOCK_HAVE_UNDO` without ensuring the corresponding undo data was committed to disk. Flush all dirty undo files before returning so the subsequent block index database write is crash safe. Update unit and functional tests to ensure the older undo file is flushed and that a flush failure aborts the node. This can be reproduced on a real system by forcing a hard power-off after a flush while older undo data is still in the OS page cache, and then restarting and running `verifychain 2 0` to observe missing or bad undo data. Co-authored-by: Crypt-iQ <Crypt-iQ@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP