fabb6af8508a9c0d05d2e9f0006b0b1d3f38119a ci: Remove duplicate CC and CXX from tsan task (MarcoFalke)
fa5d9a0e246e94eb0d0e9a75f63ff1b09ec93c03 Revert "ci: Use clang-15 in tsan task" (MarcoFalke)
faa835e7e564536fb0589ec94d3f85042a1f9cd8 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 fabb6af8508a9c0d05d2e9f0006b0b1d3f38119a, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d62203049847ab9095ee3fc89e18bdd721d1d9d5a7ef7a9f524c80e6be58d1d9f6aa2f14533df1ea77eb59597fba6fa9b987b17eb03b2c3f7cb577ab59cd59c0
090ad51c80fe02a73f8988c1d5b867d2e90ab2f3 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
faa00ca78e27a40cbafe701348e6b3674e3ddeed 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 faa00ca78e27a40cbafe701348e6b3674e3ddeed
Tree-SHA512: adb2386bb9615a3e1185e0624b0b68cd2738309530185819714a26e63bdf1c79461c4b4d3aa9cbe2fe08cc412349d7453f192abbbe9fb5adca74cf4b148ae7b7
This change reverts cc7335edc87c6ef34429b4df94f53973db520aac partially.
C++20 has introduced some new headers, and it is premature to consider
them when using the IWYU tool.
3666a06730b2229e5af24124e6ef0403e4a6792e test: add coverage for unknown wallet flag in `setwalletflag` (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
6d40a1a7e7/src/wallet/rpc/wallet.cpp (L275-L277)https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/rpc/wallet.cpp.gcov.html
ACKs for top commit:
aureleoules:
ACK 3666a06730b2229e5af24124e6ef0403e4a6792e
Tree-SHA512: b6b2415442dfbc2404aed9720cc899995686007d6ba222dae461d064e4454d5af1326d3d527770b51b1005721ac42a49972f1eabf21108f656c30d3584790747
f1e89597c803001ab9d5afd7e173184fe6886d1d test: Drop no longer required bench output redirection (Hennadii Stepanov)
4dbcdf26a301f54c60c85ceab1aaa4dae43f6aeb 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 f1e89597c803001ab9d5afd7e173184fe6886d1d. Ran as expected and is more practical than using an output redirection.
Tree-SHA512: 29086d428cccedcfd031c0b4514213cbc1670e35f955e8fd35cee212bc6f9616cf9f20d0cb984495390c4ae2c50788ace616aea907d44e0d6a905b9dda1685d8
a3f5e541523a843e834df1858e16f89188fe19a2 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
Currently `migratewallet` migrates the address book (i.e. labels and
purposes) for watchonly and solvable wallets only in RAM, but doesn't
persist them on disk. Fix this by adding another loop for both of the
special wallet types after which writes the corresponding NAME and
PURPOSE entries to the database in a single batch.
3ae76ea6ddd1fad43332b7e2b79e0c441ecf6d63 scripted-diff: Insert missed copyright header (Hennadii Stepanov)
306ccd4927a2efe325c8d84be1bdb79edeb29b04 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
Restoring a wallet backup from another chain should obviously result
in a dedicated error message (we have "Wallet files should not be
reused across chains. Restart bitcoind with -walletcrosschain to
override." for that). Unfortunately this is currently not the case
for legacy wallet restores, as in the course of cleaning up the
newly created wallet directory a `filesystem_error` exception is
thrown due to the directory not being empty; the wallet database did
indeed load successfully (otherwise we wouldn't know that the chain doesn't
match) and hence BDB-related files and directories are created in the wallet
directory.
For bitcoind, this leads to a very confusing error message:
```
$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -1
error message: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/test123"]
```
Even worse, the GUI crashes in such a scenario:
```
libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/foobar"]
Abort trap (core dumped)
```
Fix this by simply deleting the whole folder via `fs::remove_all`.
The current BlockAssembler bench only tests on a mempool where all
transactions have 0 ancestors or descendants, which does not exercise
any of the package-handling logic in BlockAssembler
This makes the contents of the mempool more realistic and iterating by
ancestor feerate order more meaningful. If transactions have varying
feerates, it's also more likely that packages will need to be updated
during block template assembly.
Allows us to test BlockAssembler on transactions without signatures or
mature coinbases (which is what PopulateMempool creates). Also means
that `TestBlockValidity()` is not included in the bench timing.
This allows us to both manually manipulate options and grab values from
ArgsManager (i.e. -blockmaxweight and -blockmintxfee config options)
when constructing BlockAssembler::Options. Prior to this change, the
only way to apply the config options is by ctoring BlockAssembler with
no options, which calls DefaultOptions().
As no process should be able to trigger this error
using the regular transaction creation process, throw
a runtime_error if happens to tell users/devs to
report the bug if happens.
and not the general "Insufficient funds" when the wallet
actually have funds.
Two new error messages:
1) If the selection result exceeds the maximum transaction weight,
we now will return: "The inputs size exceeds the maximum weight".
2) If the user preselected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return: "The preselected coins total amount does not cover the
transaction target".
b2aa9e85289fc654106a890c35935e9c76c411fb Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders)
8c5b3646b5afe8a61f5c66478d8e11f0d2ce5108 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 b2aa9e85289fc654106a890c35935e9c76c411fb
glozow:
reACK b2aa9e85289fc654106a890c35935e9c76c411fb
pablomartin4btc:
re-ACK b2aa9e8528
jonatack:
ACK b2aa9e85289fc654106a890c35935e9c76c411fb with some suggestions
Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
dc12f2e212dfacbe238cf68eb454b9ec71169bbc test: improve error msg on previous release tarball extraction failure (kdmukai)
7121fd8fa7de50ff67157f81f9e0f267b9795dbb 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 dc12f2e212dfacbe238cf68eb454b9ec71169bbc
Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
d3a84347e812b746a555e895594c595982815081 ci: remove --prefix from msan job (fanquake)
574e50addf8c65a3a9e0f2d8e933c147d1e93932 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
bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 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 bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 🍇
Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
31fdc54dba92fe9d7d8289f1e4380082838a74a9 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 31fdc54dba92fe9d7d8289f1e4380082838a74a9
Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
1b228497fa729c512a15bdfa80f61a610abfe8a5 qt: Drop no longer used `SplashScreen::finish()` slot (Hennadii Stepanov)
10811afff40efed1fda7eecab89884eaadd7146c qt: Drop no longer used `BitcoinApplication::splashFinished()` signal (Hennadii Stepanov)
5299cfe371ddad806b1c4538d17cde069e25b1c1 qt: Delete splash screen widget explicitly (Hennadii Stepanov)
Pull request description:
Fixesbitcoin-core/gui#604.
Fixesbitcoin/bitcoin#25146.
Fixesbitcoin/bitcoin#26340.
`SplashScreen::deleteLater()` [does not guarantee](https://doc.qt.io/qt-5/qobject.html#deleteLater) deletion of the `m_splash` object prior to the wallet context deletion. If the latter happens first, the [segfault](https://github.com/bitcoin-core/gui/issues/604#issuecomment-1133907013) follows.
ACKs for top commit:
dooglus:
ACK 1b228497fa
furszy:
ACK 1b228497
john-moffett:
ACK 1b228497fa729c512a15bdfa80f61a610abfe8a5
Tree-SHA512: bb01d0bf2051f5b184dc415c4f5d32dfb7b8bd772feff7ec7754ded4c6482de27f004b9712df7d53c5ee82e153f48aef4372e4a49d7bcbbb137f73e9b4947962
