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).
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.
3ba815b42d 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 3ba815b42d
kristapsk:
ACK 3ba815b42d
theStack:
Code-review ACK 3ba815b42d
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
df30247705 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow)
c3d02be536 [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 df30247705
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>
0eebd6fe7d test: Assert that a new tx with a delta of 0 is never added (kevkevin)
cfdbcd19b3 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin)
252a86729a rpc: renaming txid -> transactionid (kevkevin)
2fca6c2dd0 rpc: changed prioritisation-map -> "" (kevkevin)
3a118e19e1 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 0eebd6fe7d, 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.
878d914777 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8 test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3df 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 878d914777
hebasto:
ACK 878d914777, I have reviewed the code and it looks OK.
pablomartin4btc:
tACK 878d914777
Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc
997b9a73e5 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 997b9a73e5
achow101:
ACK 997b9a73e5
theStack:
Code-review ACK 997b9a73e5
Tree-SHA512: 69474e56c6a46bb4f30fc54f8e5844766ac2a5f8226bb0b168d11ae1e3d4eae58570c1f1b4cc2b2f6f51b5d0e055bbe2bbd11684265215e01d4eb81ab4b7b0bb
016cc807f7 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 016cc807f7
pablomartin4btc:
ACK 016cc807f7
BrandonOdiwuor:
lgtm ACK 016cc807f7
Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
The nodes are restarted with an empty addrman and populated
with addresses from different networks using a helper function.
We can safely add multiple addresses to addrman tables without
worrying about unpredictable collisions since bucket:position
is fixed in a deterministic addrman.
Currently in tests where we are interested in contents of addrman,
addresses which were added to the node's addrman in previous tests
leak into the current test. example: addresses added in addpeeraddress
test leak into getaddrmaninfo and getrawaddrman tests.
It is cleaner to design the tests to be modular and without such
leaks so that we don't need to deal with context from previous tests
this test inserts 1 address into the new table and 1 address into
the tried table so that no collisions can happen in either table
if a second address is added. this setup does not need to be
maintained anymore since we can use a deterministic addrman and
safely add many addresses in both tables without collisions. Remove
comment explaining why previous setup needed to be maintained.
-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.
Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
406b71abcb 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 406b71abcb. Just suggested change since last review
furszy:
Code review ACK 406b71ab
Tree-SHA512: 13de42b16a1d8524fe0555764744139566b2e7d29741ceffc1158a905dd537136b762330568b3b5cac28cbee1bfd363a20de97d0a6c5296738cb3aa99133945b
fa46cc22bc Remove deprecated -rpcserialversion (MarcoFalke)
Pull request description:
The flag is problematic for many reasons:
* It is deprecated
* It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
* It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see https://github.com/bitcoin/bitcoin/issues/28730#issuecomment-1780940868
* It makes performance improvements harder to implement: https://github.com/bitcoin/bitcoin/pull/17529#issuecomment-556082818
Fix all issues by removing it.
If there is a use-case, likely a per-RPC flag can be added, if needed.
ACKs for top commit:
ajtowns:
crACK fa46cc22bc
TheCharlatan:
lgtm ACK fa46cc22bc
Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.
Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
7b45744df3 tests: ensure functional tests set permitbaremultisig=1 when needed (Anthony Towns)
7dfabdcf86 tests: test both settings for permitbaremultisig in p2sh tests (Anthony Towns)
Pull request description:
Update unit and functional tests so that they continue to work if the default for `-permitbaremultisig` is changed.
ACKs for top commit:
maflcko:
lgtm ACK 7b45744df3
instagibbs:
crACK 7b45744df3
ajtowns:
> crACK [7b45744](7b45744df3)
achow101:
ACK 7b45744df3
glozow:
ACK 7b45744df3, changed default locally and all tests passed
Tree-SHA512: f89f9e2bb11f07662cfd57390196df9e531064e1bd662e1db7dcfc97694394ae5e8014e9d209b9405aa09195bf46fc331b7fba10378065cdb270cbd0669ae904
1ce45baed7 rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c66444d0 test: coverage for wallet birth time interaction with -reindex (furszy)
6f497377aa wallet: fix legacy spkm default birth time (furszy)
75fbf444c1 wallet: birth time update during tx scanning (furszy)
b4306e3c8d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)
Pull request description:
Fixing #28897.
As the user may have imported a descriptor with a timestamp newer
than the actual birth time of the first key (by setting 'timestamp=now'),
the wallet needs to update the birth time when it detects a transaction
older than the oldest descriptor timestamp.
Testing Notes:
Can cherry-pick the test commit on top of master. It will fail there.
ACKs for top commit:
Sjors:
re-utACK 1ce45baed7
achow101:
ACK 1ce45baed7
Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9
