backport: trivial 2024 08 14 #6213
Merged
Conversation
This file contains 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
…ontext structs 3e33d17 Add ipc::Context and ipc::capnp::Context structs (Russell Yanofsky) Pull request description: These are currently empty structs but they will be used to pass some function and object pointers from bitcoin application code to IPC hooks that run, for example, when a remote object is created or destroyed, or a new process is created. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR bitcoin#10102. ACKs for top commit: ariard: Code Review ACK 3e33d17 Tree-SHA512: fd949fae5f1a973d39cb97f2745821ab2f62b98e166e53bc2801f97dcde988e18faaaaa0ffc2a82c170938b3a18078b6162fa35460e6e7c635e681b3c9e5b0a6
9ba7c44 refactor: get wallet path relative to wallet_dir (Michael Dietz) Pull request description: Now that boost has been updated > 1.60 (see bitcoin#22320), we can simplify how we get wallet path relative to wallet_dir by using: `boost::filesystem::lexically_relative`, removing a TODO. Test coverage comes from `test/functional/wallet_multiwallet.py` I first tried this in bitcoin#20265 which was my first attempted PR, and funny enough exactly 1 year later I'm opening this one to hopefully finally close this. ACKs for top commit: ryanofsky: Code review ACK 9ba7c44. Basically this same code change is made in bitcoin#20744 commit b70c843, so this PR helps simplify that one lsilva01: Code Review ACK 9ba7c44 Tree-SHA512: 6ccb91a18bcb52c3ae0c789a94a18fb5be7db7769fd1121552d63f259fbd32b50c3dcf169cec0b02f978321db3bc60eb4b881b8327e9764f32e700236e0d8a35
fa52a86 fuzz: Rework rpc fuzz target (MarcoFalke) Pull request description: Changes (reason): * Return `void` in `CallRPC` (the result is unused anyway) * Reduce the `catch`-scope of `std::runtime_error` to `RPCConvertValues` (Code clarity and easier bug-finding) * Crash when an internal bug is detected (bugs are bad) ACKs for top commit: shaavan: Code Review ACK fa52a86 Tree-SHA512: 576411a0e50bca9be3e6ffaf745001b1808fd37029251f8ec2c279e0671efe91d43dd81fd4ca26871c28b119e593ee2a0043d4b75f44da578f17541ee3afd696
fa77f95 fuzz: Fix RPC internal bug detection (MarcoFalke) Pull request description: Previously the fuzz test considered any exception which contains the string `Internal bug detected` (magic string) as a bug. This is not true when the user (fuzzer) passes in the magic string from outside. Fix that by: 1. Changing the format the string in `NonFatalCheckError` to start with the magic string. 2. Only treat exceptions that start with the magic string as internal bugs. This should fix the bug because any other exception shouldn't start with the magic string. To test: ``` echo 'bG9nZ2luZ1y+bUludGVybmFsIGJ1ZyBkZXRlY3RlZAAXCqNcjqNcjuYjeg==' | base64 --decode > /tmp/a FUZZ=rpc ./src/test/fuzz/fuzz /tmp/a ``` Before: ``` fuzz: test/fuzz/rpc.cpp:365: void rpc_fuzz_target(FuzzBufferType): Assertion `error_msg.find("trigger_internal_bug") != std::string::npos' failed. ``` After: ``` Executed /tmp/a in 0 ms ACKs for top commit: shaavan: crACK fa77f95 Tree-SHA512: 079bc97b6ce0cbad8603c7b577cc1ac0fd19e884ccbaba317588b91d98b36afeaa8cb398344b52bf12c9fd1737b3fdd8452b4e833a3b06cb3c789651955f78b8
…#20744 d216bc8 Re-enable walletinit_verify_walletdir_no_trailing2 test disabled in bitcoin#20744 (Ryan Ofsky) 80cd64e Re-enable util_datadir check disabled in bitcoin#20744 (Ryan Ofsky) Pull request description: Reenable some broken tests as discussed bitcoin#20744 (comment) and bitcoin#20744 (comment) Fix windows test cases broken in bitcoin#20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests. It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine. ACKs for top commit: hebasto: ACK d216bc8, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 2099ddfa1a3ad70f7ac2ff413929414a1851d257b280da25c0f5cefb46fd1372b580a1f1ee5280681a1c16e6031f119185cadd4f7a6121298562cf001f711068
ee822d8 util: use stronger-guarantee rename method (Vasil Dimov) Pull request description: Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case. This is a rebase of bitcoin#20435 by vasild. ACKs for top commit: MarcoFalke: review ACK ee822d8 hebasto: Approach ACK ee822d8. vasild: ACK ee822d8 Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a
…ssue b223c3c test: Add functional test for symlinked blocks directory (laanwj) ddb75c2 test: Add fs_tests/create_directories unit test (Hennadii Stepanov) 1f46b6e util: Work around libstdc++ create_directories issue (laanwj) Pull request description: Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes bitcoin#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than bitcoin#24266, which worked around one instance of the problem. The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while. This introduces a function `fs::create_directories` which wraps `std::filesystem::create_directories`. This allows easiliy reverting the workaround when it is no longer necessary. ACKs for top commit: jonatack: re-ACK b223c3c per `git range-diff df08250 67019cd b223c3c` hebasto: re-ACK b223c3c w0xlt: re-ACK b223c3c vasild: ACK b223c3c Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc
knst
reviewed
Aug 14, 2024
PastaPastaPasta
force-pushed
the
develop-trivial-2024-08-14
branch
from
275e53b to
7e87d97
Compare
…hile attaching chain 2052e3a wallet: ignore chainStateFlushed notifications while attaching chain (Martin Zumsande) Pull request description: Fixes bitcoin#24487 When a rescan is performed during `CWallet::AttachChain()` (e.g. when loading an old wallet) but this is interrupted by a shutdown signal, the wallet will currently stop the rescan, receive a `chainStateFlushed` signal, set the saved best block to the tip and shut down. At next startup, the rescan is not continued or repeated because of this. But some blocks have never been scanned by the wallet, which could lead to an incorrect balance. Fix this by ignoring `chainStateFlushed` notifications until the chain is attached. Since `CWallet::chainStateFlushed` is being manually called by `AttachChain()` anyway after finishing with the rescan, it is not a problem if intermediate notifications are ignored. Manual rescans started / aborted by the `rescanblockchain` / `abortrescan` RPCs are not affected by this. I didn't choose alternative ways of fixing this issue that would delay the validationinterface registration or change anything else about the handling of `blockConnected` signals for the reasons mentioned in [this existing comment](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2937-L2944). ACKs for top commit: achow101: ACK 2052e3a ryanofsky: Code review ACK 2052e3a. This is a straightforward fix for the bug described in bitcoin#24487 where a wallet could skip scanning blocks if is shut down in the middle of a sync and a chainStateFlushed notification was received during the sync. It would be nice to write a test for this but probably would be tricky to write. w0xlt: Code Review ACK bitcoin@2052e3a Tree-SHA512: a6186173d72b26bd4adbf2315e11af365004a723ea5565a0f7b868584dc47c321a6572eafaeb2420bd21eed1c7ad92b47e6218c5eb72313a3c6bee58364e2247
…egistering for signals ba10b90 Wallet: Ensure m_attaching_chain is set before registering for signals (Luke Dashjr) Pull request description: Avoids a race where chainStateFlushed could be called before rescanning began, yet rescan gets interrupted or fails Followup for bitcoin#24984 avoiding a race between registering and setting the flag. ACKs for top commit: mzumsande: Code Review ACK ba10b90 achow101: ACK ba10b90 Tree-SHA512: 1d2fa2db189d3e87f2d0863cf2ab62166094436483f0da16760b1083a4743bf08e476a3277e0d36564213d65dd6f0a1fc16a4bf68d3338c991a14d1de9fc0fee
…No such file or directory 44904aa multiprocess build cleanup: comment on manual dependencies (Ryan Ofsky) 6e1c16c multiprocess build fix: ipc/capnp/init.capnp.h: No such file or directory (Ryan Ofsky) Pull request description: Error was reported by SatoriHoshiAiko in bitcoin#25207 and happens unpredictably because make doesn't always build dependencies in the same order. The source file `src/ipc/capnp/protocol.cpp` includes some generated headers so needs to have an explicit dependency specified in the makefile so the headers will be generated before the file is compiled. bitcoin#19160 added the explicit dependency, but it was incorrect because it referred to an old file path from before the source file was renamed (`ipc.cpp` -> `protocol.cpp`) ACKs for top commit: hebasto: re-ACK 44904aa Tree-SHA512: 578ef4ef35a97e9d8d4e6d4edf39e57a32674234bf145e8159268324cb6ba15b420517afc07f6f42bb11a562954ea74af686c3fb92ce178c1fc378421b352531
UdjinM6
previously approved these changes
Aug 14, 2024
UdjinM6
dismissed
their stale review
oops.. code looks good but feature_dirsymlinks.py fails in CI for some reason
knst
approved these changes
Aug 14, 2024
PastaPastaPasta
force-pushed
the
develop-trivial-2024-08-14
branch
from
7e87d97 to
c8092fb
Compare
knst
approved these changes
Aug 21, 2024
UdjinM6
approved these changes
Aug 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue being fixed or feature implemented
Batch of trivial backports
What was done?
How Has This Been Tested?
Built and tests locally
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.