f1e89597c8 test: Drop no longer required bench output redirection (Hennadii Stepanov)
4dbcdf26a3 bench: Suppress output when running with `-sanity-check` option (Hennadii Stepanov)
Pull request description:
This change allows to simplify CI tests, and makes it easier to integrate the `bench_bitcoin` binary into CMake custom [targets](https://cmake.org/cmake/help/latest/command/add_custom_target.html) or [commands](https://cmake.org/cmake/help/latest/command/add_custom_command.html), as `COMMAND` does not support output redirection.
ACKs for top commit:
aureleoules:
tACK f1e89597c8. Ran as expected and is more practical than using an output redirection.
Tree-SHA512: 29086d428cccedcfd031c0b4514213cbc1670e35f955e8fd35cee212bc6f9616cf9f20d0cb984495390c4ae2c50788ace616aea907d44e0d6a905b9dda1685d8
a3f5e54152 test: Drop no longer needed `race:epoll_ctl` TSan suppression (Hennadii Stepanov)
Pull request description:
The removed suppression seems no needed.
I cannot point the exact commit/PR which makes this change possible.
Top commit has no ACKs.
Tree-SHA512: 8ee79cbdb2bc62796d72c69be4a818379132eae47be33951e8b9d224b049ff77e867004801c7cb0cc564a5374f318dafd9142b5231e9bd428f80acc75253933e
3ae76ea6dd scripted-diff: Insert missed copyright header (Hennadii Stepanov)
306ccd4927 scripted-diff: Bump copyright headers (Hennadii Stepanov)
Pull request description:
This PR bumps the existing copyright headers, as we did every year, and adds a missed one.
Top commit has no ACKs.
Tree-SHA512: 5f6b02e2baad21750e3dd8f0612bb6e7e2cfa6a743c669f26baf5a39c168b2d3a92afae1ce2dad59b70492175186c38f172c4ee68fc7ac87a4d85330429ca054
b2aa9e8528 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders)
8c5b3646b5 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders)
Pull request description:
Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2)
was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
Related mailing list discussions here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html
And a couple years earlier:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html
ACKs for top commit:
achow101:
reACK b2aa9e8528
glozow:
reACK b2aa9e8528
pablomartin4btc:
re-ACK b2aa9e8528
jonatack:
ACK b2aa9e8528 with some suggestions
Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
dc12f2e212 test: improve error msg on previous release tarball extraction failure (kdmukai)
7121fd8fa7 test: self-sign previous release binaries for arm64 macOS (kdmukai)
Pull request description:
## The Problem
If you run `test/get_previous_releases.py -b` on an M1 or M2 mac, you'll get an unsigned v23.0 binary in the arm64 tarball. macOS [sets stricter requirements on ARM binaries](https://news.ycombinator.com/item?id=26996578) so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?).
This means that any test that depends on a previous release (e.g. `wallet_backwards_compatibility.py`) will fail because the v23.0 node cannot launch:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
node.wait_for_rpc_connection()
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization
```
This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac.
## Solution in this PR
(UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release's "bin/" and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success.
(note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded)
## Longer term solution
If possible, produce signed arm64 binaries in a future v23.x tarball?
Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal?
That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of `get_previous_releases.py` that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected.
## Further info:
Somewhat related to: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1265164753
And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via:
```
$ codesign -v -d bitcoin-qt
bitcoin-qt: code object is not signed at all
```
ACKs for top commit:
hebasto:
ACK dc12f2e212
Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
d3a84347e8 ci: remove --prefix from msan job (fanquake)
574e50addf ci: Use `CONFIG_SITE` variable and `--prefix` option properly (Hennadii Stepanov)
Pull request description:
When running CI scripts locally, they attempt to use a `$DEPENDS_DIR/$HOST` directory even `NO_DEPENDS=1` is provided.
This PR fixes this broken behavior.
Top commit has no ACKs.
Tree-SHA512: 5e83b921763e6d463e520bbee2ed1599e9f4de36668d19b23dd9d2d7e4441c415e275f588c585b72cadda8bfab5a938979acc1ee4963230aa47081785c741e98
bb5ea1d9a9 qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow)
Pull request description:
`istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here.
This is a regression in 24.0. https://github.com/bitcoin/bitcoin/pull/25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`.
ACKs for top commit:
furszy:
Tested ACK bb5ea1d9
MarcoFalke:
review ACK bb5ea1d9a9đ
Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
31fdc54dba test: speed up wallet_fundrawtransaction.py and wallet_sendall.py (kdmukai)
Pull request description:
## Problem
`wallet_fundrawtransaction.py` and `wallet_sendall.py` are the two slowest functional tests *when running without a RAM disk*.
```
# M1 MacBook Pro timings
wallet_fundrawtransaction.py --descriptors | â Passed | 55 s
wallet_fundrawtransaction.py --legacy-wallet | â Passed | 381 s
wallet_sendall.py --descriptors | â Passed | 43 s
wallet_sendall.py --legacy-wallet | â Passed | 327 s
```
In each case, the majority of the time is spent iterating through 1500 to 1600 `getnewaddress()` calls. This is particularly slow in the `--legacy-wallet` runs.
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py#L986-L987
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L324
## Solution
Pre-fill the keypool before iterating through those `getnewaddress()` calls.
With this change, the execution time drops to:
```
wallet_fundrawtransaction.py --descriptors | â Passed | 52 s # -3s diff
wallet_fundrawtransaction.py --legacy-wallet | â Passed | 291 s # -90s diff
wallet_sendall.py --descriptors | â Passed | 27 s # -16s diff
wallet_sendall.py --legacy-wallet | â Passed | 228 s # -99s diff
```
---
Tagging @ Sjors as he had encouraged me to take a look at speeding up the tests.
ACKs for top commit:
achow101:
ACK 31fdc54dba
Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
a4defcdd57 test, lint: add `crypted` to `ignore-words` (brunoerg)
Pull request description:
Fixes#26719
"Crypted" is used in some comments at `walletload_tests` because it refers to `DBKeys::CRYPTED_KEY`, it's not necessary
a mistake.
Obs: I can change the approach (changing `walletload_tests` comments to use `encrypted` word instead of adding it to the `ignore_words`) if reviewers think it makes more sense.
ACKs for top commit:
achow101:
ACK a4defcdd57
Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2) was the route taken.
It would not allow an "empty" OP_RETURN
but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
ec63a4892e test: call `keypoolrefill` with private keys disabled should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
cb32328d1b/src/wallet/rpc/addresses.cpp (L332-L334)
ACKs for top commit:
aureleoules:
ACK ec63a4892e
Tree-SHA512: b5deda8981ff472f290e6e16c8723a58e02cbe099afd1f672c099f4add0a1d9b192b11a2c3f0e11b96794671f6b9efa75812b7a174248d7c58d7fd7d3310e7b9
6fefd49527 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande)
Pull request description:
The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer.
Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in https://github.com/bitcoin/bitcoin/pull/26457#pullrequestreview-1181641835).
But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data.
An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see 5f900e27d0). Since this conflicts with #25923 and should be a separate discussion, I removed that commit from this PR.
ACKs for top commit:
dergoegge:
Approach ACK 6fefd49527
MarcoFalke:
review ACK 6fefd49527đ
Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532
36c201feb7 remove CBlockIndex copy construction (James O'Beirne)
Pull request description:
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer members (e.g. pprev).
(See also https://github.com/bitcoin/bitcoin/pull/24008#discussion_r891949166)
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
ACKs for top commit:
ajtowns:
ACK 36c201feb7 - code review only
MarcoFalke:
re-ACK 36c201feb7 đť
Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b
1647a11f39 tests: Reorder longer running tests in test_runner (Andrew Chow)
ff6c9fe027 tests: Whitelist test p2p connection in rpc_packages (Andrew Chow)
8c20796aac tests: Use waitfornewblock for work queue test in interface_rpc (Andrew Chow)
6c872d5e65 tests: Initialize sigops draining script with bytes in feature_taproot (Andrew Chow)
544cbf776c tests: Use batched RPC in feature_fee_estimation (Andrew Chow)
4ad7272f8b tests: reduce number of generated blocks for wallet_import_rescan (Andrew Chow)
Pull request description:
When configured with `--enable-debug`, many tests become dramatically slower. These slow downs are particularly noticed in tests that generate a lot of blocks in separate calls, make a lot of RPC calls, or send a lot of data from the test framework's P2P connection. This PR aims to improve the runtime of some of the slower tests and improve the overall runtime of the test runner. This has improved the runtime of the test runner from ~400s to ~140s on my computer.
The slowest test by far was `wallet_import_rescan.py`. This was taking ~320s. Most of that time was spent waiting for blocks to be mined and then synced to the other nodes. It was generating a new block for every new transaction it was creating in a setup loop. However it is not necessary to have one tx per block. By mining a block only every 10 txs, the runtime is improved to ~61s.
The second slowest test was `feature_fee_estimation.py`. This test spends most of its time waiting for RPCs to respond. I was able to improve its runtime by batching RPC requests. This has improved the runtime from ~201s to ~140s.
In `feature_taproot.py`, the test was constructing a Python `CScript` using a very large list of `OP_CHECKSIG`s. The constructor for the Python implementation of `CScript` was iterating this list in order to create a `bytes` from it even though a `bytes` could be created from it without iterating. By making the `bytes` before passing it into the constructor, we are able to improve this test's runtime from ~131s to ~106s.
Although `interface_rpc.py` was not typically a slow test, I found that it would occasionally have a super long runtime. It typically takes ~7s, but I have observed it taking >400s to run on occasion. This longer runtime occurs more often when `--enable-debug`. This long runtime was caused by the "exceeding work queue" test which is really just trying to trigger a race condition. In this test, it would create a few threads and try an RPC in a loop in the hopes that eventually one of the RPCs would be added to the work queue while another was processing. It used `getrpcinfo` for this, but this function is fairly fast. I believe what was happening was that with `--enable-debug`, all of the code for receiving the RPC would often take longer to run than the RPC itself, so the majority of the requests would succeed, until we got lucky after 10's of thousands of requests. By changing this to use a slow RPC, the race condition can be triggered more reliably, and much sooner as well. I've used `waitfornewblock` with a 500ms timeout. This improves the runtime to ~3s consistently.
The last test I've changed was `rpc_packages.py`. This test was one of the higher runtime variability tests. The main source of this variation appears to be waiting for the test node to relay a transaction to the test framework's P2P connection. By whitelisting that peer, the variability is reduced to nearly 0.
Lastly, I've reordered the tests in `test_runner.py` to account for the slower runtimes when configured with `--enable-debug`. Some of the slow tests I've looked at were listed as being fast which was causing overall `test_runner.py` runtime to be extended. This change makes the test runner's runtime be bounded by the slowest test (currently `feature_fee_estimation.py` with my usual config (`-j 60`).
ACKs for top commit:
willcl-ark:
ACK 1647a11
Tree-SHA512: 529e0da4bc51f12c78a40d6d70b3a492b97723c96a3526148c46943d923c118737b32d2aec23d246392e50ab48013891ef19fe6205bf538b61b70d4f16a203eb
fafcc94398 Make bitcoin-util grind_task tsan friendly (MacroFake)
Pull request description:
While there is no issue with the current code, `libtsan-12.2.1` on my machine does not seem to like it. This is understandable, because the nonce isn't protected by a mutex that the sanitizer can see (only by an atomic, which achieves the same).
Fix this by guarding the nonce by the existing atomic bool, which tsan seems to understand.
ACKs for top commit:
ajtowns:
ACK fafcc94398
hebasto:
ACK fafcc94398, I have reviewed the code and it looks OK, I agree it can be merged. Confirming that initial bug has been fixed.
Tree-SHA512: 4e67fab5833ec7d91678b85a300368892ee9f7cd89a52cc5e15a7df65b2da813b24eaffd8362d0d8a3c8951e024041d69ebddf25101b11d0a1a62c1208ddc9a5
564b580bf0 test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès)
71d9a7c03b test: Wallet imports on pruned nodes (Aurèle Oulès)
e6906fcf9e rpc: Enable wallet import on pruned nodes (Aurèle Oulès)
Pull request description:
Reopens#16037
I have rebased the PR, addressed the comments of the original PR and added a functional test.
> Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps.
For reviewers:
`python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is.
ACKs for top commit:
kouloumos:
ACK 564b580bf0
achow101:
reACK 564b580bf0
furszy:
ACK 564b580
w0xlt:
ACK 564b580bf0
Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf
a2724808ab doc: add 23.1 release notes (fanquake)
Pull request description:
Same as past releases / https://github.com/bitcoin/bitcoin/pull/26524 etc.
ACKs for top commit:
stickies-v:
ACK a2724808ab
Tree-SHA512: e9f7ad72c23c8621e8a98ffa0dc0d08ebe30ad0bc8d23e25fabda5b1a9318ff74c65344821267c6af5a8d94c26c775ce83a67cbe0c4922eac07a4319fd94eb49
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer references (e.g. pprev).
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
Delete move constructors and declare the destructor to satisfy the
"rule of 5."
062e4e9fe9 doc: add 22.1 release notes (fanquake)
Pull request description:
Same as past releases / #26524 etc.
Top commit has no ACKs.
Tree-SHA512: e41b1eaff1aacd89260f070650044629de5673020e0e70bdceb0749981403aad380e5595c494fa5ebaaa7c87e0ebea0def5f5bbd433a4b3b810e40c2de6dc448
fa34e5f3d3 test: Avoid intermittent timeout in feature_assumevalid.py (MarcoFalke)
Pull request description:
Currently the test will spin up p2p connections in the beginning, then announce the headers to all nodes, but only send the blocks sequentially. This takes a long time, so when getting to the last node, it will have already timed out, while node1 is busy eating blocks. Example:
```
node2 2022-12-06T19:31:35.419291Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e (1) peer=0
node2 2022-12-06T19:31:35.424784Z [msghand] [net.cpp:2776] [PushMessage] [net] sending getdata (577 bytes) peer=0
[...]
node2 2022-12-06T19:41:35.423257Z [msghand] [net_processing.cpp:5729] [SendMessages] Timeout downloading block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e from peer=0, disconnecting
node1 2022-12-06T19:41:35.438706Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 6575919043306ed309014d0bd79814b4fab8afaa281e026d8cc3a1c4c2336fbc (1748) peer=0
node2 2022-12-06T19:41:35.521253Z [net] [net.cpp:573] [CloseSocketDisconnect] [net] disconnecting peer=0
node2 2022-12-06T19:41:35.630417Z [net] [net_processing.cpp:1532] [FinalizeNode] [net] Cleared nodestate for peer=0
```
Fix this by only spinning up the p2p connection right before they are needed.
ACKs for top commit:
jamesob:
ACK fa34e5f3d3 ([`jamesob/ackr/26651.1.MarcoFalke.test_avoid_intermittent`](https://github.com/jamesob/bitcoin/tree/ackr/26651.1.MarcoFalke.test_avoid_intermittent))
Tree-SHA512: 7a1b114c07dfa30237c8cd8637dd6646c5c2dc2530c9de61db231738fddc800b620c31dc664237e33d35e951cf161f015fda593162efc9d85c5f68c6e37217d4
