33796a964a build: Add ability to build qt in depends with -stdlib=libc++ (Hennadii Stepanov)
Pull request description:
This PR makes possible to build the `qt` package in depends against `libc++` for x86_64 platform.
Fixes#22344.
Required for #22815.
Also this PR [fixes](https://github.com/bitcoin/bitcoin/pull/23060#discussion_r716077050) the `[no wallet] [bionic]` task on CI:
- on master (a8bbd4cc81), https://api.cirrus-ci.com/v1/task/5558609250615296/logs/ci.log:
```
Options used to compile and link:
external signer = yes
multiprocess = no
with libs = yes
with wallet = no
with gui / qt = no
```
- this PR, https://api.cirrus-ci.com/v1/task/5502605561430016/logs/ci.log:
```
Options used to compile and link:
external signer = yes
multiprocess = no
with libs = yes
with wallet = no
with gui / qt = yes
```
ACKs for top commit:
fanquake:
ACK 33796a964a - While this sort of string matching is fragile, I think the risk of this causing any actual issues is low.
Tree-SHA512: 586dde2e9864cec7a49aeb4f2b77fb8c4ae96bd10b51f9c6de0cfe8512ad61db15bb7f8d1b0eb6a5a66fd2deee52ac52218f01eb6be107ac12f1a956190de54b
1fa2e80190 Include patches for Guix (Rojar Smith)
Pull request description:
Fix .gitignore exclude the patches at 'contrib/guix/patches/'.
ACKs for top commit:
hebasto:
ACK 1fa2e80190
Tree-SHA512: 41ca0db2d2ea355341ab5e66516854e6dd91767a9499006c636fff96babf824c7f302c3a6cb9db2374593fcc55b98d06f7c0d5a04f21ebe65fb1239ffdbaad2f
c236f2e228 build: Drop redundant AC_SUBST macros (Hennadii Stepanov)
9049812106 build: Drop redundant check of PKG_CHECK_MODULES presence (Hennadii Stepanov)
Pull request description:
This PR:
- does not change behavior
- drops redundant `AC_SUBST` macros
Also checks of PKG_CHECK_MODULES presence are removed because they are redundant due to the following code ab1feadf4e/configure.ac (L16-L20)
ACKs for top commit:
fanquake:
ACK c236f2e228 - I see no difference in `config.log`s.
Tree-SHA512: 7ebbfc37123d693e034b8eea4aecbd9ef52b0ea5706b90bd282474e2d61ab0fedc8bca38aa3c9aa88cf2938339b7a491231b11865d39c682d60ef362082f22c5
Variables that are declared with AC_ARG_VAR macro are substituted via
AC_SUBST macro.
PKG_CHECK_MODULES macro already has AC_ARG_VAR(${PACKAGE}_CFLAGS) and
AC_ARG_VAR(${PACKAGE}_LIBS).
d2efb66458 test: use MiniWallet for p2p_compactblocks.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_compactblocks.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
The wallet RPCs calls (`getnewaddress`/`sendtoaddress`) can easily be substituted by generating/sending to the MiniWallet's internal address instead (`self.generate(self.wallet, ...)`/`self.wallet.send_self_transfer(...)`).
ACKs for top commit:
josibake:
ACK d2efb66458
brunoerg:
tACK d2efb66458
Tree-SHA512: 25340d95199ffd1fa734231c22b7710aad1ed4cda78c8c533c19cea90dc4eab65832f043d20e4fc5c0869692b1f300ad79af4e7fd3c4af8f2578ac9ccbbc9aeb
fa50d8b66e doc: Remove TODO comment in tx_verify (MarcoFalke)
Pull request description:
The comment has no clear motivation, so it seems better to remove it and fix it when there is a reason.
An alternative (if a fix isn't possible when there is a clear motivation) would be to create an issue thread for easier discussion.
ACKs for top commit:
fanquake:
ACK fa50d8b66e
Tree-SHA512: e9c25bab46a73b7c2db288c62ed9838a5e794b3b72db494173f4502da60b58dec4383064964c0842932cd30e4251fc01ad0c28681e2ef6cb442482eea2bad595
fa993d0e7e doc: Fix -changetype help text (MarcoFalke)
Pull request description:
This was forgotten in commit 3ac38058ce
ACKs for top commit:
shaavan:
ACK fa993d0e7e
w0xlt:
ACK fa993d0
josibake:
ACK fa993d0e7e
Tree-SHA512: 9f84b1168e3b3ab06e5c1f4915a1874598b273099eb5878ed28c3a66f1484e34c836fd3c68c4131bee541f3428052f6b66e02b192170752d1082de689d44cd4d
fa562fdd5e doc: Remove fixed TODO from wallet/feebumper (MarcoFalke)
Pull request description:
Fixed in commit 9522b53a91
ACKs for top commit:
shaavan:
ACK fa562fdd5e
Tree-SHA512: 968fda0994020c369b7acfc01db109d0f50d4c137fadf533ae55d0e14a353ebbde4320e798cf89e5489f1020c459712631b3967976c1f73d99db8a2d1cbad982
fa5d0b6ecd doc: Remove TODO from block fuzz test (MarcoFalke)
Pull request description:
I don't see a reason to fix the TODO, as all call sites assume at least one tx in `vtx`. So remove it.
ACKs for top commit:
fanquake:
ACK fa5d0b6ecd
Tree-SHA512: 8fe03a5bdebf4d6dad00f410a1dc6449c485a4d61afb79ebcaa221489cde2a465fea981742fde130238ebed6d2f1f032aa4976726ee07b23fe7fb745b2728d46
e844115dea test: use MiniWallet for rpc_scantxoutset.py (Sebastian Falbesoner)
983ca0456c test: introduce `address_to_scriptpubkey` helper (Sebastian Falbesoner)
e704d4d26f test: introduce `getnewdestination` helper for generating various address types (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (rpc_scantxoutset.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 and https://github.com/bitcoin/bitcoin/pull/23858#issue-1088320649 more recently.
Reviewer's guide:
* [commit 1/3] For replacing the getnewaddress/getaddressinfo RPC calls a helper `getnewdestination` is introduced which allows to create addresses with the common address format types ('legacy', 'p2sh-segwit' and 'bech32'), but also additionally returns the corresponding pubkey and output script.
* [commit 2/3] In order to send to addresses with MiniWallet, a helper `address_to_scriptpubkey` is introduced. It only supports legacy addresses (Base58Check) so far, which is sufficient for the scantxoutset test.
* [commit 3/3] With those helpers, the use of MiniWallet in the test is quite straight-forward. To avoid repeatedly specifying parameters like `from_node` to MiniWallet's `send_to` method, another test-internal helper `sendtodestination` is introduced which supports specifying the destination both as outputscript or as address.
ACKs for top commit:
w0xlt:
reACK [e844115](e844115dea)
Tree-SHA512: e4823dc507019b2b8e479602963c5dddc4c78211e1d934218ee0f0e32c068ab7e44e9793307f549127946364f57d684c4ea1d70f25865ea70d30d4f3855836d0
d3b0f82a43 build: Fix regression introduced in PR23603 (Hennadii Stepanov)
Pull request description:
It appears 7629efcc2c from bitcoin/bitcoin#23603 introduced a regression in build tool flag evaluation.
On macOS system:
- pre-PR23603 master (ae017b8160):
```
% make -C depends print-darwin_CXXFLAGS
darwin_CXXFLAGS=-pipe
% make -C depends print-host_CXXFLAGS
host_CXXFLAGS=-pipe
```
- the current master (369978686e):
```
% make -C depends print-darwin_CXXFLAGS
darwin_CXXFLAGS=-pipe
% make -C depends print-host_CXXFLAGS
host_CXXFLAGS=-arch x86_64
```
It's obvious a flag being set in `depends/hosts/darwin.mk`, i.e., `-pipe`, is lost.
With this PR:
```
% make -C depends print-darwin_CXXFLAGS
darwin_CXXFLAGS=-pipe
% make -C depends print-host_CXXFLAGS
host_CXXFLAGS=-pipe -arch x86_64
```
ACKs for top commit:
fanquake:
ACK d3b0f82a43
Tree-SHA512: 643099ce6858475ac9f3a4dfa72a4e493fec6fdd7042ae0f0d5fe44c5cd175e4eda63cb39fc46ac1501cadcd3466507ec88d9089235e005fe43ea7ab47ce37c1
11736dbe3d build, qt: Hardcode last modified timestamp in Qt RCC (Hennadii Stepanov)
Pull request description:
Our Guix build system sets the [`SOURCE_DATE_EPOCH`](https://reproducible-builds.org/specs/source-date-epoch/) and propagates it to the depends build subsystem. Its [default value](https://github.com/bitcoin/bitcoin/blob/master/contrib/guix/README.md#recognized-environment-variables) is the top commit timestamp.
After bumping Qt version up to 5.15.2, due to [this](1ffcca4cc2) change, every time they are going to make new Guix builds for another branch/commit they must ensure that the `qt` package will be rebuilt from scratch. Otherwise, Bitcoin Core GUI binaries will be non-deterministic.
Such behavior makes working with Guix builds suboptimal.
This PR fixes the described issue by patching Qt RCC and hardcoding last modified timestamps with `1`.
It's worth to mention that this change is compatible with a possible future [improvement](https://github.com/bitcoin/bitcoin/pull/21995) which makes each dependency package reproducible.
A drawback of such an approach is not currently applied to our project, as it effectively makes [QML cache files](https://bugreports.qt.io/browse/QTBUG-57182) useless. I can't say it's a problem for the https://github.com/bitcoin-core/gui-qml project.
---
**A note for thinkers:** For now this change is enough as only Qt source contains `SOURCE_DATE_EPOCH`. But in general we should re-think about treating the `SOURCE_DATE_EPOCH` variable in the depends build subsystem. For instance, its default value could be the output of `git log --format=%at -1 -- depends`.
ACKs for top commit:
fanquake:
ACK 11736dbe3d
Tree-SHA512: 31f104010a0a78d217aafcc5bc4606351f9060fc2a827277935b85fc8ced9f3d90a31d812c7db8c2711fb6daccd279cf0945dc1d7a7199e0eb0ade451cdbcd5d
94a7f09de6 doc: Drop outdated note (Hennadii Stepanov)
Pull request description:
It is outdated since bitcoin/bitcoin#23060.
ACKs for top commit:
MarcoFalke:
cr ACK 94a7f09de6
theStack:
Code-review ACK 94a7f09de6
Tree-SHA512: 5a98cf39c3939845d1abb64cb62dbb9bdb1135aa07a5cd0d85de7b6a6d9a3397b0e5da06cdb89991aac31151cb3c0d38eacf4898c53014cb7f7600e992230f87
This serves as a replacement for the getnewaddress RPC if no wallet is
available. In addition to the address, it also returns the corresponding
public key and output script (scriptPubKey).
fafe4dea16 test: Fix pep8 of touched file (MarcoFalke)
fa0ac9d7e3 test: Fix rpc_scantxoutset intermittent issue (MarcoFalke)
Pull request description:
I fail to see how this could have ever worked, since there is nothing that prevents the wallet from spending the coins in the utxo set.
Fixes https://github.com/bitcoin/bitcoin/issues/23847
Longer term it would be nice to remove the wallet and use MiniWallet here.
ACKs for top commit:
brunoerg:
tACK fafe4dea16
theStack:
Code-review ACK fafe4dea16
Tree-SHA512: d92b7be9a81eb62f496488dd15b8e564e9b7a64b55634af2714b53f985e6a35fdae788323833ff59c40ae7c6a0cf54fe069db8eb3be03686f7c100379f54a292
faaf9da20a test: Add missing suppressions for crypto_diff_fuzz_chacha20.cpp (MarcoFalke)
Pull request description:
Without them, CI and fuzzing is broken
ACKs for top commit:
stratospher:
ACK faaf9da. The changes look good to me.
Tree-SHA512: 46604ba1b6eb8ea321e55e043835ca1e7a4f562855007b08cf9cf96d9a7f2a0155009ca1397f34338e478b8cc5b5a6a0dc94e1878a2f00bfab11595ae58bb014
7b481f015a Fix Racy ParseOpCode function initialization (Jeremy Rubin)
Pull request description:
If multiple callers call ParseOpCode concurrently it will cause a race condition. We can either move the static to it's own area and require init be called explicitly, or just allow concurrent first callers to race to fill in an atomic variable that never changes thereafter. The second approach is taken here.
Static initialization *is* threadsafe, but only insofar as definining the variable being guaranteed to be called once. This is used incorrectly here.
practicalswift --> are there tools we can deploy to catch usage like this?
ACKs for top commit:
MarcoFalke:
re-ACK 7b481f015a 🗣
Tree-SHA512: cbf9dc3af26d7335305026f32ce8472a018309b89b3d81a67357e59fbeed72c37b5b8a6e30325ea68145c3b2403867be82de01f22decefb6e6717cf0c0045633
2da97b271b ci: use GCC 8 when building packages in native_qt5 CI (fanquake)
Pull request description:
Our minimum required GCC is GCC 8, and this change in required for
PRs like #23839 which take advantage of flags introduced in that
version of GCC.
This should have been included as part of 182de7ba10.
ACKs for top commit:
MarcoFalke:
cr ACK 2da97b271b
hebasto:
ACK 2da97b271b
Tree-SHA512: 2b64c21119fb95b959ac0f7d8d2d38f6ed98309695bb35425fad45b3b628247c2c78d0647c4d1f511669e8d333c6febe1cc44fac8027ed0bd7593eb630add548
ff5f6dea53 scripted-diff: Rename interfaces::WalletClient to interfaces::WalletLoader (Russell Yanofsky)
Pull request description:
Name has been confusing since it was introduced, and it was pointed in recent review club https://bitcoincore.reviews/10102 that it was particularly unclear how `interfaces::WalletClient` was different from `interfaces::Wallet`.
ACKs for top commit:
w0xlt:
ACK ff5f6de
Tree-SHA512: 26fa10baa457e76da1933adab187e9be61b8d76cff1cf2c73ad4320461c7e31fb9db07b7c2486998294826beb4a1aca255c14903920b443db6213e653c5f7e0a
826e12b010 test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne)
Pull request description:
for additional coverage and similarity to actual init process.
Followup to #23280.
ACKs for top commit:
dongcarl:
Code Review ACK 826e12b010
ryanofsky:
Code review ACK 826e12b010
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
Our minimum required GCC is GCC 8, and this change in required for
changes like #23839 which take advantage of flags introduced in that
version of GCC.
This should have been included as part of
182de7ba10.
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
c9a77e227e gui: address type dropdown, add bech32m (Sjors Provoost)
56113daef4 wallet: add taprootEnabled() to interface (Sjors Provoost)
Pull request description:
This PR replaces the Bech32 checkbox with an address type dropdown It "Use Taproot" checkbox to the receive screen for any wallet with a Taproot descriptor. The Bech32m option is hidden for wallets that don't have taproot descriptors.
Bech32 is kept as the default even for Taproot enabled wallets until it's more widely supported.
(an earlier attempt of this PR added a second checkbox)
<img width="469" alt="Schermafbeelding 2021-12-21 om 11 44 05" src="https://user-images.githubusercontent.com/10217/146872660-339fae1f-c0b8-4673-b8be-33c25f3933fd.png">
**Suggested testing**
* notice that the Bech32m entry only appears for wallet with a taproot descriptor
* verify that it generates a bc1p instead of bc1q address
1. Legacy wallet
2. Default descriptor wallet (current does not have taproot descriptor)
3. Wallet with taproot descriptor:
* just create a new descriptor wallet
Replaces https://github.com/bitcoin/bitcoin/pull/22260
ACKs for top commit:
Rspigler:
tACK c9a77e227e. I like the changes made now, thanks!
kristapsk:
re-ACK c9a77e227e
Tree-SHA512: bae66ac90ed55372e6c94878db0e37d32b7b5c24ed00c0f2ebb10fd127dddce3a061162df915d67e92d7b35b3da093137db17b73931a0ae1a470fff20be1f30b
a2a92317ad rpc: Add warning to user about newkeypool command (Samuel Dobson)
Pull request description:
This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.
David Harding also suggested [here](https://github.com/bitcoin/bitcoin/pull/23093#issuecomment-945350003) that the RPC help text should include a warning to the users about the interaction between newkeypool.
ACKs for top commit:
achow101:
ACK a2a92317ad
Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
fada6c65d2 wallet: Strictly match tx change type to improve privacy (MarcoFalke)
Pull request description:
Currently the change type will only match a destination by accident, making it easier to determine the change.
Fix that by strictly matching one of the destinations.
ACKs for top commit:
S3RK:
Concept & Approach ACK fada6c6. Also did light code review .
achow101:
ACK fada6c65d2
prayank23:
tACK fada6c65d2
w0xlt:
tACK fada6c6
Tree-SHA512: 2b072c3c32debac7b0bef07a6df9a8f1a631e0f7d556b859973f18894ca490225582dc13e4588b29fa205ffbcd30fb632d5313b304d10ad17a26adc3f7684471
140a49ce5e test: check that pruneblockchain RPC fails for future block or timestamp (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `pruneblockchain` RPC for the case that a future block or timestamp is passed:
8c0bd871fc/src/rpc/blockchain.cpp (L1101)8c0bd871fc/src/rpc/blockchain.cpp (L1111)
Note that the test method `manual_test` gets called twice, once each with `use_timestamp` set to True/False, respectively. Depending on that, the helper function `height` either converts the passed block height to the timestamp of that block, or just returns it without modification.
The other tests for failures in this RPC are also changed to be more detailled ("Cannot prune blocks because node is not in prune mode", "Negative block height"), as I don't think there is any value in just checking a sub-string. If there is ever an error with the same sub-string is introduced, it's not clear which error is exactly checked with the test, so it makes sense to be as specific as possible.
ACKs for top commit:
brunoerg:
tACK 140a49ce5e
Tree-SHA512: bee3cee9f35c2a63a1839d7ec1f83e354d9d3c0c2ca32d300dca2de8b755d555f769ba2b80ac37d31df6ee7e2b8eaefb8134c4727a7144e47c0f5e34f2cc5822
cfeb1942bc ci: Use Cirrus "greedy" flag to use idle CPU time when available (Luke Dashjr)
Pull request description:
Disabled for MSan to avoid excessive RAM usage
Top commit has no ACKs.
Tree-SHA512: 4351d477b8749b3578a1f808ba7e963a44e0a4a1b695605b473ab3cf424d75ade6e13e7401a39675843e7c9b48a3e46936909d6bcc4af1f7abe1b690fe5b7a55
92082ea0bb Change time variable type to std::chrono::seconds in src/net_processing.cpp (Shashwat)
Pull request description:
- This is a follow-up to PR #23758
- This changes the remaining time variable in `net_processing.cpp` from **int64_t** to **std::chrono::seconds**
ACKs for top commit:
naumenkogs:
ACK 92082ea0bb
hebasto:
re-ACK 92082ea0bb
Tree-SHA512: 559e351d9046d4ba2b842ae38da13b4befc7feee71f0762f97907812471e2840b0d43c90c92222d15619fe40cc21f11d40900500ca07b470f7ac8b0046cc1d68
82858bab64 [doc] CPFP carve out and single-conflict RBF exemption (glozow)
1fd49eb498 [doc] clarify RBF difference from BIP125 (glozow)
919ae8b8cd [doc] current rbf policy (glozow)
Pull request description:
Since RBF was first implemented and BIP125 was written, our code has changed, people have highlighted implementation differences, and some people have proposed further changes to it. Many people seem to support the idea of documenting our _current_ RBF policy as it stands today.
As the ancestor/descendant limit carve-out exemptions are very related to RBF, it seemed appropriate to group them with this PR.
Related to #22806 - it seems that these policies are the most confusing for people, or at least the most documentation-requested.
ACKs for top commit:
dunxen:
ACK 82858ba
t-bast:
ACK 82858bab64, thanks @glozow!
darosior:
re-ACK 82858bab64
ariard:
ACK 82858ba
Tree-SHA512: 5d296537cce3488c18179c0aa76c739ca02fdc424e5aa17129b4cdd0d057358f86bcc1e92a9857bd2c60495f834fe9d9406d1a9f8ac5cfc8f0f4f4c27ec4f8e1
bf4f817135 refactor: addrman_select test (josibake)
5a64dc018c refactor: addrman_evictionworks test (josibake)
e281fccd8a refactor: addrman_noevict test (josibake)
8bdd9240d4 refactor: addrman_selecttriedcollisions test (josibake)
Pull request description:
As a follow-up to #23713 , this PR refactors the remaining tests in `src/tests/addrman_tests.cpp` to use the output from `AddrMan::Good()` where appropriate.
ACKs for top commit:
naumenkogs:
ACK bf4f817135
mzumsande:
Code Review ACK bf4f817135
Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913
