e5485e8e4be7f2ee0671f58c3dcce35c68ba0ee0 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee509025d92db5311c3f5df3b61c09cad24f validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
bef61496ab5e12e38ac5794cd0836723af070ab5 test: compare `/mempool/contents` response with `getrawmempool` RPC (brunoerg)
5bc5cbaf310f60e89c72e8ecf3f6187c85499027 doc: add reference to `getrawmempool` RPC in `/mempool/contents` REST doc (brunoerg)
Pull request description:
This PR is similar to #24797, it compares `/mempool/contents` REST response with `getrawmempool` RPC (verbose=True) since they use the same `MempoolToJSON` function.
Also, adds a reference to `getrawmempool` RPC help to get details about the fields from `/mempool/contents`.
ACKs for top commit:
0xB10C:
ACK bef6149
Tree-SHA512: b7e9e9c765ee837986ba167b9234a9b95c9ef0a9ebcc2a03d50f6be6d3aba1480bd77c78111d95df1e4023cde6dfc64bf1e7908d9e5b6f96ca46b76611a4a9b4
fad6d4f952373690ef16ce27b0926c0ab762066a Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb2287b2a1a4eb7224b424f12fe59302e9 Remove duplicate static_asserts (MarcoFalke)
Pull request description:
No need to go from `arith_uint256`->`uint256` when a `uint256` can be constructed right away.
ACKs for top commit:
laanwj:
Code review ACK fad6d4f952373690ef16ce27b0926c0ab762066a
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
3ec6504a2e5b4afb7a2719a82191e0b96fe23214 qt: Do not use `QKeyEvent` copy constructor (Hennadii Stepanov)
Pull request description:
This PR is preparation for [Qt 6](https://github.com/bitcoin/bitcoin/pull/24798), and it fixes an experimental build with Qt 6.2.4 as copying of `QEvent` has been [disabled](19f9b0d5f5) in Qt 6.0.0.
ACKs for top commit:
w0xlt:
tACK 3ec6504a2e on Ubuntu 21.10, Qt 5.15.2
shaavan:
reACK 3ec6504a2e5b4afb7a2719a82191e0b96fe23214
Tree-SHA512: 583a9dad0c621d9f02f77ccaa9f55ee79e12e3c47f418911ef2dfe0de357d772d1928ae3ec19b6f0c0674da858bab9d4542a26cc14b06ed921370dfeabd1c194
99a6b699cd650f13d7200d344bf5e2d4b45b20ac Fix race condition for SetBannedSetDirty() calls (Hennadii Stepanov)
83c76467157bbca023bffda0f0bc2f01eb76a040 Avoid calling BanMan::SweepBanned() twice in a row (Hennadii Stepanov)
33bda6ab87cc1b569e96da337296eb3e9ce6db1a Fix data race condition in BanMan::DumpBanlist() (Hennadii Stepanov)
5e20e9ec3859205c220867ca49efb752b8edaacc Prevent possible concurrent CBanDB::Write() calls (Hennadii Stepanov)
Pull request description:
This PR split from bitcoin/bitcoin#24097 with some additions. This makes the following switch from `RecursiveMutex` to `Mutex` a pure refactoring.
See details in commit messages.
ACKs for top commit:
w0xlt:
reACK 99a6b69
shaavan:
ACK 99a6b699cd650f13d7200d344bf5e2d4b45b20ac
Tree-SHA512: da4e7268c7bd3424491f446145f18af4ccfc804023d0a7fe70e1462baab550a5e44f9159f8b9f9c7820d2c6cb6447b63883616199e4d9d439ab9ab1b67c7201b
f13e642c831c5689cb2bb7f5c4f9cb4c0c03ef21 build: Disable valgrind when building zeromq package in depends (Hennadii Stepanov)
b970f03beae0f3ae6a796f0e3b97732fc579f6aa build: Disable libbsd when building zeromq package in depends (Hennadii Stepanov)
77899991b1e29a45bc377b21330148cb7cc08923 build: Update netbsd_kevent_void.patch (Hennadii Stepanov)
Pull request description:
Since v4.3.3 (068385c951) `libzmq` uses `libbsd` by default.
This PR disables `libbsd` explicitly, as it's not a part of our depends. Zeromq will fallback to its internal `strlcpy` implementation.
Otherwise, on systems with installed `libbsd-dev` package the `zeromq` package build system erroneously detects `libbsd` package from the host system:
```diff
--- a/libzmq.pc
+++ b/libzmq.pc
@@ -8,5 +8,5 @@
Version: 4.3.4
Libs: -L${libdir} -lzmq
Libs.private: -lpthread
-Requires.private:
+Requires.private: libbsd
Cflags: -I${includedir}
```
This causes the `configure` fails to detect the `zeromq` package:
```
configure: WARNING: libzmq version 4.x or greater not found, disabling
```
---
Other minor improvements:
- fixed `netbsd_kevent_void.patch` offset
- disabled valgrind as it's used in unit tests which we do not run:
```diff
--- a/zmq-configure-output
+++ b/zmq-configure-output
@@ -119,11 +119,6 @@
checking whether the g++ -m64 linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
checking dynamic linker characteristics... (cached) GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
-checking for valgrind... valgrind
-checking for Valgrind tool memcheck... memcheck
-checking for Valgrind tool helgrind... helgrind
-checking for Valgrind tool drd... drd
-checking for Valgrind tool exp-sgcheck... exp-sgcheck
checking linker version script flag... --version-script
checking if version scripts can use complex wildcards... yes
checking for working posix_memalign... yes
```
ACKs for top commit:
fanquake:
ACK f13e642c831c5689cb2bb7f5c4f9cb4c0c03ef21
Tree-SHA512: d4c86d4a841eb6e7c32157e84972243072f905496c2a4c14ec6f6ab4216df6695cbf29baa2233ce27eaede35d1e250ad2f9975b16f570d01509f0c5da4597cad
6897c4bdf51a4aa74320ebfffa9467db14670109 build: patch depends zeromq to fix building on NetBSD Current (fanquake)
ce6dd2f1a2e2c56d86d00e0eeec34c9982017416 zeromq 4.3.4 (fanquake)
Pull request description:
This is a prerequisite for #23955. It updates zeromq to the latest available version, and adds a patch, [that I've sent upstream](https://github.com/zeromq/libzmq/pull/4326), to fix building on NetBSD Current (10).
ACKs for top commit:
hebasto:
ACK 6897c4bdf51a4aa74320ebfffa9467db14670109, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d05d9753630faebe842e1ca70c8c4af660a38e7331a9d95e84df3a3b14564c5118ca41c4fc49fb71dfee563b63e1014e5a3f8874d652e26de59e8e188a12970e
c62d763fc313585d79ad833c9d729f6acf2652aa Necessary improvements to make configure work without libevent installed (Perlover)
091ccc38c2e589b649648cbcc99aca4802f98775 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover)
Pull request description:
The second parameter of evhttp_connection_get_peer in libevent already has type as `const char **`
The compilation of bitcoind with the fresh libevent occurs errors
Details: https://github.com/bitcoin/bitcoin/issues/23606
ACKs for top commit:
laanwj:
Code review ACK c62d763fc313585d79ad833c9d729f6acf2652aa
luke-jr:
tACK c62d763fc313585d79ad833c9d729f6acf2652aa
Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83
64e1ddd255771e57a88a20f07dbde04a83bf0c75 log: call LogPrint only once with time data samples (Martin Zumsande)
Pull request description:
When timedata samples are logged, `LogPrint()` is currently invoked multiple times on the same log entry.
This can lead to chaos in the log when other threads log concurrently, as in this example which motivated this PR:
```
2021-09-20T00:28:57Z -48 -26 -11 -8 -6 Addrman checks started: new 37053, tried 83, total 37136
2021-09-20T00:28:57Z -3 -1 -1 -1 -1 +0 | nTimeOffset = -3 (+0 minutes)
```
Fix this by building the log message in a string and logging it one `LogPrint()` call. I also changed the wording slightly so that it becomes understandable what is being logged, example:
```
2021-09-21T21:03:24Z time data samples: -43 -18 -12 -4 -1 -1 +0 +0 +268 | median offset = -1 (+0 minutes)
```
ACKs for top commit:
jnewbery:
Tested ACK 64e1ddd255
laanwj:
Tested ACK 64e1ddd255771e57a88a20f07dbde04a83bf0c75, new message lgtm
Tree-SHA512: ffb7a93166cc8fd6a39200b9e03a9d1e8e975b7ded822ccddd015f553258b991162a5cb867501f426d3ebcfef4f32f0e06e17b18e6b01486b967595d102f8379
cc3044ccdbefa9fae58d1762477e377883b39c5e fix misleading comment about call to non-existing function (pox)
Pull request description:
The comment seems to be describing the subsequent call to `SyncTransaction` but refers to it as `SyncNotifications`, which is not any function currently in the codebase.
It's best to just remove the "what" aspect of the comment and focus on the "why", which also reduces the risk of similar documentation errors in the future, in case the function ever gets renamed, for example.
ACKs for top commit:
laanwj:
ACK cc3044ccdbefa9fae58d1762477e377883b39c5e
Tree-SHA512: 882ff17836ef585a603dc504f3dd21f56f682e49b28a0998f23fd16025826fbb083b7978db3ee70d0e0ff2c86fd6c3fd99a2361e5d45c765fdc5822c5f14c0a7
198fff88f385e090b57a0ee902719bcc22a6b86b GUI: Define MAX_DIGITS_BTC for magic number in BitcoinUnits::format (Luke Dashjr)
Pull request description:
A magic number snuck in with https://github.com/bitcoin/bitcoin/pull/16432
ACKs for top commit:
hebasto:
ACK 198fff88f385e090b57a0ee902719bcc22a6b86b, I have reviewed the code and it looks OK, I agree it can be merged.
kristapsk:
utACK 198fff88f385e090b57a0ee902719bcc22a6b86b
Tree-SHA512: 78dc23c2ae61bac41e5e34eebf57274599cb2ebb0b18d46e8a3228d42b256a1bc9bb17091c748f0f692ef1c4c241cfbd3e30a12bcd12222a234c1a9547ebe786
2ea62cae483b764e30f61c06d8ac65755bbd864c Improve docs about feeler connections (Gleb Naumenko)
Pull request description:
"feeler" and "test-before-evict" are two different strategies suggest in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://www.usenix.org/system/files/conference/usenixsecurity15/sec15-paper-heilman.pdf). In our codebase, we use `ConnType::FEELER` to implement both.
It is confusing, up to the point that our documentation was just incorrect.
This PR:
- ~clarifies this aspect by renaming "ConnType::FEELER" to "ConnType::PROBE", meaning that this connections only probes that the node is operational, and then disconnects.~
- fixes the documentation
ACKs for top commit:
amitiuttarwar:
ACK 2ea62cae48. thank you!
practicalswift:
ACK 2ea62cae483b764e30f61c06d8ac65755bbd864c
Tree-SHA512: c9c03c09eefeacec28ea199cc3f697b0a98723f2f849f7a8115edc43791f8165e296e0e25a82f0b5a4a781a7de38c8954b48bf74c714eba02cdc21f7460673e5
## Issue being fixed or feature implemented
Client version string is inconsistent. Building `v20.0.0-beta.8` tag
locally produces binaries that report `v20.0.0-beta.8` version but
binaries built in guix would report
`v20.0.0rc1-g3e732a952226a20505f907e4fd9b3fdbb14ea5ee` instead. Building
any commit after `v20.0.0-beta.8` locally would result in versions like
`v20.0.0rc1-8c94153d2497` which is close but it's still yet another
format. And both versions with `rc1` in their names are confusing cause
you'd expect them to mention `beta.8` instead maybe (or is it just me?
:D ).
## What was done?
Change it so that the version string would look like this:
on tag: ~`v20.0.0-beta.8-dev` or `v20.0.0-beta.8-gitarc`~
`v20.0.0-beta.8`
post-tag: ~`v20.0.0-beta.8-1-gb837e08164-gitarc`~
`v20.0.0-beta.8-1-gb837e08164`
post-tag format is
`recent tag`-`commits since that tag`-`g+12 chars of commit hash`-`dirty
(optional)` ~-`dev or gitarc`~
~`dev`/`gitarc` suffixes should help avoiding confusion with the release
versions and they also indicate the way non-release binaries were
built.~
Note that release binaries do not use any of this, they still use
`PACKAGE_VERSION` from `configure` like before.
Also, `CLIENT_VERSION_RC` is no longer used in this setup so it was
removed.
Few things aren't clear to me yet:
1. Version bump in `configure.ac` no longer affects the reported version
(unless it's an actual release). Are there any downsides I might be
missing?
2. Which tag should we use on `develop` once we bump version in
configure? `v21.0.0-init`? `v21.0.0-alpha1`?
3. How is it going to behave once `merge master back into develop` kind
of PR is merged? E.g. say `develop` branch is on `v21.0.0-alpha1` tag
and we merge v20.1.0 from `master` back into it. Will this bring
`v20.1.0` release tag into `develop`? Will it become the one that will
be used from that moment? If so we will probably need another tag on
`develop` every time such PR is merged e.g. `v21.0.0-alpha2` (or
whatever the next number is).
Don't think these are blockers but would like to hear thoughts from
others.
## How Has This Been Tested?
Built binaries locally, built them using guix at a specific tag and at
some commit on top of it.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
We should avoid return by reference; especially return by reference with
a bool flag indicating validity.
## What was done?
Instead we use a std::optional
## How Has This Been Tested?
Unit tests pass
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
## Issue being fixed or feature implemented
Implement a new code-singing certificate for windows.
Previously we used a certificate issued by DigiCert, however that
certificate recently expired. A renewed certificate would cost roughly
$200/year at the cheapest CAs and $370/year with DigiCert. EV
certificates are relatively novel types of certificates that start out
with positive reputation, reducing smart screen popups for users. EV
certificates start at $270/year.
As a result we had (/have) 4 options:
1. Get a new code signing certificate from a trusted CA
- - Pro: Certificate gains reputation over time in smart screen and
binaries are signed
- - Pro: Shows "Verified Publisher" and "Dash Core Group Inc" on install
- - Con: Costs, feels manipulative to pay at least $600 simply for
someone to sign a certificate
2. Get a new EV code signing certificate
- - Pro: Certificate starts with good reputation and gains reputation
over time
- - Con: Even greater costs for a signature that says that we are from
Dash Core Group
3. Continue signing with the expired certificate
- - Con: This is, it has been discovered, a terrible idea and these
binaries are treated worse than unsigned binaries
4. Deliver unsigned windows binaries
- - Pro: Binary will gain reputation over time as users download it
- - Pro: Easy, is what it says on the tin
- - Con: Binaries are completely unsigned, could be tampering or
corruption issues that go undetected
- - Con: Will visibly state "Unknown Publisher"
5. Deliver self-signed windows binaries
- - Pro: Binary will gain reputation over time as users download it
- - Pro: *Possibility* that certificate will gain reputation over time
as users download binaries signed by it. It may also be that only
certificates issued by a CA will gain reputation over time.
- - Pro: Binaries are still signed
- - Pro: Users have the option to import certificate into keychain to
remove "Unknown Publisher"
- - Pro: In limited testing, install is sometimes is treated better than
unsigned, otherwise is treated the same
- - Con: may appear sketchy, as Root CA is not a trusted Root CA
- - Con: will display "Unknown Publisher" to most users
- - Con: greater potential uncertainty around future changes to
treatment of self signing systems
Based on the above discussion and testing, the best route currently is
option 5; that is what this PR implements. In the future it may make
sense to move towards a codesigning certificate issued by a trusted CA.
The root certificate authority has the following information
with a sha256 fingerprint of `46 84 FF 27 11 D7 C8 C5 BB FA D1 55 41 B3
F0 43 77 97 AC 67 4C 32 19 AE B4 E7 15 11 1F BB 42 A0`
The code signing certificate is issued by the root CA, has a common name
of "Dash Core Windows Signing" and a sha256 fingerprint of `1A 09 54 6E
D3 81 E9 FC AD 62 44 32 35 40 39 FF 5F A7 30 0E 5E 03 C4 E0 96 5A 62 AA
19 2B 79 EE`. This certificate is only authorized for the purpose of
code signing.
## What was done?
## How Has This Been Tested?
Multiple users installing binaries of type 1,3,4 and 5.
## Breaking Changes
This new windows signing certificate should be documented in the release
notes.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- - [x] I have performed a self-review of my own code
- - [ ] I have commented my code, particularly in hard-to-understand
areas
- - [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- - [ ] I have made corresponding changes to the documentation
- - [x] I have assigned this pull request to a milestone _(for
repository code-owners and collaborators only)_
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEKVkDYuyHioH9PCArUlJ77avoeYQFAmWfAbUACgkQUlJ77avo
eYTSCBAAuDEoWABdonIMs/4RaYP+DGTULltRu9CHBAqYuksXrl/4iV0r17DPSWWW
L/5vLNAUTI47Tsa7R45ZPb0hR8VPMBkvxTQipKBYK7vZpwefcR4VOprEBJJ0Bl3g
ZHtAVjZbcANEIAW3SlaiOgWbxWGKfDyM7gN3aNfoidMFBefbcYKEttuAGCnktWRI
Y3eLMGPCpxOVB0O1nLU+pzwixAWXOeVChiK31ecFfQrF3JmUc12yiFUI+OJTogg4
0G2GMIQYHiVwclj8hSWT/yZfjcyxXdLYqkmH4Nr5mye39hRI2aUQEkmkYOy8pjcB
ykKLg8JpUg/zg6GSuS6mFJnd5NHq5iSBxSRHPfR8xij1xFpmdgAaNCw4/6j9PEXB
l8cfuJ7hgX3yX09L4p2E4t7MYpM8igaenAIWAK37hmKs1WADBmaj/nf6ThKhjvzI
2GR0FOzm6Is36KYvdUQJDE0g70g31SvGy+qjlcK49MtX6BvecYt+dg8AaNZ5FIn7
d1kFI4NXM6JX2WdiHMenz5d+oFYRS/P1sXjQ1wtl9HSkiZQQkEBbgiWXfh+EXjpW
fNc8cej2LLCNZlhVcpffF8UaINsMTZVQsEGWGInjSi5eCs/YNrqL8XDdC/8mmZCu
cNvp0QBtQ+4lpbUSdhFUdgic0MRCsdeHuYIBfvPJN9tl8McbknA=
=kL6E
-----END PGP SIGNATURE-----
## What was done?
drop version from README.md which is not really useful.
And we will care about one less thing during each release
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
## Issue being fixed or feature implemented
Implement a new code-singing certificate for windows.
Previously we used a certificate issued by DigiCert, however that
certificate recently expired. A renewed certificate would cost roughly
$200/year at the cheapest CAs and $370/year with DigiCert. EV
certificates are relatively novel types of certificates that start out
with positive reputation, reducing smart screen popups for users. EV
certificates start at $270/year.
As a result we had (/have) 4 options:
1. Get a new code signing certificate from a trusted CA
- - Pro: Certificate gains reputation over time in smart screen and
binaries are signed
- - Pro: Shows "Verified Publisher" and "Dash Core Group Inc" on install
- - Con: Costs, feels manipulative to pay at least $600 simply for
someone to sign a certificate
2. Get a new EV code signing certificate
- - Pro: Certificate starts with good reputation and gains reputation
over time
- - Con: Even greater costs for a signature that says that we are from
Dash Core Group
3. Continue signing with the expired certificate
- - Con: This is, it has been discovered, a terrible idea and these
binaries are treated worse than unsigned binaries
4. Deliver unsigned windows binaries
- - Pro: Binary will gain reputation over time as users download it
- - Pro: Easy, is what it says on the tin
- - Con: Binaries are completely unsigned, could be tampering or
corruption issues that go undetected
- - Con: Will visibly state "Unknown Publisher"
5. Deliver self-signed windows binaries
- - Pro: Binary will gain reputation over time as users download it
- - Pro: *Possibility* that certificate will gain reputation over time
as users download binaries signed by it. It may also be that only
certificates issued by a CA will gain reputation over time.
- - Pro: Binaries are still signed
- - Pro: Users have the option to import certificate into keychain to
remove "Unknown Publisher"
- - Pro: In limited testing, install is sometimes is treated better than
unsigned, otherwise is treated the same
- - Con: may appear sketchy, as Root CA is not a trusted Root CA
- - Con: will display "Unknown Publisher" to most users
- - Con: greater potential uncertainty around future changes to
treatment of self signing systems
Based on the above discussion and testing, the best route currently is
option 5; that is what this PR implements. In the future it may make
sense to move towards a codesigning certificate issued by a trusted CA.
The root certificate authority has the following information
with a sha256 fingerprint of `46 84 FF 27 11 D7 C8 C5 BB FA D1 55 41 B3
F0 43 77 97 AC 67 4C 32 19 AE B4 E7 15 11 1F BB 42 A0`
The code signing certificate is issued by the root CA, has a common name
of "Dash Core Windows Signing" and a sha256 fingerprint of `1A 09 54 6E
D3 81 E9 FC AD 62 44 32 35 40 39 FF 5F A7 30 0E 5E 03 C4 E0 96 5A 62 AA
19 2B 79 EE`. This certificate is only authorized for the purpose of
code signing.
## What was done?
## How Has This Been Tested?
Multiple users installing binaries of type 1,3,4 and 5.
## Breaking Changes
This new windows signing certificate should be documented in the release
notes.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- - [x] I have performed a self-review of my own code
- - [ ] I have commented my code, particularly in hard-to-understand
areas
- - [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- - [ ] I have made corresponding changes to the documentation
- - [x] I have assigned this pull request to a milestone _(for
repository code-owners and collaborators only)_
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEKVkDYuyHioH9PCArUlJ77avoeYQFAmWfAbUACgkQUlJ77avo
eYTSCBAAuDEoWABdonIMs/4RaYP+DGTULltRu9CHBAqYuksXrl/4iV0r17DPSWWW
L/5vLNAUTI47Tsa7R45ZPb0hR8VPMBkvxTQipKBYK7vZpwefcR4VOprEBJJ0Bl3g
ZHtAVjZbcANEIAW3SlaiOgWbxWGKfDyM7gN3aNfoidMFBefbcYKEttuAGCnktWRI
Y3eLMGPCpxOVB0O1nLU+pzwixAWXOeVChiK31ecFfQrF3JmUc12yiFUI+OJTogg4
0G2GMIQYHiVwclj8hSWT/yZfjcyxXdLYqkmH4Nr5mye39hRI2aUQEkmkYOy8pjcB
ykKLg8JpUg/zg6GSuS6mFJnd5NHq5iSBxSRHPfR8xij1xFpmdgAaNCw4/6j9PEXB
l8cfuJ7hgX3yX09L4p2E4t7MYpM8igaenAIWAK37hmKs1WADBmaj/nf6ThKhjvzI
2GR0FOzm6Is36KYvdUQJDE0g70g31SvGy+qjlcK49MtX6BvecYt+dg8AaNZ5FIn7
d1kFI4NXM6JX2WdiHMenz5d+oFYRS/P1sXjQ1wtl9HSkiZQQkEBbgiWXfh+EXjpW
fNc8cej2LLCNZlhVcpffF8UaINsMTZVQsEGWGInjSi5eCs/YNrqL8XDdC/8mmZCu
cNvp0QBtQ+4lpbUSdhFUdgic0MRCsdeHuYIBfvPJN9tl8McbknA=
=kL6E
-----END PGP SIGNATURE-----
9b4fa0af40cd88ed25dd77962235fbf268bdcaa7 net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server) (practicalswift)
Pull request description:
Exit with error message if `-proxy` is specified without arguments (instead of continuing without proxy server).
Continuing without a proxy server when the end-user has specified `-proxy` may result in accidental loss of privacy. (The end-user might think he/she is using a proxy when he/she is not.)
Before this patch:
```
$ src/bitcoind -proxy
…
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -listen=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -upnp=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
…
2020-09-23T00:24:33Z init message: Starting network threads...
```
`bitcoind` is now running *without* a proxy server (`GetProxy(…, …) == false`, `HaveNameProxy() == false`, etc.).
Note that the "-proxy set" log messages above which the end-user might interpret as "good, my traffic is now routed via the proxy".
After this patch:
```
$ src/bitcoind -proxy
Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.
$ echo $?
1
```
ACKs for top commit:
laanwj:
re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7
kristapsk:
ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7, I have tested the code.
hebasto:
re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7
Tree-SHA512: 4ba7a011991699a54b5bb87ec68367c681231bf5dcd36f8c89ff9ddc2e8d29df453817b7e362597e652ad6b341a22b7274be0fd78d435e5f0fd8058e5221c4ce
916d3596c493fec44da86aeb92b61eafeea0b596 help: Generate checkpoint height from chainparams (Luke Dashjr)
Pull request description:
Not sure if this is worth putting in Core, but might as well until checkpoints are removed entirely.
ACKs for top commit:
laanwj:
re-ACK 916d3596c493fec44da86aeb92b61eafeea0b596
Tree-SHA512: d8eb26b570ee730fdd75ca916507134db5f2f68987a911e33544b7f1c9ccfd1c76b9c9db63056971956b6daf16910f17ecfc197481c2f7b0773afdfbf7d381cf
d11020019a0c93dcc56859cdfcd9f0c6a777424f Add OpenBSD instructions for building the Qt GUI (grubles)
Pull request description:
Using OpenBSD as a desktop OS is prevalent enough IMO to warrant updating the documentation for building the GUI.
ACKs for top commit:
fanquake:
ACK d11020019a0c93dcc56859cdfcd9f0c6a777424f - looks fine. Have not tested.
Tree-SHA512: a8078334fdd35438bcf87c3f5eae851c2a1ce961eb48ae50770bf2c556489da86b6ee198fe9fb732dcaddb2e0f2f4f55a3126971aae8f7d4e2e320dbb024e204
d3e8adfada889a3c9fba930086eda609509aca07 util: remove c-string interfaces for DecodeBase58{Check} (Sebastian Falbesoner)
Pull request description:
This micro-PR gets rid of base58 function interfaces that are redundant in terms of c-string / std::string variants; the c-string interface for `DecodeBase58Check` is completely unused outside the base58 module, while the c-string interface for `DecodeBase58` is only used in unit tests, where an implicit conversion to std::string is not problematic.
ACKs for top commit:
practicalswift:
ACK d3e8adfada889a3c9fba930086eda609509aca07 -- patch looks correct
laanwj:
Code review ACK d3e8adfada889a3c9fba930086eda609509aca07
Tree-SHA512: 006a4a1e23b11385f60820c188b8e6b1634a182ca36e29a6580f72150214c65a3fdb273ec439165f26ba88a42d2bf5bab1cf3666a9eaee222fb4e1c00aeba433
1ccb9f30c040daf688f89f0d63e9f5e7b131d193 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr)
Pull request description:
#9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else.
So rather than just lose it, might as well get it merged in independently.
I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.
ACKs for top commit:
fanquake:
ACK 1ccb9f30c040daf688f89f0d63e9f5e7b131d193 - checked that the binaries produced are the same.
Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
1e58bcc9afefcf009653567c6373b4f7facba8f5 wallet: Fix clang build in Mac (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Top commit has no ACKs.
Tree-SHA512: 19312929af14dab97c37cf4547fbd6589a6de960f1a499c2118bb684240639af4b127cf8dc4d201b41d253cfbb645614a0606d4ecce29f300b10c210d38a961b
It includes only this commit for sake of backporting HasWalletSpent
[RPC] bumpfee
This command allows a user to increase the fee on a wallet transaction T, creating a "bumper" transaction B.
T must signal that it is BIP-125 replaceable.
T's change output is decremented to pay the additional fee. (B will not add inputs to T.)
T cannot have any descendant transactions.
Once B bumps T, neither T nor B's outputs can be spent until either T or (more likely) B is mined.
Includes code by @jonasschnelli and @ryanofsky
9c59f9c285303659ee1beed7555bbb322e6e6981 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
ACKs for top commit:
achow101:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981
ryanofsky:
Code review ACK 9c59f9c285303659ee1beed7555bbb322e6e6981. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981 tested rebased on current master b33136b6ba9887f7d and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285303659ee1beed7555bbb322e6e6981
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
c8583022800410afeb75e0154df7290d080d581d Change format of log2_work for uniform output (zero-padded) (jmorgan)
Pull request description:
Motivation:
It's jarring to watch the output of `tail -f ~/btcdata/debug.log` scroll by and very frequently see columns not lining up correctly because `log2_work` somtimes has less precision than 8 digits.
Current display:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
Display with this commit:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
ACKs for top commit:
practicalswift:
ACK c8583022800410afeb75e0154df7290d080d581d -- patch looks great :)
achow101:
ACK c8583022800410afeb75e0154df7290d080d581d
laanwj:
Tested ACK c8583022800410afeb75e0154df7290d080d581d
Tree-SHA512: 16cbe419c4993ad51019c676e8ca409ef1025b803cc598437c780dd7ca003d7e4ad421f451e9a374e0070ee9b3ee601b7aba849e1f346798f9321d1bce5c4401
a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b43ed16252428e935d3aeb5a38f582e0 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d5bb5a1564c339a0e6a16bac8f02c98 refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf57ccd6cdf5ec706ac328c62142d7a2 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6d0f2543948d64ed26f7d9a903f67e5 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8bb733d7d89e36b28671cb47f436292 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
It is partial de-circularisation of dependencies between that includes net_processing
Classes that still depends on net_processing but should not:
- llmq/dkgsessionmgr
- llmq/signing
- llmq/instantsend
They have asynchronous processing and with current impl that's impossible to do
