Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.
Github-Pull: #28542
Rebased-From: 782701ce7d
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.
Additionaly it tests the -regtestonly option -acceptstalefeeestimates.
Github-Pull: #27622
Rebased-From: d2b39e09bc
The migration process must skip any invalid script inside the legacy
spkm and all the addressbook records linked to them.
These scripts are not being watched by the current wallet, nor should
be watched by the migrated one.
IsMine() returns ISMINE_NO for them.
Github-Pull: #28125
Rebased-From: 8e7e3e6149
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
Github-Pull: #28067
Rebased-From: cc781a2180
Prior to PR #27468 (commit 11422cc572) all
call-sites of `GetQueryParameter(...)` in the REST module could trigger
a crash. Add missing test cases for all possible code-paths as a
regression test, as a foundation for possible follow-up fixes (which aim
to resolve this issue in a more general and robust way).
e07dd5fff9 test: fix bumpfee 'spend_one_input' occasional failure (furszy)
Pull request description:
CI test failure, in master: https://cirrus-ci.com/task/5975232842825728.
In #27469https://cirrus-ci.com/task/6452468402356224
Most of the subtests in `wallet_bumpfee.py` expect to find spendable UTXOs of 0.001 btc in the rbf wallet. They use the `spend_one_input()` method which fails if none of them exist.
The sporadic failure comes from the recently added `test_feerate_checks_replaced_outputs` subtest that can spend all them. Leaving the next subtests with no 0.001 UTXOs to spend.
To solve it, this PR moves the recently added case into a "context independent subtests" section.
Which is placed at the end to not affect other cases.
ACKs for top commit:
achow101:
ACK e07dd5fff9
pablomartin4btc:
ACK e07dd5fff9.
Tree-SHA512: c150ed6fcfbb6f1502eaf6370a57aae0da991c0fc48e8bb3d446805f4336abba5d80ff0de26094914da95432dd0255030fe527001af4510dfdcefbc7230f14d6
`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query parameters are used (e.g. `rest_headers`)
can cause a segfault.
This bugfix is designed to be minimal and without additional behaviour change.
Follow-up work should be done to resolve this in a more general and robust way,
so not every endpoint has to handle it individually.
Most of the subtests in wallet_bumpfee.py expect to
find spendable UTXOs of 0.001 btc in the rbf wallet
(they use the 'spend_one_input()' method that tries
to spend one of them and if it doesn't find any, it
throws an exception).
The sporadic failure comes from the recently added
'test_feerate_checks_replaced_outputs' subtest that
can spend all them. Leaving the next subtests with
no 0.001 UTXOs to spend.
To solve it, this PR moves the recently added case
into a "context independent subtests" section.
Which is placed at the end to not affect other cases.
d52fa1b0a5 tests: Make sure that bumpfee feerate checks work when replacing outputs (Andrew Chow)
be177c15a4 bumpfee: Check the correct feerate when replacing outputs (Andrew Chow)
Pull request description:
When replacing the outputs of a transaction during `bumpfee`, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use `bumpfee` and think the transaction has been bumped when it actually has not.
This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.
Also added a test for this scenario.
ACKs for top commit:
ishaanam:
reACK d52fa1b0a5
Xekyo:
reACK d52fa1b0a5
Tree-SHA512: d18301b587465322dd3fb1bb86496c3675265a56072047576e2baa5cf907dd3b54778f30721f662f0c235709a5568427c18542eb7efbfb6fdd9f481fe676c66b
7ccdd741fe test: fix importmulti/importdescriptors assertion (Jon Atack)
19d888ce40 rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack)
01df011ca2 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack)
9ea8b3739a test: createwallet "warning" field deprecation test (Jon Atack)
645d7f75ac rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack)
2f4a926e95 test: add test coverage for "warnings" field in createwallet (Jon Atack)
4a1e479ca6 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack)
079d8cdda8 rpc: extract wallet "warnings" fields to a util helper (Jon Atack)
f73782a903 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack)
Pull request description:
Based on discussion and concept ACKed in #27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields.
The first commit f73782a903 implements https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198 as an alternative to #27138. One of those two could potentially be backported to our currently supported releases.
ACKs for top commit:
achow101:
ACK 7ccdd741fe
1440000bytes:
utACK 7ccdd741fe
vasild:
ACK 7ccdd741fe
pinheadmz:
re-ACK 7ccdd741fe
Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
Instead of storing and passing around fixed strings for the purpose of
an address, use an enum.
This also rationalizes the CAddressBookData struct, documenting all fields and
making them public, and simplifying the representation to avoid bugs like
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
When replacing the outputs of a transaction, we can end up with
fees that are drastically different from the original. This tests that
the feerate checks we perform will properly detect when the bumping tx
will have an insufficient feerate.
f842ed9a40 test: refactor: replace unnecessary `BytesIO` uses (Sebastian Falbesoner)
Pull request description:
Rather than needing to create intermediate stream variables, we can use helper functions like `tx_from_hex` instead or access the result directly, leading both to increased readability and less code.
ACKs for top commit:
stickies-v:
ACK f842ed9a40
brunoerg:
crACK f842ed9a40
aureleoules:
ACK f842ed9a40 - It seems that these are the only instances that can be changed and it simplifies test code.
Tree-SHA512: 7f4fd7a26720d1988bf27f66c817ff6cd7060350a3be62d28e7848c768fd43578719ca475193d4057ccf4f2458af18564fd513fe3a1d458a11c799927c12bd65
Rather than needing to create intermediate stream variables, we can use
helper functions like `tx_from_hex` instead or access the result
directly, leading both to increased readability and less code.
Also, move the burden of checking for a timeout to the client and
disable the timeout on the server. This should avoid intermittent issues
in slow tests (for example mining_getblocktemplate_longpoll.py, or
feature_dbcrash.py), or possibly when the server is running slow (for
example in valgrind). There shouldn't be any downside in tests caused
by a high rpcservertimeout.
71b3e9b0ad sanitizers: remove GetRNGState lsan suppression (fanquake)
Pull request description:
I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37).
Can anyone recreate the false-positive?
ACKs for top commit:
MarcoFalke:
lgtm ACK 71b3e9b0ad
hebasto:
ACK 71b3e9b0ad, tested on Ubuntu 22.04 x86_64.
Tree-SHA512: 63020327d61acd6c94c6c278c9c4d72aedc10253fa172bcf9353bcad4c28d068bee824969eb3ce92152244831df8fe92cffae536453c8073a4fda74dfdfbcefa
e669833943 test: dedup package limit checks via decorator in mempool_package_limits.py (Sebastian Falbesoner)
72f25e238c test: refactor: use Satoshis for fees in mempool_package_limits.py (Sebastian Falbesoner)
Pull request description:
The subtests in the functional test mempool_package_limits.py all follow the same pattern:
1. first, check that the mempool is currently empty
2. create and submit certain single txs to the mempool, prepare list of hex transactions
3. check that `testmempoolaccept` on the package hex fails with a "package-mempool-limits" error on each tx result
4. after mining a block, check that submitting the package succeeds
Note that steps 1,3,4 are identical for each of the subtests and only step 2 varies, so this might be a nice opportunity to deduplicate code by using a newly introduced decorator which executes the necessary before and after the essential part of the subtest. This also makes it easier to add new subtests without having to copy-paste those parts once again.
In addition, the first commit switches the fee unit from BTC to Satoshis, which allows to get rid of some imports (`COIN` and `Decimal`) and a comment for the `test_desc_size_limits` subtest is fixed (s/25KvB/21KvB/).
ACKs for top commit:
ismaelsadeeq:
ACK e669833943
glozow:
utACK e669833943
Tree-SHA512: 84a85e739de7387391c13bd46aeb015a74302ea7c6f0ca3d4e2b1b487d38df390dc118eb5b1c11d3e4206bff316a4dab60ef6b25d8feced672345d4e36ffd205
e47ce42f67 refactor: use address_to_scriptpubkey to retrieve addresses scriptpubkey (ismaelsadeeq)
4142d19d74 refactor: move address_to_scriptpubkey to address.py (ismaelsadeeq)
Pull request description:
PR #27269 enables the function address_to_scriptpubkey() to decode all address types and return their corresponding scriptpubkeys. As a result, there is no longer any need to call getaddressinfo or validateaddress RPCs in order to retrieve an address scriptpubkey, as explained in the comments on this pull request (see https://github.com/bitcoin/bitcoin/pull/27269#pullrequestreview-1353681933 and https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1481016118).
Instead of using RPC calls, this update replaces the process of obtaining an address scriptPubkey with the address_to_scriptpubkey method, resulting in improved performance for functional tests.
ACKs for top commit:
josibake:
re-ACK e47ce42f67
theStack:
ACK e47ce42f67🌱
Tree-SHA512: 05285349a7d5ce7097b8f2582e573a5135c6deef85ea9936f68f6ce94e9ebb1d84d94f7fc7e5ed833a698e01585addd80deb52e6338f8aee985bf14db45417d2
This avoids having to convert from BTC to Sats and needs less imports.
Also specify the tx's target size in vsize rather than in weight, which
allows us to specify the fee-rate by a simple multiplication, rather
than having another magic number for it.
This commit updates the code by replacing the RPC call used to
decode an address and retrieve its corresponding scriptpubkey
with the address_to_scriptpubkey function. address_to_scriptpubkey
function can now decode all addresses formats, which makes
it more efficient to use.
The COINBASE_MATURITY constant in blocktools.py is imported in wallet.py.
However, importing address_to_scriptpubkey to blocktools.py will
generate a circular import error. Since the method is related to
addresses, it is best to move it to address.py, which will also
fix the circular import error.
Update imports of address_to_scriptpubkey accordingly.
8aab5157c5 test: wallet_create_tx.py fix race (furszy)
Pull request description:
Fixes#27316
Because wallets are internally synchronized through the validation interface,
and the interface dispatches events on a worker thread, it is possible for a
transaction created by the first wallet to not arrive to the second wallet
before the second wallet attempts to use one of its outputs. This is because
we do not wait for the `BroadcastTransaction` callback during the wallet's
"submit to mempool" process. To address this in the tests, we need to
manually sync the validation queue.
ACKs for top commit:
josibake:
ACK 8aab5157c5
theStack:
ACK 8aab5157c5
Tree-SHA512: 76364370ab292a5c3ea1ed61cd353fc626a9e9cd6ce18464c24da1b3dcb34b65006e2bc42b84bbd25af03f9449231990bf789504728972db3217b569099eb309
d178082996 test: add bech32 decoding support to address_to_scriptpubkey() (ismaelsadeeq)
aac8793c7a test: test_bech32_decode in address.py (ismaelsadeeq)
Pull request description:
[rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L26)) sendtodestination only sends to legacy addresses and scriptPubkeys because [wallet.py](e695d8536e/test/functional/test_framework/wallet.py (L415)) address_to_scriptpubkey does not support conversion of segwit address.
This update enables address_to_scriptpubkey to support the conversion of testnet segwit addresses to scriptPubkeys.
This change will enable [rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L22)) ScantxoutsetTest to have more test coverage by adding more sendtodestination calls with bech32 and bech32m testnet addresses, then test the bech32 and bech32m derivation subsets UTXO amount in [Test extended key derivation](e695d8536e/test/functional/rpc_scantxoutset.py (L84)).
I will add the test coverage in a subsequent Pull request.
ACKs for top commit:
josibake:
ACK d178082996
theStack:
ACK d178082996✔️
willcl-ark:
ACK d17808299
Tree-SHA512: 312c20ce192c648faf7dd178622700c9b871d755db56c246250e25508c3c19e7b02c0ae901dda11a1794629b9a9429c877168c05e1c4c1dbf41493316e30e7e9
Because wallets are internally synchronized
through the validation interface, and the
interface dispatches events on a worker thread,
it is possible for a transaction created by the
first wallet to not arrive at the second wallet
before the second wallet attempts to use one of
its outputs. This is because we do not wait for
the BroadcastTransaction callback during the wallet's
"submit to mempool" process. To address this in the
tests, we need to sync the validation queue.
f3221d373a test: add wallet too-long-mempool-chain error coverage (furszy)
acf0119d24 wallet: return error msg for too-long-mempool-chain failure (furszy)
Pull request description:
Fixes#23144.
We currently return a general "Insufficient funds" from Coin
Selection when we actually skipped unconfirmed UTXOs that
surpassed the mempool ancestors limit.
This PR make the error clearer by returning:
"Unconfirmed UTXOs are available, but spending them creates
a chain of transactions that will be rejected by the mempool"
Also, added an early return from Coin Selection if the sum of
the discarded coins decreases the available balance below the
target amount.
ACKs for top commit:
achow101:
ACK f3221d373a
S3RK:
Code review ACK f3221d373a
Xekyo:
ACK f3221d373a
Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66
fa0696e786 test: Replace threading with concurrent.futures (MarcoFalke)
Pull request description:
`threading` has no easy way to get the return value or exception once the target function stops. Not checking the return value or exception can make tests more fragile and failures harder to debug.
Fix this by checking the return value (or exception) by wrapping the function execution into a future and calling `result()` on it.
Can be reviewed with `--ignore-all-space`.
(There are still some uses of `threading` around, because some tests do expect an exception to be thrown and caught in the target function)
ACKs for top commit:
ishaanam:
ACK fa0696e786
stickies-v:
ACK fa0696e786
Tree-SHA512: d9ddf6b3c530cd8c485a030a3c84d4e03d3e9f9ea8240b050afcd566a884f5cabe816ac56910cec9ea9fa299239e5abb99e672dda05a74974f61bb68dc3c1d65
fa18504d57 rpc: Add submit option to generateblock (MarcoFalke)
fab9a08e14 refactor: Replace block_hash with block_out (MarcoFalke)
Pull request description:
When submit is turned off, a block can be generated and returned as hex, to be used for further tests. For example, it can be submitted on a different node, on a different interface (like p2p), or just never submitted and be used for other testing purposes.
ACKs for top commit:
instagibbs:
ACK fa18504d57
TheCharlatan:
tACK fa18504d57
Tree-SHA512: 1b2ab6b71bb7e155c6482d75f5373f4e77de6446cb16bc2dfd19e7a4075b3a6ad87d7ad7a049a9eed934cb71574acfd27202f54c8bb3b03fac869f2e95db7ee5
Adds bech32_to_bytes() which can decode a bech32 address and return the
version as an `int` and the payload in bytes.
bech32_to_bytes() is used by the test_bech32_decode unit test to test
decoding of segwit addresses.
