f6bb11fd37 Add test for ArgsManager::GetChainName (Russell Yanofsky)
4b331159df Add unit test NextString, ForEachNoDup functions (Russell Yanofsky)
05bfee3451 util_SettingsMerge test cleanup (Russell Yanofsky)
Pull request description:
There was some test coverage previously, but it was limited and didn't test conflicting and negated arguments.
ACKs for commit f6bb11:
MarcoFalke:
re-utACK f6bb11fd37f8a2c985786b688ea07699ba75780e
Tree-SHA512: d03596614dc48584c7a9440117b107c6abb23fd4c7fa15fb4015351ec3de08b2656bc956ce05310663675672343d7a6aff35421657f29172080c7005045680b0
151f3e9cf1 Add settings merge test to prevent regresssions (Russell Yanofsky)
Pull request description:
Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior.
ACKs for commit 151f3e:
jonasschnelli:
utACK 151f3e9cf1bbcf30a4fc7749682e66b4a73ddfc2.
MarcoFalke:
utACK 151f3e9cf1bbcf30a4fc7749682e66b4a73ddfc2
Tree-SHA512: f9062f078da02855cdbdcae37d0cea5684e82adbe5c701a8eb042ee4a57d899f0ffb6a9db3bcf58b639dff22b2b2d8a75f9a7917402df58904036753d65a1e3e
9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift)
Pull request description:
`LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise).
Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises.
This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`).
Issues like the one described in #16028 will be discovered immediately with this PR merged.
Changes in this PR:
* Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code)
* Move `LockAnnotation` in `wallet_tests` to make it reflect the truth
* Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`)
* Rename `LockAnnotation` to `LockAssertion`
ACKs for commit 9f85e9:
ryanofsky:
utACK 9f85e9cb3d687862128ddf464d2bc2462b8627f0. No changes at all since last review except clean rebase after base PR #16033 was merged
Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
f1a77b0c51 [docs] Add doxygen comment for CReserveKey (John Newbery)
37796b2dd4 [docs] Add doxygen comment for CKeyPool (John Newbery)
ef2d515af3 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery)
Pull request description:
Docs/move-only
Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393).
These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.
ACKs for commit f1a77b:
jonatack:
Thanks, John. Re-ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281, doc-only changes with respect to previous review.
jb55:
ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281
Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b
We overshoot (intentionally) and nBalanceToDenominate can become negative because of that. It was fine with raw loops but irange throws an exception here.
19324 follow-up (merged via 4714) - `AutoBackupWallet()` call was unreachable.
NOTE: It's safe to call it after `Verify()` because 18918 backport moved salvage logic out of `Verify*()`.
5100deee5822795d385570a380d3c117d05d851d clientversion: No suffix #if CLIENT_VERSION_IS_RELEASE (Carl Dong)
Pull request description:
```
Previously, building from a release source tarball would result in a
version string like v22.0.0-<commithash>, but we expect just v22.0.0.
This commit solves this problem.
Also use PACKAGE_VERSION instead of reconstructing it.
```
Fixes the underlying problem of #22623
ACKs for top commit:
achow101:
ACK 5100deee5822795d385570a380d3c117d05d851d
fanquake:
ACK 5100deee5822795d385570a380d3c117d05d851d - tested that prior the output of `src/bitcoind -version` on the `22.x` branch was `Bitcoin Core version v22.0.0-d3bd5410f64e`, and with this commit cherry-picked it is `Bitcoin Core version v22.0.0rc2`.
Tree-SHA512: 78705e285ff1271d5012e888837049044db4d11d66c252c6b964685892ef078c56fe122f12daa87c71532f4352f695d1e88a228665adcd7afe3ddce3f209b49f
d20d756752 rpc: faster getblockstats using BlockUndo data (Felix Weis)
Pull request description:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.
```
# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200 0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats 0.21s user 0.19s system 17% cpu 2.302 total
# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200 0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats 0.24s user 0.22s system 0% cpu 3:19.42 total
```
ACKs for commit d20d75:
MarcoFalke:
re-utACK d20d7567528e216badb8475df298bb3cec008985
Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
07cae5287c [wallet] remove unused GetScriptForMining (Sjors Provoost)
8bb3e4c487 [rpc] remove deprecated generate method (Sjors Provoost)
Pull request description:
As announced in v0.18, the wallet generate rpc method is deprecated and will be fully removed in v0.19.
Clients should transition to using the node rpc method `generatetoaddress`.
Tree-SHA512: 9e5e913b59f3e18440b2b7b356124c7b87ad19f81a1ab6ada06a6c396b84e734895465f569296f1ba8c12abf74863bab5fd77765c9e806c239713aa83a59485f
* wallet: Use temporary structure to update metadata correctly while generating new hd keys
* wallet: Make sure to never update an already existing key_origin while deriving hd keys
0da49b5 Skip precompute sighash for transactions without witness (Johnson Lau)
Pull request description:
This saves unnecessary hash caching for non-segwit transactions, but I am not sure if the difference is noticeable.
Tree-SHA512: 5cd733a729a52a45781510b3572b26e76837a94155caa14311c6d23a27a12e9613ff278dfc2592e21f640202782f22c5ad00fca85c4de5efacaa617c48ccb08d
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
e9440aeb5cad98fea9971f5126461e0a2b30ab54 build: use __SIZEOF_INT128__ for checking __int128 availability (fanquake)
Pull request description:
We already use this in the blockfilter code,
bf66e258a8/src/blockfilter.cpp (L34-L36)
so not sure we need to maintain two different ways of testing
for the same functionality. Consolidate on testing for `__SIZEOF_INT128__`,
which we already use, is supported by the compilers we care about, and is
also used by libsecp256k1.
ACKs for top commit:
sipa:
utACK e9440aeb5cad98fea9971f5126461e0a2b30ab54
Zero-1729:
crACK e9440aeb5cad98fea9971f5126461e0a2b30ab54
Tree-SHA512: 8aeef1734486a863b5091123bb5f9ba8868b1e2b4b35114586e3eb5862a38d4a1518ed069f37f41cb5e5ce2f6c87d95671996366d5ee990e0c90f268a8978ba3
8f7b93047581c67f2133cdb8c7845471de66c30f Drop the leading 0 from the version number (Andrew Chow)
Pull request description:
Removes the leading 0 from the version number. The minor version, which we had been using as the major version, is now the major version. The revision, which we had been using as the minor version, is now the minor version. The revision number is dropped. The build number is promoted to being part of the version number. This also avoids issues where it was accidentally not included in the version number.
The CLIENT_VERSION remains the same format as previous as previously, as the Major version was 0 so it never actually got included in it.
The user agent string formatter is updated to follow this new versioning.
***
Honestly I'm just tired of all of the people asking for "1.0" that maybe this'll shut them up. Skip the whole 1.0 thing and go straight to version 22.0!
Also, this means that the terminology we commonly use lines up with how the variables are named. So major versions are actually bumping the major version number, etc.
ACKs for top commit:
jnewbery:
Code review ACK 8f7b930475
MarcoFalke:
review ACK 8f7b93047581c67f2133cdb8c7845471de66c30f 🎻
Tree-SHA512: b5c3fae14d4c0a9c0ab3b1db7c949ecc0ac3537646306b13d98dd0efc17c489cdd16d43f0a24aaa28e9c4a92ea360500e05480a335b03f9fb308010cdd93a436
3ed8e3d079a3860dcdf944f7c1aa37765a53da32 doc: Remove explicit network name references (Fabian Jahr)
d6e493f0c2850b522a676a005935163beddaa2cc wallet: Remove left-over BIP70 comment (Fabian Jahr)
Pull request description:
A small follow-up to #17165 which removed BIP70 support.
1. Removes one leftover mention of BIP70 in a comment.
2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.
If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet.
ACKs for top commit:
MarcoFalke:
ACK 3ed8e3d079a3860dcdf944f7c1aa37765a53da32
Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee
Issues with current implementation: params list is not mentioning `baseBlockHashes`, `baseBlockHashesNb` looks excessive, no default values, handling of baseBlockHash-es is off by 1 (`3 + i` should be `4 + i`).
before:
```
> help quorum rotationinfo
quorum rotationinfo "blockRequestHash" baseBlockHashesNb extraShare
Get quorum rotation information
Arguments:
1. blockRequestHash (string, required) The blockHash of the request.
2. baseBlockHashesNb (numeric, required) Number of baseBlockHashes
3. extraShare (boolean, required) Extra share
```
after:
```
> help quorum rotationinfo
quorum rotationinfo "blockRequestHash" ( extraShare "baseBlockHash..." )
Get quorum rotation information
Arguments:
1. blockRequestHash (string, required) The blockHash of the request.
2. extraShare (boolean, optional, default=false) Extra share
3. baseBlockHash... (string, optional, default=) baseBlockHashes
```
84547fa6d408bdda1685f6d5972232bb19d97a7d Avoid creating a temporary vector for size-prefixed elements (Pieter Wuille)
Pull request description:
This is a simple improvement to the PSBT serialization code, avoiding the need for temporary vectors everywhere.
Tree-SHA512: 9f7243b7169ec8ba00ffad31af03c016ab84e4f76ebac810167f91f5e8008f3827ad59fbcee0cb2bd2334fc26466eb222404af24e7fb6ec040fd78229ebe0fd1
* Edge case fix
* Simpler syntax
* add a bit of documentation, and adjust scopes
* use a switch statment
* adjust how returning happens
Co-authored-by: pasta <pasta@dashboost.org>
fad140e311028f904635126e3c77352afac1b75e test: Set correct nValue for multi-op-return policy check (MarcoFalke)
Pull request description:
`CTxOut::nValue` is default-initialized to `-1`. The dust-threshold for `OP_RETURN` outputs is `0`. Thus, the policy failure would be `dust` instead of `multi-op-return`. The test only passes because the dust check is currently not run.
Avoid that confusion by setting the value to `0`, to ensure the dust check passes.
ACKs for top commit:
theStack:
ACK fad140e311028f904635126e3c77352afac1b75e
Tree-SHA512: f0c7a68eb2c573d6595b2b129fa8fa2a34fa35c17691f448bf1c54ccf66059c37562e7480cde7b51c4de677038d7717873da4257147a5f60acc8bbcd25fb7e3f
e95aaefe2540cb76969818fcc2ff77d33448ed5a build: Avoid secp256k1.h include from system (Niklas Gögge)
Pull request description:
While building i ran into an error because i had a version of `secp256k1.h` under `/usr/local/include` that was incompatible with the secp256k1 code in the repository. This caused a problem because `$(BOOST_CPPFLAGS)` contained `-I/usr/local/include` and the include paths are searched by the compiler in order from left to right, so in the end `$(BITCOIN_INCLUDES)` contained `-I/usr/local/include` before `-I$(srcdir)/secp256k1/include` which caused the compiler to find `secp256k1.h` under `/usr/local/include`.
Looking at git blame i am wondering how this has not happened to anyone else in several years: cb89e18845/src/Makefile.am (L25)
I am on macOS 10.15.
ACKs for top commit:
laanwj:
Code review ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a
hebasto:
ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a, tested on macOS 11 Big Sur by adding `#error` into `/usr/local/include/secp256k1.h`.
Tree-SHA512: 1f0b395725936c179ab60dee3582ec7b21e2f9c0f1895e160d84a487cf0db16d0c7aa47d05800e0aded31685b4362056cac9b9ecca1bb8c308a4c5a810e8dc1d
330cb33985d0ce97c20f4a0f0bbda0fbffe098d4 src/randomenv.cpp: fix build on uclibc (Fabrice Fontaine)
Pull request description:
Check for HAVE_STRONG_GETAUXVAL or HAVE_WEAK_GETAUXVAL before using
getauxval to avoid a build failure on uclibc
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
ACKs for top commit:
laanwj:
Code review ACK 330cb33985d0ce97c20f4a0f0bbda0fbffe098d4
Tree-SHA512: 94fbbdb0e859f0220d64b2d04565f575b410327f080125fec7fb74205d0bea0e8133561c83a696033d6dc377871133871b72c1aad19aca61e972ce67e0fdf707
bd5215103eb3985c1622eddea45a040e6173829c random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
Pull request description:
As shown below when resizing the read buffer `vData` `std::max((vData.size() * 3) / 2, nMaxSize)` is used. This means that the buffer size immediately jumps to `nMaxSize`. I believe the intend of this code is to grow the buffer size through several steps rather than immediately resize it to the max size.
```cpp
std::vector<unsigned char> vData(250000, 0);
long ret = 0;
unsigned long nSize = 0;
const size_t nMaxSize = 10000000; // Bail out at more than 10MB of performance data
while (true) {
nSize = vData.size();
ret = RegQueryValueExA(HKEY_PERFORMANCE_DATA, "Global", nullptr, nullptr, vData.data(), &nSize);
if (ret != ERROR_MORE_DATA || vData.size() >= nMaxSize)
break;
vData.resize(std::max((vData.size() * 3) / 2, nMaxSize)); // Grow size of buffer exponentially
}
```
vData always starts at size 250,000 and nMaxSize is always 10,000,000 so the first time this line is reached:
```cpp
vData.resize(std::max((vData.size() * 3) / 2, nMaxSize));
```
the effect will always be to resize vData to nMaxSize. Then because the loop terminates when vData.size >= 10,000,000 only one resize operation will take place.
To fix this issue we replace `std::min` with `std::max`
This PR also adds a comment clarifying the behavior of this function the first time it is called.
ACKs for top commit:
fanquake:
ACK bd5215103eb3985c1622eddea45a040e6173829c - thanks for taking a look at this Ethan. Swapping from `std::max` to `std::min` here certainly seems correct.
Tree-SHA512: 7c65f700e5bbe44bc2f1ffdcdc99ec19c542894c95b5ee9791facd09d02afae88d1f8f35af129719e4860db94bc790856e7adb1d218a395381e7c2913b95f1d0
e90e3e684ffa7b25f0dfb5b45e70bb0c358261fb build: fix sysctl() detection on macOS (fanquake)
Pull request description:
[`sysctl()` on *BSD](https://www.unix.com/man-page/FreeBSD/3/sysctl/) takes a "const int *name", whereas [`sysctl()` on macOS](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/sysctl.3.html)
it takes an "int *name". So our configure check and `sysctl()` detection on
macOS currently fails:
```bash
/usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
no known conversion from 'const int [2]' to 'int *' for 1st argument
int sysctl(int *, u_int, void *, size_t *, void *, size_t);
```
The simplest change seems to be to change the param to a "int *name", which
will work during configure on macOS and *BSD systems.
For consistency I've changed both calls, but note that macOS doesn't
have `KERN_ARND`, so that check will always fail regardless. We can revert/add
documentation if preferred.
ACKs for top commit:
laanwj:
Re-ACK e90e3e684ffa7b25f0dfb5b45e70bb0c358261fb
Tree-SHA512: 29e9348136fc72882f63079bf10d2490e845d7656aae2c003e282bea49dd2778204a7776a67086bd88c2852af9a07dd04ba358eede7e37029e1c10f73c85d6a5
d36146009fb3fc9b9a772823b4df139a85173481 Drop unused mach time headers (Ben Woosley)
Pull request description:
Now that we're no longer special-casing clock usage for MacOS (see #17800), we're
not referencing anything defined in these headers.
Incidentally, this removes our last reference to the `__MACH__` system def. 🎉
ACKs for top commit:
jonasschnelli:
utACK d36146009fb3fc9b9a772823b4df139a85173481
fanquake:
ACK d36146009fb3fc9b9a772823b4df139a85173481 - thanks.
Tree-SHA512: 246045b0683a705ad034416e8ace2024e652026a6c0517b6797320e52fc18a6e111ec2e405ca40653bd1d6421bb7755232e8fec22651fff8e448eb7d5646a954
dc9305b6162ec615ff5fb2876e4f312051b543af random: don't special case clock usage on macOS (fanquake)
Pull request description:
`clock_gettime()`, `CLOCK_MONOTONIC` and `CLOCK_REALTIME` are all available for use on
macOS (now that we require macOS >=10.12 and build against 10.14). Use them rather than the [deprecated](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KernelProgramming/Mach/Mach.html) `mach_timespec_t` time API.
I mentioned the possibility for this change [in #17270](https://github.com/bitcoin/bitcoin/pull/17270#discussion_r346090606).
[master](1dbf3350c683f93d7fc9b861400724f6fd2b2f1d):
```bash
2019-12-23T20:49:43Z Feeding 216 bytes of dynamic environment data into RNG
2019-12-23T20:50:43Z Feeding 216 bytes of dynamic environment data into RNG
```
This PR:
```bash
2019-12-23T20:32:41Z Feeding 232 bytes of dynamic environment data into RNG
2019-12-23T20:33:42Z Feeding 232 bytes of dynamic environment data into RNG
```
~~Depends on #16392.~~ Merged.
ACKs for top commit:
laanwj:
ACK dc9305b6162ec615ff5fb2876e4f312051b543af
Tree-SHA512: 18c2f336ea628f9cf7339b817381d230a18893fd9c0351bf99a39ca6f45c5b0a20af9d599d48d6c09515627d5edafa91337c17f9f790264251d2cdcb3763bbd5
55b2cb199c276781b6daa5438af2da57dea3ac52 random: mark RandAddPeriodic and SeedPeriodic as noexcept (fanquake)
461e547877da0c04db69e067c923cc4540aab03a doc: correct random.h docs after #17270 (fanquake)
Pull request description:
The usage of `MilliSleep()` in SeedPeriodic (previously SeedSleep) was
[removed](d61f2bb076) in #17270, meaning it, and its users can now be marked `noexcept`.
This also corrects the docs in random.h for some of the changes in #17270.
ACKs for top commit:
practicalswift:
ACK 55b2cb199c276781b6daa5438af2da57dea3ac52
laanwj:
ACK 55b2cb199c276781b6daa5438af2da57dea3ac52
sipa:
ACK 55b2cb199c276781b6daa5438af2da57dea3ac52
Tree-SHA512: 672d369796e7c4f9b4d98dc545e5454999fa1bef373871994a26041d6163c58909e2255e4f820d3ef011679aa3392754eb57477306a89f5fd3d57e2bd7f0811a
f93fc61c65d605eae2d3e2c98bdd30ae587fcdab Put bounds on the number of CPUID leaves explored (Pieter Wuille)
ba2c5fe1477cec80d7e02f824daba21a1021758e Fix CPUID subleaf iteration (Pieter Wuille)
Pull request description:
This fixes#17523.
The code to determine which CPUID subleaves to explore was incorrect in #17270. The new code here is based on Intel's reference documentation for CPUID (a document called "Intel® Processor Identification and the CPUID Instruction - Application Note 485", which I cannot actually find on their own website).
ACKs for top commit:
laanwj:
ACK f93fc61c65d605eae2d3e2c98bdd30ae587fcdab
jonatack:
ACK f93fc61c65d605eae2d3e2c98bdd30ae587fcdab code review, tested rebased on current master bb862d7 with Debian 4.19 x86_64
mzumsande:
ACK f93fc61, reviewed code and compared with the intel doc, tested on an AMD and an Intel processor.
Tree-SHA512: 2790b326fa397b736c0f39f25807bea57de2752fdd58bf6693d044b8cb26df36c11cce165a334b471f8e33724f10e3b76edab5cc4e0e7776601aabda13277245
* compat: remove bswap_* check on macOS
This was originally added in #9366 to fix the gui build, as
Protobuf would also define these macros. Now that we're no-longer
using Protobuf, remove the additional check.
* build: skip building OpenSSL lib_ssl
* build: remove OpenSSL from Qt build
More info available from:
https://doc.qt.io/qt-5/ssl.html#enabling-and-disabling-ssl-support
* build: remove EVP_MD_CTX_new detection
This was added in #9475 to fix LibreSSL compatibility for
BIP70, so is no longer required.
* build: remove SSL lib detection
* gui: update BIP70 support message
* build: remove BIP70 entries from macOS Info.plist
* gui: remove payment request file handling from OpenURI dialog
* gui: remove BIP70 Support
* build: remove protobuf from depends and contrib
Decided to also apply the same logic to the other items so that we don't do nullptr dereferences
replication
```
./src/qt/dash-qt --regtest --resetguisettings
Enable governance
shutdown
```
* refactor(instantsend): make cs_db a Mutex
This introduces GetInstantSendLockByHashInternal and GetInstantSendLockHashByTxidInternal as they are called in Locked and Unlocked contexts. The Internal functions do not lock cs_db, the non-internal functions simply lock cs_db then call the internal function. This ensures saftety (all public functions lock cs_db immediately & all private functions have cs_db already locked) enforced via clang thread safety, while allowing us to use a Mutex here
* refactor(instantsend): remove CInstantSendManager::cs, replace it with cs_inputReqests, cs_creating, cs_pendingLocks, cs_nonLocked, cs_pendingRetry
LOCKS_EXCLUDED are used everywhere where the associated mutex is locked ensuring that deadlocks are impossible
* clang-tidy: avoid implicit conversion to bool
* clang-tidy: pointer / reference adjustments
* clang-tidy: avoid implicit conversion to bool
* clang-tidy: use braces for if
* clang-tidy: don't use else after return
* clang-tidy: mark auto stuff as pointers / const pointers
* clang-tidy: make static
* clang-tidy: avoid implicit conversion to bool
* clang-tidy: use emplace back
* clang-tidy: avoid implicit conversion to bool, and use WITH_LOCK
* use WITH_LOCK
* clang-tidy: avoid implicit conversion to bool
* clang-tidy: use more const / * with auto
* refactor: adjust formatting, and move IsNull to .h
* refactor: remove unused functions
* Update src/llmq/clsig.h
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* fix(instantsend): avoid an iterator invalidation in pendingInstantSendLocks handling
this also removes the following, as it doesn't take into account deterministic / not deterministic
```
if (pendingInstantSendLocks.size() <= maxCount) {
pend = std::move(pendingInstantSendLocks);
}
```
Also uses structured bindings
* suggestions
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
275e9390e1c84ac021b3c781ee239ad9ba7b78d4 mining, refactor: add m_mempool.cs thread safety lock assertions (Jon Atack)
Pull request description:
in src/node/miner to
- BlockAssembler::addPackageTxs()
- BlockAssembler::SkipMapTxEntry()
- BlockAssembler::UpdatePackagesForAdded()
These functions have thread safety lock annotations in their declarations but are missing the corresponding run-time lock assertions in their definitions.
Per doc/developer-notes.md: "Combine annotations in function declarations with run-time asserts in function definitions."
ACKs for top commit:
shaavan:
ACK 275e9390e1c84ac021b3c781ee239ad9ba7b78d4. Thanks for catching and fixing this!
Tree-SHA512: 1c6f1ad1bbd94ff391fc8ce1e3b95d88bd3db5db804a1a5ef4636e54b29f5801f79aa9ed753d34c9a79a58cf01c7ed890e7681ff1c7b0f16335dc062bbac31cc
fab2527515e8db944ae044bea8580e2cd9414bcd test: Disable mockforward scheduler unit test for now (MarcoFalke)
Pull request description:
This should be a workaround to fix#18174 in the short run and buy us more time to investigate the issue while ci runs are green again 🙏
ACKs for top commit:
fanquake:
ACK fab2527515e8db944ae044bea8580e2cd9414bcd - be good to get Travis back.
laanwj:
ACK fab2527515e8db944ae044bea8580e2cd9414bcd
Tree-SHA512: 027e86b3dfec203a464e5bf528e9933c208c36633c2d4bfcdbc10da1799637a5d6ea0a63af33a4174fb1ad7115df631a4cb838f56e31f4cbd15498e1e9fdf9cc
9dd58ca611f6f2b59c25d727a4e955333525d345 init: Stop indexes on shutdown after ChainStateFlushed callback. (Jim Posen)
Pull request description:
Replaces https://github.com/bitcoin/bitcoin/pull/17852.
Currently, the latest index state may not be committed to disk on shutdown. The state is committed on `ChainStateFlushed` callbacks and the current init order unregisters the indexes as validation interfaces before the final `ChainStateFlushed` callback is called on them.
Issue identified by paulyc.
For review: an alternative or supplemental solution would be to call `Commit` at the end of `BaseIndex::Stop`. I don't see any harm in doing so and it makes the less prone to user error. However, the destructor would have to be modified to not call `Stop` because `Commit` calls a virtual method, so I figured it wasn't worth it. But I'm curious how others feel.
ACKs for top commit:
fjahr:
tested ACK 9dd58ca611f6f2b59c25d727a4e955333525d345
paulyc:
> Code review ACK [9dd58ca](9dd58ca611), but failed to test because I can't reproduce the original problem.
kallewoof:
Tested ACK 9dd58ca611f6f2b59c25d727a4e955333525d345
promag:
Code review ACK 9dd58ca611f6f2b59c25d727a4e955333525d345, but failed to test because I can't reproduce the original problem.
Tree-SHA512: 2918380b699833cb7eab07456d1667dbf8ebbe2d2b5988300a3cf5b6a6cfc818b6d9086e1936ffe7881f67e409306c4b91d61a08a169cfd0a301383479d4f3cb
-BEGIN VERIFY SCRIPT-
sed -i 's/\<strCommand\>/msg_type/g' src/coinjoin/client.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/coinjoin/client.h
sed -i 's/\<strCommand\>/msg_type/g' src/coinjoin/server.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/coinjoin/server.h
sed -i 's/\<strCommand\>/msg_type/g' src/evo/mnauth.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/evo/mnauth.h
sed -i 's/\<strCommand\>/msg_type/g' src/governance/governance.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/governance/governance.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/blockprocessor.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/blockprocessor.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/chainlocks.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/chainlocks.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/dkgsessionhandler.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/dkgsessionhandler.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/dkgsessionmgr.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/dkgsessionmgr.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/instantsend.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/instantsend.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/quorums.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/quorums.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/signing.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/signing.h
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/signing_shares.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/llmq/signing_shares.h
sed -i 's/\<strCommand\>/msg_type/g' src/masternode/sync.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/masternode/sync.h
sed -i 's/\<strCommand\>/msg_type/g' src/net_processing.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/spork.cpp
sed -i 's/\<strCommand\>/msg_type/g' src/spork.h
-END VERIFY SCRIPT-
39393479c514f271c42750ffcd0adc6bc1db2e2f p2p: pass strings to NetPermissions::TryParse functions by const ref (Jon Atack)
Pull request description:
instead of by value, as these are "in" params that are not cheap to copy.
Reference: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const
ACKs for top commit:
MarcoFalke:
cr ACK 39393479c514f271c42750ffcd0adc6bc1db2e2f
Tree-SHA512: 294fe0f2d900293b4447d4e1f0ccc60c1ed27b3bdbd0f5d71d3dbf71de86879638b1b813fadfb44c58b4acff4e7d75b7ed6a4f9cc5fcf627108224e6a21b524c
f1a0314c537791f202dfb7c1209f0e04ba7988c3 gui: change combiner for signals to optional_last_value (Cory Fields)
Pull request description:
[`optional_last_value`](https://www.boost.org/doc/libs/1_73_0/doc/html/boost/signals2/optional_last_value.html), which does not throw, has replaced `last_value` as
Boosts default combiner. Besides being better supported, it also doesn't
trigger gcc's `-Wmaybe-unitialized` warning, presumably because exceptions no
longer bubble-up out of signals:
```bash
In file included from ui_interface.cpp:9:
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
```
The change in default happened in [Boost 1.39.0](https://www.boost.org/users/history/version_1_39_0.html) (along with the introduction of the Signals2 library.
More information is also available here https://www.boost.org/doc/libs/1_73_0/doc/html/signals2/rationale.html#id-1.3.36.9.4:
> The default combiner for Boost.Signals2 has changed from the last_value combiner used by default in the original Boost.Signals library.
> This is because last_value requires that at least 1 slot be connected to the signal when it is invoked (except for the last_value<void> specialization).
> In a multi-threaded environment where signal invocations and slot connections and disconnections may be happening concurrently, it is difficult to fulfill this requirement. When using optional_last_value, there is no requirement for slots to be connected when a signal is invoked, since in that case the combiner may simply return an empty boost::optional.
ACKs for top commit:
laanwj:
ACK f1a0314c537791f202dfb7c1209f0e04ba7988c3
Tree-SHA512: 3600f85019a3591b141dc9207f8a7e66d16d9996cf97fdf08f5133a212d55c591955ab835ffbdca20b5d62711578bc305d5525c75546fa957f180192e2a80c1e
b83cc0fc94df99f0334430e63e8c9fa6ae3790e1 Fix link error with --enable-debug (Hennadii Stepanov)
Pull request description:
Fixes a link error on master (39bd9ddb8783807b9cde6288233e86ad7c85d61f):
```
$ ./configure --enable-debug
$ make
...
bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x0): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet_tool.a(libbitcoin_wallet_tool_a-wallettool.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-salvage.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-walletdb.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): more undefined references to `InitError(bilingual_str const&)' follow
collect2: error: ld returned 1 exit status
```
See:
- https://github.com/bitcoin/bitcoin/pull/19295#issuecomment-645471771
- https://github.com/bitcoin/bitcoin/pull/19295#issuecomment-645487182
ACKs for top commit:
achow101:
Re-ACK b83cc0fc94df99f0334430e63e8c9fa6ae3790e1
Tree-SHA512: f563d978b6725284049449bb0b3a184d356f32e9b63bcadb0ba71352d3d521af3dbb4a7b4fc0a5a620ed99c357e59f62249c10d0defc0cbe7775f2c06791dabe
* Added GET_SNAPSHOT_INFO message handling
* Quorum members by rotation
* Quorum utils functions
* Handle GET_QUORUM_ROTATION_INFO with baseBlockHash from client
* Storing QuorumSnaphots in evoDB when requesting them
* Added DIP Enforcement param
* quorumIndex cache
* Quorum Rotation deployment control
* Usage of Bitsets for storing CQuorumSnapshots
* Correct handling of early quorum quarters
* More asserts
* Corrections
* Handling of quorumIndex
* Refactoring of truncate mechanism
* Various fixes
* Interface correction
* Added template type for indexed cache
* Added quorumIndex into commitmenHash
* Various changes
* Needs to update maqQuorumsCache along with indexedQuorumsCache
* Added CFinalCommitment version 2
* Renamed variables
* Fixes
* Refactoring & correct caching of quorumMembers by rotation
* Added assertions
* Refactoring
* Interface change
* Handling of previous DKG session failure
* Applied refactoring
* Build quarter members improvments
* Merge Quorum Rotation and Decreased fee into one deployment (DIP24)
* Added new LLMQ Type
* Added functional tests + refactoring
* Refactoring
* Spreaded Quorum creation and Quorum Index adaptation
* quorumIndex adaptations
* Added quorumIndex in CFinalCommitment
* Latest work
* Final refactoring
* Batch of refactoring
* Fixes for tests
* Fix for CFinalCommitment
* Fix for Quorums
* Fix
* Small changes
* Thread sync fic
* Safety changes
* Reuse mns when needed
* Refactoring
* More refactoring
* Fixes for rotationinfo handling
* Fix for rotation of members
* Correct order of MNs lists in Quorum Snapshots
* Adding extra logs
* Sync rotation quorums + qrinfo changes
* Fix + extra logs
* Removed redundant field
* Fix for null final commitment + refactoring
* Added timers in tests
* Fix for qrinfo message: quorumdiff and merkleRootQuorums
* Small changes for rotation test
* Remove reading from scanQuorumCache
* Added quorum list output
* Crash fix
* Experimental commit
* apply changes to specialtxman.cpp from specialtx.cpp
* all the changes
* substancially speed up feature_llmq_rotation.py
* reenable asserts, add check for reorgs
* Refactoring
* Added extra logs
* format
* trivial
* drop extra boost includes
* drop ContainsMN
* fix ScanQuorums
* check quorum hash and index in CFinalCommitment::Verify
* fix/tweak tests
* IsQuorumRotationEnabled should be aware of the context
* Calculating members based on earlier block.
* Fix for Quorum Members Cache
* Removed duplicate size of baseBlockHashes
* Adaptations of qrinfo to -8 mn lists
* Introduction of llmqTypeDIP24InstantSend
* Adaptation for llmqTypeDIP24InstantSend
* Adaptations for IS
* bump protocol version
* Added feature_llmq_is_migration test
* Various cleanups
* use unordered_lru_cache for quorumSnapshotCache
* trivial refactor ComputeQuorumMembersByQuarterRotation
* Reduced CFinalCommitment::quorumIndex from 32 to 16 bits
* Keep verified LLMQ relay connections
* Experimental Relay connection fix
* Fix for EnsureQuorumConnections rotation
* Using only valid Mns for checking
* Override of nPowTargetSpacing (devnet only)
* Show penalty score in masternode rpc
* fixups
* Rotation refactoring
* Update src/chainparams.cpp
* Replaced LogPrintf with LogPrint
* IS locking fix once DIP24 activation
* Various cleanup
* Updated MIN_MASTERNODE_PROTO_VERSION
* Introduce LLMQ_TEST_INSTANTSEND reg-test only quorum and actually test switching to dip0024 quorums
* Renamed field lastQuorumHashPerIndex
* Renamed to DIP0024
* chore: update nStartTime and nTimeout for mainnet / testnet for DEPLOYMENT_DIP0024
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
c1c6c410a66996b2d60d5172189b5a5ec8100842 test: add reason checks for non-standard txs in test_IsStandard (Sebastian Falbesoner)
Pull request description:
While taking a look at #17272 I noticed that for some reason the unit test `test_IsStandard` (which was not adapted to the policy change in the referenced PR commits) didn't fail as expected:
6a97e8a060/src/test/transaction_tests.cpp (L758-L762)
It turned out that `IsStandardTx()` returned `"dust"` as rejection reason (instead of the expected `"multi-op-return"`), leading to the conclusion that 5fe6f052bd erroneously performs the `IsDust()` check also for TX_NULL_DATA transactions. To avoid cases like this in the future, this PR makes the unit test `test_IsStandard` more strict by also checking for the concrete reason after each occurence of `IsStandardTx()` returning false.
ACKs for top commit:
instagibbs:
utACK c1c6c410a6
Tree-SHA512: c7419884cc52977c73f8f8c476eaebed80ba7bda4d03509d3f46dd977be911389f7b53daefa5ef31d2f7df9402243152e01e83f1b8a9fb300c19d1a0f69a89a9
