fa5cd66f0a test: Assumeutxo with more than just coinbase transactions (MarcoFalke)
Pull request description:
Currently the AU tests only check that loading a txout set with only coinbase outputs works.
Fix that by adding other transactions.
ACKs for top commit:
jamesob:
ACK fa5cd66f0a
glozow:
concept ACK fa5cd66f0a
Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
4da76ca247 test: Test migration of tx with both spendable and watchonly (Ava Chow)
c62a8d03a8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow)
71cb28ea8c test: Make sure that migration test does not rescan on reloading (Ava Chow)
78ba0e6748 wallet: Reload the wallet if migration exited early (Ava Chow)
9332c7edda wallet: Write bestblock to watchonly and solvable wallets (Ava Chow)
Pull request description:
A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.
I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen.
The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits.
ACKs for top commit:
ryanofsky:
Code review ACK 4da76ca247. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
furszy:
Code review ACK 4da76ca2
Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
We want to make sure that all of the transactions are being copied to
the watchonly and solvable wallets as expected. The automatic rescanning
behavior can cause us to pass a test by finding the transaction
on loading rather than having it be copied as expected.
9642aefb81 test: fix intermittent failure in p2p_v2_earlykeyresponse (Martin Zumsande)
Pull request description:
The test fails intermittently, see https://cirrus-ci.com/task/6403578080788480?logs=ci#L3521 and https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1916996716.
I think it's because of a race between the python NetworkThread and the actual
test, which will both call `initiate_v2_handshake`. I could reproduce it by adding a sleep into `initiate_v2_handshake` after the line `self.sent_garbage = random.randbytes(garbage_len)`.
Fix this by waiting for the first `initiate_v2_handshake` to have finished before calling it a second time.
ACKs for top commit:
stratospher:
tested ACK 9642aef.
achow101:
ACK 9642aefb81
theStack:
Tested ACK 9642aefb81
Tree-SHA512: f728bbceaf816ddefeee4957494ccb608ad4fc912cb5cbf5f2acf09836df969c4e8fa2bb441aadb94fa39b3ffbb005d4132e7b6a5a98d80811810d8bd1d624e3
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
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
26ad2aeb29 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
55556a64a8 test: Remove struct import from messages.py (MarcoFalke)
fa3fa86dda scripted-diff: test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fafc0d68ee test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fa3886b7c6 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 55556a64a8
theStack:
re-ACK 55556a64a8
Tree-SHA512: 1cef8cdfd763fb424ed4b8be850a834b83fd0ef47fbea626a29784eb4f4832d44e42c4fe05b298b6070a933ef278b0222289a9955a97c86707e091e20bbb247a
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
fab97d81ce fuzz: Print coverage summary after run_once (MarcoFalke)
Pull request description:
This can be used to quickly check the coverage effects of a code change or qa-assets change.
ACKs for top commit:
dergoegge:
ACK fab97d81ce
Tree-SHA512: 0ac913c14698f39e76e0e7bf124f182220031796d6443edb34c6e4615e128157cf746da661b216c4640a41964e977249712445ca9c005b1b4a3737adabdb4a7d
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
3bfc5bd36e 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 3bfc5bd36e
glozow:
utACK 3bfc5bd36e, 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).
9d09c873a5 fuzz: Exit and log stderr for parse_test_list errors (dergoegge)
Pull request description:
We should log all errors that occur when attempting to print the harness list in the fuzz test runner.
ACKs for top commit:
maflcko:
lgtm ACK 9d09c873a5
Tree-SHA512: 50471b732c8cbe287dacba14487e7c8a5826f146432d93aa3bb55d063a8ba158d01641d6cb1360241dd4cd54ef5e045b0412f9cc34d06c181134921d1f1ceced
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 ]
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
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
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
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.
