035eef4be6 lint: Convert lint-python-utf8-encoding.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-python-utf8-encoding.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
laanwj:
Code review ACK 035eef4be6
Tree-SHA512: a8a2f505bf7953d318837182101346c44e73cfd1bf3b5342ff1400fb1c67c5292519fa99db1035da87cf27fb5f5ac5d28871bf55a1c085b5f8a3bb33ff0fa3fb
3043a1bc9d lint: Make known violations more specific in lint-locale-dependence (Dimitri)
229917d3d4 lint: Convert lint-locale-dependence.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-locale-dependence.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
laanwj:
Tested and code review ACK 3043a1bc9d
Tree-SHA512: 80555cf7aac156bab5488f85098731d1c12a42667fe7d0df0c35487ab8fc951654a70a15351a759282eabab8319f5aabd8bdb153412b9edc3a9033bef64fd609
9f5ab670e7 tests: Use descriptor that requires both legacy and segwit (Andrew Chow)
8a04a386f7 tests: Calculate input weight more accurately (Andrew Chow)
Pull request description:
The external input tests with specifying input weight would sometimes result in a test failure because it would add 2 to the calculated byte size in order to account for some of the variation in signature and script sizes. However 1 in 128 signatures are actually 1 byte smaller than we expect, so the difference between the actual signature size and our calculated size becomes 3 bytes which is outside of the tolerance of `assert_fee_amount` and would thus cause the test failure.
To resolve this, the 2 byte buffer is reduced to 1 byte, so in the above scenario, the difference is 2 bytes which is within the tolerance of `assert_fee_amount`. Additionally, instead of putting a fixed size that we assume is the correct size for the length of the compact size length prefix of data, we actually get the length of the compact size uint.
Lastly, the size calculation for a scriptWitness was simply incorrect and used fields that did not exist. This is fixed, and the test slightly modified so that it also produces a scriptWitness.
Fixes#24151
ACKs for top commit:
jonatack:
re-ACK 9f5ab670e7
glozow:
code review ACK 9f5ab670e7
Tree-SHA512: b7c7ffe8fb0c07bc9e72fbff1f9ef57ee01a57c56bf54b8873345c8b9572c3ce9402b24dc211910b478114a9e6420faef5a4bf8866f38c299971354e54ec4745
ee02c8bd9a util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9a
MarcoFalke:
ACK ee02c8bd9a🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
MiniWallet's core method for creating txs (`create_self_transfer`)
right now always executes the `testmempoolaccept` RPC to check for
mempool validity or invalidity. In some test cases where we use
MiniWallet to create a huge number of transactions this can lead
to performance issues (e.g. feature_fee_estimation.py where the
execution time after MiniWallet usage almost doubled). Providing
the possibility to skip the mempool checks is a mitigation for
this.
master branch:
$ time ./test/functional/feature_fee_estimation.py
real 3m20.771s
user 2m52.360s
sys 0m39.340s
PR branch:
$ time ./test/functional/feature_fee_estimation.py
real 2m1.386s
user 1m42.510s
sys 0m22.980s
Also explicitly rehash in the cases where we modify a tx after signing
in feature_csv_activation.py. Parts of this test relied on the fact that
rehashing of transactions is done in the course of calculating a block's
merkle root (`calc_merkle_root`), which only works if no hash was
calculated before due to a caching mechanism.
In the following commit the txid in MiniWallet is calculated via
`rehash()`, i.e. this doesn't work anymore and we always have to
explicitely have the right hash before we calculate the merkle root.
bef61496ab test: compare `/mempool/contents` response with `getrawmempool` RPC (brunoerg)
5bc5cbaf31 doc: add reference to `getrawmempool` RPC in `/mempool/contents` REST doc (brunoerg)
Pull request description:
This PR is similar to #24797, it compares `/mempool/contents` REST response with `getrawmempool` RPC (verbose=True) since they use the same `MempoolToJSON` function.
Also, adds a reference to `getrawmempool` RPC help to get details about the fields from `/mempool/contents`.
ACKs for top commit:
0xB10C:
ACK bef6149
Tree-SHA512: b7e9e9c765ee837986ba167b9234a9b95c9ef0a9ebcc2a03d50f6be6d3aba1480bd77c78111d95df1e4023cde6dfc64bf1e7908d9e5b6f96ca46b76611a4a9b4
fa2153b05b test: Remove unused taproot node from wallet_taproot.py (MarcoFalke)
Pull request description:
Now that the wallet considers taproot always active after commit 064c729a96, there is no need to test for it.
ACKs for top commit:
dunxen:
Code review ACK fa2153b
brunoerg:
crACK fa2153b05b
Tree-SHA512: 24e4a66e43d1391acb63fd0c0c52677b0eef7f618b87a5b1a75224a9be58c9c3f8bba2de3b7510f25a686865b027f7f535e653d40d519d0e00ace38f0c7aba0c
67b41678c8 lint: Convert lint-includes.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-includes.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
KevinMusgrave:
Tested ACK 67b41678c8
Tree-SHA512: 05b4b114dc101e571004aee8aea1480e4dda1dc645426100649e9cb81e56e8667f88d6d5646a9860ea1c7abc36754eda2a77ec10156c54b62db00e2c00b8ceae
917a89a814 test: use MiniWallet for p2p_segwit.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_segwit.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
This change only affects the subtest `test_superfluous_witness`. Note that instead of creating a raw transaction first and then signing it, we go the other direction here: MiniWallet creates a transaction spending a segwit v1 output (i.e. including a witness), then we turn it into a raw transaction by dropping the witness. Therefore, the debug log asserts are swapped.
Top commit has no ACKs.
Tree-SHA512: 163a93a527f60100487f0aff49a9d7baf392ceb4417c54521157b2678685f5728dd751a9747c6cf51666aae78252dd3bc44130e659f7a1262ec1c86e30225622
47b66ac4ac lint: Convert Python linter to Python (Fabian Jahr)
Pull request description:
The outputs provided by the Python version should be exactly the same as the ones from the shell version.
There is small improvement here: Previously only the dependency of `flake9` was checked, now all dependencies are checked before running.
I also tried to mostly follow the [recommendations here](https://github.com/bitcoin/bitcoin/pull/24766#pullrequestreview-932953476) but happy to make more changes if there is still room for improvement.
ACKs for top commit:
laanwj:
Tested ACK 47b66ac4ac
Tree-SHA512: 1630188e176c1063b8905669b76682b361a858cde6990ab17e51ad4333bf376eab796050cdb9f2967b84f1f74379d9e860c4258561b1964e1a47183c593e5bb4
f27fcd9bf4 lint: Convert lint-git-commit-check.sh to Python (Dimitri)
Pull request description:
A port of `/test/lint/lint-git-commit-check.sh` to a Python-script as part of the request of #24783 . Checked for output-consistency.
ACKs for top commit:
laanwj:
re-ACK f27fcd9bf4
Tree-SHA512: afc4a662f4aec1796c023b98a875c1591940ecdfc709eefe2df29d33e51e807c3c2e2b5c410aa3ad1cd3f6f8207f5c15b638637ff9f5659cafa7543bbe8a0bae
a75f6d86d1 lint: Convert lint-whitespace.sh to Python (Dimitri)
Pull request description:
A port of `/test/lint/lint-whitespace.sh` to a Python-script as part of the request of #24783 . Checked for output-consistency.
ACKs for top commit:
laanwj:
Code review and tested ACK a75f6d86d1
Tree-SHA512: 982041b0beb1b3866493ad523950c9a536a8b1ec79b773fe86dbc1166844c13a30b384e92025f845d45d25334f90f3abda5fa23f0f28e7c2cddc5e496f84c445
6f29409ad1 test: Add a test that creates a wallet with invalid parameters (w0xlt)
0359d9b6a3 Change wallet validation order (w0xlt)
Pull request description:
In the current code, the database is created before the last validation, which checks that passphrase is set and private keys are disabled.
Therefore, if this validation fails, it will result in an empty database and the user will not be able to recreate a wallet with the same name and with the correct parameters.
Behavior on the master branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01"
error code: -4
error message:
Wallet file verification failed. Failed to create database path '/home/w/.bitcoin/regtest/wallets/invalid_wallet'. Database already exists.
```
Behavior on the PR branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02"
{
"name": "invalid_wallet_01",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6f29409ad1
Tree-SHA512: d192955fc2285bf27ae5dd4c1b7cfd3d85441a7f3554b189b974aefb319c6b997543991dbb0ca2c8cb980f7058913a77cf0164c02e9b51ceb9c2cb601317c428
This change only affects the subtest `test_superfluous_witness`.
Note that instead of creating a raw transaction first and then
signing it, we go the other direction here: MiniWallet creates a
transaction spending a segwit v1 output (i.e. including a witness),
then we turn it into a raw transaction by dropping the witness.
Therefore, the debug log asserts are swapped.
88376c623c test: Test for disabling wallet flags (Andrew Chow)
17ab31aa46 rpc, wallet: setwalletflags warnings are optional (Andrew Chow)
Pull request description:
Trying to disable a wallet flag with `setwalletflag` results in `Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'`. This occurs because the `warnings` field was not marked as optional. This PR makes `warnings` optional to avoid this error.
Also added a test case because apparently we didn't already have one.
ACKs for top commit:
w0xlt:
ACK 88376c6
Tree-SHA512: 4f5d3bebf0d022a5ad0f75d70c6562a43c7da6e39e9c3118733327d015c435e2c8d5004fdb039d42407dde5b21231a0f8827623d718abf611a1f06c15af5c806
Use raw string
Use re.search instead of grep in check_matching_test_names
Replaced bash commands in check_unique_test_names with python commands
Use set and sort output
Use set comprehension
Use .splitlines()
Call grep_boost_fixture_test_suite once
splitlines() once
Fixed copyright date
Use check_output() instead of run()
add encoding='utf8'
Use clearer code for getting duplicates
038d2a607f test: add test for signet miner script (Sebastian Falbesoner)
449b96ed97 test: add `is_bitcoin_util_compiled` helper (Sebastian Falbesoner)
dde33eca63 test: determine path to `bitcoin-util` in test framework (Sebastian Falbesoner)
Pull request description:
This PR adds a very basic test for the signet miner script (contrib/signet/miner). ~~It was based on #24553 (merged by now) which fixes a bug (and was also the motivation to write this test).~~
The test roughly follows the steps from https://en.bitcoin.it/wiki/Signet#Custom_Signet, except that the challenge key-pair is created solely with the test framework. Calibration is also skipped, the difficulty is simply set to the first mainnet target `0x1d00ffff` (see also https://bitcoin.stackexchange.com/a/57186).
ACKs for top commit:
laanwj:
re-ACK 038d2a607f
Tree-SHA512: 150698a3c0cda3679661b47688e3b932c9761e777fdd284776b867b485db6a8895960177bd02a53f838a4c9b9bbe6a9beea8d7a5b14825b38e4e43b3177821b3
The path is stored in `self.options.bitcoinutil`, points to
`src/bitcoin-util` by default and can be overrided with the
`BITCOINUTIL` environment variable.
e8e48fa82b Converted lint-python-mutable-default-parameters.sh to python (TakeshiMusgrave)
Pull request description:
This converts one of the linter scripts to Python. Reference issue: https://github.com/bitcoin/bitcoin/issues/24783
The approach is to just call git grep using subprocess.run.
Alternative approaches could be to use Python instead of git grep (I'm not sure how) or use ```pylint --disable=all --enable=W0102```, though that requires installation of pylint.
ACKs for top commit:
MarcoFalke:
review ACK e8e48fa82b
Tree-SHA512: 7f6f4887dee02c9751b225a6a131fb705868859c4a9af25bb3485cda2358650486b110f17adf89d96a20f212d7d94899922a07aab12c8dc11984cfd5feb7a076
494455f8a5 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 494455f8a5
Tree-SHA512: 89789fc34a4374c79c4b90acd926ac69153aad655dab50450ed796f03c770bd675ad872e906f516f90e8d4cb40b83b55f3c78a94b13bfb8fe8f5e27624937748
0f7dc893ea 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 0f7dc893ea. Belts-and-spenders.
Tree-SHA512: 51cbcf988090272e406a47dc869710740b74e2222af29c05ddcbf53bd49765cdc59efb525e970867f091b3d2efec4fb13371a342d9e484e51144b760265bc5b8
4394733331 Add DEBUG_LOCKCONTENTION documentation to the developer notes (Jon Atack)
39a34b6877 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 ccd73de1dd 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 4394733331
Tree-SHA512: 89b1271cae1dca0eb251914b1a60fc5b68320aab4a3939c57eec3a33a3c8f01688f05d95dfc31f91d71a6ed80cfe2d67b77ff14742611cc206175e47b2e5d3b1
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)
4105a54381 lint: remove boost::bind linter (fanquake)
Pull request description:
I don't think we need to maintain a linter for reintroducing boost::bind at this point.
ACKs for top commit:
hebasto:
ACK 4105a54381, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 86bda91ee7ed11f0aa7ac95e9e7b62dbba626dcea75444d2851a3d40e794ab16bef09a1f0c956a716d43602b23c1cf67e1ff3a51184ea1ee7d686fbb76316cb0
65c49ac750 test: throw `ValueError` for invalid base58 checksum (Sebastian Falbesoner)
219d2c7ee1 contrib: testgen: use base58 methods from test framework (Sebastian Falbesoner)
605fecfb66 scripted-diff: rename `chars` to `b58chars` in test_framework.address (Sebastian Falbesoner)
11c63e374d contrib: testgen: import OP_* constants from test framework (Sebastian Falbesoner)
7d755bb31c 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 65c49ac750
Tree-SHA512: 92e1534cc320cd56262bf455de7231c6ec821bfcd0ed58aa5718271ecec1a89df7951bf31527a2306db6398e7f2664d2ff8508200c28163c0b164d3f5aaf8b0e
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
cccc4e879a Remove nHeightEnd and nHeight in generateBlocks helper (MarcoFalke)
fa38b1c8bd 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 cccc4e879a
Tree-SHA512: d8a3989ad280ebd4b1b574159b3a396b8a42134347e6be3c88445162d86624d221c416456f45ae75aea62ed8c8a1a9bb3a2532924abca2ef7a879cb8e6b15654
54b39cfb34 Add release notes (stickies-v)
f959fc0397 Update /<count>/ endpoints to use a '?count=' query parameter instead (stickies-v)
a09497614e Add GetQueryParameter helper function (stickies-v)
fff771ee86 Handle query string when parsing data format (stickies-v)
c1aad1b3b9 scripted-diff: rename RetFormat to RESTResponseFormat (stickies-v)
9f1c54787c 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 54b39cfb34
theStack:
re-ACK 54b39cfb34
Tree-SHA512: 3b393ffde34f25605ca12c0b1300799a19684b816a1d03aed38b0f5439df47bfe6a589ffbcd7b83fd2def6c9d00a1bae5e45b1d18df4ae998c617c709990f83f
4685463301 doc: Update lint test docs (Fabian Jahr)
77f98df41f lint: convert spell check lint test to python (Fabian Jahr)
Pull request description:
The new python version should produce the exact same output as the bash version but be easier to maintain.
ACKs for top commit:
MarcoFalke:
cr ACK 4685463301
Tree-SHA512: 242b802b750b42b299b93d1de4bcf17d92ad0a633d31894145d8590782a1db1041de59a283f133a4f75898d95444eb3c842005a6aa5cb919543625addad596d8
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.
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
d2ba43fec8 test: use MiniWallet for mempool_unbroadcast.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (mempool_unbroadcast.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
Top commit has no ACKs.
Tree-SHA512: e4c577899b66855dafca9dab875fa9b9c68b762a8cdb14f3a7547841c4f001e79d62641e6ae202fb56a3f28aeea1779143164c872507ff8da0bd9930a8ed182e
d6bc2322ed test: -peerblockfilters without -blockfilterindex raises an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
2a3e8fb359/src/init.cpp (L850)
Setting -peerblockfilters without -blockfilterindex should raise an error when initializing.
ACKs for top commit:
ccdle12:
Tested ACK d6bc2322ed
Tree-SHA512: e740c2ccde6bb1bb8381bb676a6d01bd5746cf9ce0c8dadd62067a6b9b380027bfe8b8cdeae9846a0ab18385f3dc5dff607fe5274cb55107d47470db00015fb2
17648493df doc: Speed up functional test runs using ramdisk (willcl-ark)
Pull request description:
Using a ramdisk for the functional tests can give noticable speedups for developers and reviewers.
Local testing with an 8GB ramdisk saw a full test run using `test/functional/test_runner.py --jobs=100 --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp` reduced from ~280 seconds to ~99 seconds.
Possible bikeshedding opportunity to be had over whether this might best fit into `doc/productivity.md`, but IMO more people will likely see it (and it will therefore be more useful) if it is here.
It seems best to select `tmpfs` over `ramfs` as `ramfs` can grow dynamically (good) but cannot be limited in size and might cause the system to hang if you run out of ram (bad), whereas `tmpfs` is size-limited and will overflow into swap.
ACKs for top commit:
josibake:
ACK 17648493df
jamesob:
ACK 17648493df
Tree-SHA512: b8e0846d4558a7a33fbb7cd190e30c36182db36095e1c1feae8c10a12042cff9d97739964bd9211d8564231dc99b4be5eed806d12a1d11dfa908157d7f26cc67
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
Using a ramdisk for the functional tests can give worthwhile speed-ups
for developers and reviewers.
Add notes to test/README.md on how to setup, use and erase a ramdisk on
Linux.
_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.
fa0758e145 test: Add diamond-shape prioritisetransaction test (MarcoFalke)
fa450c18db 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 fa0758e145
Tree-SHA512: d261184a81df77d24fc256f58ad5ed4a13b7cd4e33f74c8b79495c761ff417817602d8e5d4f63f4bb1000ac63f89bbfa54d8d8994a7b2bb2e8a484c467330984
45e67b2695 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:
2f0f056e08/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
89bb25d22a 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:
515200298b/src/init.cpp (L449)
ACKs for top commit:
jonatack:
ACK 89bb25d22a
Tree-SHA512: 3c765c7060b6972c1ae5a1104734cd7669b650b5f6aa4f623f4299567732260da5083fef306a7c1e71c931f5d1396f24abad251d95c3d82b1f3ee0efee7fcd1f
9053f64fcb [doc] release notes for random change target (glozow)
46f2fed6c5 [wallet] remove MIN_CHANGE (glozow)
a44236addd [wallet] randomly generate change targets (glozow)
1e52e6bd0a 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 9053f64fcb
Xekyo:
reACK 9053f64fcb
Tree-SHA512: 45ce5d064697065549473347648e29935733f3deffc71a6ab995449431f60302d1f9911a0994dfdb960b48c48b5d8859f168b396ff2a62db67d535a7db041d35
da2bc865d6 [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#9752Closes#10004
ACKs for top commit:
MarcoFalke:
re-ACK da2bc865d6 only change is fixing typos in tests 🎏
Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
0000ff0d6b 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 0000ff0d6b
Tree-SHA512: 8269eb05649a871011bbfbd1838d0f7d1dac4a35b3b198fc43fe85131fda8a53803b75da78cbf422eabf086006dee4421e622fbe706f6781a3848b989024001b
1066d10f71 scripted-diff: rename TxRelay members (John Newbery)
575bbd0dea [net processing] Move tx relay data to Peer (John Newbery)
785f55f7ee [net processing] Move m_wtxid_relay to Peer (John Newbery)
36346703f8 [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 1066d10f71 - This is a good layer separation improvement with no behavior changes.
glozow:
utACK 1066d10f71
Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
cccc1e70b8 Enforce Taproot script flags whenever WITNESS is set (MarcoFalke)
fa42299411 Remove nullptr check in GetBlockScriptFlags (MarcoFalke)
faadc606c7 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 0a8b7b4b33.
### 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 cccc1e70b8
achow101:
ACK cccc1e70b8
laanwj:
Code review ACK cccc1e70b8
ajtowns:
ACK cccc1e70b8 ; 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 cccc1e70b8 ([`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
58a14795b8 test: passing -onlynet=onion with -onion=0/-noonion raises expected init error (Jon Atack)
7000f66d36 test: passing -onlynet=onion without -proxy/-onion raises expected init error (Jon Atack)
8332e6e4cf test: passing invalid -onion raises expected init error (Jon Atack)
d5edb08708 test: passing invalid -proxy raises expected init error (Jon Atack)
bd57dcbaf2 test: hoist proxy out of 2 network loops in feature_proxy.py (Jon Atack)
afdf2de282 test: add CJDNS to LimitedAndReachable_Network unit tests (Jon Atack)
2b7a8180a9 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 58a14795b8
brunoerg:
ACK 58a14795b8
dongcarl:
Code Review ACK 58a14795b8
Tree-SHA512: bdee6dd0c12bb63591ce7c9321fe77b509ab1265123054e774adc38a187746dddafe1627cbe89e990bcc78b45e194bfef8dc782710d5b217e2e2106ab0158827
bc90b8d869 [move only] remove `is_wallet_compiled` checks (josibake)
0bfbf7fb24 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 bc90b8d869
Tree-SHA512: c618e23d00635d72dafdef28e68cbc88b9cc2030d4898fc5b7eac926fd621684c1958c075ed167192716b18308da5a0c1f1393396e31b99d0d3bde78b78fefc5
The external input test with specifying input weight would make a
pessimistic estimate of the input weight. However this would result in a
test failure as it is sometimes too pessimistic when an ECDSA signature
ends up being smaller than usual. To correct this, we can calculate the
input weight more accurately.
b2813980b8 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](91d12344b1/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](91d12344b1/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 b2813980b8
Tree-SHA512: 7220842daaf9a4f972d82b13b81fdeac2833bf5e665c5b0f8eaf6a4bcd0725c8e97d19ec956ca4b730065a983475bb3a2732713d338f4caf8666ccbf63d4d988
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
2b6dd4e75b test: use MiniWallet for mempool_package_onemore.py (Sebastian Falbesoner)
eb3c5c4ef2 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 2b6dd4e75b💾
Tree-SHA512: 0c97fa0519ca5eaa6df8953a04678aa8a6a66905a82db6ff40042a675d0c0682aee829a48db84e4e7983d8f766875021f0d39d65e12889342610b8861bc29cd5
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
fa7a576391 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 fa7a576391
Tree-SHA512: 2a135acf3c3c83a2704ae11f40c72882b23a676828647be1a066653c4d00e4523704f377eb8745c6386829601cc5d643abdce376831c1db91a07e999e1d5e01f
fa8593f898 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 fa8593f898
Tree-SHA512: b034077b85e6c978a80aa4de493797b4ae451d686cfb3e4fe40f37a38f41f7cb886f8e00a1c245a284be3502164b17414097fcb0bef66d155a1c1db5cfbe9e8f
fa48ea3067 Use MiniWallet in feature_coinstatsindex (MarcoFalke)
fab61437f6 test: Refactor MiniWallet get_utxo helper (MarcoFalke)
Pull request description:
Allows the test to be run even without a wallet compiled
ACKs for top commit:
josibake:
ACK fa48ea3067
ayush933:
tACK fa48ea3 . The test runs successfully with the wallet disabled.
willcl-ark:
tACK fa48ea3067 both with and without wallet compiled in.
Tree-SHA512: e04e04ea0f236c062d6be68909ece2770130ce1d5343823893073d95aebc6eedb1ad1dc5bc41e5b0cb0bf2cd9018bb1d668f0e7f5f1101ed4e0b007ed6b00f69
61152183ab wallet: Add a deprecation warning for newly created legacy wallets (Andrew Chow)
Pull request description:
As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future.
ACKs for top commit:
jonatack:
ACK 61152183ab
S3RK:
reACK 61152183ab
theStack:
ACK 61152183ab
Tree-SHA512: e89bfb8168869542498958f0c9a2ab302dfd43287f8a49e7d9e09f60438a567bb8b7219a4e569797ee819b30b624f532fcc0b70c6aa0edcb392a301b8ce8b541
Check that within 24h addr of a given node is forwarded
to the same peer(s), and then the destination is
rotated every 24h.
Co-authored-by: Jon Atack <jon@atack.com>
5d7c69b887 rpc: rename getdeploymentinfo status-next to status_next (Jon Atack)
Pull request description:
Rename the `status-next` field to `status_next` in getdeploymentinfo before the RPC is released in v23.
Before
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status-next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
After
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status_next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
Top commit has no ACKs.
Tree-SHA512: 4facfd7af3cfb7b6f5495758c4387602802f5e39d9270b162d17350a7f954eab0b74d895f17f0d8dfbc7814d36db7cff56d08c42728432885ea6f4e37aea4aa8
40e871d9b4 [miner] always assume we can create witness blocks (glozow)
Pull request description:
Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there.
ACKs for top commit:
gruve-p:
ACK 40e871d9b4
jnewbery:
utACK 40e871d9b4, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: https://github.com/bitcoin/bitcoin/pull/24421#discussion_r822933721.
achow101:
ACK 40e871d9b4
theStack:
Code-review ACK 40e871d9b4
Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019
db27ac9354 tests: Ensure sorted/multi_a descriptors always generate different addrs (Andrew Chow)
Pull request description:
Sometimes the multi_a and sortedmulti_a descriptors will produce some of the same addresses in the tests. This causes the wallets to start generating addresses at a different index as they detect that one of the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique addresses by putting one of the multi_a in a different branch.
ACKs for top commit:
ajtowns:
ACK db27ac9354
theStack:
Tested ACK db27ac9354
Tree-SHA512: 0f57822bf4c7c79da304f092d7d43d6118e78a087cbeb0766fbbf634dc27911ae723d5d41350884d3b63a24d3b3817944f7e5fa534afb849161dd008a1e4a62f
7abd8b21ba doc: include wtxid in TransactionDescriptionString (brunoerg)
2d596bce6f doc: add wtxid info in release-notes (brunoerg)
a5b66738f1 test: add wtxid in expected_fields for wallet_basic (brunoerg)
e8c659a297 wallet: add wtxid in WalletTxToJSON (brunoerg)
7482b6f895 wallet: add GetWitnessHash() (brunoerg)
Pull request description:
This PR add `wtxid` in `WalletTxToJSON` which allows to return this field in `listsinceblock`, `listtransactions` and `gettransaction` (RPCs).
ACKs for top commit:
achow101:
re-ACK 7abd8b21ba
w0xlt:
crACK 7abd8b2
luke-jr:
re-utACK 7abd8b21ba
Tree-SHA512: f86f2dbb5e38e7b19932006121802f47b759d31bdbffe3263d1db464f6a3a30fddd68416f886a44f6d3a9fd570f7bd4f8d999737ad95c189e7ae5e8ec1ffbdaa
fa097d074b addrman: Log too low compat value (MarcoFalke)
Pull request description:
Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.
In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.
ACKs for top commit:
mzumsande:
re-ACK fa097d074b
Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
Sometimes the multi_a and sortedmulti_a descriptors will produce some of
the same addresses in the tests. This causes the wallets to start
generating addresses at a different index as they detect that one of
the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique
addresses by putting one of the multi_a in a different branch.
4828d53ecc Add (sorted)multi_a descriptors to doc/descriptors.md (Pieter Wuille)
b5f33ac1f8 Simplify wallet_taproot.py functional test (Pieter Wuille)
eb0667ea96 Add tests for (sorted)multi_a derivation/signing (Pieter Wuille)
c17c6aa08d Add signing support for (sorted)multi_a scripts (Pieter Wuille)
3eed6fca57 Add multi_a descriptor inference (Pieter Wuille)
79728c4a3d Add (sorted)multi_a descriptor and script derivation (Pieter Wuille)
25e95f9ff8 Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount (Pieter Wuille)
Pull request description:
This adds a new `multi_a(k,key_1,key_2,...,key_n)` (and corresponding `sortedmulti_a`) descriptor for k-of-n policies inside `tr()`. Semantically it is very similar to the existing `multi()` descriptor, but with the following changes:
* The corresponding script is `<key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL`, rather than the traditional `OP_CHECKMULTISIG`-based script, making it usable inside the `tr()` descriptor.
* The keys can optionally be specified in x-only notation.
* Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit
I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that.
Limitations:
* The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts.
* The multi_a script construction is (slightly) suboptimal for n-of-n (where a `<key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG` would be better). Such a construction is not included here.
ACKs for top commit:
achow101:
ACK 4828d53ecc
gruve-p:
ACK 4828d53ecc
sanket1729:
code review ACK 4828d53ecc
darosior:
Code review ACK 4828d53ecc
Tree-SHA512: 5dcd434b79585f0ff830f7d501d27df5e346f5749f47a3109ec309ebf2cbbad0e1da541eec654026d911ab67fd7cf7793fab0f765628d68d81b96ef2a4d234ce
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.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
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
0eea83a85e scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505db net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85e
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85e code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
c7376cc8d7 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43 wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d7
benthecarman:
tACK c7376cc8d7 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
