04609284ad rpc: Improve error when wallet is already loaded (Aurèle Oulès)
Pull request description:
Currently, trying to load a descriptor (sqlite) wallet that is already loaded throws the following error:
> error code: -4
> error message:
> Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
I don't think it is very clear what it means for a user.
While a legacy wallet would throw:
> error code: -35
> error message:
> Wallet file verification failed. Refusing to load database. Data file '/home/user/.bitcoin/signet/wallets/test_wallet/wallet.dat' is already loaded.
This PR changes the error message for both types of wallet to:
> error code: -35
> error message:
> Wallet file verification failed. Wallet "test_wallet" is already loaded.
ACKs for top commit:
achow101:
ACK 04609284ad
hernanmarino:
ACK 0460928
theStack:
Tested ACK 04609284ad
Tree-SHA512: a8f3d5133bfaef7417a6c05d160910ea08f32ac62bfdf7f5ec305ff5b62e9113b55f385abab4d5a4ad711aabcb1eb7ef746eb41f841b196e8fb5393ab3ccc01e
9622fe64b8 test: move coins result test to wallet_tests.cpp (furszy)
f69347d058 test: extend and simplify availablecoins_tests (furszy)
212ccdf2c2 wallet: AvailableCoins, add arg to include/skip locked coins (furszy)
Pull request description:
Negative PR with extended test coverage :).
1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`.
So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`.
3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount.
Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.
ACKs for top commit:
achow101:
ACK 9622fe64b8
theStack:
ACK 9622fe64b8
aureleoules:
reACK 9622fe64b8, nice cleanup!
Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
81d4a2b14f refactor: Move feerate comparison invariant outside of the loop (yancy)
365aca4045 refactor: Simplify feerate comparison statement (yancy)
Pull request description:
This is a small nit, however I think it's more understandable to write:
`utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee`
vs
`(utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0`
ACKs for top commit:
Xekyo:
ACK 81d4a2b14f
achow101:
ACK 81d4a2b14f
aureleoules:
ACK 81d4a2b14f
Tree-SHA512: 3e89377989c36716b53114fe40178261671dde5688075fab1c21ec173ac310f8c84ed6af90354d7c329176cb7262dfcaa7191fd19847d3b7147a9a10c3e31176
f496528556 walletdb: refactor: drop unused `FindWalletTx` parameter and rename (Sebastian Falbesoner)
Pull request description:
Since commit 3340dbadd3 ("Remove -zapwallettxes"), the `FindWalletTx` helper is only needed to read tx hashes, so drop the other parameter and rename the method accordingly.
ACKs for top commit:
S3RK:
code review ACK f496528556
achow101:
ACK f496528556
vincenzopalazzo:
ACK f496528556
Tree-SHA512: ead85bc724462f9e920f9d7fe89679931361187579ffd6e63427c8bf5305cd5f71da24ed84f3b1bd22a12be46b5abec13f11822e71a3e1a63bf6cf49de950ab5
fabb6af850 ci: Remove duplicate CC and CXX from tsan task (MarcoFalke)
fa5d9a0e24 Revert "ci: Use clang-15 in tsan task" (MarcoFalke)
faa835e7e5 Revert "test: Drop no longer needed `race:epoll_ctl` TSan suppression" (MarcoFalke)
Pull request description:
Looks like there are still bugs in clang-15, so we need to roll back all the way to the previously used version (clang-13).
ACKs for top commit:
hebasto:
ACK fabb6af850, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d62203049847ab9095ee3fc89e18bdd721d1d9d5a7ef7a9f524c80e6be58d1d9f6aa2f14533df1ea77eb59597fba6fa9b987b17eb03b2c3f7cb577ab59cd59c0
090ad51c80 rpc: Remove duplicate field in RPCHelpMan for gettransactions (Joshua Kelly)
Pull request description:
The field 'comment' appears twice in `TransactionDescriptionString`, incorrectly - this commit removes the instance of the comment field without a description, preserving the one with a description.
On master, the duplicate fields can be be viewed here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L419-L423
`TransactionDescriptionString` is included in RPC calls such as `listtransactions` which have functional tests.
ACKs for top commit:
w0xlt:
ACK 090ad51c80
Tree-SHA512: 4bacdafdb517dda2af6d1c193f331b634ae74bd62ac6289c0c288957f39f98a73d07aeab72fbe5bf1ece5532406d4a40a5b8a2277be50115f76c92bb938e21fa
The field 'comment' appears twice in TransactionDescriptionString,
incorrectly - this commit removes the instance of the comment field
without a description, preserving the one with a description
faa00ca78e ci: Use clang-15 in tsan task (MarcoFalke)
Pull request description:
Generally it is best to use the latest clang version for sanitizers, because it comes with the most features and bugfixes.
So bump to clang-15, the latest release, for the tsan task.
The task was using clang-13 (instead of 14) due to a bug, see https://github.com/bitcoin/bitcoin/pull/24572#issue-1169970859. Bumping to 15 will hopefully fix this bug, as well as https://github.com/bitcoin/bitcoin/pull/26759#issuecomment-1367360491
ACKs for top commit:
hebasto:
ACK faa00ca78e
Tree-SHA512: adb2386bb9615a3e1185e0624b0b68cd2738309530185819714a26e63bdf1c79461c4b4d3aa9cbe2fe08cc412349d7453f192abbbe9fb5adca74cf4b148ae7b7
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
