backport: trivial 2024 10 23 pr8 #6351
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
…so it doesn't persist in memory 3a11adc Zero out wallet master key upon lock (John Moffett) Pull request description: When an encrypted wallet is locked (for instance via the RPC `walletlock`), the documentation indicates that the key is removed from memory: https://github.com/bitcoin/bitcoin/blob/b92d609fb25637ccda000e182da854d4b762eee9/src/wallet/rpc/encrypt.cpp#L157-L158 However, the vector (a `std::vector<unsigned char, secure_allocator<unsigned char>>`) is merely _cleared_. As it is a member variable, it also stays in scope as long as the wallet is loaded, preventing the secure allocator from deallocating. This allows the key to persist indefinitely in memory. I confirmed this behavior on my macOS machine by using an open-source third party memory inspector ("Bit Slicer"). I was able to find my wallet's master key in Bit Slicer after unlocking and re-locking my encrypted wallet. I then confirmed the key data was at the address in LLDB. This PR manually fills the bytes with zeroes before calling `clear()` by using our `memory_cleanse` function, which is designed to prevent the compiler from optimizing it away. I confirmed that it does remove the data from memory on my machine upon locking. Note: An alternative approach could be to call `vMasterKey.shrink_to_fit()` after the `clear()`, which would trigger the secure allocator's deallocation. However, `shrink_to_fit()` is not _guaranteed_ to actually change the vector's capacity, so I think it's unwise to rely on it. ## Edit: A little more clarity on why this is an improvement. Since `mlock`ed memory is guaranteed not to be swapped to disk and our threat model doesn't consider a super-user monitoring the memory in realtime, why is this an improvement? Most importantly, consider hibernation. Even `mlock`ed memory may get written to disk. From the `mlock` [manpage](https://man7.org/linux/man-pages/man2/mlock.2.html): > (But be aware that the suspend mode on laptops and some desktop computers will save a copy of the system's RAM to disk, regardless of memory locks.) As far as I can tell, this is true of [Windows](https://web.archive.org/web/20190127110059/https://blogs.msdn.microsoft.com/oldnewthing/20140207-00/?p=1833#:~:text=%5BThere%20does%20not%20appear%20to%20be%20any%20guarantee%20that%20the%20memory%20won%27t%20be%20written%20to%20disk%20while%20locked.%20As%20you%20noted%2C%20the%20machine%20may%20be%20hibernated%2C%20or%20it%20may%20be%20running%20in%20a%20VM%20that%20gets%20snapshotted.%20%2DRaymond%5D) and macOS as well. Therefore, a user with a strong OS password and a strong wallet passphrase could still have their keys stolen if a thief takes their (hibernated) machine and reads the permanent storage. ACKs for top commit: S3RK: Code review ACK 3a11adc achow101: ACK 3a11adc Tree-SHA512: c4e3dab452ad051da74855a13aa711892c9b34c43cc43a45a3b1688ab044e75d715b42843c229219761913b4861abccbcc8d5cb6ac54957d74f6e357f04e8730
588fad8 descriptors: fix docstring (param [in] vs [out]) (SomberNight) Pull request description: As in title, these docstrings look incorrect. ACKs for top commit: john-moffett: ACK 588fad8 Tree-SHA512: 1ab343a1b1fc57a7d6bd8363b84db9d96e8ea11a4cec85bcf79885c9df53da889fe2fb10b1fa92d824ddf0dee800c07353f46f1fea9887d2ad518bed0afebe3d
9066314 s/transcation/transaction/ (Greg Sanders) Pull request description: ACKs for top commit: fanquake: ACK 9066314 - looks like other comments are being addressed elsewhere. Tree-SHA512: c835a14db2e0cf5e0317c95c8c7441df1f7c6cb14be7809fd947e07ea9d23f1f171f111429aabd0509b7f17601bc742041316b18e1135e547a966961f2c65038
304ae6d doc: remove mention of "proper signing key" (fanquake) Pull request description: This key is no-longer in use: https://lists.linuxfoundation.org/pipermail/bitcoin-core-dev/2023-February/000115.html > Please remove it from verification pipelines. ACKs for top commit: hebasto: ACK 304ae6d Tree-SHA512: 3dfd221a48f69ac56b4568db06b5d5b5d6a60b7d027a26157912219a2073589a0a3934cb30e11a161d48db55d3a637338f96617e3f3b92cb9e60e0d1d1dd372a
…d `--timeout-factor 0`) 14302a4 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner) Pull request description: On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`: ``` $ ./test/functional/wallet_basic.py --timeout-factor 0 2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5 2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes node.wait_for_rpc_connection() File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection rpc.getblockcount() File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__ response, status = self._request('POST', self.__url.path, postdata.encode('utf-8')) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request self.__conn.request(method, path, postdata, headers) File "/usr/local/lib/python3.9/http/client.py", line 1285, in request self._send_request(method, url, body, headers, encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request self.endheaders(body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders self._send_output(message_body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output self.send(msg) File "/usr/local/lib/python3.9/http/client.py", line 980, in send self.connect() File "/usr/local/lib/python3.9/http/client.py", line 946, in connect self.sock = self._create_connection( File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection raise err File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection sock.connect(sa) OSError: [Errno 22] Invalid argument ``` This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail). // EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12. ACKs for top commit: MarcoFalke: lgtm ACK 14302a4 Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
4d84eae Raise PRNG seed log to INFO. (roconnor-blockstream) Pull request description: Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log (stdout/stderr) of the failed build. For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test. By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build. ACKs for top commit: MarcoFalke: lgtm ACK 4d84eae theStack: ACK 4d84eae Tree-SHA512: 3ccb4a4e7639a3babc3b2a6456a6d0bffc090da34e4545b317f7bfbed4e9950d1b38ea5b2a90c37ccb49b3454bdeff03a6aaf86770b9c4dd14b26320aba50b94
f36d1d5 Use void* throughout support/lockedpool.h (Jeffrey Czyz) Pull request description: Replace uses of char* with void* in Arena's member variables. Instead, cast to char* where needed in the implementation. Certain compiler environments disallow std::hash<char*> specializations to prevent hashing the pointer's value instead of the string contents. Thus, compilation fails when std::unordered_map is keyed by char*. Explicitly using void* is a workaround in such environments. For consistency, void* is used throughout all member variables similarly to the public interface. Changes to this code are covered by src/test/allocator_tests.cpp. ACKs for top commit: achow101: ACK f36d1d5 theStack: Code-review ACK f36d1d5 jonatack: ACK f36d1d5 review, debug build, unit tests, checked clang 15 raises "error: arithmetic on a pointer to void" without the conversions here from the generic void* pointer back to char* Tree-SHA512: f9074e6d29ef78c795a512a6e00e9b591e2ff34165d09b73eae9eef25098c59e543c194346fcd4e83185a39c430d43744b6f7f9d1728a132843c67bd27ea5189
PastaPastaPasta
force-pushed
the
develop-trivial-2024-10-23-pr8
branch
from
aeaf459
to
989f2ae
Compare
PastaPastaPasta
requested review from
UdjinM6,
knst and
kwvg
and removed request for
UdjinM6 and
knst
UdjinM6
requested changes
Oct 24, 2024
… docs 84ca5b3 doc: mention sanitizer suppressions in developer docs (fanquake) Pull request description: Should be enough to close bitcoin#17834. ACKs for top commit: MarcoFalke: lgtm ACK 84ca5b3 Tree-SHA512: 233c688a3cef1006c9a00f7b7a52fd6ee0ec150367e5e56904b6f1bbdca21b9217c69f8fcf653a4943613d12c3178a39f761b25eb24fc1954a563cfb1f832f5e
PastaPastaPasta
force-pushed
the
develop-trivial-2024-10-23-pr8
branch
from
989f2ae
to
bc6ffc6
Compare
knst
reviewed
Oct 24, 2024
| @@ -1,16 +1,5 @@ | |||
| ### Verify Binaries | |||
|
|
|||
| #### Preparation: | |||
There was a problem hiding this comment.
27107: just a note: maybe we want to dashify contrib/verifybinaries/README.md someday?
src/util/system.h
Outdated
| @@ -294,15 +294,13 @@ class ArgsManager | |||
| * Get data directory path | |||
| * | |||
| * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned | |||
There was a problem hiding this comment.
27176: we didn't backport 27073, which is dependency for this backport
29b62c0 valgrind: remove libsecp256k1 suppression (fanquake) Pull request description: I am no-longer been able to recreate this issue, atleast after the most recent libsecp256k1 changes. Can someone else confirm? ACKs for top commit: MarcoFalke: lgtm ACK 29b62c0 sipa: utACK 29b62c0 Tree-SHA512: e61e5b88ac2af3c779f23d999938bec4497f6433a029c07dd57a9481fe9cc104d8d8f63586910b29f41e66bbf0ed094bc7539c0df84754a1783c4b1b15af072e
…ture 8847ce4 util: add missing include and fix function signature (Cory Fields) Pull request description: ping hebasto Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it. Similar to the fix in bitcoin#27144. The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along. ACKs for top commit: fanquake: ACK 8847ce4 Tree-SHA512: 5eb9a6691ee37cbc5033a48aedcbf5c93af3b234614ae14c3fcc858f1ee505f630ad68f8bbb69ffa280080c8d0f91451362cb3819290b741ce906b2b3224a622
faa6715 test: Use self.wait_until over wait_until_helper (MarcoFalke) Pull request description: `wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling. ACKs for top commit: theStack: Code-review ACK faa6715 Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
…isten 5c938e7 Use string interpolation for default value of -listen (ekzyis) Pull request description: This is a refactoring change. So I have read the following and will try to answer why this change should be accepted > * Refactoring changes are only accepted if they are required for a feature or bug fix or **_otherwise improve developer experience significantly_**. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. I have noticed in bitcoin#26899 (comment) that the helper message for `-listen` does not use string interpolation. That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change. However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be). Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future. ACKs for top commit: stratospher: ACK 5c938e7. vasild: ACK 5c938e7 kristapsk: cr utACK 5c938e7 Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f
…e achieved 54c4d03 doc: Show how less noisy clang-tidy output can be achieved (TheCharlatan) Pull request description: Adds a paragraph to the clang-tidy section explaining how to de-noise its output. By default clang-tidy will print errors arrising from included headers in leveldb and other dependencies. By passing `--enable-suppress-external-warnings` flag to configure, errors arising from external dependencies are suppressed. Additional errors arrising from internal dependencies such as leveldb are suppressed by passing the `src/.bear-tidy-config` configuration file to bear. This file includes exclusionary rules for leveldb. ACKs for top commit: MarcoFalke: utACK 54c4d03 Tree-SHA512: c3dd8fb0600157582a38365a587e02e1d249fb246d6b8b4949a800fd05d3473dee49e2a4a556c60e51d6508feff810024e55fe09f5a0875f560fde30f3b6817c
PastaPastaPasta
force-pushed
the
develop-trivial-2024-10-23-pr8
branch
from
bc6ffc6
to
65f578c
Compare
knst
reviewed
Oct 24, 2024
doc/developer-notes.md
Outdated
|
|
||
| - *Rationale*: These functions do overflow checking and avoid pesky locale issues. | ||
|
|
||
| - Avoid using locale dependent functions if possible. You can use the provided | ||
| [`lint-locale-dependence.sh`](/test/lint/lint-locale-dependence.sh) | ||
| [`lint-locale-dependence.py`](/test/lint/lint-locale-dependence.py) |
There was a problem hiding this comment.
27220: we don't have lint-locale-dependence.py yet; can be done after bitcoin#24932 is backported
knst
reviewed
Oct 24, 2024
doc/developer-notes.md
Outdated
|
|
||
| - *Rationale*: These functions do overflow checking and avoid pesky locale issues. | ||
|
|
||
| - Avoid using locale dependent functions if possible. You can use the provided | ||
| [`lint-locale-dependence.sh`](/test/lint/lint-locale-dependence.sh) | ||
| [`lint-locale-dependence.py`](/test/lint/lint-locale-dependence.py) |
There was a problem hiding this comment.
27220: we don't have lint-locale-dependence.py yet; can be done after bitcoin#24932 is backported
8fcbdad util: fix argsman dupe key error (willcl-ark) Pull request description: fixes bitcoin#22638 Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when `settings.json` contains duplicate keys. This change has no effect on `bitcoind` because it treats a corrupt `settings.json` file as a hard error and doesn't attempt to modify it. If we find a duplicate key and error, clear `values` before returning so that `WriteSettings()` will write an empty file, therefore clearing it. This aligns with GUI behaviour added in 1ee6d0b. The test added only checks that `values` is empty after a duplicate key is detected. This paves the way for the `abort` option in the GUI to properly clear `settings.json`, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents). ACKs for top commit: TheCharlatan: Code review ACK 8fcbdad ryanofsky: Code review ACK 8fcbdad. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR: Tree-SHA512: a5fd49b30ede0a24188623192825bccb952e427cc35f96ff9bfdc737361dcc35ac6480589ddf7f0ddeaebd34361bdaee31e7a91f2c0d857e4ff682614bb6bc04
…ages impossible fa1eb0e test: Make the unlikely race in p2p_invalid_messages impossible (MarcoFalke) Pull request description: After `add_p2p_connection` both sides have the verack processed. However the pong from conn in reply to the ping from the node has not been processed and recorded in totalbytesrecv. Flush the pong from conn by sending a ping from conn. This should make the unlikely race impossible. ACKs for top commit: mzumsande: ACK fa1eb0e pinheadmz: ACK fa1eb0e Tree-SHA512: 44166587572e8c0c758cac460fcfd5cf403b2883880128b13dc62e7f74ca5cb8f145bb68a903df177ff0e62faa360f913fd409b009d4cd1360f1f4403ade39ae
PastaPastaPasta
force-pushed
the
develop-trivial-2024-10-23-pr8
branch
from
65f578c
to
7a3c56f
Compare
knst
previously approved these changes
Oct 24, 2024
contrib/guix/manifest.scm
Outdated
| @@ -28,6 +28,7 @@ | |||
| (gnu packages bison) | |||
| (gnu packages tls) | |||
| (gnu packages version-control) | |||
| (guix build-system cmake) | |||
There was a problem hiding this comment.
27779, 27179 - need to check GUIX build succeed
There was a problem hiding this comment.
dropped both for now. May investigate more which caused the issue
|
This pull request has conflicts, please rebase. |
|
guix fails |
fa67b81 Refactor: Remove unused FlatFilePos::SetNull (MarcoFalke) Pull request description: This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor. Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also bitcoin#26296 (comment)) If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method. ACKs for top commit: dergoegge: Code review ACK fa67b81 TheCharlatan: ACK fa67b81 john-moffett: ACK fa67b81 Tree-SHA512: 465c5e3eb4625405c445695d33e09a1fc5185c7dd1e766ba06034fb093880bfc65441d5334f7d9b20e2e417c2075557d86059f59d9648ca0e62a54c699c029b9
PastaPastaPasta
force-pushed
the
develop-trivial-2024-10-23-pr8
branch
from
7a3c56f
to
0dbafce
Compare
knst
approved these changes
Oct 24, 2024
UdjinM6
approved these changes
Oct 25, 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?
See commits
How Has This Been Tested?
built locally; large combined merge passed tests locally
Breaking Changes
Should be none
Checklist: