772671245d test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
f227e153e8/src/net_processing.cpp (L2752-L2762)
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
f227e153e8/src/net_processing.cpp (L2368-L2372)
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d
achow101:
ACK 772671245d
brunoerg:
crACK 772671245d
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
588fad868d 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 588fad868d
Tree-SHA512: 1ab343a1b1fc57a7d6bd8363b84db9d96e8ea11a4cec85bcf79885c9df53da889fe2fb10b1fa92d824ddf0dee800c07353f46f1fea9887d2ad518bed0afebe3d
e4e17907b6 Modernize rpcauth.py and its tests (Pieter Wuille)
Pull request description:
Use Python3 constructions, and f-strings.
ACKs for top commit:
jamesob:
Github ACK e4e17907b6
Tree-SHA512: 005573d967e04400fec727f45739f138879be703e692745c0a639272d37d221d230f388de23f2615cb954bb47179fb46e53da0410ae9f0865319b91bb2dc01f4
3a11adc700 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:
b92d609fb2/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 3a11adc700
achow101:
ACK 3a11adc700
Tree-SHA512: c4e3dab452ad051da74855a13aa711892c9b34c43cc43a45a3b1688ab044e75d715b42843c229219761913b4861abccbcc8d5cb6ac54957d74f6e357f04e8730
561848aaf2 Exercise non-DIRTY spent coins in caches in fuzz test (Pieter Wuille)
59e6828bb5 Add deterministic mode to CCoinsViewCache (Pieter Wuille)
b0ff310840 Add CCoinsViewCache::SanityCheck() and use it in fuzz test (Pieter Wuille)
3c9cea1340 Add simulation-based CCoinsViewCache fuzzer (Pieter Wuille)
Pull request description:
The fuzzer goes through a sequence of operations that get applied to both a real stack of `CCoinsViewCache` objects, and to simulation data, comparing the two at the end.
ACKs for top commit:
jamesob:
re-ACK 561848aaf2
dergoegge:
Code review ACK 561848aaf2
Tree-SHA512: 68634f251fdb39436b128ecba093f651bff12ac11508dc9885253e57fd21efd44edf3b22b0f821c228175ec507df7d46c7f9f5404fc1eb8187fdbd136a5d5ee2
fa8e92c022 doc: Update ci docs (721217.xyz)
5fffff54e9 ci: Cache stuff in volumes, not host folders (MarcoFalke)
Pull request description:
Storing cached stuff in host system folders may lead to unexpected issues when the ci-built stuff is used for a non-ci build or a ci task leaks into another ci task.
ACKs for top commit:
john-moffett:
ACK fa8e92c022
Tree-SHA512: 8b0c9019452fbe507a272c1037c3dce3c178c21f85ab1096ed3372ad9d4b3c7aa27d89e5bf80c9a6260ea652e0268be0cbe61d6a4fcb3add569fa38076d32287
ab4efad51b test: fix immediate tx relay in wallet_groups.py (Sebastian Falbesoner)
Pull request description:
In the functional test wallet_groups.py we whitelist peers on all nodes (`-whitelist=noban@127.0.0.1`) to enable immediate tx relay for fast mempool synchronization. However, considering that this setting only applies to inbound peers and the default test topology looks like this:
```
node0 <--- node1 <---- node2 <--- ... <-- nodeN
```
txs propagate fast only from lower- to higher-numbered nodes (i.e. "left to right" in the above diagram) and take long from higher- to lower-numbered nodes ("right to left") since in the latter direction we only have outbound peers, where the trickle relay is still active. As a consequence, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
This PR fixes this by simply adding another connection from node0 to the last node, leading to a ~2-3x speedup (5 runs measured via `time ./test/functional/wallet_groups.py` are shown):
```
master:
0m53.31s real 0m08.22s user 0m05.60s system
0m32.85s real 0m07.44s user 0m04.08s system
0m46.40s real 0m09.18s user 0m04.23s system
0m46.96s real 0m11.10s user 0m05.74s system
0m57.23s real 0m10.53s user 0m05.59s system
PR:
0m19.64s real 0m09.58s user 0m05.50s system
0m18.05s real 0m07.77s user 0m04.03s system
0m18.99s real 0m07.90s user 0m04.25s system
0m17.49s real 0m07.56s user 0m03.92s system
0m18.11s real 0m07.74s user 0m03.88s system
```
Note that in most tests this is not a problem since txs very often originate from node0.
ACKs for top commit:
brunoerg:
utACK ab4efad51b
Tree-SHA512: 12675357e6eb5a18383f2bfe719a184c0790863b37a98749d8e757dd5dc3a36212e16a81f0a192340c11b793eda00db359c7011f46f7c27e3a093af4f5b62147
This is a "dumb" way of randomly generating a Miniscript node from
fuzzer input. It defines a strict binary encoding and will always generate
a node defined from the encoding without "helping" to create valid nodes.
It will cut through as soon as it encounters an invalid fragment so
hopefully the fuzzer can tend to learn the encoding and generate valid
nodes with a higher probability.
On a valid generated node a number of invariants are checked, especially
around the satisfactions and testing them against the Script
interpreter.
The node generation and testing is modular in order to later introduce
other ways to generate nodes from fuzzer inputs with minimal code.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
We'll need a better integration of the hash preimages PSBT fields to
satisfy Miniscript with such challenges from the RPC.
Thanks to Greg Sanders for his examples and suggestions to improve this
test.
This is a workaround for Miniscript descriptors containing hash
challenges. For those we can't mock the signature creator without making
OP_EQUAL mockable in the interpreter, so CalculateMaximumInputSize will
always return -1 and outputs for these descriptors would appear
unsolvable while they actually are.
Try to solve a script using the Miniscript satisfier if the legacy
solver fails under P2WSH context. Only solve public key and public key
hash challenges for now.
We don't entirely replace the raw solver and especially rule out trying to
solve CHECKMULTISIG-based multisigs with the Miniscript satisfier since
some features, such as the transaction input combiner, rely on the
specific behaviour of the former.
Cherry-picked and squashed from
https://github.com/sipa/bitcoin/commits/202302_miniscript_improve.
- Explain thresh() and multi() satisfaction algorithms
- Comment on and_v dissatisfaction
- Mark overcomplete thresh() dissats as malleable and explain
- Add comment on unnecessity of Malleable() in and_b dissat
When an encrypted wallet is locked (for instance via the
RPC `walletlock`), the docs indicate that the key is
removed from memory. However, the vector (with a secure
allocator) is merely cleared. This allows the key to persist
indefinitely in memory. Instead, manually fill the bytes with
zeroes before clearing.
To prepare for the removal of laanwj's key from trusted key, the trusted
git root needs to be newer than the most recent merge commit signed by
his key.
2d955ff006 net: add `Ensure{any}Banman` (brunoerg)
Pull request description:
This PR adds `Ensure{any}Banman` functions to avoid code repetition and make it cleaner. Same approach as done with argsman, chainman, connman and others.
ACKs for top commit:
davidgumberg:
ACK [2d955ff](2d955ff006)
Tree-SHA512: 0beb7125312168a3df130c1793a1412ab423ef0f46023bfe2a121630c79df7e55d3d143fcf053bd09e2d96e9385a7a04594635da3e5c6be0c5d3a9cafbe3b631
Use DBParams struct to remove ArgsManager uses from txdb.
To reduce size of this commit, this moves references to gArgs variable out of
txdb.cpp to calling code in chainstate.cpp. But these moves are temporary. The
gArgs references in chainstate.cpp are moved out to calling code in init.cpp in
later commits.
This commit does not change behavior.
Add CoinsViewOptions struct to remove ArgsManager uses from txdb.
To reduce size of this commit, this moves references to gArgs variable out of
txdb.cpp to calling code in validation.cpp. But these moves are temporary. The
gArgs references in validation.cpp are moved out to calling code in init.cpp in
later commits.
This commit does not change behavior.
Add DBParams and DBOptions structs to remove ArgsManager uses from dbwrapper.
To reduce size of this commit, this moves references to gArgs variable out of
dbwrapper.cpp to calling code in txdb.cpp. But these moves are temporary. The
gArgs references in txdb.cpp are moved out to calling code in init.cpp in later
commits.
This commit does not change behavior.
fa6f67837b test: Fix intermittent sync issue in wallet_pruning (721217.xyz)
Pull request description:
Setting the mocktime on each loop iteration will make net processing racy and cause a disconnect due to timeout.
Fix that by setting the mocktime only once.
Fixes https://github.com/bitcoin/bitcoin/issues/27065
ACKs for top commit:
brunoerg:
crACK fa6f67837b
Tree-SHA512: 128b962c05a6fa3caf3ce392e870fff6609ce2206a43bbae6661ecb45291df93bed77fe362a514d4472056f83fb6631df39a5170fa34e41a7577b9685dd26b1f
it adds `Ensure{any}Banman` functions to avoid
code repetition and make it cleaner. Similar
approach as done with argsman, chainman, connman
and others.
4de02def84 qt: Persist Mask Values option (Andrew Chow)
Pull request description:
The mask values option is memory only. If a user has enabled this option, it's reasonable to expect that they would want to have it enabled on the next start.
ACKs for top commit:
RandyMcMillan:
tACK 4de02def84
jarolrod:
tACK 4de02def84
pablomartin4btc:
> tACK [4de02de](4de02def84)
john-moffett:
tACK 4de02def84
Tree-SHA512: 247deb78df4911516625bf8b25d752feb480ce30eb31335cf9baeb07b7c6c225fcc37d5c45de62d6e6895ec10c7eefabb15527e3c9723a3b8ddda1e12ebbf46b
73a3b161b7 ci: Inline `MACOS_NATIVE_TASK_TEMPLATE` (Hennadii Stepanov)
8a61527cf6 ci: Re-introduce `depends_built` cache back in macOS and Android tasks (Hennadii Stepanov)
Pull request description:
This PR brings a `depends_built` cache back to the "macOS 10.15" and "ARM64 Android APK" CI tasks.
Fixes#27031.
ACKs for top commit:
MarcoFalke:
reACK 73a3b161b7🌻
Tree-SHA512: 2eb845f865ee2ee453c1fd284d5eeddbebb2653586b17946822fec03d46e73c5eb483499761a0de6c3c466b06623957664e22dee01f7312ad18e212f1c9c6439