48262a00f5 Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bb Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f5
sipa:
ACK 48262a00f5
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
e8959000b6 test: Use MiniWallet in rpc_rawtransaction.py (Daniela Brozzoni)
e93046c10b MOVEONLY: Move signrawtransactionwithwallet test (Daniela Brozzoni)
Pull request description:
This PR allows `rpc_rawtransaction.py` to be run even without the Core wallet by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`. Since this would have meant running the same test twice
if the wallet wasn't compiled, now we run it just once with the legacy
wallet.
ACKs for top commit:
jonatack:
ACK e8959000b6
Tree-SHA512: d1580570a54dad8e30a5df1ab7d03ecb3f824efe6843323e1f3aef63592045d823c7d54fc86321dc7c1d414854a253431a01a7baa9f30426ea9a09ef11ae3a04
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`.
Now we run it only with `--legacy-wallet`, as all the tests has been
ported to the MiniWallet but `raw_multisig_transaction_legacy_tests`,
which can be run only with the legacy wallet.
We also decrease the number of nodes used from 4 to 3, making the test
run slightly faster.
4185570340 Add RPC to get mempool txs spending outputs (t-bast)
Pull request description:
We add an RPC to fetch mempool transactions spending any of the given outpoints.
Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool).
For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly.
I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it.
ACKs for top commit:
darosior:
re-utACK 4185570340
vincenzopalazzo:
re-ACK 4185570340
danielabrozzoni:
re-tACK 4185570340
w0xlt:
reACK 4185570340
Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc
3258bad996 changes color of skipped functional tests (Jacob P. Fickes)
Pull request description:
changes the color of skipped functional tests (currently grey and can be hard to read/invisible on dark backgrounds) to yellow.
resolves#24791
ACKs for top commit:
theStack:
Tested ACK 3258bad996
jarolrod:
Tested ACK 3258bad996
Tree-SHA512: 3fe5ae0d3b4902b2b6bda6e89ab780feb8bf4b7cb1ce7e8467057b94a1e0a26ddeaf3cac0bc19b06ef10d8bccaac9c495029d42740fbedab8fb0d5fdd7d02eaf
We add an RPC to fetch the mempool transactions spending given outpoints.
Without this RPC, application developers would need to first call
`getrawmempool` which returns a long list of `txid`, then fetch each of
these txs individually to check whether they spend the given outpoint(s).
This RPC can later be enriched to also find confirmed transactions instead
of being restricted to mempool transactions.
5f213213cb tests: add tests for cross-chain wallet use prevention (Seibart Nedor)
968765973b wallet: ensure wallet files are not reused across chains (Seibart Nedor)
Pull request description:
This implements a proposal in #12805 and is a rebase of #14533.
This seems to be a working approach, but I'm not sure why the `p2p_segwit.py` functional test needed a change, so I'll look into it more.
ACKs for top commit:
achow101:
ACK 5f213213cb
dongcarl:
Code Review ACK 5f213213cb
[deleted]:
tACK 5f213213cb
Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec
fafd67479a test: Remove previous release check (MarcoFalke)
Pull request description:
Now that the commit (7c08d81e11) which changes taproot to be enforced for all blocks is sufficiently buried by other commits, and thus less likely to be reverted, it seems a good time to remove no longer needed test code.
The `feature_taproot` functional test is cleaned up to no longer run against a previous release. Since previous releases are static and impossible to change, it is sufficient to run the test once against the release. Now that this is done, the check can be removed without decreasing test coverage.
ACKs for top commit:
laanwj:
Concept and code review ACK fafd67479a
vincenzopalazzo:
ACK fafd67479a
Tree-SHA512: fcb1a93f3bf9deb5f5c7327a7cd23be10ba09c9f4cbfa73ee2764a93c6ce7d6fa98ca32f2cf4023c20ab624aee601beec949fd02a57a3a658fdbd4be1a9ff338
71c3f0356c move-only: Rename index + pruning functional test (Fabian Jahr)
de08932efa test: Update test for indices on pruned nodes (Fabian Jahr)
825d19839b Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb0c6 Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823531 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b035 refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)
Pull request description:
# Motivation
The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).
# Background
`coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.
Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.
# Alternative approach
Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
- Usage of globals
- Blocks pruning with a start and a stop height
- Can persist blockers across restarts
- Blockers can be set/unset via RPCs
Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.
ACKs for top commit:
mzumsande:
Code review ACK 71c3f0356c
ryanofsky:
Code review ACK 71c3f0356c. Changes since last review: just tweaking comments and asserts, and rebasing
w0xlt:
tACK 71c3f0356c on signet.
Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
ab5af9ca72 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0 wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181 wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca72
josibake:
crACK ab5af9ca72
0xB10C:
ACK ab5af9ca72. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
Changes the color of skipped functional tests to the default text color of the terminal. This will make skipped tests easy to read on the majority of background colors rather than the original grey color (hard to read on dark backgrounds) and the proposed yellow change (hard to read on white backgrounds)
76c60d7b31 test: validation:block_connected tracepoint test (0xb10c)
260e28ece8 test: utxocache:* tracepoint tests (0xb10c)
34b27bac68 test: net:in/out_message tracepoint tests (0xb10c)
c934087b62 test: checks for tracepoint tests (0xb10c)
Pull request description:
This adds functional tests for the USDT tracepoints added in https://github.com/bitcoin/bitcoin/pull/22006 and https://github.com/bitcoin/bitcoin/pull/22902. This partially fixes#23296. The tests **are probably skipped** on most systems as these tests require:
- a Linux system with a kernel that supports BPF (and available kernel headers)
- that Bitcoin Core is compiled with tracepoints for USDT support (default when compiled with depends)
- [bcc](https://github.com/iovisor/bcc) installed
- the tests are run with a privileged user that is able to e.g. do BPF syscalls and load BPF maps
The tests are not yet run in our CI as the CirrusCI containers lack the required permissions (see https://github.com/bitcoin/bitcoin/issues/23296#issuecomment-1024920845). Running the tests in a VM in the CI could work, but I haven't experimented with this yet. The priority was to get the actual tests done first to ensure the tracepoints work as intended for the v23.0 release. Running the tracepoint tests in the CI is planned as the next step to finish #23296.
The tests can, however, be run against e.g. release candidates by hand. Additionally, they provide a starting point for tests for future tracepoints. PRs adding new tracepoint should include tests. This makes reviewing these PRs easier.
The tests require privileges to execute BPF sycalls (`CAP_SYS_ADMIN` before Linux kernel 5.8 and `CAP_BPF` and `CAP_PERFMON` on 5.8+) and permissions to `/sys/kernel/debug/tracing/`. It's currently recommended to run the tests in a virtual machine (or on a VPS) where it's sensible to use the `root` user to gain these privileges. Never run python scripts you haven't carefully reviewed with `root` permissions! It's unclear if a non-root user can even gain the required privileges. This needs more experimenting.
The goal here is to test the tracepoint interface to make sure the [documented interface](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#tracepoint-documentation) does not break by accident. The tracepoints expose implementation details. This means we also need to rely on implementation details of Bitcoin Core in these functional tests to trigger the tracepoints. An example is the test of the `utxocache:flush` tracepoint: On Bitcoin Core shutdown, the UTXO cache is flushed twice. The corresponding tracepoint test expects two flushes, too - if not, the test fails. Changing implementation details could cause these tests to fail and the tracepoint API to break. However, we purposefully treat the tracepoints only as [**semi-stable**](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#semi-stable-api). The tracepoints should not block refactors or changes to other internals.
ACKs for top commit:
jb55:
tACK 76c60d7b31
laanwj:
Tested ACK 76c60d7b31
Tree-SHA512: 9a63d945c68102e59d751bd8d2805ddd7b37185408fa831d28a9cb6641b701961389b55f216c475df7d4771154e735625067ee957fc74f454ad7a7921255364c
fa9112aac0 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519 (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0c (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac0
laanwj:
Code review ACK fa9112aac0
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
bb84b7145b add tests for no recipient and using send_max while inputs are specified (ishaanam)
49090ec402 Add sendall RPC née sweep (Murch)
902793c777 Extract FinishTransaction from send() (Murch)
6d2208a3f6 Extract interpretation of fee estimation arguments (Murch)
a31d75e5fb Elaborate error messages for outdated options (Murch)
35ed094e4b Extract prevention of outdated option names (Murch)
Pull request description:
Add sendall RPC née sweep
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _given UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _given budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending a given set of
UTXOs such as paying the value from one or more specific UTXOs, emptying
a wallet, or burning dust, we realized that there are some cases in
which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual
computation of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific
subset of the wallet's UTXO pool (by default all of them) and assigns
the funds to one or more receivers. Receivers can either be specified
with a given amount or receive an equal share of the remaining
unassigned funds. At least one recipient must be provided without
assigned amount to collect the remainder. The `sendall` call will
never create change. The call has a `send_max` option that changes the
default behavior of spending all UTXOs ("no UTXO left behind"), to
maximizing the output amount of the transaction by skipping uneconomic
UTXOs. The `send_max` option is incompatible with providing a specific
set of inputs.
---
Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.
ACKs for top commit:
achow101:
re-ACK bb84b7145b
Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
fa76d8d4d7 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2.
ACKs for top commit:
fanquake:
utACK fa76d8d4d7
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
2726b60a3a test: use MiniWallet for rpc_createmultisig.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (rpc_createmultisig.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
ACKs for top commit:
danielabrozzoni:
re-ACK 2726b60a3a
Tree-SHA512: fb0ef22d3f1c161ca5963cb19ce76533ac3941f15102fc0aa2286ef3bec48f219e5934d504b41976f9f295fb6ca582b737e0fea896df4eb964cdaba1b2c91650
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
c4d76c6faa tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
75656adfd2 test: add functional test for `-maxtipage` parameter (Sebastian Falbesoner)
Pull request description:
This PR adds a missing test for the `-maxtipage` parameter which controls what is the allowed maximum tip age for leaving IBD:
792d0d8d51/src/init.cpp (L540)
Relevant code path in the `CChainState::IsInitialBlockDownload` method:
792d0d8d51/src/validation.cpp (L1479-L1480)
The test is pretty simple and should be self-explanatory.
ACKs for top commit:
MarcoFalke:
review ACK 75656adfd2
Tree-SHA512: 0a10dca13cb18c29e64fc8412f4c8f2bcaff1bab8645bd85266c242ba88ce036a150c03cbbe9810c3bb44649810af0aa9cb3584dbae886a7bdb16b72150d08de
975097f424 Let test_runner.py start multiple jobs per timeslot (Pieter Wuille)
Pull request description:
test_runner.py currently only checks every 0.5s whether any job has finished, and if so, starts at most one new job. At higher parallellism it becomes increasingly likely that multiple jobs have finished at the same time. Fix this by always noticing *all* finished jobs every timeslot, and starting as many new ones.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 975097f424
prayank23:
ACK 975097f424
Tree-SHA512: b70c51f05efcde9bc25475c192b86e86b4c399495b42dee20576af3e6b99e8298be8b9e82146abdabbaedb24a13ee158a7c8947518b16fc4f33a3b434935b550
126853214a test: add functional test for -startupnotify (Bruno Garcia)
Pull request description:
This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.
ACKs for top commit:
theStack:
Tested ACK 126853214a
kristapsk:
re-ACK 126853214a
Tree-SHA512: 5bf3e46124ee5c9d609c9993e6465d5a71a8d2275dcf07c8ce0549f013f7f8863d483b46b7164152f566468a689371ccb87f01cf118c3c9cac5b2be673b61a5c
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
2f9515f37a rpc: move fees object to match help (josibake)
07ade7db8f doc: add release note for fee field deprecation (josibake)
2ee406ce3e test: add functional test for deprecatedrpc=fees (josibake)
35d928c632 rpc: deprecate fee fields from mempool entries (josibake)
Pull request description:
per #22682 , top level fee fields for mempool entries have been deprecated since 0.17 but are still returned. this PR properly deprecates them so that they are no longer returned unless `-deprecatedrpc=fees` is passed.
the first commit takes care of deprecation and also updates `test/functional/mempool_packages.py` to only use the `fees` object. the second commit adds a new functional test for `-deprecatedrpc=fees`
closes#22682
## questions for the reviewer
* `-deprecatedrpc=fees` made the most sense to me, but happy to change if there is a name that makes more sense
* #22682 seems to indicate that after some period of time, the fields will be removed all together. if we have a rough idea of when this will be, i can add a `TODO: fully remove in vXX` comment to `entryToJSON`
## testing
to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:
```bash
./src/bitcoind -daemon -deprecatedrpc=fees
```
you should see entries with the deprecated fields like so:
```json
{
"<txid>": {
"fees": {
"base": 0.00000671,
"modified": 0.00000671,
"ancestor": 0.00000671,
"descendant": 0.00000671
},
"fee": 0.00000671,
"modifiedfee": 0.00000671,
"descendantfees": 671,
"ancestorfees": 671,
"vsize": 144,
"weight": 573,
...
},
```
you can also check `getmempoolentry` using any of the txid's from the output above.
next start the node without the deprecated flag, repeat the commands from above and verify that the deprecated fields are no longer present at the top level, but present in the "fees" object
ACKs for top commit:
jnewbery:
reACK 2f9515f37a
glozow:
utACK 2f9515f37a
Tree-SHA512: b175f4d39d26d96dc5bae26717d3ccfa5842d98ab402065880bfdcf4921b14ca692a8919fe4e9969acbb5c4d6e6d07dd6462a7e0a0a7342556279b381e1a004e
e4a54af6b8 test: add wallet_transactiontime_rescan.py --descriptors to test_runner.py (Sebastian Falbesoner)
b60e02e993 test: fix test wallet_transactiontime_rescan.py for descriptor wallets (Sebastian Falbesoner)
a905ed1a61 test: refactor: use `set_node_times` helper in wallet_transactiontime_rescan.py (Sebastian Falbesoner)
Pull request description:
The functional test wallet_transactiontime_rescan.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, the test framework maps the importaddress RPC calls to the importdescriptors RPC (rescan=False -> timestamp='now'), which always rescans blocks of the past 2 hours, based on the current MTP timestamp. In order to avoid importing the last address (wo3), we generate 10 more blocks with advanced time, to ensure that the balance after importing is zero:
681b25e3cd/test/functional/wallet_transactiontime_rescan.py (L125-L134)
Calling this test with descriptor wallets is also added to test runner. Fixes#23562.
ACKs for top commit:
Sjors:
tACK e4a54af
brunoerg:
tACK e4a54af6b8
Tree-SHA512: 9fd8e298d48dd7947b1218d61a1a66c1241b3dbb14451b0ec7cd30caa74ee540e7ee5a7bd10d421b9e3b6e549fa5c3e85bd02496436128b433b328118642f600
