3a68546fd0981e032005d3a5d2ca7b2ab9acc70b ci: Build and cache static Qt instead of downloading a pre-built one (Hennadii Stepanov)
Pull request description:
This PR makes the MSVC build CI task free of [pre-built static Qt binaries](https://github.com/sipsorcery/qt_win_binary/releases). It uses the approach which is documented in #22890.
It takes about 13 minutes to build a static Qt dependency (for 8 CPUs):
with the maximum total time:
There is an additional benefit of this PR. It is no longer required to build a new static Qt package when a CI Windows image upgrades its building tools, and breaks the compatibility with the recent Qt package.
ACKs for top commit:
sipsorcery:
utACK 3a68546fd0981e032005d3a5d2ca7b2ab9acc70b.
Tree-SHA512: 2cf358ccecb26293b52c04158d6d3366ae6257cc3c04262e02234f7d7a03086885c67f0aad5702fcaa6f035fe4a09967a81245c561614875ecd2e90e2e00bbaa
DecopeAsmap is a pure utility function and doesn't have any
dependencies on addrman, so move it to util/asmap.
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
64015eb01495689d8e02fb82c06eaa3f442ff6e5 ci: Add missed comments and test_bitcoin.exe command line option (Hennadii Stepanov)
Pull request description:
This PR is a #21551 follow up, and it:
- adds missed comments, see https://github.com/bitcoin/bitcoin/pull/21551#discussion_r703342550
- restores missed `-l test_suite` command line option for `test_bitcoin.exe`, see https://github.com/bitcoin/bitcoin/pull/21551#discussion_r703348955
ACKs for top commit:
MarcoFalke:
cr ACK 64015eb01495689d8e02fb82c06eaa3f442ff6e5
Tree-SHA512: ad1c91544da39a94f033bc55ae5fdaf5774475702edd026635389e68d20e2608cb599dd51f3c1412e0287beef073352e48d9ec005c94df38cfe4fe2d21a94fe3
97292b19140db32c6d85d63b70382e7bf60a55c4 ci: Drop AppVeyor CI integration (Hennadii Stepanov)
1fb70793b237b9a3a00ff744739e512dd7755937 ci: Add Windows task to Cirrus CI (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid off unreliable AppVeyor CI
- places all CI configs in one place
- allows to enable functional tests in the future (using a persistent worker)
| no populated `vcpkg` cache | populated `vcpkg` cache |
|---|---|
|  |  |
Currently, AppVeyor builds take about 44..48 minutes.
ACKs for top commit:
sipsorcery:
re-ACK 97292b19140db32c6d85d63b70382e7bf60a55c4.
Tree-SHA512: 3af50d9fd68eb12f39724810dacf948e4068573b5dfd0dbaeb05d19d4bd6f10bd9a432656dcc32b742684b438d31305eace85c602296d7a1bf84b2f1fcc06f02
The class only stores the file path, reading it from a global. Globals
are confusing and make testing harder.
The method reading from a stream does not even use any class members, so
putting it in a class is also confusing.
5fabde6fadd1b07e981c97f5087d67c4179340ba wallet: AddWalletDescriptor requires cs_wallet lock (João Barbosa)
32d036e8dab5f5b24096d9765236441e7b6a3b34 wallet: GetLabelAddresses requires cs_wallet lock (João Barbosa)
Pull request description:
This is another small change towards non recursive wallet lock.
ACKs for top commit:
hebasto:
ACK 5fabde6fadd1b07e981c97f5087d67c4179340ba, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 00506f0159c56854a171e58a451db8dd9b9f735039697b1cf2ca7f54de61fb51cc1e5eff42265233e041b4b1bfd29c2247496dc4456578e1a23c323bdec2901b
d319c4dae9ed7d59d71b926e677707fce4194d0c qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView (Hennadii Stepanov)
92ddc02a16a74e10f24190929f05e2dcf2b55871 qt, refactor: Declare getWalletModel with const and noexcept qualifiers (Hennadii Stepanov)
ca0e680bdcaa816c53355777d788a4c8478bb117 qt, refactor: Drop redundant checks of walletModel (Hennadii Stepanov)
404373bc6ac0589e9e28690c9d09114d626a3dc3 qt, refactor: Pass WalletModel object to WalletView constructor (Hennadii Stepanov)
Pull request description:
An instance of the `WalletView` class without the `walletModel` data member being set is invalid. So, it is better to set it in the constructor.
Establishing one more `WalletView` class's invariant in constructor:
- allows to drop all of checks of the`walletModel` in member functions
- makes reasoning about the code that uses instances of the `WalletView` class easier
Possible follow ups could extend this approach to other classes, e.g., `OverviewPage`, `TransactionView`, `ReceiveCoinsDialog`, `SendCoinsDialog`, `AddressBookPage`.
ACKs for top commit:
ShaMan239:
Code review ACK d319c4dae9ed7d59d71b926e677707fce4194d0c
promag:
Code review ACK d319c4dae9ed7d59d71b926e677707fce4194d0c.
jarolrod:
ACK d319c4dae9ed7d59d71b926e677707fce4194d0c
Tree-SHA512: b0c61f82811bb5aba2738067b53dc9ea4439230d547ce5c8fd85c480d8d70ea15f9942dbf13842383acbce467fba1ab4e132e37c56b654b46ba897301a41066e
2445df4eb36ba0d90e1283f36e629e1cf69eeef7 build: Fix macOS Apple Silicon build with miniupnpc and libnatpmp (Hennadii Stepanov)
Pull request description:
On master (7a49fdc58115845ece3a9890bf9498bee6b559de) the `configure` script does not pick up Homebrew's `miniupnpc` and `libnatpmp` packages on macOS Apple Silicon:
```
% ./configure --with-miniupnpc
...
checking for miniupnpc/miniupnpc.h... no
checking for miniupnpc/upnpcommands.h... no
checking for miniupnpc/upnperrors.h... no
...
checking whether to build with support for UPnP... configure: error: "UPnP requested but cannot be built. Use --without-miniupnpc."
```
```
% ./configure --with-natpmp
...
checking for natpmp.h... no
...
checking whether to build with support for NAT-PMP... configure: error: NAT-PMP requested but cannot be built. Use --without-natpmp
```
The preferred Homebrew [prefix for Apple Silicon](https://docs.brew.sh/Installation) is `/opt/homebrew`. Therefore, if we do not use `pkg-config` to detect packages, we should set the `CPPFLAGS` and `LDFLAGS` variables for them explicitly.
ACKs for top commit:
Zero-1729:
re-tACK 2445df4eb36ba0d90e1283f36e629e1cf69eeef7 (re-tested on an M1 Machine running macOS 11.4).
jarolrod:
re-ACK 2445df4eb36ba0d90e1283f36e629e1cf69eeef7
Tree-SHA512: d623d26492f463812bf66ca519847ff4b23d517466b6c51c3caf3642a582d02e5f03ce57915742b29f01bf9bceb731a3978ef9a5fdc82e568bcb62548eda758a
77f37f58ad2f349cecb2eda28b415267d3d7d76e doc: update enum naming in developer notes (Jon Atack)
Pull request description:
Per our current doc, the general rule is we follow the C++ Core Guidelines, which for enumerator naming stipulate:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps
```
Don’t use ALL_CAPS for enumerators
Reason: Avoid clashes with macros.
```
but our examples (and often, codebase) are contradictory to it (and perhaps to common usage), which can be confusing when a contributor needs to choose a style to use. This patch:
- updates the enumerator examples to snake_case per CPP Core Guidelines
- clarifies for contributors that in this project enumerators may be snake_case, PascalCase or ALL_CAPS, and to use what seems appropriate.
ACKs for top commit:
practicalswift:
cr ACK 77f37f58ad2f349cecb2eda28b415267d3d7d76e
ryanofsky:
ACK 77f37f58ad2f349cecb2eda28b415267d3d7d76e. I think this is good because it modernizes the example and clarifies current conventions.
promag:
ACK 77f37f58ad2f349cecb2eda28b415267d3d7d76e.
Tree-SHA512: 7facc607fe5e1abab0f635864340143f13c2e4bb074eb17eac7d829dcd0cf244c5c617fc49d35e8774e8af1fa1205eeebe0cca81f538a2a61f6a7ba200878bc6
724c4975622bc22cedc3f3814dfc8e66cf8371f7 [fuzz] Add ConsumeAsmap() function (John Newbery)
5840476714ffebb2599999c85a23b52ebcff6090 [addrman] Make m_asmap private (John Newbery)
f9002cb5dbd573cd9ca200de21319fa296e26055 [net] Rename the copyStats arg from m_asmap to asmap (John Newbery)
f572f2b2048994b3b50f4cfd5de19e40b1acfb22 [addrman] Set m_asmap in CAddrMan initializer list (John Newbery)
593247872decd6d483a76e96d79433247226ad14 [net] Remove CConnMan::SetAsmap() (John Newbery)
50fd77045e2f858a53486b5e02e1798c92ab946c [init] Read/decode asmap before constructing addrman (John Newbery)
Pull request description:
Commit 181a1207 introduced an initialization order bug: CAddrMan's m_asmap must be set before deserializing peers.dat.
The first commit restores the correct initialization order. The remaining commits make `CAddrMan::m_asmap` usage safer:
- don't reach into `CAddrMan`'s internal data from `CConnMan`
- set `m_asmap` in the initializer list and make it const
- make `m_asmap` private, and access it (as a reference to const) from a getter.
This ensures that peers.dat deserialization must happen after setting m_asmap, since m_asmap is set during CAddrMan construction.
ACKs for top commit:
mzumsande:
Tested ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
amitiuttarwar:
code review but utACK 724c497562
naumenkogs:
utACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
vasild:
ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
MarcoFalke:
review ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7 👫
Tree-SHA512: 684a4cf9e3d4496c9997fb2bc4ec874809987055c157ec3fad1d2143b8223df52b5a0af787d028930b27388c8efeba0aeb2446cb35c337a5552ae76112ade726
6919c823cbce92248647880fb1d912828449ae57 MOVEONLY: Expose BanMapToJson / BanMapFromJson (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
CSubNet serialization code that was removed in #22570 fa4e6afdae7b82df638b60edf37ac36d57a8cb4f was needed by multiprocess code to share ban map between gui and node processes.
Rather than adding it back, use suggestion from MarcoFalke https://github.com/bitcoin/bitcoin/pull/10102#discussion_r690922929 to use JSON serialization. This requires making BanMapToJson / BanMapFromJson functions public.
ACKs for top commit:
promag:
reACK 6919c823cbce92248647880fb1d912828449ae57.
Tree-SHA512: ce909a61b7869d16cf2e9f91b643dd9d2604efc5777703d3b77a4c40cb0ccdd20396ba87b1ec85aade142e12ff9ea4c95c7155840354873579565471779f5a33
7e698732836121912f179b7c743a72dd6fdffa72 sync: remove DEBUG_LOCKCONTENTION preprocessor directives (Jon Atack)
9b08006bc502e67956d6ab518388fad6397cac8d log, sync: improve lock contention logging and add time duration (Jon Atack)
3f4c6b87f1098436693c4990f2082515ec0ece26 log, timer: add timing macro in usec LOG_TIME_MICROS_WITH_CATEGORY (Jon Atack)
b7a17444e0746c562ae97b26eba431577947b06a log, sync: add LOCK logging category, apply it to lock contention (Jon Atack)
Pull request description:
To enable lock contention logging, `DEBUG_LOCKCONTENTION` has to be defined at compilation. Once built, the logging is not limited to a category and is high frequency, verbose and in all-caps. With these factors combined, it seems likely to be rarely used.
This patch:
- adds a `lock` logging category
- adds a timing macro in microseconds, `LOG_TIME_MICROS_WITH_CATEGORY`
- updates `BCLog::LogMsg()` to omit irrelevant decimals for microseconds and skip unneeded code and math
- improves the lock contention logging, drops the all-caps, and displays the duration in microseconds
- removes the conditional compilation directives
- allows lock contentions to be logged on startup with `-debug=lock` or at run time with `bitcoin-cli logging '["lock"]'`
```
$ bitcoind -signet -debug=lock
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 completed (4μs)
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 completed (4μs)
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 completed (20μs)
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 started
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 completed (3μs)
$ bitcoin-cli -signet logging
"lock": true,
$ bitcoin-cli -signet logging [] '["lock"]'
"lock": false,
$ bitcoin-cli -signet logging '["lock"]'
"lock": true,
```
I've tested this with Clang 13 and GCC 10.2.1, on Debian, with and without `--enable-debug`.
ACKs for top commit:
hebasto:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72, added a contention duration to the log message since my [previous](https://github.com/bitcoin/bitcoin/pull/22736#pullrequestreview-743764606) review.
theStack:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72 🔏⏲️
Tree-SHA512: c4b5eb88d3a2c051acaa842b3055ce30efde1f114f61da6e55fcaa27476c1c33a60bc419f7f5ccda532e1bdbe70815222ec2b2b6d9226f29c8e94e598aacfee7
fa0a5fa744108d81bee9600c80bfda6ca11e5255 ci: Fuzz with -ftrivial-auto-var-init=pattern (MarcoFalke)
Pull request description:
This makes memory bugs deterministic. `-ftrivial-auto-var-init=pattern` is incompatible with other memory sanitizers (like valgrind and msan), but that is irrelevant here, because the address sanitizer in this fuzz CI config is already incompatible with them.
`-ftrivial-auto-var-init=pattern` goes well with `-fsanitize=bool` and `-fsanitize=enum`, but those are already enabled via `-fsanitize=undefined`. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
ACKs for top commit:
practicalswift:
cr ACK fa0a5fa744108d81bee9600c80bfda6ca11e5255
Tree-SHA512: ed6be953cd99eadb1ba245ba30170747eff66be54d2773c8d26a3a6aee0fdcd6967c596f4f4ab1d238de6a6526623dac5211f0ba77f1986639395d7921bdc19f
e952d7557eaf2610e302e9d70381ef057d07f6bf netinfo: clarify client and server versions in header (Jon Atack)
Pull request description:
Clarify in -netinfo output that both the client and the server versions are provided.
before
```
Bitcoin Core v22.0.0rc3 - 70016/Satoshi:22.99.0/
```
after
```
Bitcoin Core client v22.0.0rc3 - server 70016/Satoshi:22.99.0/
```
Closes #22873.
ACKs for top commit:
benthecarman:
utACK e952d7557eaf2610e302e9d70381ef057d07f6bf
prayank23:
ACK e952d7557e
Zero-1729:
tACK e952d7557eaf2610e302e9d70381ef057d07f6bf
Tree-SHA512: 3e817892d398aabacb1401fd5b1816c4d4f563b4f8cf1096bdb8b53f7c4ef82d4caee09f5c7724f1fe292f837434a332acefba735152ed24a238bb6f006df909
d1267fdbb01120827785451d6468137a0bed46c5 build: Update default platform toolset in msvc-autogen.py (Hennadii Stepanov)
Pull request description:
The platform toolset was updated from v141 to v142 in #17364.
ACKs for top commit:
sipsorcery:
ACK d1267fdbb01120827785451d6468137a0bed46c5.
Tree-SHA512: 390dc4876948af7f6b9f26eb1e64262f6386c2541a4647a6cb7abd8f1283c63ffbf5efba5e67a8075f0085a0557fc3f1b41e70f3ede0e8cef60d0fe2d6bf3be8
2b071265c37da22f15769945fd159b50a14792a3 error if settings.json exists, but is unreadable (Tyler Chambers)
Pull request description:
If settings.json exists, but is unreadable, we should error instead of overwriting.
Fixes #22571
ACKs for top commit:
Zero-1729:
tACK 2b071265c37da22f15769945fd159b50a14792a3
ShaMan239:
tACK 2b071265c37da22f15769945fd159b50a14792a3
prayank23:
tACK 2b071265c3
ryanofsky:
Code review ACK 2b071265c37da22f15769945fd159b50a14792a3. Thanks for the fix! Note that PR https://github.com/bitcoin-core/gui/pull/379 will change the appearance of dialogs shown in screenshots above. So it could be interesting to test the two PRs together (but current testing seems more than sufficient)
theStack:
ACK 2b071265c37da22f15769945fd159b50a14792a3 📁
Tree-SHA512: 6f7f96ce8a13213d0335198a2245d127264495c877105058d1503252435915b332a6e55068ac21088f4c0c017d564689f4956213328d5bdee81d73711efc5511
696c76d6604c9c4faddfc4b6684e2788bb577ba6 tests: Add TrimString(...) tests (practicalswift)
4bf18b089e1bb1f3ab513cbdf6674bd1074f4621 Replace use of boost::trim_right with locale-independent TrimString (Ben Woosley)
93551862a18965bcee0c883c54807e8726e2f50f Replace use of boost::trim use with locale-independent TrimString (Ben Woosley)
Pull request description:
This is [#18130 rebased](https://github.com/bitcoin/bitcoin/pull/18130#issuecomment-900158759).
> `TrimString` is an existing alternative.
> Note `TrimString` uses `" \f\n\r\t\v"` as the pattern, which is consistent with the default behavior of `std::isspace`. See: https://en.cppreference.com/w/cpp/string/byte/isspace
ACKs for top commit:
jb55:
utACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
practicalswift:
ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
jonatack:
ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
theStack:
Code-review ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
Tree-SHA512: 6a70e3777602dfa65a60353e5c6874eb951e4a806844cd4bdaa4237cad980a4f61ec205defc05a29f9707776835975838f6cc635259c42adfe37ceb02ba9358d
Available in line 59 is made a reference , so contents of the coin can be modified
While generating transactions we select ancestors from available_coins ,in case we exhaust all the outputs of an entry in available_coins
then we need to remove it from available_coins before the next iteration of choosing a potential ancestor , it is now implemented with
this patch by ,As the index of the entry is randomly chosen from available_coins , In order to remove it from the vector if index of the
selected entry is not at the end of available_coins vector, it is swapped with the entry at the back of the vector , then the entry at the
end of available_coins is popped out.
Code generating outputs for the transaction is moved out of the loop, as it needs to be done only once before adding the transaction to ordered_coins
fa7e6c56f58678b310898a158053ee9ff8b27fe7 Add LIFETIMEBOUND to InitializeChainstate (MarcoFalke)
fa5c896724bb359b4b9a3f89580272bfe5980c1b Add LIFETIMEBOUND to CScript where needed (MarcoFalke)
Pull request description:
Without this, stack-use-after-scope can only be detected at runtime with ASan or code review, both of which are expensive.
Use `LIFETIMEBOUND` to turn this error into a compile warning.
See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound
Example:
```cpp
const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
```
Before: (no warning)
After:
```
warning: returning reference to local temporary object [-Wreturn-stack-address]
const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
^~~~~~~~~
./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
^~~~
ACKs for top commit:
theuni:
utACK fa7e6c56f58678b310898a158053ee9ff8b27fe7.
jonatack:
Light ACK fa7e6c56f58678b310898a158053ee9ff8b27fe7 debug build with clang 13, reproduced the example compiler warning in the pull description, and briefly looked at `clang::lifetimebound` support in earlier versions of clang; it is in clang 7 (https://releases.llvm.org/7.0.0/tools/clang/docs/AttributeReference.html#lifetimebound-clang-lifetimebound), did not see references to it in earlier docs
Tree-SHA512: e915acdc4532445205b7703fab61a5d682231ace78ecfb274cb8523ca2bddefd85828f50ac047cfb1afaff92a331f5f7b5a1472539f999e30f7cf8ac8c3222f3
