aboutsummaryrefslogtreecommitdiff
path: root/test/functional
AgeCommit message (Collapse)Author
2022-04-11Merge bitcoin/bitcoin#24817: test: use MiniWallet for feature_fee_estimation.pyMarcoFalke
494455f8a56c93cef4162a0cedc792901577314e test: use MiniWallet for feature_fee_estimation.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (feature_fee_estimation.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078. It takes use of the recently introduced methods `{create,send}_self_transfer_multi` (#24637) which allows to specify multiple UTXOs to be spent rather than only one. Very likely the test can still be simplified (e.g. coin selection in `small_txpuzzle_randfee`), but this is a first step. ACKs for top commit: ayush933: tACK 494455f8 . The test runs successfully with the wallet disabled. vincenzopalazzo: tACK https://github.com/bitcoin/bitcoin/pull/24817/commits/494455f8a56c93cef4162a0cedc792901577314e Tree-SHA512: 89789fc34a4374c79c4b90acd926ac69153aad655dab50450ed796f03c770bd675ad872e906f516f90e8d4cb40b83b55f3c78a94b13bfb8fe8f5e27624937748
2022-04-11Merge bitcoin/bitcoin#24797: test: compare `/chaininfo` response with ↵MarcoFalke
`getblockchaininfo` RPC 0f7dc893ea1776515173dcd0bfe6826e963c90f3 test: compare `/chaininfo` response with `getblockchaininfo` RPC (brunoerg) Pull request description: The `/chaininfo` REST endpoint gets its infos from `getblockchaininfo` RPC, so this PR adds an `assert_equal` (in `interface_rest`) to ensure both responses are the same. Obs: other endpoints do the same for their respective RPC. ACKs for top commit: 0xB10C: Concept and Code Review ACK 0f7dc893ea1776515173dcd0bfe6826e963c90f3. Belts-and-spenders. Tree-SHA512: 51cbcf988090272e406a47dc869710740b74e2222af29c05ddcbf53bd49765cdc59efb525e970867f091b3d2efec4fb13371a342d9e484e51144b760265bc5b8
2022-04-10test: use MiniWallet for feature_fee_estimation.pySebastian Falbesoner
This test can now be run even with the Bitcoin Core wallet disabled.
2022-04-08Merge bitcoin/bitcoin#24770: Put lock logging behind DEBUG_LOCKCONTENTION ↵fanquake
preprocessor directive 43947333315d07f59e1247bd76e0ba9d35a99e31 Add DEBUG_LOCKCONTENTION documentation to the developer notes (Jon Atack) 39a34b6877945908759f6a2322f60852e521e2ee Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive (Jon Atack) Pull request description: This is a more minimal, no-frills version of #24734 for backport. The other fixes and improvements in that pull can be done after. *Copy of the PR 24734 description:* PRs #22736, #22904 and #23223 changed lock contention logging from a `DEBUG_LOCKCONTENTION` compile-time preprocessor directive to a runtime `lock` log category and improved the logging output. This changed the locking from using `lock()` to `try_lock()`: - `void Mutex::UniqueLock::lock()` acquires the mutex and blocks until it gains access to it - `bool Mutex::UniqueLock::try_lock()` doesn't block but instead immediately returns whether it acquired the mutex; it may be used by `lock()` internally as part of the deadlock-avoidance algorithm In theory the cost of `try_lock` might be essentially the [same](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-697) relative to `lock`. The test-and-set logic of these calls is purported to be ~ constant time, optimised and light/quick if used carefully (i.e. no mutex convoying), compared to system calls, memory/cache coherency and fences, wait queues, and (particularly) lock contentions. See the discussion around https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-902851054 and after with respect to performance/cost aspects. However, there are reasonable concerns (see [here](https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691277896) and [here](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-620)) that `Base::try_lock()` may be potentially [costly](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-700) or [risky](https://github.com/bitcoin/bitcoin/pull/22904#issuecomment-930484001) compared to `Base::lock()` in this very frequently called code. One alternative to keep the run-time lock logging would be to gate the `try_lock` call behind the logging conditional, for example as proposed in https://github.com/bitcoin/bitcoin/commit/ccd73de1dd969097d34634c2be2fc32b03fbd09e and ACKed [here](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-901980815). However, this would add the [cost](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-910102353) of `if (LogAcceptCategory(BCLog::LOCK))` to the hotspot, instead of replacing `lock` with `try_lock`, for the most frequent happy path (non-contention). It turns out we can keep the advantages of the runtime lock contention logging (the ability to turn it on/off at runtime) while out of prudence putting the `try_lock()` call and `lock` logging category behind a `DEBUG_LOCKCONTENTION` compile-time preprocessor directive, and also still retain the lock logging enhancements of the mentioned PRs, as suggested in https://github.com/bitcoin/bitcoin/pull/24734#issuecomment-1085785480 by W. J. van der Laan, in https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691280693, and in the linked IRC discussion. Proposed here and for backport to v23. ACKs for top commit: laanwj: Code review ACK 43947333315d07f59e1247bd76e0ba9d35a99e31 Tree-SHA512: 89b1271cae1dca0eb251914b1a60fc5b68320aab4a3939c57eec3a33a3c8f01688f05d95dfc31f91d71a6ed80cfe2d67b77ff14742611cc206175e47b2e5d3b1
2022-04-06test: compare `/chaininfo` response with `getblockchaininfo` RPCbrunoerg
2022-04-06Merge bitcoin/bitcoin#24576: contrib: testgen: remove redundant base58 ↵fanquake
implementation 65c49ac750ba39801b349d0a59c27471dfa9868e test: throw `ValueError` for invalid base58 checksum (Sebastian Falbesoner) 219d2c7ee1d35a353a96c55d4c411d43fe39548a contrib: testgen: use base58 methods from test framework (Sebastian Falbesoner) 605fecfb66ba51467b35a3f269116ec786aedd05 scripted-diff: rename `chars` to `b58chars` in test_framework.address (Sebastian Falbesoner) 11c63e374d058d3bde64a725068d29c874950b45 contrib: testgen: import OP_* constants from test framework (Sebastian Falbesoner) 7d755bb31cd58099cd97b604e04a6a4bb99cd2a9 contrib: testgen: avoid need for manually setting PYTHONPATH (Sebastian Falbesoner) Pull request description: This PR removes the redundant base58 implementation [contrib/testgen/base58.py](https://github.com/bitcoin/bitcoin/blob/master/contrib/testgen/base58.py) for the test generation script `gen_key_io_test_vectors.py` and uses the one from the test framework instead. Additionally, three other cleanups/improvements are done: - import script operator constants `OP_*` from test framework instead of manually defining them - add Python path to test framework directly in the script (via `sys.path.append(...)`) instead of needing the caller to specify `PYTHONPATH=...` on the command line (the same approach is done for the signet miner and the message capture scripts) - rename `chars` to `b58chars` in the test_framework.address module (is more explicit and makes the diff for the base58 replacement smaller) ACKs for top commit: laanwj: Code review ACK 65c49ac750ba39801b349d0a59c27471dfa9868e Tree-SHA512: 92e1534cc320cd56262bf455de7231c6ec821bfcd0ed58aa5718271ecec1a89df7951bf31527a2306db6398e7f2664d2ff8508200c28163c0b164d3f5aaf8b0e
2022-04-06Merge bitcoin/bitcoin#24358: test: USDT tracepoint interface testslaanwj
76c60d7b31ccc50b226cdbc5e38be0bd67603408 test: validation:block_connected tracepoint test (0xb10c) 260e28ece87ba2e732ff8d8a379c4b27e77e3c0d test: utxocache:* tracepoint tests (0xb10c) 34b27bac684f2f373c5e1d90697d6bc8a014f45a test: net:in/out_message tracepoint tests (0xb10c) c934087b627f7d368458781944f990b0eb479634 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 76c60d7b31ccc50b226cdbc5e38be0bd67603408 laanwj: Tested ACK 76c60d7b31ccc50b226cdbc5e38be0bd67603408 Tree-SHA512: 9a63d945c68102e59d751bd8d2805ddd7b37185408fa831d28a9cb6641b701961389b55f216c475df7d4771154e735625067ee957fc74f454ad7a7921255364c
2022-04-06Merge bitcoin/bitcoin#24732: Remove buggy and confusing IncrementExtraNonceMarcoFalke
cccc4e879a8cb9d858a88ea46b28ea27ab79ca55 Remove nHeightEnd and nHeight in generateBlocks helper (MarcoFalke) fa38b1c8bd29e2c792737f6481ab928e46396b7e Remove buggy and confusing IncrementExtraNonce (MarcoFalke) Pull request description: IncrementExtraNonce has many issues: * It is test-only code, but part of bitcoind * It is using the block height of the tip, as opposed to the block's previous block as reference for the new height. See https://github.com/bitcoin/bitcoin/issues/24730#issuecomment-1085586193 * It has no use case in regtest testing. With a low difficulty the extra nonce won't be incremented. With a high difficulty the test-only functions are clumsy to handle anyway. For example, the generate* RPCs will return an empty array once they reached `maxtries`, as opposed to an error. Also the calls can't be aborted early unless the node shuts down completely. So I think it is fine to just remove the extra nonce functionality and leave it to the outside to implement, if needed. For example, a wrapper script can call the `generate*` RPCs once every second, to use the timestamp as extra nonce. ACKs for top commit: ajtowns: ACK cccc4e879a8cb9d858a88ea46b28ea27ab79ca55 Tree-SHA512: d8a3989ad280ebd4b1b574159b3a396b8a42134347e6be3c88445162d86624d221c416456f45ae75aea62ed8c8a1a9bb3a2532924abca2ef7a879cb8e6b15654
2022-04-06Merge bitcoin/bitcoin#24098: rest: Use query parameters to control resource ↵MarcoFalke
loading 54b39cfb342d10a448d49299c715e3a25c2aca4a Add release notes (stickies-v) f959fc0397c3f3615e99bc28d2df549d9d52f277 Update /<count>/ endpoints to use a '?count=' query parameter instead (stickies-v) a09497614e9bb603fff36286d9611a25b23eeb02 Add GetQueryParameter helper function (stickies-v) fff771ee864975cee8c831651239bac95503c37a Handle query string when parsing data format (stickies-v) c1aad1b3b95b7c6bdf05e0c2095aba2f2db8310b scripted-diff: rename RetFormat to RESTResponseFormat (stickies-v) 9f1c54787c81177dd56a31c881a9ad2834a122dc Refactoring: move declarations to rest.h (stickies-v) Pull request description: In RESTful APIs, [typically](https://rapidapi.com/blog/api-glossary/parameters/query/) path parameters (e.g. `/some/unique/resource/`) are used to represent resources, and query parameters (e.g. `?sort=asc`) are used to control how these resources are being loaded through e.g. sorting, pagination, filtering, ... As first [discussed in #17631](https://github.com/bitcoin/bitcoin/pull/17631#discussion_r733031180), the [current REST api](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) contains two endpoints `/headers/` and `/blockfilterheaders/` that rather unexpectedly use path parameters to control how many (filter) headers are returned in the response. While this is no critical issue, it is unintuitive and we are still early enough to easily phase this behaviour out and ensure new endpoints (if any) do not have to stick to non-standard behaviour just for internal consistency. In this PR, a new `HTTPRequest::GetQueryParameter` method is introduced to easily parse query parameters, as well as two new `/headers/` and `/blockfilterheaders/` endpoints that use a count query parameter are introduced. The old path parameter-based endpoints are kept without too much overhead, but the documentation now points to the new query parameter-based endpoints as the default interface to encourage standardness. ## Behaviour change ### New endpoints and default values `/headers/` and `/blockfilterheaders/` now have 2 new endpoints that contain query parameters (`?count=<count>`) instead of path parameters (`/<count>/`), as described in REST-interface.md. Since query parameters can easily have default values, I have set this at 5 for both endpoints. **headers** `GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>` should now be used instead of `GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>` **blockfilterheaders** `GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>` should now be used instead of `GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>` ### Some previously invalid API calls are now valid API calls that contained query strings in the URI could not be parsed prior to this PR. This PR changes behaviour in that previously invalid calls (e.g. `GET /rest/headers/5/somehash.json?someunusedparam=foo`) would now become valid, as the query parameters are properly parsed, and discarded if unused. For example, prior to this PR, adding an irrelevant `someparam` parameter would be illegal: ``` GET /rest/headers/5/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true -> Invalid hash: 0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true ``` **This behaviour change affects all rest endpoints, not just the 2 new ones introduced here.** *(Note: I'd be open to implementing additional logic to refuse requests containing unrecognized query parameters to minimize behaviour change, but for the endpoints that we currently have I don't really see the point for that added complexity. E.g. I don't see any scenarios where misspelling a parameter could lead to harmful outcomes)* ## Using the REST API To run the API HTTP server, start a bitcoind instance with the `-rest` flag enabled. To use the `blockfilterheaders` endpoint, you'll also need to set `-blockfilterindex=1`: ``` ./bitcoind -signet -rest -blockfilterindex=1 ``` As soon as bitcoind is fully up and running, you should be able to query the API, for example by using curl on the command line: ```curl "127.0.0.1:38332/rest/chaininfo.json"```. To more easily parse the JSON output, you can also use tools like 'jq' or `json_pp`, e.g.: ``` curl -s "localhost:38332/rest/blockfilterheaders/basic/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?count=2" | json_pp . ``` ## To do - [x] update `doc/release-notes` ## Feedback This is my first PR (hooray!). Please don't hold back on any feedback/comments/nits/... you may have, big or small, whether they are code, process, language, ... related. I welcome private messages too if there's anything you don't want to clutter the PR with. I'm here to learn and am grateful for everyone's input. ACKs for top commit: stickies-v: I've had to push a tiny doc update to `REST-interface.md` (`git range-diff 219d728 9aac438 54b39cf`) since this was not merged for v23, but since there are no significant changes beyond theStack and jnewbery's ACKs I think this PR is now ready to be considered for merging? @MarcoFalke jnewbery: ACK 54b39cfb342d10a448d49299c715e3a25c2aca4a theStack: re-ACK 54b39cfb342d10a448d49299c715e3a25c2aca4a Tree-SHA512: 3b393ffde34f25605ca12c0b1300799a19684b816a1d03aed38b0f5439df47bfe6a589ffbcd7b83fd2def6c9d00a1bae5e45b1d18df4ae998c617c709990f83f
2022-04-05test: throw `ValueError` for invalid base58 checksumSebastian Falbesoner
2022-04-05scripted-diff: rename `chars` to `b58chars` in test_framework.addressSebastian Falbesoner
-BEGIN VERIFY SCRIPT- sed -i 's/chars/b58chars/g' ./test/functional/test_framework/address.py -END VERIFY SCRIPT-
2022-04-05Update /<count>/ endpoints to use a '?count=' query parameter insteadstickies-v
In most RESTful APIs, path parameters are used to represent resources, and query parameters are used to control how these resources are being filtered/sorted/... The old /<count>/ functionality is kept alive to maintain backwards compatibility, but new paths with query parameters are introduced and documented as the default interface so future API methods don't break consistency by using query parameters.
2022-04-05Merge bitcoin/bitcoin#24236: Remove utxo db upgrade codelaanwj
fa9112aac07dc371bfda437d40eb1b841f36f392 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 19a56d1519fb493c3e1bd5cad55360b6b80fa52b (released in version 22.0). Any Bitcoin Core version with the new database format after commit 1088b02f0ccd7358d2b7076bb9e122d59d502d02 (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 fa9112aac07dc371bfda437d40eb1b841f36f392 laanwj: Code review ACK fa9112aac07dc371bfda437d40eb1b841f36f392 Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
2022-04-05Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directiveJon Atack
2022-04-04test: use MiniWallet for mempool_unbroadcast.pyAyush Sharma
This test can now be run even with the Bitcoin Core wallet disabled.
2022-04-01Remove buggy and confusing IncrementExtraNonceMarcoFalke
2022-03-31Merge bitcoin/bitcoin#24698: test: -peerblockfilters without ↵MarcoFalke
-blockfilterindex raises an error d6bc2322ed2e0674e027d39825fdadbb0db2c24a test: -peerblockfilters without -blockfilterindex raises an error (brunoerg) Pull request description: This PR adds test coverage for the following init error: https://github.com/bitcoin/bitcoin/blob/2a3e8fb3592e42300ec96c9f6724e15346e30ea7/src/init.cpp#L850 Setting -peerblockfilters without -blockfilterindex should raise an error when initializing. ACKs for top commit: ccdle12: Tested ACK https://github.com/bitcoin/bitcoin/pull/24698/commits/d6bc2322ed2e0674e027d39825fdadbb0db2c24a Tree-SHA512: e740c2ccde6bb1bb8381bb676a6d01bd5746cf9ce0c8dadd62067a6b9b380027bfe8b8cdeae9846a0ab18385f3dc5dff607fe5274cb55107d47470db00015fb2
2022-03-30Merge bitcoin/bitcoin#24118: Add 'sendall' RPC née sweepMarcoFalke
bb84b7145b31dbfdcb4cf0b9b6e612a57e573993 add tests for no recipient and using send_max while inputs are specified (ishaanam) 49090ec4025152c847be8a5ab6aa6f379e345260 Add sendall RPC née sweep (Murch) 902793c7772e5bdd5aae5b0d20a32c02a1a6dc7c Extract FinishTransaction from send() (Murch) 6d2208a3f6849a3732af6ff010eeea629b9b10d0 Extract interpretation of fee estimation arguments (Murch) a31d75e5fb5c1304445d698595079e29f3cd3a3a Elaborate error messages for outdated options (Murch) 35ed094e4b0e0554e609709f6ca1f7d17096882c 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 bb84b7145b31dbfdcb4cf0b9b6e612a57e573993 Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
2022-03-29add tests for no recipient and using send_max while inputs are specifiedishaanam
2022-03-29Add sendall RPC née sweepMurch
_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.
2022-03-28test: -peerblockfilters without -blockfilterindex raises an errorbrunoerg
2022-03-28Merge bitcoin/bitcoin#24623: test: Add diamond-shape prioritisetransaction testMarcoFalke
fa0758e1457552e9243bc9663408e88a8cccc8f6 test: Add diamond-shape prioritisetransaction test (MarcoFalke) fa450c18db7326b2924db435a753620895b41c53 test: Rework create_self_transfer_multi (MarcoFalke) Pull request description: Looks like there is no test for diamonds, only for chains (in `mempool_packages.py`) ACKs for top commit: jamesob: ACK https://github.com/bitcoin/bitcoin/pull/24623/commits/fa0758e1457552e9243bc9663408e88a8cccc8f6 Tree-SHA512: d261184a81df77d24fc256f58ad5ed4a13b7cd4e33f74c8b79495c761ff417817602d8e5d4f63f4bb1000ac63f89bbfa54d8d8994a7b2bb2e8a484c467330984
2022-03-28Merge bitcoin/bitcoin#24687: test: Check an invalid -i2psam will raise an ↵MarcoFalke
init error 45e67b2695852c94d5da3f9b82f2b567cbbd2cc3 test: invalid -i2psam will raise an init error (brunoerg) Pull request description: This PR adds test coverage (at `feature_proxy.py`) for the following init error: https://github.com/bitcoin/bitcoin/blob/2f0f056e08cd5a1435120592a9ecd212fcdb915b/src/init.cpp#L1791 It starts the node with an invalid -i2psam (`-i2psam=invalidhere`) and test if it raises an error when initializing. ACKs for top commit: dunxen: Code review ACK 45e67b2 Tree-SHA512: b24e3f6e7a9316b9ebc0b6c8bcf1315faff60a9e258d7bb3dbeb9f6695a728bb3083aea2f81114072fe13822bfca34d4a0f44f229825f7c97a81619d810010c0
2022-03-28Merge bitcoin/bitcoin#24258: test: check localaddresses in getnetworkinfo ↵MarcoFalke
for nodes with proxy 89bb25d22a0e1c700dba4e3b754984c9b2b14836 test: check localaddresses in getnetworkinfo for nodes with proxy (brunoerg) Pull request description: This PR adds test coverage for the field `localaddresses` for `getnetworkinfo`. In this case, it verifies if this field is empty for all nodes since they are using proxy. Reference: https://github.com/bitcoin/bitcoin/blob/515200298b555845696a07ae2bc0a84a5dd02ae4/src/init.cpp#L449 ACKs for top commit: jonatack: ACK 89bb25d22a0e1c700dba4e3b754984c9b2b14836 Tree-SHA512: 3c765c7060b6972c1ae5a1104734cd7669b650b5f6aa4f623f4299567732260da5083fef306a7c1e71c931f5d1396f24abad251d95c3d82b1f3ee0efee7fcd1f
2022-03-26test: invalid -i2psam will raise an init errorbrunoerg
2022-03-25Merge bitcoin/bitcoin#24494: wallet: generate random change target for each ↵fanquake
tx for better privacy 9053f64fcbd26d87c26ae6b982d17756a6ea0896 [doc] release notes for random change target (glozow) 46f2fed6c5e0fa623bfeabf61ba4811d5cf8f47c [wallet] remove MIN_CHANGE (glozow) a44236addd01cff4e4d751e0f379d399fbfc8eae [wallet] randomly generate change targets (glozow) 1e52e6bd0a8888efb4ed247d74ec7ca9dfc2e002 refactor coin selection for parameterizable change target (glozow) Pull request description: Closes #24458 - the wallet always chooses 1 million sats as its change target, making it easier to fingerprint transactions created by the Core wallet. Instead of using a fixed value, choose one randomly each time (within a range). Using 50ksat (around $20) as the lower bound and `min(1 million sat, 2 * average payment value)` as the upper bound. RFC: If the payment is <25ksat, this doesn't work, so we're using the range (payment amount, 50ksat) instead. ACKs for top commit: achow101: ACK 9053f64fcbd26d87c26ae6b982d17756a6ea0896 Xekyo: reACK 9053f64fcbd26d87c26ae6b982d17756a6ea0896 Tree-SHA512: 45ce5d064697065549473347648e29935733f3deffc71a6ab995449431f60302d1f9911a0994dfdb960b48c48b5d8859f168b396ff2a62db67d535a7db041d35
2022-03-25Merge bitcoin/bitcoin#24502: wallet: don't create long chains by defaultMarcoFalke
da2bc865d644f6be748c305556bdd02f02d1b161 [wallet] don't create long chains by default (glozow) Pull request description: Default mempool policy doesn't let you have chains longer than 25 transactions. This is locally configurable of course, but it's not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set `DEFAULT_WALLET_REJECT_LONG_CHAINS` to true. Closes #9752 Closes #10004 ACKs for top commit: MarcoFalke: re-ACK da2bc865d644f6be748c305556bdd02f02d1b161 only change is fixing typos in tests 🎏 Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
2022-03-25[wallet] don't create long chains by defaultglozow
2022-03-25Merge bitcoin/bitcoin#24670: test: move-only: Move all generate* tests to a ↵MarcoFalke
single file 0000ff0d6b2442706a588fd906ebf1adf8ff8226 test: move-only: Move all generate* tests to a single file (MarcoFalke) Pull request description: Seems a bit overkill to spread tests for the `generate*` methods over several files. Combining them into a single file has also a nice side-effect of requiring less node (re)starts, which are expensive in valgrind. ACKs for top commit: glozow: utACK 0000ff0d6b2442706a588fd906ebf1adf8ff8226 Tree-SHA512: 8269eb05649a871011bbfbd1838d0f7d1dac4a35b3b198fc43fe85131fda8a53803b75da78cbf422eabf086006dee4421e622fbe706f6781a3848b989024001b
2022-03-25Merge bitcoin/bitcoin#21160: net/net processing: Move tx inventory into ↵fanquake
net_processing 1066d10f71e6800c78012d789ff6ae19df0243fe scripted-diff: rename TxRelay members (John Newbery) 575bbd0dea6d12510fdf3220d0f0e47d969da6e9 [net processing] Move tx relay data to Peer (John Newbery) 785f55f7eeab0dedbeb8e0d0b459f3bdc538b621 [net processing] Move m_wtxid_relay to Peer (John Newbery) 36346703f8558d6781c079c29ddece5a97477beb [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607. For motivation, see #19398. ACKs for top commit: dergoegge: ACK 1066d10f71e6800c78012d789ff6ae19df0243fe - This is a good layer separation improvement with no behavior changes. glozow: utACK 1066d10f71e6800c78012d789ff6ae19df0243fe Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
2022-03-25Merge bitcoin/bitcoin#23536: Enforce Taproot script flags whenever WITNESS ↵laanwj
is set cccc1e70b8a14430cc94143da97936a60d6c83d3 Enforce Taproot script flags whenever WITNESS is set (MarcoFalke) fa422994116a7a053789304d56159760081479eb Remove nullptr check in GetBlockScriptFlags (MarcoFalke) faadc606c7644f2934de390e261d9d65a81a7592 refactor: Pass const reference instead of pointer to GetBlockScriptFlags (MarcoFalke) Pull request description: Now that Taproot is active, it makes sense to enforce its rules on all blocks, even historic ones, regardless of the deployment status. ### Benefits: (With "script flags" I mean "taproot script verification flags".) * Script flags are known ahead for all blocks (even blocks not yet created) and do not change. This may benefit static analysis, code review, and development of new script features that build on Taproot. * Any future bugs introduced in the deployment code won't have any effect on the script flags, as they are independent of deployment. * Enforcing the taproot rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases. * It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack where the node is eclipsed. While `nMinimumChainWork` already protects against this, the cost for a few months worth of POW might be lowered until a major version release of Bitcoin Core reaches EOL. The needed work for the attack is the difference between `nMinimumChainWork` and the work at block 709632. For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4b33c9d78574627fc606267e2d8955cd1c. ### Implementation: I found one block which fails verification with the flags applied, so I added a `TaprootException`, similar to the `BIP16Exception`. For reference, the debug log: ``` ERROR: ConnectBlock(): CheckInputScripts on b10c007c60e14f9d087e0291d4d0c7869697c6681d979c6639dbd960792b4d41 failed with non-mandatory-script-verify-flag (Witness program was passed an empty witness) BlockChecked: block hash=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad state=non-mandatory-script-verify-flag (Witness program was passed an empty witness) InvalidChainFound: invalid block=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad height=692261 log2_work=92.988459 date=2021-07-23T08:24:20Z InvalidChainFound: current best=0000000000000000000067b17a4c0ffd77c29941b15ad356ca8f980af137a25d height=692260 log2_work=92.988450 date=2021-07-23T07:47:31Z ERROR: ConnectTip: ConnectBlock 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad failed, non-mandatory-script-verify-flag (Witness program was passed an empty witness) ``` Hint for testing, make sure to set `-noassumevalid`. ### Considerations Obviously this change can lead to consensus splits on the network in light of massive reorgs. Currently the last block before Taproot activation, that is the last block without the Taproot script flags set, is only buried by a few days of POW. However, when and if this patch is included in the next major release, it will be buried by a few months of POW. BIP90 considerations apply when looking at reorgs this large. ACKs for top commit: Sjors: tACK cccc1e70b8a14430cc94143da97936a60d6c83d3 achow101: ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 laanwj: Code review ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ajtowns: ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ; code review; wrote a "getblockscriptflags" rpc to quickly check that blocks just had bit 17 (taproot) added; review of earlier revisions had established non-exception blocks do validate with taproot rules enabled. jamesob: ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ([`jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f`](https://github.com/jamesob/bitcoin/tree/ackr/23536.1.MarcoFalke.enforce_taproot_script_f)) Tree-SHA512: 00044de68939caef6420ffd588c1291c041a8b397c80a3df1e3e3487fbeae1821d23975c51c95e44e774558db76f943b00b4e27cbd0213f64a9253116dc6edde
2022-03-25[wallet] randomly generate change targetsglozow
If the wallet always chooses 1 million sats as its change target, it is easier to fingerprint transactions created by the Core wallet.
2022-03-25test: move-only: Move all generate* tests to a single fileMarcoFalke
Can be reviewed with --color-moved=dimmed-zebra
2022-03-24Merge bitcoin/bitcoin#24205: init, test: improve network reachability test ↵MarcoFalke
coverage and safety 58a14795b89a6bd812e0b71cb8b3088b8ab55c11 test: passing -onlynet=onion with -onion=0/-noonion raises expected init error (Jon Atack) 7000f66d367123d1de303fc15ce2ce60df379c11 test: passing -onlynet=onion without -proxy/-onion raises expected init error (Jon Atack) 8332e6e4cf45455fea0bf1f7527256cdb7bb1e6d test: passing invalid -onion raises expected init error (Jon Atack) d5edb087082a50e6f7d413c3b43fdf1e6a20d29b test: passing invalid -proxy raises expected init error (Jon Atack) bd57dcbaf2b5e5f50833912c894a1f1239ceb25b test: hoist proxy out of 2 network loops in feature_proxy.py (Jon Atack) afdf2de28296660fd0284453a241aece8494eea8 test: add CJDNS to LimitedAndReachable_Network unit tests (Jon Atack) 2b7a8180a94738c2fcb21232a2eca07a7b27656d net, init: assert each network reachability is true by default (Jon Atack) Pull request description: Adds missing network reachability test coverage and an assertion during init, noticed while reviewing #22834: - assert during init that each network reachability is true by default - add CJDNS to the `LimitedAndReachable_Network` unit tests - hoist proxy out of two network loops in feature_proxy.py - test that passing invalid `-proxy` raises expected init error - test that passing invalid `-onion` raises expected init error - test that passing `-onlynet=onion` without `-proxy` and `-onion` raises expected init error - test that passing `-onlynet=onion` with `-onion=0` and with `-noonion` raises expected init error ACKs for top commit: vasild: ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11 brunoerg: ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11 dongcarl: Code Review ACK 58a14795b89a6bd812e0b71cb8b3088b8ab55c11 Tree-SHA512: bdee6dd0c12bb63591ce7c9321fe77b509ab1265123054e774adc38a187746dddafe1627cbe89e990bcc78b45e194bfef8dc782710d5b217e2e2106ab0158827
2022-03-24Merge bitcoin/bitcoin#24653: test: use `MiniWallet` in ↵MarcoFalke
`test/functional/interface_zmq` bc90b8d86916d43867762a391633664676550bd8 [move only] remove `is_wallet_compiled` checks (josibake) 0bfbf7fb2488753d795ffc1c63a8977e4fe4a3bc test: use MiniWallet in `interfaces_zmq` (josibake) Pull request description: While working on #24584 , `interface_zmq` started failing due to coin selection not running deterministically. The test doesn't actually need the wallet, so this PR migrates it to use MiniWallet _Note for reviewers:_ the second commit moves large chunks of code out of an if block, so it may be helpful to review with something that ignores whitespace, e.g `git diff -w master` ACKs for top commit: vincenzopalazzo: ACK https://github.com/bitcoin/bitcoin/pull/24653/commits/bc90b8d86916d43867762a391633664676550bd8 Tree-SHA512: c618e23d00635d72dafdef28e68cbc88b9cc2030d4898fc5b7eac926fd621684c1958c075ed167192716b18308da5a0c1f1393396e31b99d0d3bde78b78fefc5
2022-03-24test: Add diamond-shape prioritisetransaction testMarcoFalke
2022-03-24test: Rework create_self_transfer_multiMarcoFalke
* Add fallback for utxos_to_spend if none are provided * Refactor a for-loop
2022-03-24Merge bitcoin/bitcoin#24626: init: disallow reindex-chainstate when pruningMarcoFalke
b2813980b81034ff9b40bd45080fa67dea475d39 init: disallow reindex-chainstate when pruning (Martin Zumsande) Pull request description: The combination of `-reindex-chainstate` and `-prune` currently makes the node stuck in an endless loop: - `LoadChainstate()` will wipe the existing chainstate (so we have no genesis block anymore). It won't clean up unusable block files by calling `CleanupBlockRevFiles()` as for full `-reindex`. - `ThreadImport()` has [logic](https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/node/blockstorage.cpp#L855) of reloading Genesis after reindexing. This is what makes full `-reindex` work with `-prune` but it's not executed for `-reindex-chainstate`. - Since we still don't have a genesis block, init will wait for it forever in an endless loop ([code](https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/init.cpp#L1630-L1640)). Fix this by disallowing `-reindex-chainstate` together with `-prune`. This is discouraged in the help for `-reindex-chainstate` anyway ("When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.") but wasn't enforced. Fixes #24242 ACKs for top commit: MarcoFalke: cr ACK b2813980b81034ff9b40bd45080fa67dea475d39 Tree-SHA512: 7220842daaf9a4f972d82b13b81fdeac2833bf5e665c5b0f8eaf6a4bcd0725c8e97d19ec956ca4b730065a983475bb3a2732713d338f4caf8666ccbf63d4d988
2022-03-24init: disallow reindex-chainstate when pruningMartin Zumsande
This fixes a bug where the node would be stuck in an endless loop when combining these parameters.
2022-03-24Merge bitcoin/bitcoin#24574: test: Actually print TSan tracebacksfanquake
fa76d8d4d71d844e217686881d4f630eac3a8e10 test: Actually print TSan tracebacks (MarcoFalke) Pull request description: Commit 5e5138a721738f47053d915e4c65f925838ad5b4 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 451b96f7d2796d00eabaec56d831f9e9b1a569cc. ACKs for top commit: fanquake: utACK fa76d8d4d71d844e217686881d4f630eac3a8e10 Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
2022-03-24Merge bitcoin/bitcoin#24637: test: use MiniWallet for mempool_package_onemore.pyMarcoFalke
2b6dd4e75b3ad2daff553fde018fe4c8f1187878 test: use MiniWallet for mempool_package_onemore.py (Sebastian Falbesoner) eb3c5c4ef2eeb1d37d729d4487ed067a24cf81c8 test: MiniWallet: add helper methods `{send,create}_self_transfer_multi` (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (mempool_package_onemore.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078. For this purpose helper methods `MiniWallet.{create,send}_self_transfer_multi` are introduced which serve as a replacement for `chain_transaction`. With this, it should be also quite straight-forward to change the larger related test `mempool_packages.py` to use MiniWallet. ACKs for top commit: MarcoFalke: ACK 2b6dd4e75b3ad2daff553fde018fe4c8f1187878 💾 Tree-SHA512: 0c97fa0519ca5eaa6df8953a04678aa8a6a66905a82db6ff40042a675d0c0682aee829a48db84e4e7983d8f766875021f0d39d65e12889342610b8861bc29cd5
2022-03-24Merge bitcoin/bitcoin#24587: test: use MiniWallet for rpc_createmultisig.pyfanquake
2726b60a3ac098b44f2970bed21147b70e12a1c2 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 2726b60a3ac098b44f2970bed21147b70e12a1c2 Tree-SHA512: fb0ef22d3f1c161ca5963cb19ce76533ac3941f15102fc0aa2286ef3bec48f219e5934d504b41976f9f295fb6ca582b737e0fea896df4eb964cdaba1b2c91650
2022-03-24[move only] remove `is_wallet_compiled` checksjosibake
2022-03-24test: use MiniWallet in `interfaces_zmq`josibake
make interfaces_zmg run deterministically. this test is for the zmg notifications, so it doesn't need the wallet compiled to run
2022-03-23Merge bitcoin/bitcoin#24635: test: Run non-wallet tests only oncefanquake
fa7a576391ad5d61af937dd62496ded44105c671 test: Run non-wallet tests only once (MarcoFalke) Pull request description: I don't see why non-wallet tests should run for two wallet configs, even though they never use a wallet. ACKs for top commit: achow101: ACK fa7a576391ad5d61af937dd62496ded44105c671 Tree-SHA512: 2a135acf3c3c83a2704ae11f40c72882b23a676828647be1a066653c4d00e4523704f377eb8745c6386829601cc5d643abdce376831c1db91a07e999e1d5e01f
2022-03-22test: check localaddresses in getnetworkinfo for nodes with proxybrunoerg
2022-03-22test: use MiniWallet for mempool_package_onemore.pySebastian Falbesoner
This test can now be run even with the Bitcoin Core wallet disabled.
2022-03-22test: MiniWallet: add helper methods `{send,create}_self_transfer_multi`Sebastian Falbesoner
2022-03-22test: use MiniWallet for rpc_createmultisig.pyAyush Sharma
This test can now be run even with the Bitcoin Core wallet disabled.
2022-03-22Merge bitcoin/bitcoin#24535: test: Fix generate calls and comments in ↵MarcoFalke
feature_segwit fa8593f89892ddb09b64a7740087b725b3d7b5eb test: Fix generate calls and comments in feature_segwit (MarcoFalke) Pull request description: There are currently a few incorrect comments: Block `432` is mined "twice" (The second one is actually 433). There isn't any need to mine this many blocks anyway, so remove a few calls. ACKs for top commit: theStack: Tested ACK fa8593f89892ddb09b64a7740087b725b3d7b5eb Tree-SHA512: b034077b85e6c978a80aa4de493797b4ae451d686cfb3e4fe40f37a38f41f7cb886f8e00a1c245a284be3502164b17414097fcb0bef66d155a1c1db5cfbe9e8f
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-07 09:42:07 +0000