There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
180dc3c886 build: miniupnpc 2.2.2 (fanquake)
Pull request description:
Creating the dll subdir is no-longer required.
We can drop our wingen patch.
One issue that came up in [#19867](https://github.com/bitcoin/bitcoin/pull/19867#discussion_r483516359):
> unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal
> My assumption is that it was being inserted to make depends more deterministic. However I think we can improve this, as there's no reason to reveal more of the version information either. Could leave the version as is /2.0 and either drop the architecture, or insert something else?
I've dropped our `sed` and added a patch that just removes the OS string and miniupnpc version from the User-Agent. i.e:
```bash
# master
strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent
User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203
User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203
# this PR
strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent
User-Agent: UPnP/1.1
User-Agent: UPnP/1.1
```
Note that built unmodified (22c1386351), the User-Agent would be:
```bash
strings libminiupnpc.dylib | rg User-Agent
User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0
User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0
```
ACKs for top commit:
laanwj:
Code review ACK 180dc3c886
hebasto:
ACK 180dc3c886.
Tree-SHA512: b0b6e623dbc5499e28faedf992d84278d6a11887a45a3806957b9e08886c5e56044cdfa2e7d7ec81cb1dd55f89be99834367905315d6bc611ba530e91d889ad1
This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects 'Disable Private' keys, unselecting it will also unselect the 'Disable Private Keys' checkbox, which in turn re-enables the 'Encrypt Wallet' checkbox.
fac921f23f fuzz: Fix tx_pool target to properly fuzz immature outpoints (MarcoFalke)
fa2b95f861 fuzz: Style fixups (MarcoFalke)
Pull request description:
Also includes a commit for minor style fixups
ACKs for top commit:
glozow:
utACK fac921f23f this fixes it 👍
Tree-SHA512: 1575ba115b2009b653921511c163bd846cd381d6fc92b04a899c0686d23a02bdcdd95c81776b515b80ae187bcec3ccaca3aa88fcecbec888f73ca2d875eef506
d09ebc4723 Fix wrong(1024) divisor for 1000-based prefixes (wodry)
Pull request description:
v.0.21.0
I saw in the GUI peer window in the "received" column `1007 KB`, and after increasing to >=1024 I guess, it switched to `1 MB`. I would have expected the display unit to change from KB to MB already at value >=1000.
I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct.
But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.
To be correct, this PR changes ~~KB/MB/GB to KiB/MiB/GiB.~~ KB to kB and the divisor from 1024 to 1000.
ACKs for top commit:
hebasto:
ACK d09ebc4723, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".
jarolrod:
ACK d09ebc4723
leonardojobim:
Tested ACK d09ebc4723 on Ubuntu 20.04 Qt 5.12.8
Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
810b1310d6 build: Do not build unused CoreWLAN stuff in depends for macOS (Hennadii Stepanov)
Pull request description:
We [do not use](d2a78ee928/contrib/devtools/symbol-check.py (L96-L111)) any macOS CoreWLAN Framework stuff.
Changes in Qt Configure summary with `HOST=x86_64-apple-darwin18`:
```diff
--- wlan-master/summary2021-03-22 00:26:04.377387806 +0200
+++ wlan-pr/summary2021-03-22 00:37:07.060997990 +0200
@@ -49,7 +49,7 @@
slog2 ................................ no
Using system PCRE2 ..................... no
Qt Network:
- CoreWLan ............................... yes
+ CoreWLan ............................... no
getifaddrs() ........................... yes
IPv6 ifname ............................ yes
libproxy ............................... no
```
ACKs for top commit:
fanquake:
ACK 810b1310d6 - Only obvious difference I could see in the build is skipping a configure check.
Tree-SHA512: 54e177c4ad528ef48cc80c3a39ab1b66267dd0ca4fe6cc4f70579c87b74051a04ebeeca1a26afee7fc29f750af456804578abbe7e8a9ad2717297291f206547e
1404c57403 [doc] Coin: explain that IsSpent() can also mean never existed (Sjors Provoost)
Pull request description:
This can be especially confusing where `AccessCoin()` is used with logic like this:
```c++
while (iter.n < MAX_OUTPUTS_PER_BLOCK) {
const Coin& alternate = view.AccessCoin(iter);
if (!alternate.IsSpent()) return alternate;
```
ACKs for top commit:
practicalswift:
ACK 1404c57403
MarcoFalke:
ACK 1404c57403
jnewbery:
utACK 1404c57403
Tree-SHA512: 418618dd7e08bd5cc8360e3501d0f57e34100e5101ad3b8e0a819923fa860f44c7f2fada0f8447a1af3c2601fd72bfe619b91ff2f26f7133ceaeb0c98b017b12
55554463c1 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke)
Pull request description:
This has minimally better performance. Reported by me in https://github.com/bitcoin/bitcoin/pull/20228#discussion_r598081787
ACKs for top commit:
practicalswift:
Tested ACK 55554463c1
vasild:
ACK 55554463c1
Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261
7b3434f800 build: don't try and use -fstack-clash-protection on Windows (fanquake)
Pull request description:
This has never worked with any of the mingw-w64 compilers we use, and the `-O0` is causing issues for builders applying spectre mitigations (see [IRC logs](http://www.erisian.com.au/bitcoin-core-dev/log-2021-03-12.html#l-15)).
Recent discussion on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90458 would also indicate that this should just not be used on Windows.
ACKs for top commit:
laanwj:
Concept and code review ACK (but untested) 7b3434f800
hebasto:
ACK 7b3434f800, I've verified that this change does not affect builds for `HOST=x86_64-w64-mingw32` by comparing sizes of the output `*.exe` files.
Tree-SHA512: 72b582321ddff8db3201460fa42a53304e70f141ae078d76a4d4eeb1ca27c8dd567ccb468cc8616179c8df784bd8ca038dcb9a277f9e29f9d98c3cc842916b18
663f6cd9dd contrib: Use -daemonwait in systemd init script (Wladimir J. van der Laan)
Pull request description:
Make systemd invoke dependencies only when ready by using `-daemonwait` in the service file instead of `-daemon`.
Closes#21322 by making bitcoind conform to behavior specified for `type=forking`.
This may need some tuning of timeouts.
ACKs for top commit:
darosior:
ACK 663f6cd
hebasto:
re-ACK 663f6cd9dd
Tree-SHA512: 890005852b632a202caa578e6c796ebdc9da0b2379a9157a4f56f7db9d193c0ffbb78d120bbf112ab2f273855f2a08c3da000b1f7a9fb5222a3b94dcdb16b878
fa4cebadcf util: Make Assume() usable as unary expression (MarcoFalke)
Pull request description:
Assume shouldn't behave different at the call site depending on build flags. Currently compilation fails if it is used as expression. Fix that by using the lambda approach from `Assert()` without the `assert()`.
ACKs for top commit:
jnewbery:
ACK fa4cebadcf
practicalswift:
cr ACK fa4cebadcf: patch looks correct and commit hash starts with `fa`
Tree-SHA512: 9ec9ac8d410cdaf5e4e28df571a89e3d23d38e05a7027bb726cae3da6e9314734277e5a218e9e090cc17e10db763da71052c229ad642077ca5824ee42022f3ed
5294f0d5a9 refactor: return std::nullopt instead of {} (fanquake)
Pull request description:
In #21415 [we decided](https://github.com/bitcoin/bitcoin/pull/21415#issuecomment-800236640) to return `std::optional` rather than `{}` for
uninitialized values. This PR replaces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
ACKs for top commit:
hebasto:
ACK 5294f0d5a9, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
Introduce an android directory under qt and allow one to package bitcoin-qt for Android by running make apk.
Add bitcoin-qt Android build instructions.
3d086f42ab test: add ParseUInt16() test coverage (Jon Atack)
Pull request description:
`ParseUInt16()` was just added in #21328.
ACKs for top commit:
practicalswift:
cr ACK 3d086f42ab: patch looks correct & more coverage is better than less coverage
Tree-SHA512: bf7f96deb7c1531419565907f0ea8a8e32b368d4b823a3e80928b2c118edbf643ea06e357b4b5504a89f855caeed289daa9f823c740231ed6ad1b8ed00285ce8
52a43b0c7d build: Fix fuzz-cuckoocache cross-compiling for Windows with DEBUG=1 (Hennadii Stepanov)
Pull request description:
Fix#21348.
ACKs for top commit:
practicalswift:
Tested ACK 52a43b0c7d
Tree-SHA512: 6592f829edfb740a1e9d0691acf04b2372e91b0a53ca395b08350cb0b80031d3b55fa7331bdaddf857d450eb30b605af9fe8fe02559cda19374a48f9634fae70
787df19b09 validation: don't try to invalidate genesis block (Sebastian Falbesoner)
Pull request description:
In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.
Fixes#20914.
ACKs for top commit:
sipa:
ACK 787df19b09. Tested invalidation of generic on regtest.
practicalswift:
Tested ACK 787df19b09
Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
