This is some very ugly and brittle code that switches the global network
based on a provided address, remove it. I think in practice it's very
unlikely for testnet BIP21 payment URIs to be used, and if so it's for
testing so it's easy enough to manually copy it. Or to specify
`-testnet` explicitly.
There is already no case for `-regtest` or `-signet`.
2f5fd3cf9225aed439d1de767312bb340972d665 test: Correctly decode UTF-8 literal string paths (Ryan Ofsky)
Pull request description:
Call `fs::u8path()` to convert some UTF-8 string literals to paths, instead of relying on the implicit conversion. Fake Macro pointed out in https://github.com/bitcoin/bitcoin/pull/24306#discussion_r818566106 that `fs_tests` are incorrectly decoding some literal UTF-8 paths using the current windows codepage, instead of treating them as UTF-8. This could cause test failures depending what environment windows tests are run under.
The `fs::path` class exists to avoid problems like this, but because it is lenient with `const char*` conversions, under assumption that they are ["safe as long as the literals are ASCII"](727b0cb592/src/fs.h (L39)), bugs like this are still possible.
If we think this is a concern, followup options to try to prevent this bug in the future are:
0. Do nothing
1. Improve the "safe as long as the literals are ASCII" comment. Make it clear that non-ASCII strings are invalid.
2. Drop the implicit `const char*` conversion functions. This would be nice because it would simplifify the `fs::path` class a little, while making it safer. Drawback is that it would require some more verbosity from callers. For example, instead of `GetDataDirNet() / "mempool.dat"` they would have to write `GetDataDirNet() / fs::u8path("mempool.dat")`
3. Keep the implicit `const char*` conversion functions, but make them call `fs::u8path()` internally. Change the "safe as long as the literals are *ASCII*" comment to "safe as long as the literals are *UTF-8*".
I'd be happy with 0, 1, or 2. I'd be a little resistant to 3 even though it was would add more safety, because it would slightly increase complexity, and because I think it would encourage representing paths as strings, when I think there are so many footguns associated with paths as strings, that it's best to convert strings to paths at the earliest point possible, and convert paths to strings at the latest point possible.
ACKs for top commit:
laanwj:
Code review ACK 2f5fd3cf9225aed439d1de767312bb340972d665
w0xlt:
crACK 2f5fd3c
Tree-SHA512: 9c56714744592094d873b79843b526d20f31ed05eff957d698368d66025764eae8bfd5305d5f7b6cc38803f0d85fa5552003e5c6cacf1e076ea6d313bcbc960c
e8023100be7ab2e32addfff75a133fb429b8492b guix: only check for the macOS SDK once (fanquake)
Pull request description:
If we are building for both macOS HOSTS, there's no need to check and
print that the SDK exists two times.
Currently a Guix build for both HOSTS will print:
```bash
./contrib/guix/guix-build
Found macOS SDK at '/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers', using...
Found macOS SDK at '/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers', using...
Checking that we can connect to the guix-daemon...
```
ACKs for top commit:
laanwj:
Code review ACK e8023100be7ab2e32addfff75a133fb429b8492b
achow101:
ACK e8023100be7ab2e32addfff75a133fb429b8492b
Tree-SHA512: 7e9ee7793c5dc1eb485806ca3d613742397d2cc62525203a168cad1ec96aabfd4e63dc3f2e8d205bdb2f3c2079f731d75c5d162d55ff0d42a7e6f3d01d3a7db1
c3296b21e40be3a5cb7060ceb8f1b6db1fd79e65 build: Drop `double-conversion` from MSVC dependencies (Hennadii Stepanov)
7ff43e5372c4606ecb75d6892b4bb0ccb4165b80 ci: Invalidate vcpkg binary cache if dependencies changed (Hennadii Stepanov)
20b6c871178f20661b849ad5677bd8ecae55cf19 build: Specify `zeromq` port explicitly for MSVC builds (Hennadii Stepanov)
Pull request description:
The current MSVC builds are broken due to the bug in the `zeromq` [port](https://github.com/microsoft/vcpkg/pull/22681#issuecomment-1061312320). From [IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-03-08#787145):
> \<sipsorcery> Looks like it's a problem downloading the zeromq dependency from https://patch-diff.githubusercontent.com/raw/zeromq/libzmq/pull/4311.diff
> \<dhruv> sipsorcery: I'm definitely misunderstanding, i actually have no clue which file the CI is failing to download. I'll DM you more details.
> \<sipsorcery> It's saying the hash of the patch file has changed.
> \<dhruv> so we'd need to verify that the change is not malicious and then commit the new hash?
> \<sipsorcery> No that dependency is managed by the vcpkg repo. Seems they might be working on it https://github.com/microsoft/vcpkg/pull/22681#issuecomment-1061312320
> \<dhruv> ok, thanks
This PR fixes this issue with specifying the previous port version [explicitly](https://github.com/microsoft/vcpkg/blob/master/docs/users/versioning.md).
The current CI task does not fail due to the cached binaries.
---
The second commit makes vcpkg binary cache invalid if dependencies changed.
The third commit drops `double-conversion` from dependencies as Qt is configured as follows:
```
Configure summary:
Build type: win32-msvc (x86_64, CPU features: sse sse2)
Compiler: msvc 193131104
Configuration: sse2 aesni sse3 ssse3 sse4_1 sse4_2 avx avx2 avx512f avx512bw avx512cd avx512dq avx512er avx512ifma avx512pf avx512vbmi avx512vl compile_examples f16c largefile msvc_mp precompile_header rdrnd rdseed shani silent x86SimdAlways release c++11 c++14 c++17 c++1z concurrent no-pkg-config static static_runtime stl
Build options:
...
Qt Core:
DoubleConversion ....................... yes
Using system DoubleConversion ........ no
...
```
ACKs for top commit:
sipsorcery:
tACK c3296b21e40be3a5cb7060ceb8f1b6db1fd79e65.
Tree-SHA512: 4d694a7d0930889a53eb6ee7a09929f6ffa3f078122b34abe6d75430769bb87c353f7c11146da53c3804e51d4bbfcbb7bc8453f525bcc432928d98eeb66ee35e
a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1 util: Fix ReadBinaryFile reading beyond maxsize (klementtan)
Pull request description:
Currently `ReadBinaryFile` will read beyond `maxsize` if `maxsize` is not a multiple of `128` (size of buffer)
This is due to `fread` being called with `count = 128` instead of `count = min(128, maxsize - retval.size()` at every iteration
The following unit test will fail:
```cpp
BOOST_AUTO_TEST_CASE(util_ReadWriteFile)
{
fs::path tmpfolder = m_args.GetDataDirBase();
fs::path tmpfile = tmpfolder / "read_binary.dat";
std::string expected_text(300,'c');
{
std::ofstream file{tmpfile};
file << expected_text;
}
{
// read half the contents in file
auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2);
BOOST_CHECK_EQUAL(text.size(), 150);
}
}
```
Error:
```
test/util_tests.cpp:2593: error: in "util_tests/util_ReadWriteFile": check text.size() == 150 has failed [256 != 150]
```
ACKs for top commit:
laanwj:
Code review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1
theStack:
Code-review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1
Tree-SHA512: 752eebe58bc2102dec199b6775f8c3304d899f0ce36d6a022a58e27b076ba945ccd572858b19137b769effd8c6de73a9277f641be24dfb17657fb7173ea0eda0
0189df1d3171082caf743ef3b0968f43c71303f5 build, mac: Include arch in codesignature tarball (Andrew Chow)
6e9308c6d4ed9fbf909c7234ae31245747183be3 guix: use latest signapple (Andrew Chow)
Pull request description:
Since we have two architectures for Mac binaries, having the architecture in the code signature tarball generated by `detached-sig-create.sh` allows us to avoid accidentally overwriting an existing code signature tarball during the code signing process.
ACKs for top commit:
fanquake:
ACK 0189df1d3171082caf743ef3b0968f43c71303f5
Tree-SHA512: 7e0d282e4ced1094f36f1d26ff6e18d53449561ab3a1a95ac69eb5ff3d7b33ee4bd8fad004884806064a29541c47f9e5879c2a1fd0f54453413245bdcf53c4c7
5b1aae12ca4a99c6b09349981a4902717a6a5d3e qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky)
84b0973e35dae63cd1b60199b481e24d54e58c97 test: Add tests for GetArg methods / settings.json type coercion (Ryan Ofsky)
Pull request description:
Should probably add this change to 23.x as suggested by Luke https://github.com/bitcoin/bitcoin/issues/24457#issuecomment-1059825678. If settings like `prune` are added to `settings.json` in the future, it would be preferable for 23.x releases to respect the setting instead of crash.
---
Fix GUI startup crash reported by Rspigler in https://github.com/bitcoin/bitcoin/issues/24457 that happens if `settings.json` contains an integer value for any of the configuration options which GUI settings can currently clash with (-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
The fix is a one-line change in `ArgsManager::GetArg`. The rest of the PR just adds a regression test for the GUI and unit tests for ArgsManager::GetArg methods.
ACKs for top commit:
laanwj:
Code review ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
achow101:
ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
jonatack:
Code review ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
Tree-SHA512: 958991b4bead9b82a3879fdca0f8d6405e2a212b7c46cf356f078843a4f156e27fd75fc46e2013aa5159582ead06d343c1ed248d678b3e5bbd312f247e37894c
691d45fdc83ec14f87a400f548553168ac70263f Add coinstatsindex_unclean_shutdown test (Ryan Ofsky)
eb6cc05da32c5bde122725a0bc907d3767a791cd index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together (Martin Zumsande)
Pull request description:
Fixes#24076
Coinstatsindex currently writes the MuHash (`DB_MUHASH`) to disk in `CoinStatsIndex::WriteBlock()` and `CoinStatsIndex::ReverseBlock()`, but the best synced block is written in `BaseIndex::Commit()`. These are called at different points in time, both during the ThreadSync phase, and also after the initial sync is finished and validation callbacks (`BlockConnected()` vs `ChainStateFlushed()`) perform the syncing.
As a result, the index DB is temporarily in an inconsistent state, and if bitcoind is terminated uncleanly (so that there is no time to call `Commit()` by receiving an interrupt or by flushing the chainstate) this leads to problems:
On the next startup, `Init()` will read the best block and a MuHash that corresponds to a different (higher) block. Indexing will be picked up at the the best block processing some blocks again, but since MuHash is a rolling hash, it will process some utxos twice and the muhashes for all future blocks will be wrong, as was observed in #24076.
Fix this by always committing `DB_MUHASH` together with `DB_BEST_BLOCK`.
Note that the block data for the index is still written at different times, but this does not corrupt the index - at worst, these entries will be processed another time and overwritten after an unclean shutdown and restart.
ACKs for top commit:
ryanofsky:
Code review ACK 691d45fdc83ec14f87a400f548553168ac70263f. Only change since last review is adding test
fjahr:
ACK 691d45fdc83ec14f87a400f548553168ac70263f
Tree-SHA512: e1c3b5f06fa4baacd1b070abb0f8111fe2ea4a001ca8b8bf892e96597cf8b5d5ea10fa8fb837cfbf46648f052c742d912add4ce26d4406294fc5fc20809a0e1b
db27ac935480aeec40a1cfc9d5f10a48be55d61b tests: Ensure sorted/multi_a descriptors always generate different addrs (Andrew Chow)
Pull request description:
Sometimes the multi_a and sortedmulti_a descriptors will produce some of the same addresses in the tests. This causes the wallets to start generating addresses at a different index as they detect that one of the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique addresses by putting one of the multi_a in a different branch.
ACKs for top commit:
ajtowns:
ACK db27ac935480aeec40a1cfc9d5f10a48be55d61b
theStack:
Tested ACK db27ac935480aeec40a1cfc9d5f10a48be55d61b
Tree-SHA512: 0f57822bf4c7c79da304f092d7d43d6118e78a087cbeb0766fbbf634dc27911ae723d5d41350884d3b63a24d3b3817944f7e5fa534afb849161dd008a1e4a62f
7a68fe4831787a66986a76306180c7876ecba37f bitcoin-chainstate: Lock cs_main to UnloadBlockIndex (Carl Dong)
Pull request description:
This was introduced because of a silent merge conflict.
ACKs for top commit:
promag:
ACK 7a68fe4831787a66986a76306180c7876ecba37f
jonatack:
ACK 7a68fe4831787a66986a76306180c7876ecba37f
Tree-SHA512: 4c135efd68604452485a129e731675ff5917c157a70c77dd702211d9902c21b3b29380a881723f43ecba4762bc864b036881bb502b3b792e581565dcaa7a7ed4
7abd8b21ba34f6a42db2cd2d59a4c0e08561f609 doc: include wtxid in TransactionDescriptionString (brunoerg)
2d596bce6fefeff6033b21b391e81f1dda846937 doc: add wtxid info in release-notes (brunoerg)
a5b66738f19a0ad46d7bc287e0db08552c5a1fec test: add wtxid in expected_fields for wallet_basic (brunoerg)
e8c659a2970ec8855de3e1dbf91c6b614b8e5644 wallet: add wtxid in WalletTxToJSON (brunoerg)
7482b6f89500d9783cc6af0dccab7a08d0128206 wallet: add GetWitnessHash() (brunoerg)
Pull request description:
This PR add `wtxid` in `WalletTxToJSON` which allows to return this field in `listsinceblock`, `listtransactions` and `gettransaction` (RPCs).
ACKs for top commit:
achow101:
re-ACK 7abd8b21ba34f6a42db2cd2d59a4c0e08561f609
w0xlt:
crACK 7abd8b2
luke-jr:
re-utACK 7abd8b21ba34f6a42db2cd2d59a4c0e08561f609
Tree-SHA512: f86f2dbb5e38e7b19932006121802f47b759d31bdbffe3263d1db464f6a3a30fddd68416f886a44f6d3a9fd570f7bd4f8d999737ad95c189e7ae5e8ec1ffbdaa
fa097d074bc1afcc2a52976796bb618f7c6a68b3 addrman: Log too low compat value (MarcoFalke)
Pull request description:
Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.
In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.
ACKs for top commit:
mzumsande:
re-ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3
Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
31846b006da46852cfce91e53427dc8f871b3fda test: refactor: use `random.sample` for choosing random keys in wallet_taproot.py (Sebastian Falbesoner)
Pull request description:
The Python3 standard library method `random.sample` has the exact same functionality as the helper method `rand_keys(...)` (that is, random sampling without replacement) on a generic set or sequence, i.e. we can simply replace it. See https://docs.python.org/3/library/random.html#random.sample
Note that this is also safer: in case that the sample size `k` is larger than the population count, `random.sample` throws an error:
```
$ python3
Python 3.8.12 (default, Sep 26 2021, 13:12:50)
[Clang 11.1.0 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import random
>>> random.sample([23, 42], 3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/local/lib/python3.8/random.py", line 363, in sample
raise ValueError("Sample larger than population or is negative")
ValueError: Sample larger than population or is negative
```
while the custom method would get stuck in an endless loop.
ACKs for top commit:
shaavan:
Code Review ACK 31846b006da46852cfce91e53427dc8f871b3fda
Tree-SHA512: d9bd7f8128e43401a5b0388e48ba838155b21db5b4b6ba95c91285880788bc3917cb656b74bbe2d97faf7b44862d20b0899dc3c56aa48b9d2b33b50e37d089f6
Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).
Fix is a one-line change in ArgsManager::GetArg.
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
The macOS and Windows builds do not require a GCC 7 toolchain, and this
is actually causing build issues, i.e #24211. So switch to using a GCC
10 native toolchain for both.
956f7322f60db7b8be551c9074b4c633e514079d build: Bump minimum Qt version to 5.11.3 (Hennadii Stepanov)
e22d10b936eb7563b2b6611332d9e4c73a2f59d4 ci: Switch from bionic to buster (Hennadii Stepanov)
Pull request description:
The current minimum Qt version is 5.9.5 which has been set in bitcoin/bitcoin#21286.
Distro support:
- centos 7 -- unsupported since bitcoin/bitcoin#23511
- centos 8 -- [5.15.2](http://mirror.centos.org/centos/8/AppStream/x86_64/os/Packages/qt5-qtbase-5.15.2-3.el8.x86_64.rpm)
- buster -- [5.11.3](https://packages.debian.org/buster/libqt5core5a)
- bullseye -- [5.15.2](https://packages.debian.org/bullseye/libqt5core5a)
- _bionic_ -- [5.9.5](https://packages.ubuntu.com/bionic/libqt5core5a)
- focal -- [5.12.8](https://packages.ubuntu.com/focal/libqt5core5a)
As another Ubuntu LTS is coming soon, it seems unreasonable to stick to Qt 5.9 which support [ended](https://www.qt.io/blog/2017/06/07/renewed-qt-support-services) on 2020-05-31. Anyway, it's still possible to build Bitcoin Core GUI with depends on bionic system.
Bumping the minimum Qt version allows to make code safer and more reliable, e.g.:
- functor-parameter overload of [`QMetaObject::invokeMethod`](https://doc.qt.io/qt-5/qmetaobject.html#invokeMethod-4)
- fixed https://bugreports.qt.io/browse/QTBUG-10907
An example of the patch using the functor-overload of `QMetaObject::invokeMethod`:
```diff
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -349,7 +349,7 @@ bool WalletModel::changePassphrase(const SecureString &oldPass, const SecureStri
static void NotifyUnload(WalletModel* walletModel)
{
qDebug() << "NotifyUnload";
- bool invoked = QMetaObject::invokeMethod(walletModel, "unload");
+ bool invoked = QMetaObject::invokeMethod(walletModel, &WalletModel::unload);
assert(invoked);
}
```
It uses the same new syntax as signal-slot connection with compile-time check. Also see bitcoin/bitcoin#16348.
This PR is intended to be merged early [after](https://github.com/bitcoin/bitcoin/issues/22969) branching `23.x` off.
ACKs for top commit:
MarcoFalke:
cr ACK 956f7322f60db7b8be551c9074b4c633e514079d
fanquake:
ACK 956f7322f60db7b8be551c9074b4c633e514079d
Tree-SHA512: 3d652bcdcd990ce785ad412ed70234d4f27743895e535a53ed44b35d4afc3052e066c4c84f417e30bc53d0a3dd9ebed62444c57b7c765cb1e9aa687fbf866877
6833aceac97dd170970338f0bde5029f7d7aaa03 build: Move guix time machine to prelude (laanwj)
Pull request description:
This deduplicates some code, and enforces consistency of the time machine configuration between scripts.
ACKs for top commit:
achow101:
ACK 6833aceac97dd170970338f0bde5029f7d7aaa03
dongcarl:
ACK 6833aceac97dd170970338f0bde5029f7d7aaa03
Tree-SHA512: c02ded154cdb982293101986ef863d46554fc428eb5617bee0288dbef0543f994de5044123ac9958e455d0d24276a1c4512149a10dd1efaca8677c8f6b74b0a9
6c23c415613d8b847e6f6a2f872be893da9f4384 refactor: Rewrite AddToBlockIndex with try_emplace (Carl Dong)
c05cf7aa1e1c15089753897a10c14762027d4b99 style: Modernize range-based loops over m_block_index (Carl Dong)
c2a1655799c5d5dab9b14bd2a6b2d2296efd6964 style-only: Use using instead of typedef for BlockMap (Carl Dong)
dd79dad17545424d145e846026518d70da594380 refactor: Rewrite InsertBlockIndex with try_emplace (Carl Dong)
531dce034718523967808a89c18ba69a1e3e5a1f tests: Remove now-unnecessary manual Unload's (Carl Dong)
bec86ae32683ac56b4e6ba9c9b7d21cfbdf4ac03 blockstorage: Make m_block_index own CBlockIndex's (Carl Dong)
Pull request description:
Part of: #24303
Split off from: #22564
```
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
```
The second commit demonstrates how this makes calls to `Unload()` to satisfy the address sanitizer unnecessary.
ACKs for top commit:
ajtowns:
ACK 6c23c415613d8b847e6f6a2f872be893da9f4384
MarcoFalke:
re-ACK 6c23c415613d8b847e6f6a2f872be893da9f4384 🎨
Tree-SHA512: 81b2b5119be27cc0f8a9457b11da60cc60930315d2a5be36be89fe253d32073ffe622348ff153114b9b3212197bddbc791810913a43811b33cc58e7162bd105b
```bash
test3.c: In function 'main':
test3.c:6:21: warning: implicit declaration of function 'CoFreeUnusedLibrariesEx' [-Wimplicit-function-declaration]
6 | CoFreeUnusedLibrariesEx(0,0);
```
1b20109b04061304eab84cf921a030300b4f9fe3 Squashed 'src/leveldb/' changes from f8ae182c1e..330dd6235f (MarcoFalke)
Pull request description:
A minor change to:
* Consistently use the same symbol names in the whole project.
* Fix compiling with C++20.
ACKs for top commit:
fanquake:
ACK fa0c32eb74f448c67d0ff9d91dfdd66e2f1e4c48
Tree-SHA512: b5d4540dd621cf4aa8caac811bae03bb74e502a31dbdda9354182e4caa39905550e62ad3cf8ea7d7f9bfc3e5120d119d34ab0f1e633716ec8089876037cbf192
ae9ceed3e23288b163b7d7b1840b06b8d332f4ce validation, refactoring: remove ChainstateManager::Reset() (Jon Atack)
daad0093e3d1466789d0ce687902636c80cd74a1 validation: replace lock with annotation in UnloadBlockIndex() (Jon Atack)
Pull request description:
Thread safety refactoring seen in #24177:
- replace re-acquiring lock cs_main with a thread safety annotation in UnloadBlockIndex()
- remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing)
ACKs for top commit:
laanwj:
Code review ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
vasild:
ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
klementtan:
crACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
Tree-SHA512: cebb782572997cc2dda01590d6bb6c5e479e8202324d8b6ff459b814ce09e818b996c881736bfebd1b8bf4b6d7a0f79faf3ffea176a4699dd7d7429de2db2d13
Sometimes the multi_a and sortedmulti_a descriptors will produce some of
the same addresses in the tests. This causes the wallets to start
generating addresses at a different index as they detect that one of
the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique
addresses by putting one of the multi_a in a different branch.
a1db99adea36dbee1ec97ca1851edad12137feea init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack)
Pull request description:
including review feedback from https://github.com/bitcoin/bitcoin/pull/22834#discussion_r795253056 and https://github.com/bitcoin/bitcoin/pull/24205#discussion_r818629106 concerning `src/init.cpp`, `doc/tor.md` and `doc/i2p.md`
- s/outgoing/automatic outbound/
- s/Incoming/Inbound and manual/ (are not affected by this option.)
- s/only through network/only to network/
- s/this option. This option/this option. It/
- s/network types/networks/
and pick up a few nits in `doc/p2p-bad-ports.md` from https://github.com/bitcoin/bitcoin/pull/23542#pullrequestreview-881415043.
ACKs for top commit:
laanwj:
ACK a1db99adea36dbee1ec97ca1851edad12137feea
w0xlt:
ACK a1db99a
theStack:
ACK a1db99adea36dbee1ec97ca1851edad12137feea
Tree-SHA512: dd727904b9b3dadb16053e2b0350e6c0814ef68fb0cca7d34880b883123cfe3aa03b15813b40a863f6367d596d17ee4517eab55281cfe35cd00767b8a39593ca
68c4a9ed3885c26ac1e7a7296ca9693b3f8cde96 ci: Bump vcpkg to the latest version (Hennadii Stepanov)
Pull request description:
It seems reasonable to run a CI task against the most recent dependencies.
Dependency changes:
- boost 1.75.0 -> 1.78.0
- double-conversion 3.1.5 -> 3.2.0
- sqlite3 3.35.4 -> 3.37.2
ACKs for top commit:
fanquake:
ACK 68c4a9ed3885c26ac1e7a7296ca9693b3f8cde96
Tree-SHA512: 8d8ea42cb37b5eb2e6b82db4fd14b2984a1dee87a5d79e2378feff2e2576403207f5a27d3da7c4b351c1cc570ec8d971ae963c179789ef0ee55e004fbd399fe1
60aa179d8f9a675efa2d78eaadc09e3ba450f50f Use GetPathArg where possible (Pavol Rusnak)
5b946edd73640c6ecdfb4cbac1d4351e634678dc util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky)
687e655ae2970f2f13aca0267c7de86dc69be763 util: Add GetPathArg default path argument (Ryan Ofsky)
Pull request description:
Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options.
- Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values.
- Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated.
- Add unit tests for default and negated cases
- Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.
ACKs for top commit:
w0xlt:
Tested ACK 60aa179 on Ubuntu 21.10
hebasto:
re-ACK 60aa179d8f9a675efa2d78eaadc09e3ba450f50f
Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd
faa329fd46b4d688a5aa728b8b22a8d098987588 refactor: Release cs_main before MaybeSendFeefilter (MarcoFalke)
Pull request description:
There is no need for any lock to be held, because net processing is single threaded. So holding the validation lock cs_main for sending a feefilter is confusing and might even degrade blockchain-related RPC performance minimally.
ACKs for top commit:
ajtowns:
ACK faa329fd46b4d688a5aa728b8b22a8d098987588 ; code review only
vasild:
ACK faa329fd46b4d688a5aa728b8b22a8d098987588
Tree-SHA512: 3e7f9faff1631cc64c86fc1a354ada67617ad1e7a046625cc741f4711854eb41ca8aad5a51ef0d94ff65947b68dba8345c9f786b20ee0a8b7a2e8741cfced21f
