ad06e68399 test: write functional test results to csv (tdb3)
Pull request description:
Adds argument `--resultsfile` to test_runner.py.
Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving).
Test name, status, and duration are written to the file provided with the argument.
Since `test_runner.py` is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired.
#### Notes
- Total runtime of functional tests has seemed to have increased on my development machines over the past few months (more tests added, individual test runtime increase, etc.). Was interested in recording test runtime data over time to detect trends. Initially searched `doc/benchmarking.md`, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability to `test_runner` to facilitate this type of data analysis (and potentially other use cases)
- Saw https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#benchmarking-with-perf, and this PR's higher level data seems complimentary.
- Was on the fence as to whether to expand `print_results()` (i.e. take advantage of the same loop over `test_results`) or implement in a separate `write_results()` function. Decided on the latter for now, but interested in reviewers' thoughts.
#### Example 1: all tests pass
```
$ test/functional/test_runner.py --resultsfile functional_test_results.csv --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp feature_blocksdir wallet_startup feature_config_args mempool_accept
Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240614_201625
Test results will be written to functional_test_results.csv
...
$ cat functional_test_results.csv
test,status,duration(seconds)
feature_blocksdir.py,Passed,1
feature_config_args.py,Passed,29
mempool_accept.py,Passed,9
wallet_startup.py,Passed,2
ALL,Passed,29
```
#### Example 2: one test failure
```
$ cat functional_test_results.csv
test,status,duration(seconds)
feature_blocksdir.py,Passed,1
feature_config_args.py,Passed,28
wallet_startup.py,Passed,2
mempool_accept.py,Failed,1
ALL,Failed,28
```
ACKs for top commit:
maflcko:
re-ACK ad06e68399
kevkevinpal:
tACK [ad06e68](ad06e68399)
achow101:
ACK ad06e68399
rkrux:
tACK [ad06e68](ad06e68399)
brunoerg:
ACK ad06e68399
marcofleon:
Good idea, tested ACK ad06e68399
Tree-SHA512: 561194406cc744905518aa5ac6850c07c4aaecdaf5d4d8b250671b6e90093d4fc458f050e8a85374e66359cc0e0eaceba5eb24092c55f0d8f349d744a32ef76c
Move the output serialization size and dust calculation into the loop where the
outputs are iterated over to calculate the total sum.
Move the code for adding the the txoutputs to the transaction to after
coin selection.
While this code structure generally follows a more logical flow,
the primary motivation for moving the code for adding outputs to the
transaction sets us up nicely for silent payments (in a future PR):
we need to know the input set before generating the final output scriptPubKeys.
Now that a CRecipient holds a CTxDestination, we can get the serialized
size and determine if the output is dust using the CRecipient directly.
This does not change any current behavior, but provides a nice generalization
that can be used to apply special logic to a CTxDestination serialization
and dust calculations in the future.
Specifically, in a later PR when support for `V0SilentPayment` destinations is
added, we need to use `WitnessV1Taproot` as the scriptPubKey for serialized
size calcuations whenever the `CRecipient` destination is a `V0SilentPayment`
destination.
Prior to this commit, TestEncryptedP2PState would always
send initial_v2_handshake bytes in 2 parts (as required
by early key response test).
For generalising this test and having different v2 handshake
behaviour based on the test type, special behaviours like
sending initial_v2_handshake bytes in 2 parts are executed
only if test_type is set to EARLY_KEY_RESPONSE.
Adds argument --resultsfile to test_runner.py.
Writes comma-separated functional test name, status,
and duration to the file provided with the argument.
Also fixes minor typo in test_runner.py
f58beabe75 test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq)
436e88f433 bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate (ismaelsadeeq)
Pull request description:
Fixes#26973
When using the `bumpfee` RPC and manually specifying `fee_rate`, there should be no requirement that the new fee must be at least the sum of the original fee and `incrementalFee` (maximum of `relayIncrementalFee` and `WALLET_INCREMENTAL_RELAY_FEE`).
This restriction should only apply when user did not specify `fee_rate`.
> because the GUI doesn't let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte.
The restriction should instead be the new fee must be at least the sum of the original fee and `incrementalFee` (`relayIncrementalFee`)
ACKs for top commit:
achow101:
ACK f58beabe75
murchandamus:
ACK f58beabe75
Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
fae3a1f006 log: use error level for critical log messages (MarcoFalke)
Pull request description:
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
ACKs for top commit:
stickies-v:
re-ACK fae3a1f006, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.
kevkevinpal:
ACK [fae3a1f](fae3a1f006)
achow101:
ACK fae3a1f006
Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
Allow the callers of `CreateSock()` to pass all 3 arguments to the
`socket(2)` syscall. This makes it possible to create sockets of
any domain/type/protocol.
a37778d4d3 Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake)
Pull request description:
Includes https://github.com/bitcoin-core/leveldb-subtree/pull/41 which is used in #30234.
ACKs for top commit:
theuni:
utACK 95812d912b
Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
Whilst these remain aliases for each other, the later is preferred,
and I assume the former will be removed at some point in the future;
see: https://github.com/llvm/llvm-project/pull/95374.
e2779ce98b test: cover more errors for `signrawtransactionwithkey` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `signrawtransactionwithkey` RPC:
- Invalid private key
- TX decode failed
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
ACK e2779ce98b
kevkevinpal:
ACK [e2779ce](e2779ce98b)
tdb3:
ACK e2779ce98b
BrandonOdiwuor:
Code Review ACK e2779ce98b
Tree-SHA512: 41c7e990684b60645cf4ccec8aad5ebbe61da221871eb3c1685b2bb1eebda58b29358502cb1525b7c7a2b612e2bebf449ed0bae14ab663b4641c528a9c013b5b
07f64177a4 Reduce memory copying operations in bech32 encode (Lőrinc)
d5ece3c4b5 Reserve hrp memory in Decode and LocateErrors (Lőrinc)
Pull request description:
Started optimizing the base conversions in [TryParseHex](https://github.com/bitcoin/bitcoin/pull/29458), [Base58](https://github.com/bitcoin/bitcoin/pull/29473) and [IsSpace](https://github.com/bitcoin/bitcoin/pull/29602) - this is the next step.
Part of this change was already merged in https://github.com/bitcoin/bitcoin/pull/30047, which made decoding `~26%` faster.
Here I've reduced the memory reallocations and copying operations in bech32 encode, making it `~15%` faster.
> make && ./src/bench/bench_bitcoin --filter='Bech32Encode' --min-time=1000
Before:
```
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 19.97 | 50,074,562.72 | 0.1% | 1.06 | `Bech32Encode`
```
After:
```
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17.33 | 57,687,668.20 | 0.1% | 1.10 | `Bech32Encode`
```
ACKs for top commit:
josibake:
ACK 07f64177a4
sipa:
utACK 07f64177a4
achow101:
ACK 07f64177a4
Tree-SHA512: 511885217d044ad7ef2bdf9203b8e0b94eec8b279bc193bb7e63e29ab868df6d21e9e4c7a24390358e1f9c131447ee42039df72edcf1e2b11e1856eb2b3e10dd
Support package RBF where the conflicting package would result
in a mempool cluster of size two, and each of its direct
conflicts are also part of an up-to-size-2 mempool cluster.
This restricted topology allows for exact calculation
of miner scores for each side of the equation, reducing
the surface area for new pins, or incentive-incompatible
replacements.
This allows wallets to create simple CPFP packages
that can fee bump other simple CPFP packages. This,
leveraged with other restrictions such as V3 transactions,
can create pin-resistant applications.
Future package RBF relaxations can be considered when appropriate.
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
Fix this by always updating the status bar when the warnings are
changed.
Instead of having separate warning functions (and globals) for each
different warning that can be raised, encapsulate this logic into
a single class and allow to (un)set any number of warnings.
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Removes warnings.cpp from kernel.
0fcbfdb7ad Support running individual lint checks (David Gumberg)
Pull request description:
This PR was split out from #29965:
Adds support for running individual tests in the rust lint suite by passing `--lint=LINT_TO_RUN` to the lint runner. This PR also adds a corresponding help message.
When running with `cargo run`, arguments after a double dash (`--`) are passed to the binary instead of the cargo command. For example, in order to run the linter check that tabs are not used as whitespace:
```console
cd test/lint/test_runner && cargo run -- --lint=tabs_whitespace
```
ACKs for top commit:
maflcko:
ACK 0fcbfdb7ad
achow101:
ACK 0fcbfdb7ad
marcofleon:
Tested ACK 0fcbfdb7ad. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good.
Tree-SHA512: 48fe4aa9fbb2acef5f8e3c17382ae22e0e350ae6ad9aeeb1a3c0a9192de98809f98728e32b8db24a36906ace999e35626ebd6cb2ca05f74146d21e9b6fb14615
c7376babd1 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334 util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b4 util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9a common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae3 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608d util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad4792 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff4 util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420c build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24d test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd1
TheCharlatan:
Re-ACK c7376babd1
hebasto:
re-ACK c7376babd1.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
Before, interruption was printed as an error. Also,
it did not log the reason when an interruption happened,
e.g. "Error accepting:".
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
faa41e29d5 fuzz: Use std::span in FuzzBufferType (MarcoFalke)
Pull request description:
The use of `Span` is problematic, because it lacks methods such as `rbegin`, leading to compile failures when used:
```
error: no member named 'rbegin' in 'Span<const unsigned char>'
```
One could fix `Span`, but it seems better to use `std::span`, given that `Span` will be removed anyway in the long term.
ACKs for top commit:
dergoegge:
utACK faa41e29d5
Tree-SHA512: 54bcaf51c83a1b48739cd7f1e8445c6eba0eb04231bce5c35591a47dddb3890ffcaf562cf932930443c80ab0e66950c4619560e6692240de0c52aeef3214facd
193c748e44 fuzz: add I2P harness (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/28803. This updated harness sets mock time at the beginning of each iteration and deletes the private key file at the end of each iteration. Mock time is used to make the fuzz test more stable, as `GetTime` is called at points in `i2p`. Deleting the private key file ensures that each iteration is independent from the last. Now, a new key is generated in `i2p` every time, so the fuzzer can eventually make progress through the target code.
Re-working this harness also led me and dergoegge to resolve a couple of issues in `FuzzedSock`, which allows for full coverage of the `i2p` code. Those changes can be seen in https://github.com/bitcoin/bitcoin/pull/30211.
The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.
<details>
<summary>I2P dict</summary>
```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SESSION STATUS RESULT=INVALID_ID"
"SESSION STATUS RESULT=INVALID_KEY"
"SESSION STATUS RESULT=I2P_ERROR MESSAGE="
"SESSION ADD"
"SESSION REMOVE"
"STREAM CONNECT"
"STREAM STATUS RESULT=OK"
"STREAM STATUS RESULT=INVALID_ID"
"STREAM STATUS RESULT=INVALID_KEY"
"STREAM STATUS RESULT=CANT_REACH_PEER"
"STREAM STATUS RESULT=I2P_ERROR MESSAGE="
"STREAM ACCEPT"
"STREAM FORWARD"
"DATAGRAM SEND"
"RAW SEND"
"DEST GENERATE"
"DEST REPLY PUB= PRIV="
"DEST REPLY RESULT=I2P_ERROR"
"NAMING LOOKUP"
"NAMING REPLY RESULT=OK NAME= VALUE="
"DATAGRAM RECEIVED DESTINATION= SIZE="
"RAW RECEIVED SIZE="
"NAMING REPLY RESULT=INVALID_KEY NAME="
"NAMING REPLY RESULT=KEY_NOT_FOUND NAME="
"MIN"
"MAX"
"STYLE"
"ID"
"SILENT"
"DESTINATION"
"NAME"
"SIGNATURE_TYPE"
"CRYPTO_TYPE"
"SIZE"
"HOST"
"PORT"
"FROM_PORT"
"TRANSIENT"
"STREAM"
"DATAGRAM"
"RAW"
"MASTER"
"true"
"false"
```
</details>
I'll add this dict to qa-assets later on.
ACKs for top commit:
dergoegge:
tACK 193c748e44
brunoerg:
ACK 193c748e44
vasild:
ACK 193c748e44
Tree-SHA512: 09ae4b3fa0738aa6f159f4d920493bdbce786b489bc8148e7a135a881e9dba93d727b40f5400c9510e218dd2cfdccc7ce2d3ac9450654fb29c78aac59af92ec3
eb37a9b8e7 Merge sipa/minisketch#87: Avoid copy in self-assign
fe6557642e Merge sipa/minisketch#88: build: Add `-Wundef`
8ea298bfa7 Avoid copy in self-assign
978a3d8869 build: Add `-Wundef`
3387044179 Merge sipa/minisketch#86: doc: fix typo in sketch_impl.h
15c2d13b60 doc: fix typo in sketch_impl.h
7be08b8a46 Merge sipa/minisketch#85: Fixes for integer precision loss
00fb4a4d83 Avoid or make integer precision conversion explicit
9d62a4d27c Avoid the need to cast/convert to size_t for vector operations
19e06cc7af Prevent overflows from large capacity/max_elements
git-subtree-dir: src/minisketch
git-subtree-split: eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c
f51da34ec1 utils: add missing include (Cory Fields)
Pull request description:
Noticed when testing `VecDeque` with no other includes.
For libc++, need type_traits for `std::is_trivially_destructible_v`.
ACKs for top commit:
maflcko:
ACK f51da34ec1
glozow:
ACK f51da34ec1
sipa:
utACK f51da34ec1
Tree-SHA512: bf96910abe9aaddd8586e6cc8f68a9bbac4c26d976ebeebcfa86b86c0da5783c1cbdbc7fa09b62cdcfde19e6442eb65a66bf1e2e80408d68e9dd9689dc22b0fa
Set minimum required glibc to 2.31.
The glibc 2.31 branch is still maintained:
https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/release/2.31/master.
Remove the stack-protector check from test-security-check, as the test
no-longer fails, and given the control we have of the end, the actual
security-check test seems sufficient (this might also be applied to some
of the other checks).
Drops runtime support for Ubuntu Bionic 18.04 and RHEL-8 from the release binaries.
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
