c11c404281d2d0e22967e30e16c0733db84f4eee tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6a371f26474410397da435547e58786 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d43b802db12768e620588ed179e92b29 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1edb6b9a607bf1ad247893347252a38e3 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 c11c404281d2d0e22967e30e16c0733db84f4eee. CI failure is unrelated.
ryanofsky:
Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee
Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
0bef1042ce6c459acb1de965cbccd98867a417f1 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 0bef1042ce6c459acb1de965cbccd98867a417f1
naumenkogs:
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
theStack:
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
willcl-ark:
crACK 0bef1042ce6c459acb1de965cbccd98867a417f1
BrandonOdiwuor:
utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
pablomartin4btc:
re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
kristapsk:
utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
26ad2aeb29dd0875e8509917ddaa586997e977d2 test: fix wallet_import_rescan unrounded minimum amount (stickies-v)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1468842089.
Fixes a `JSONRPCException: Invalid amount (-3)` exception by ensuring the amount sent to `sendtoaddress` is rounded to 8 decimals.
See https://cirrus-ci.com/task/5562947183837184?logs=ci#L2559
Note: since `round` can also round down, `min_amount` is not _exactly_ guaranteed, but this is not a problem for the current usage. I've added a docstring to highlight this.
ACKs for top commit:
sr-gi:
ACK [26ad2ae](26ad2aeb29)
Tree-SHA512: 82ce16447f30535f17fa73336f7e4f74639e33215a228294b9b8005b8050a760b90a3726de279cce98c7e439f09104172b74072be3a300dbd461bf0c3f54b954
55556a64a8e4e6238f990cf66295c3b9594c2c3d test: Remove struct import from messages.py (MarcoFalke)
fa3fa86ddaaa2246e873b7a3f19bc589a3f46c11 scripted-diff: test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fafc0d68eef9c9381b1a3d8e160aad9eeb8540a7 test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa test: Treat msg_version.relay as unsigned (MarcoFalke)
Pull request description:
`struct` has many issues in messages.py:
* For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
* For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
* For packing and unpacking of a single value, the format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
I presume the above issues lead to accidentally treat `msg_version.relay` as a "signed bool", which is fine, but confusing.
Fix all issues by using the built-in `int` helpers `to_bytes` and `from_bytes` via a scripted diff.
Review notes:
* `struct.unpack` throws an error if the number of bytes passed is incorrect. `int.from_bytes` doesn't know about "missing" bytes and treats an empty byte array as `int(0)`. "Extraneous" bytes should never happen, because all `read` calls are limited in this file. If it is important to keep this error behavior, a helper `int_from_stream(stream, num_bytes, bytes, byteorder, *, **kwargs)` can be added, which checks the number of bytes read from the stream.
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
ACKs for top commit:
stickies-v:
ACK 55556a64a8e4e6238f990cf66295c3b9594c2c3d
theStack:
re-ACK 55556a64a8e4e6238f990cf66295c3b9594c2c3d
Tree-SHA512: 1cef8cdfd763fb424ed4b8be850a834b83fd0ef47fbea626a29784eb4f4832d44e42c4fe05b298b6070a933ef278b0222289a9955a97c86707e091e20bbb247a
bc9283c4415a932ec1eeb70ca2aa4399c80437b3 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher)
ffe6a56d75c0b47d0729e4e0b7225a827b43ad89 [test] Check whether v2 TestNode performs downgrading (stratospher)
ba737358a37438c18f0fba723eab10ccfd9aae9b [test] Add functional tests to test v2 P2P behaviour (stratospher)
4115cf995647d1a513caecb54a4ff3f51927aa8e [test] Ignore BIP324 decoy messages (stratospher)
8c054aa04d33b247744b3747cd5bf3005a013e90 [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher)
382894c3acd2dbf3e4198814f547c75b6fb17706 [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher)
a94e350ac0e5b65ef23a84b05fb10d1204c98c97 [test] Build v2 P2P messages (stratospher)
bb7bffed799dc5ad8b606768164fce46d4cbf9d0 [test] Use lock for sending P2P messages in test framework (stratospher)
5b91fb14aba7d7fe45c9ac364526815bec742356 [test] Read v2 P2P messages (stratospher)
05bddb20f5cc9036fd680500bde8ece70dbf0646 [test] Perform initial v2 handshake (stratospher)
a049d1bd08c8cdb3b693520f24f8a82572dcaab1 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher)
b89fa59e715a185d9fa7fce089dad4273d3b1532 [test] Construct class to handle v2 P2P protocol functions (stratospher)
8d6c848a48530893ca40be5c1285541b3e7a94f3 [test] Move MAGIC_BYTES to messages.py (stratospher)
595ad4b16880ae1f23463ca9985381c8eae945d8 [test/crypto] Add ECDH (stratospher)
4487b8051797173c7ab432e75efa370afb03b529 [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 bc9283c4415a932ec1eeb70ca2aa4399c80437b3
mzumsande:
Code Review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
theStack:
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
glozow:
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
The C++ code treats bool as uint8_t, so the python tests should as well.
This also allows to simplify the code, because converting an empty byte
array to int gives int(0).
>>> int.from_bytes(b'')
0
3bfc5bd36e38ca07306a41a3f56ea102ffe02b67 test: ensure output is large enough to pay for its fees (stickies-v)
Pull request description:
Fixes a (rare) intermittency issue in wallet_import_rescan.py
Since we [use](03752444cd/test/functional/wallet_import_rescan.py (L296)) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.
Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log
```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is rescanned as well if the rescan parameter is set to true
2024-01-18T22:16:20.187000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_import_rescan.py", line 292, in run_test
child = self.nodes[1].send(
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: The transaction amount is too small to pay the fee (-4)
```
Can be reproduced locally by forcing usage of the lowest possible value produced by `get_rand_amount()` ([thanks furszy](https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1832956095)):
<details>
<summary>git diff on 5f3a0574c4</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 7f01d23941..925849d5c0 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -270,7 +270,7 @@ class ImportRescanTest(BitcoinTestFramework):
address_type=variant.address_type.value,
))
variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
- variant.initial_amount = get_rand_amount() * 2
+ variant.initial_amount = Decimal(str(round(AMOUNT_DUST, 8))) * 2
variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
variant.confirmation_height = 0
variant.timestamp = timestamp
```
</details>
ACKs for top commit:
achow101:
ACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67
glozow:
utACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67, didn't experience this issue but in theory a minimum of `AMOUNT_DUST` could be too low to pay the fees
furszy:
utACK 3bfc5bd36
Tree-SHA512: 821ab94a510772e90528b2cef368bbf70309d8fd1dcda53dce75dd1bf91622358e80fea4d9fc68249b9d598892306c66f6c843b4a6855a9f9a9175f7b41109c6
- A node initiates a v2 connection by sending 64 bytes ellswift
- In BIP 324 "The responder waits until one byte is received which does not match the
V1_PREFIX (16 bytes consisting of the network magic followed by "version\x00\x00\x00\x00\x00".)"
- It's possible that the 64 bytes ellswift sent by an initiator starts with a prefix of V1_PREFIX
- Example form of 64 bytes ellswift could be:
4 bytes network magic + 60 bytes which aren't prefixed with remaining V1_PREFIX
- We test this behaviour:
- when responder receives 4 byte network magic -> no response received by initiator
- when first mismatch happens -> response received by initiator
- Add an optional `supports_v2_p2p` parameter to specify if the inbound
and outbound connections support v2 P2P protocol.
- In the `addconnection_callback` which gets called when creating
outbound connections, call the `addconnection` RPC with v2 P2P protocol
support enabled.
- When a v2 TestNode makes an outbound connection to a P2PInterface node
which doesn't support v2 but is advertised as v2 by some malicious
intermediary, the TestNode sends 64 bytes ellswift. The v1 node doesn't
understand this and disconnects. Then the v2 TestNode reconnects by
sending a v1/version message.
Messages are built, encrypted and sent over the socket in v2
connections. If a race condition happens between python's main
thread and p2p thread with both of them trying to send a message,
it's possible that the messages get encrypted with wrong keystream.
Messages are built and sent over the socket in v1 connections.
So there's no problem if messages are sent in the wrong order.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
Instantiate this object when the connection supports v2 P2P transport
protocol.
- When a P2PConnection is opened, perform initiate_v2_handshake() if the
connection is an initiator. application layer messages are only sent after
the initial v2 handshake is over (for both initiator and responder).
The class `EncryptedP2PState` stores the 4 32-byte keys, session id,
garbage terminators, whether it's an initiator/responder, whether the
initial handshake has been completed etc.. It also contains functions
to perform the v2 handshake and to encrypt/decrypt p2p v2 messages.
- In an inbound connection to TestNode, P2PConnection is the initiator
and `initiate_v2_handshake()`, `complete_handshake()`, `authenticate_handshake()`
are called on it. [ TestNode <----------------- P2PConnection ]
- In an outbound connection from TestNode, P2PConnection is the responder
and `respond_v2_handshake()`, `complete_handshake()`, `authenticate_handshake()`
are called on it. [ TestNode -----------------> P2PConnection ]
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake)
5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake)
47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake)
6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake)
435fe5cd96599c518e26efe444c9d94d1277996b 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 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
e9014042a6bed8c16cc9a31fc35cb709d4b3c766 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 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 e9014042a6bed8c16cc9a31fc35cb709d4b3c766
hebasto:
re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766.
ryanofsky:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
d55fdb1a495190e213b1b5127f5d91e4a409765e Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee63eebc760e2a9226afcd0c46d3ec2bd 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 d55fdb1a495190e213b1b5127f5d91e4a409765e
achow101:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
murchandamus:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
This avoids circular dependency happening when importing MAGIC_BYTES.
Before,
p2p.py <--import for EncryptedP2PState-- v2_p2p.py
| ^
| |
└---------import for MAGIC_BYTES----------┘
Now, MAGIC_BYTES are kept separately in messages.py
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
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>
Fixes a (rare) intermittency issue in wallet_import_rescan.
Since we use `subtract_fee_from_outputs=[0]` in the `send` command,
the output amount must at least be as large as the fee we're paying.
If the serialized transaction passed to `fundrawtransaction` contains
duplicates, they will be deserialized and added to the transaction. Add
a test to ensure this behavior is not changed during the refactor.
A user can pass any number of duplicated and unrelated addresses as an
SFFO argument to `sendmany` and the RPC will not throw an error (note,
all the rest of the RPCs which take SFFO as an argument will error if
the user passes duplicates or specifies outputs not present in the
transaction). Add a test to ensure this behavior is not changed during
the refactor.
3ba815b42db74804e341ce15f648c2b297af55ca Make v2transport default for addnode RPC when enabled (Pieter Wuille)
Pull request description:
Since #29058, several types of manually configured connections will attempt v2 connections when `-v2transport` is enabled, except for the `addnode` RPC, as that one has an explicit argument to enable or disable.
Make the default for that RPC match the `-v2transport` setting so the behavior matches that of other manual connections from a user perspective.
ACKs for top commit:
achow101:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
kristapsk:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
theStack:
Code-review ACK 3ba815b42db74804e341ce15f648c2b297af55ca
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
df30247705940c50c5eaafd74e2abbeb8b0cec07 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow)
c3d02be536ac3f35c10efa03653186a17ebbfc12 [test] rescan legacy wallet with reorged parent + IsFromMe child in mempool (Gloria Zhao)
Pull request description:
Originally motivated by #29019, which reverts back to having `requestMempoolTransactions` emit `transactionAddedToMempool` in `mapTx` default order instead of `GetSortedDepthAndScore` order.
It's important that these notifications happen in topological order, otherwise the wallet rescan may miss transactions that belong to it. Notably, checking whether a transaction `IsFromMe` requires knowing its inputs, which may be from a mempool parent.
When using `mapTx` order, a parent may come later than its child if it was added from a block disconnected in a reorg.
This PR adds a test for this case.
ACKs for top commit:
achow101:
ACK df30247705940c50c5eaafd74e2abbeb8b0cec07
furszy:
Code review ACK df30247705, nits can be disregarded.
Tree-SHA512: 2f1d9ef92313228adbbef94e634e5f7a9ec6e6a2c88e16aa343bdc95ffc9b9f9c82a569b412c9a3841db9d789e52f9283e8b9385731668d59355903e26e58a5d
Test that wallet rescans process transactions topologically, even if a
parent's entry into the mempool is later than that of its child.
This behavior is important because IsFromMe requires the ability to look
up a transaction's inputs.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e test: Assert that a new tx with a delta of 0 is never added (kevkevin)
cfdbcd19b32fd63954d7947dcc639aef291fb6b2 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin)
252a86729a15e47ed168d8da7c4a8d6113673909 rpc: renaming txid -> transactionid (kevkevin)
2fca6c2dd03c3955d86efb0b8d2a7961e42115fd rpc: changed prioritisation-map -> "" (kevkevin)
3a118e19e100110300d3290d4c1434f963721d94 test: Directly constructing 2 entry map for getprioritisedtransactions (kevkevin)
Pull request description:
In this PR I am addressing some comments in https://github.com/bitcoin/bitcoin/pull/27501 as a followup.
- changed `prioritisation-map` in the `RPCResult` to `""`
- Directly constructing 2 entry map for getprioritisedtransactions in functional tests
- renamed `txid` to `transactionid` in `RPCResult` to be more consistent with naming elsewhere
- exposed the `modified_fee` field instead of having it be a useless arg
- Created a new test that asserts when `prioritisedtransaction` is called with a fee_delta of 0 it is not added to mempool
ACKs for top commit:
glozow:
reACK 0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e, only change is the doc suggestion
Tree-SHA512: e99056e37a8b1cfc511d87c83edba7c928b50d2cd6c2fd7c038976779850677ad37fddeb2b983e8bc007ca8567eb21ebb78d7eae9b773657c2b297299993ec05
Test that wallet rescans process transactions topologically, even if a
parent's entry into the mempool is later than that of its child.
This behavior is important because IsFromMe requires the ability to look
up a transaction's inputs.
878d914777a03a04ecb84217152e8b7fd73a5062 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8b021c107783adce3e8afe4f8edee6e test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3dfb0e50daaf752dc863a880559fa4637 test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner)
Pull request description:
There are at least three ways to detect the operating system in Python3:
- `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
- `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
- `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)
We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release.
Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via
```
$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
```
| OS | os.name | sys.platform | platform.system() |
|--------------|---------|--------------|--------------------|
| Linux 6.2.0 | posix | linux | Linux |
| MacOS* | posix | darwin | Darwin |
| OpenBSD 7.4 | posix | openbsd7 | OpenBSD |
| Windows* | nt | win32 | Windows |
\* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed.
ACKs for top commit:
kevkevinpal:
ACK [878d914](878d914777)
achow101:
ACK 878d914777a03a04ecb84217152e8b7fd73a5062
hebasto:
ACK 878d914777a03a04ecb84217152e8b7fd73a5062, I have reviewed the code and it looks OK.
pablomartin4btc:
tACK 878d914777a03a04ecb84217152e8b7fd73a5062
Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc
997b9a73e5166b4244f7c5b4fe144d524f3005f4 test: add assumeutxo wallet test (Sjors Provoost)
Pull request description:
Extracted from #28616, this adds a (very) basic wallet test for assume utxo. It checks some circumstances where a backup can and can't be loaded.
ACKs for top commit:
maflcko:
lgtm ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
achow101:
ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
theStack:
Code-review ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
Tree-SHA512: 69474e56c6a46bb4f30fc54f8e5844766ac2a5f8226bb0b168d11ae1e3d4eae58570c1f1b4cc2b2f6f51b5d0e055bbe2bbd11684265215e01d4eb81ab4b7b0bb
016cc807f77a9128d430a0df1edd133521628a33 test: wallet migration, add coverage for tx extra data (furszy)
Pull request description:
Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.
Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
as well as the extra tx comments.
ACKs for top commit:
jamesob:
Nice, ACK 016cc807f7
achow101:
ACK 016cc807f77a9128d430a0df1edd133521628a33
pablomartin4btc:
ACK 016cc807f77a9128d430a0df1edd133521628a33
BrandonOdiwuor:
lgtm ACK 016cc807f77a9128d430a0df1edd133521628a33
Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
406b71abcb72f234ddf9245a3f57e748343c774f wallet: Migrate entire address book entries (Andrew Chow)
Pull request description:
Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.
ACKs for top commit:
ryanofsky:
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
furszy:
Code review ACK 406b71ab
Tree-SHA512: 13de42b16a1d8524fe0555764744139566b2e7d29741ceffc1158a905dd537136b762330568b3b5cac28cbee1bfd363a20de97d0a6c5296738cb3aa99133945b
