b851c5385d fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses (Vasil Dimov)
Pull request description:
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
ACKs for top commit:
achow101:
ACK b851c5385d
mzumsande:
ACK b851c5385d
brunoerg:
utACK b851c5385d
Tree-SHA512: 9905acff0e996f30ddac0c14e5ee9e1db926c7751472c06d6441111304242b563f7c942b162b209d80e8fb65a97249792eef9ae0a96100419565bf7f59f59676
987a1b51ee init: settings, do not load auto-generated warning msg (furszy)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1907071391.
The settings warning message is meant to be used only to discourage users from
modifying the file manually. Therefore, there is no need to keep it in memory.
ACKs for top commit:
achow101:
ACK 987a1b51ee
ryanofsky:
Code review ACK 987a1b51ee. Seems like a clean, simple fix
Tree-SHA512: 3f2bdcf4b4a9cadb396dcff9b43155211eeed018527a07356970a341d139ad18edbd1a4d369377c8907b8ec1f19ee2ab8aacf85a887379e6d57a8a6db2403d51
c11c404281 tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d4 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1ed wallet: Skip key and script migration for blank wallets (Andrew Chow)
Pull request description:
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110
ACKs for top commit:
furszy:
reACK c11c404281. CI failure is unrelated.
ryanofsky:
Code review ACK c11c404281
Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
ff9039f6ea Remove GetAdjustedTime (dergoegge)
Pull request description:
This picks up parts of #25908.
The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains.
ACKs for top commit:
naumenkogs:
ACK ff9039f6ea
achow101:
ACK ff9039f6ea
maflcko:
lgtm ACK ff9039f6ea🤽
stickies-v:
ACK ff9039f6ea
Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
0bef1042ce net: enable v2transport by default (Pieter Wuille)
Pull request description:
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.
ACKs for top commit:
stratospher:
ACK 0bef104.
josibake:
reACK 0bef1042ce
achow101:
ACK 0bef1042ce
naumenkogs:
ACK 0bef1042ce
theStack:
ACK 0bef1042ce
willcl-ark:
crACK 0bef1042ce
BrandonOdiwuor:
utACK 0bef1042ce
pablomartin4btc:
re ACK 0bef1042ce
kristapsk:
utACK 0bef1042ce
Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
b298242c8d test: sqlite, add coverage for dangling to-be-reverted db txns (furszy)
fc0e747192 sqlite: guard against dangling to-be-reverted db transactions (furszy)
472d2ca981 sqlite: introduce HasActiveTxn method (furszy)
dca874e838 sqlite: add ability to interrupt statements (furszy)
fdf9f66909 test: wallet db, exercise deadlock after write failure (furszy)
Pull request description:
Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931.
If the db handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reverted; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the isolation property but also results in the
improper storage of incomplete information on disk, impacting
the wallet consistency.
This PR fixes the issue by resetting the db connection, automatically
rolling back the transaction (per https://www.sqlite.org/c3ref/close.html)
when the handler object is being destroyed and the txn abortion failed.
Testing Notes
Can verify the failure by reverting the fix e5217fea and running the test.
It will fail without e5217fea and pass with it.
ACKs for top commit:
achow101:
ACK b298242c8d
ryanofsky:
Code review ACK b298242c8d. Just fix for exec result codes and comment update since last review.
Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb
27f260aa6e net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92b15 test: add coverage for peerman adaptive connections service flags (furszy)
6ed53602ac net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e591c5 net: move state dependent peer services flags (furszy)
f9ac96b8d6 net: decouple state independent service flags from desirable ones (furszy)
97df4e3887 net: store best block tip time inside PeerManager (furszy)
Pull request description:
Derived from #28120 discussion.
By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.
The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections with limited peers, filling up all available outbound slots. Resulting
in an inability to synchronize the chain (because limited peers will not provide
blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).
ACKs for top commit:
achow101:
ACK 27f260aa6e
vasild:
ACK 27f260aa6e
naumenkogs:
ACK 27f260aa6e
mzumsande:
Light Code Review ACK 27f260aa6e
andrewtoth:
ACK 27f260aa6e
Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
If the handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reversed; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the database isolation property but also results
in the improper storage of incomplete information on disk, impacting
the wallet consistency.
By encapsulating sqlite3_exec into its own standalone method
and introducing the 'SQliteExecHandler' class, we enable the
ability to test db statements execution failures within the
unit test framework.
This is used in the following-up commit to exercise a deadlock
and improve our wallet db error handling code.
Moreover, the future encapsulation of other sqlite functions
within this class will contribute to minimize the impact of
any future API changes.
31cce4a1bd doc: update `BroadcastTransaction` comment (ismaelsadeeq)
Pull request description:
`BroadcastTransaction` is also called by `submitpackage` RPC.
All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
ea4ddd8652/src/rpc/mempool.cpp (L926)
It's not maintainable to list all the callers of a function.
ACKs for top commit:
stickies-v:
ACK 31cce4a1bd
kristapsk:
ACK 31cce4a1bd
naumenkogs:
ACK 31cce4a1bd
Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
9819db4cca validation: move nChainTx assert down in CheckBlockIndex (Martin Zumsande)
033477dba6 doc: fix checkblockindex comments (Martin Zumsande)
Pull request description:
The two assumptions there were described as test-only, which has led to confusion whether they should exist.
However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled.
The second commit moves this assert down to the other checks.
Closes #29261
ACKs for top commit:
maflcko:
ACK 9819db4cca 🌦
naumenkogs:
ACK 9819db4cca
ryanofsky:
Code review ACK 9819db4cca. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.
Tree-SHA512: 3f77791253eb0c97f8153dd8ae1c567f43f6387ea7a53efea94817463c672a4e11d548aa7eff62235346ff0713ff4d6fe08f9ec50d0c30a1e6b6d27b9918b419
bc9283c441 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher)
ffe6a56d75 [test] Check whether v2 TestNode performs downgrading (stratospher)
ba737358a3 [test] Add functional tests to test v2 P2P behaviour (stratospher)
4115cf9956 [test] Ignore BIP324 decoy messages (stratospher)
8c054aa04d [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher)
382894c3ac [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher)
a94e350ac0 [test] Build v2 P2P messages (stratospher)
bb7bffed79 [test] Use lock for sending P2P messages in test framework (stratospher)
5b91fb14ab [test] Read v2 P2P messages (stratospher)
05bddb20f5 [test] Perform initial v2 handshake (stratospher)
a049d1bd08 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher)
b89fa59e71 [test] Construct class to handle v2 P2P protocol functions (stratospher)
8d6c848a48 [test] Move MAGIC_BYTES to messages.py (stratospher)
595ad4b168 [test/crypto] Add ECDH (stratospher)
4487b80517 [rpc/net] Allow v2 p2p support in addconnection (stratospher)
Pull request description:
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
### commits overview
1. introduces a new class `EncryptedP2PState` to store the keys, functions for performing the initial v2 handshake and encryption/decryption.
3. this class is used by `P2PConnection` in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection)
- `v2_state` is the object of class `EncryptedP2PState` in `P2PConnection` used to store its keys, session-id etc.
- a node [advertising](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#advertising-to-support-v2-p2p) support for v2 P2P is different from a node actually [supporting v2 P2P](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#supporting-v2-p2p) (differ when false advertisement of services occur)
- introduce a boolean variable `supports_v2_p2p` in `P2PConnection` to denote if it supports v2 P2P.
- introduce a boolean variable `advertises_v2_p2p` to denote whether `P2PConnection` which mimics peer behaviour advertises V2 P2P support. Default option is `False`.
- In the test framework, you can create Inbound and Outbound connections to `TestNode`
1. During **Inbound Connections**, `P2PConnection` is the initiator [`TestNode` <--------- `P2PConnection`]
- Case 1:
- if the `TestNode` advertises/signals v2 P2P support (means `self.nodes[i]` set up with `"-v2transport=1"`), different behaviour will be exhibited based on whether:
1. `P2PConnection` supports v2 P2P
2. `P2PConnection` does not support v2 P2P
- In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if `P2PConnection` should support v2 P2P or not. So `supports_v2_p2p` boolean variable is used as an option to enable support for v2 P2P in `P2PConnection`.
- Since the `TestNode` advertises v2 P2P support (using "-v2transport=1"), our initiator `P2PConnection` would send:
1. (if the `P2PConnection` supports v2 P2P) ellswift + garbage bytes to initiate the connection
2. (if the `P2PConnection` does not support v2 P2P) version message to initiate the connection
- Case 2:
- if the `TestNode` doesn't signal v2 P2P support; `P2PConnection` being the initiator would send version message to initiate a connection.
2. During **Outbound Connections** [TestNode --------> P2PConnection]
- initiator `TestNode` would send:
- (if the `P2PConnection` advertises v2 P2P) ellswift + garbage bytes to initiate the connection
- (if the `P2PConnection` advertises v2 P2P) version message to initiate the connection
- Suppose `P2PConnection` advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario)
- `TestNode` sends ellswift + garbage bytes
- `P2PConnection` receives but can't process it and disconnects.
- `TestNode` then tries using v1 P2P and sends version message
- `P2PConnection` receives/processes this successfully and they communicate on v1 P2P
4. the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC
5. includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious
intermediary)
### run the tests
* functional test - `test/functional/p2p_v2_encrypted.py` `test/functional/p2p_v2_earlykeyresponse.py`
I'm also super grateful to @ dhruv for his really valuable feedback on this branch.
Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md
ACKs for top commit:
naumenkogs:
ACK bc9283c441
mzumsande:
Code Review ACK bc9283c441
theStack:
Code-review ACK bc9283c441
glozow:
ACK bc9283c441
Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
bbf218d061 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields)
4dbd0475d8 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields)
Pull request description:
Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.
Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.
Removing the define should have the effect of enabling sha256 optimizations for the kernel.
ACKs for top commit:
TheCharlatan:
Re-ACK bbf218d061
hebasto:
re-ACK bbf218d061
Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
8023640a71 qt: Avoid non-self-contained Windows header (Hennadii Stepanov)
Pull request description:
Using the `windows.h` header guarantees correctness regardless of the content of other headers.
For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio
Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.
Related to https://github.com/hebasto/bitcoin/pull/77.
ACKs for top commit:
theuni:
ACK 8023640a71. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.
Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
fa3373d3ad refactor: Compile unreachable code (MarcoFalke)
Pull request description:
When unreachable code isn't compiled, compile failures are not detected.
Fix this by leaving it unreachable, but compiling it.
Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916
ACKs for top commit:
achow101:
ACK fa3373d3ad
ryanofsky:
Code review ACK fa3373d3ad. This looks good, and should prevent code in the else blocks from accidentally breaking.
Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e
d298ff8b62 During IBD, prune as much as possible until we get close to where we will eventually keep blocks (Luke Dashjr)
Pull request description:
This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache.
Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks.
ACKs for top commit:
andrewtoth:
ACK d298ff8b62
fjahr:
utACK d298ff8b62
achow101:
ACK d298ff8b62
Tree-SHA512: 2a482376bfb177e2ba7c2f0bb0b58b02efdb38b34755a18d1fc3e869df5959c85b6f1009e1386fa8b89c4f90d520383e36bd3e21dec221042315134efb1a455b
00c1e2aa44 build: fix optimisation flags used for --coverage (fanquake)
1dc2c9b385 ci: cleanup C*FLAG usage in Valgrind jobs (fanquake)
6cc2a38c13 build: add sanitizer flags to configure output (fanquake)
08cd5aca18 build: always set -g -O2 in CORE_CXXFLAGS (fanquake)
Pull request description:
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults).
Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`.
Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here:
```bash
CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4
```
Fixes configure output to reflect actual compilation flag ordering, so it's useful.
Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize.
ACKs for top commit:
TheCharlatan:
lgtm ACK 00c1e2aa44
hebasto:
ACK 00c1e2aa44, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.
theuni:
ACK 00c1e2aa44
Tree-SHA512: cf6c7acf813ba10b198561e83eb72e9b2532a39cb1767c452d031e82921dcd42a47b129735b24c4e36131fd0c8fe7457f7cae870c1e011cdfdd430bdc4d4912b
The settings warning message is meant to be used only to discourage
users from modifying the file manually. Therefore, there is no need
to keep it in memory.
These exceptions are not related to situations specific to tests,
but are required in general:
Without the first check CheckBlockindex could fail for blocks where we
only know the header.
Without the second, it could fail when blocks are received out of order.
18ad1b9142 refactor: pass CRecipient to FundTransaction (josibake)
5ad19668db refactor: simplify `CreateRecipients` (josibake)
47353a608d refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c refactor: move parsing to new function (josibake)
6f569ac903 refactor: move normalization to new function (josibake)
435fe5cd96 test: add tests for fundrawtx and sendmany rpcs (josibake)
Pull request description:
## Motivation
The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.
As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.
I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.
ACKs for top commit:
S3RK:
reACK 18ad1b9142
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
6acec6b9ff multiprocess: Add type conversion code for UniValue types (Ryan Ofsky)
0cc74fce72 multiprocess: Add type conversion code for serializable types (Ryan Ofsky)
4aaee23921 test: add ipc test to test multiprocess type conversion code (Ryan Ofsky)
Pull request description:
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.
The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 6acec6b9ff
dergoegge:
reACK 6acec6b9ff
Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
e9014042a6 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a init: improve corrupted/empty settings file error msg (furszy)
Pull request description:
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Improving a confusing situation reported by users who did not understand why a
settings parsing error occurred when the file was empty and did not know how to solve it.
Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content.
In both scenarios, the 'Unable to parse settings file' error does not help the user move forward.
ACKs for top commit:
achow101:
ACK e9014042a6
hebasto:
re-ACK e9014042a6.
ryanofsky:
Code review ACK e9014042a6. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
32a9f13cb8 wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov)
Pull request description:
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
---
_Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit 856c88776f was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_
Avoid this unsafe pattern from `ArgsManager` and `CWallet`:
```cpp
class A
{
Mutex mutex;
Foo member GUARDED_BY(mutex);
const Foo& Get()
{
LOCK(mutex);
return member;
} // callers of `Get()` will have access to `member` without owning the mutex.
```
ACKs for top commit:
achow101:
ACK 32a9f13cb8
ryanofsky:
Code review ACK 32a9f13cb8. This seems like a potentially real race condition, and the fix here is pretty simple.
furszy:
ACK 32a9f13c
Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
d55fdb1a49 Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee6 wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)
Pull request description:
This is a bugfix for from when [optional was introduced](758501b713) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.
I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.
You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`.
ACKs for top commit:
0xB10C:
ACK d55fdb1a49
achow101:
ACK d55fdb1a49
murchandamus:
ACK d55fdb1a49
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
97181decf5 Add test for negative transaction version w/ CSV to tx_valid.json (Chris Stewart)
Pull request description:
This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by dergoegge
For more information see:
https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455
ACKs for top commit:
darosior:
ACK 97181decf5
dergoegge:
ACK 97181decf5
Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
This test-only RPC is required when a TestNode initiates
an outbound v2 p2p connection. Add a new arg `v2transport`
so that the node can attempt v2 connections.
Introduces functionality to detect when limited peers connections
are desirable or not. Ensuring that the new connections desirable
services flags stay relevant throughout the software's lifecycle.
(Unlike the previous approach, where once the validation IBD flag
was set, the desirable services flags remained constant forever).
This will let us recover from stalling scenarios where the node had
successfully synced, but subsequently dropped connections and remained
inactive for a duration longer than the limited peers threshold (the
timeframe within which limited peers can provide blocks). Then, upon
reconnection to the network, the node may end up only establishing
connections with limited peers, leading to an inability to synchronize
the chain.
This also fixes a possible limited peers threshold violation during IBD,
when the user configures `-maxtipage` further than the BIP159's limits.
This rule violation could lead to sync delays and, in the worst-case
scenario, trigger the same post-IBD stalling scenario (mentioned above)
but during IBD.
In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.
Hopefully, refraining users from modifying the file unless they are
certain about the potential consequences.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The preceding "Unable to parse settings file" message lacked
the necessary detail and guidance for users on what steps to
take next in order to resolve the startup error.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
282b12ddb0 refactor: remove CTxMemPool::queryHashes() (stickies-v)
Pull request description:
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
ACKs for top commit:
dergoegge:
Code review ACK 282b12ddb0
TheCharlatan:
ACK 282b12ddb0
glozow:
ACK 282b12ddb0. Looks like there's no conflicts.
Tree-SHA512: 16160dec8e1f2457fa0f62dc96d2d2efd92c4bab810ecdb0e08918b8e85a667702c8e41421eeb4ea6abe92a5956a2a39a7a6368514973b78be0d22de2ad299b2
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction`
take a `CRecipient` vector directly. This allows us to remove SFFO logic from
the wrapper RPC `FundTransaction` since the `CRecipient` objects have already
been created with the correct SFFO values. This also allows us to remove
SFFO from both `FundTransaction` function signatures.
This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.
Move validation logic out of `CreateRecipients` and instead take the
already validated outputs from `ParseOutputs` as an input.
Move SFFO parsing out of `CreateRecipients` into a new function,
`InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions
from `sendmany` and `sendtoaddress` and turns them into a set of integers.
In a later commit, we will also move the SFFO parsing logic from
`FundTransaction` into this function.
Worth noting: a user can pass duplicate addresses and addresses that dont exist
in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress`
without triggering a warning. This behavior is preserved in to keep this commit
strictly a refactor.
Move the parsing and validation out of `AddOutputs` into its own function,
`ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a
later commit, where the code is currently duplicated.
The new `ParseOutputs` function returns a CTxDestination,CAmount tuples.
This allows the caller to then translate the validated outputs into
either CRecipients or CTxOuts.
Move the univalue formatting logic out of AddOutputs and into its own function,
`NormalizeOutputs`. This allows us to re-use this logic in later commits.
5555d8db33 test: Use blocks_path where possible (MarcoFalke)
fa9108941f rpc: Fix race in loadtxoutset (MarcoFalke)
Pull request description:
The tip may have advanced, also if it did not, there is no reason to
have two variables point to the same block.
Fixes https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344694600
ACKs for top commit:
achow101:
ACK 5555d8db33
pablomartin4btc:
ACK 5555d8db33
BrandonOdiwuor:
Code Review ACK 5555d8db33
Tree-SHA512: 23a82924a915b61bb1adab8ad20ec8914139c8ee647817af34ca27ee310a2e45833d8b285503e0feebe63e4667193d6d98cfcbbc1509bf40712225e04dd19e8b
