1ccb9f30c0 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 1ccb9f30c0 - checked that the binaries produced are the same.
Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
daed542a12 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery)
c556770b5e [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery)
Pull request description:
Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.
ACKs for top commit:
jonatack:
ACK daed542a12 code review and debug tested
promag:
Code review ACK daed542a12.
MarcoFalke:
re-ACK daed542a12, only change is removing second commit 🎴
theStack:
Code Review ACK daed542a12
Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e
366913e307 build: AX_BOOST_THREAD serial 33 (Igor Cota)
cf0681133a build: disable D-Bus on Android by default (Igor Cota)
Pull request description:
I've been trying to build for Android on different OSes/Gitian with varying success. Build system is quite the beast and sometimes it doesn't get it right. To make sure it does these three little tweaks make the Android build more robust:
- disable D-Bus (Android doesn't support it and has its own way to trigger notifications)
- don't flag `-lpthread` when linking Boost, [Bionic has built-in support](https://stackoverflow.com/questions/30801752/android-ndk-and-pthread)
- ~~add `-static-libstdc++` to linker flags. This avoids having to bundle `libc++_shared` with CLI apps, still necessary with `bitcoin-qt` though (thanks Sjors)~~
I think these are small and fairly straightforward so I put them all into this one PR.
ACKs for top commit:
fanquake:
ACK 366913e307
Tree-SHA512: 31465fd228a5877c20aa2a05f98242d4eeb328b9b35bd1a7a3dcfb1ef51379d84053a81ade5a65436ffc1bc8ccd21f11ed0539eb10e827d182c0c04394629af0
fa30d5282c doc: Remove label from good first issue template (MarcoFalke)
Pull request description:
Good first issues aren't that frequent that manually assigning the label is a problem, but this fixes the spam problem (e.g. https://twitter.com/GoodFirstIssues/status/1295455089491161088 )
ACKs for top commit:
jnewbery:
ACK fa30d5282c
Tree-SHA512: 59e7c707637cc328e2443c2b7e5d2c82ef151739ad5afb6cea1a60501318dc8c4c81c95591eed8172581ac99d43cf826dcdd547e096eff1038137853af67a975
8ed2f1ed78 Remove unused includes (Marcin Jachymiak)
cf095a53fc Move comment about BaseIndex::DB from TxIndex::DB (Marcin Jachymiak)
Pull request description:
Moves a comment about the `BaseIndex::DB` from the `TxIndex::DB` into the correct place. Originally part of https://github.com/bitcoin/bitcoin/pull/14053.
ACKs for top commit:
fanquake:
ACK 8ed2f1ed78
Tree-SHA512: cb4e2b916c7ab996961cc2e1d910bc4b8a1700eb32b70fc1657ca720117a7a84f7337fe5e4fb30e047aa92c31eaa976eaaa5cb8f861877f2ff6f4a59bb94f4e9
ed5cd12869 test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py (Dhruv Mehta)
f6f082b934 test: remove `CNodeNoVersionIdle` from p2p_leak.py (Dhruv Mehta)
45cf55ccac test: remove `CNodeNoVersionMisbehavior` from p2p_leak.py (Dhruv Mehta)
Pull request description:
- Removes `CNodeNoVersionMisbehavior` per recommendation at https://github.com/bitcoin/bitcoin/pull/19657#issuecomment-669926458
- Removes `CNodeNoVersionIdle` because it is similarly unnecessary
- As someone new to the codebase, I found it easier to understand it if `no_version_disconnect_node` tries to overwhelm the peer with any message that is not version/verack.
- Per recommendation at https://github.com/bitcoin/bitcoin/pull/19727#pullrequestreview-468093555, made a clear distinction between nodes(bitcoind) and peers(mininode interface implementations)
ACKs for top commit:
jnewbery:
tested ACK ed5cd12869
amitiuttarwar:
utACK ed5cd12869
Tree-SHA512: 310a24c91fd837e7f65177edb55fe6142fb3559fae7867c5cdd9c9a23b1a02202b935ca9a82633fa7649f3de2fa221f6da906a7b5e499fc20f7254085033757d
2f8a4c9a06 build: Enable some commonly enabled compiler diagnostics (practicalswift)
Pull request description:
Enable some commonly enabled compiler diagnostics as discussed in #17344.
| Compiler diagnostic | no# of emitted unique GCC warnings in `master` | no# of emitted unique Clang warnings in `master` |
| ------------- | ------------- | ------------- |
| `-Wduplicated-branches`: Warn if `if`/`else` branches have duplicated code | 0 | Not supported |
| `-Wduplicated-cond`: Warn if `if`/`else` chain has duplicated conditions | 0 | Not supported |
| `-Wlogical-op`: Warn about logical operations being used where bitwise were probably wanted | 0 | Not supported |
| `-Woverloaded-virtual`: Warn if you overload (not `override`) a virtual function | 0 | 0 |
| ~~`-Wunused-member-function`: Warn on unused member function~~ | Not supported | 2 |
| ~~`-Wunused-template`: Warn on unused template~~ | Not supported | 1 |
There is a large overlap between this list and [Jason Turner's list of recommended compiler diagnostics in the Collaborative Collection of C++ Best Practices (`cppbestpractices`) project](https://github.com/lefticus/cppbestpractices/blob/master/02-Use_the_Tools_Available.md#gcc--clang). There is also an overlap with the recommendations given in the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines) (with editors Bjarne Stroustrup and Herb Sutter).
Closes#17344.
ACKs for top commit:
jonatack:
ACK 2f8a4c9a06 no warnings for me with these locally on debian 5.7.10-1 (2020-07-26) x86_64 with gcc 10 and clang 12
fanquake:
ACK 2f8a4c9a06 - no-longer seeing any obvious issues with doing this.
hebasto:
ACK 2f8a4c9a06, no new warnings in Travis jobs.
Tree-SHA512: f669ea22b31263a555f999eff6a9d65750662e95831b188c3192a2cf0127fb7b5136deb762a6b0b7bbdfb0dc6a40caf48251a62b164fffb81dd562bdd15ec3c8
fa55c1d5fd build: Add Werror=range-loop-analysis (MarcoFalke)
Pull request description:
The warning is implicitly enabled for Bitcoin Core. Also explicitly since commit d92204c900.
To avoid "fix range loop" follow-up refactors, we have two options:
* Disable the warning, so that issues never appear
* Enable it as an error, so that issues are either caught locally or by ci
ACKs for top commit:
fanquake:
ACK fa55c1d5fd
practicalswift:
ACK fa55c1d5fd -- pre-review fix-up is better than post-review fix-up
hebasto:
re-ACK fa55c1d5fd
Tree-SHA512: 019aa133f254af8882c1d5d10c420d9882305db0fc2aa9dad7d285168e2556306c3eedcc03bd30e63f11eae4cc82b648d83fb6e9179d6a6364651fb602d70134
7966aa424a Add variables for repeated scripts (MeshCollider)
fec8336ad9 Remove GetScriptForWitness function (MeshCollider)
b887060d06 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider)
Pull request description:
As per this TODO in the code:
> TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.
The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/16841/commits/7966aa424a8b78983f73742cbdb3d11eccaf9f3a
jonatack:
Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4`
achow101:
re-ACK 7966aa424a only changes since last is rebase.
Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
e7448d6680 wallet: Don't override signing errors (Fabian Jahr)
Pull request description:
While reviewing #17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one.
Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed.
On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach.
ACKs for top commit:
achow101:
ACK e7448d6680
meshcollider:
Code review ACK e7448d6680
Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23
01e283068b [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b67 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc4 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df629 [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b0 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee3 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e9 [doc] Describe different connection types (Amiti Uttarwar)
442abae2ba [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb052 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812d [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e9 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)
Pull request description:
**This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.
**This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.
This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
(some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)
Overview of this PR:
* rename `oneshot` to `addrfetch`
* introduce `ConnectionType` enum
* one by one, add different connection types to the enum
* expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
* fix the bug in counting different type of connections
* some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.
ACKs for top commit:
jnewbery:
utACK 01e283068b
laanwj:
Code review ACK 01e283068b, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b.
fanquake:
ACK 01e283068b - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
jb55:
wow this code was messy before... ACK 01e283068b
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
dac7a111bd refactor: test: use _ variable for unused loop counters (Sebastian Falbesoner)
Pull request description:
This tiny PR substitutes Python loops in the form of `for x in range(N): ...` by `for _ in range(N): ...` where applicable. The idea is indicating to the reader that a block (or statement, in list comprehensions) is just repeated N times, and that the loop counter is not used in the body, hence using the throwaway variable. This is already done quite often in the current tests (see e.g. `$ git grep "for _ in range("`). Another alternative would be using `itertools.repeat` (according to Python core developer Raymond Hettinger it's [even faster](https://twitter.com/raymondh/status/1144527183341375488)), but that doesn't seem to be widespread in use and I'm not sure about a readability increase.
The only drawback I see is that whenever one wants to debug loop iterations, one would need to introduce a loop variable again. Reviewing this is basically a no-brainer, since tests would fail immediately if a a substitution has taken place on a loop where the variable is used.
Instances to replace were found by `$ git grep "for.*in range("` and manually checked.
ACKs for top commit:
darosior:
ACK dac7a111bd
instagibbs:
manual inspection ACK https://github.com/bitcoin/bitcoin/pull/19674/commits/dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64
practicalswift:
ACK dac7a111bd -- the updated code is easier to reason about since the throwaway nature of a variable is expressed explicitly (using the Pythonic `_` idiom) instead of implicitly. Explicit is better than implicit was we all know by now :)
Tree-SHA512: 5f43ded9ce14e5e00b3876ec445b90acda1842f813149ae7bafa93f3ac3d510bb778e2c701187fd2c73585e6b87797bb2d2987139bd1a9ba7d58775a59392406
01cd24c226 doc: set CC_FOR_BUILD when building on OpenBSD (fanquake)
Pull request description:
Closes: #19559
While #19559 has been fixed upstream, it makes sense to not only
recommend using `CC_FOR_BUILD`here until the fix is pulled in as
part of our next libsecp update, but after discussing with Cory,
he suggested we should be setting this on OpenBSD (which still has
the an ancient GCC) regardless.
ACKs for top commit:
real-or-random:
ACK 01cd24c226 I looked at the diff (but can't test the instructions on OpenBSD)
laanwj:
Code review ACK 01cd24c226
Tree-SHA512: 322802b9303771f1be2ad9628f268dfa71dc7ee77948fa2a34f21eceb19b2d8efdd8876c8f0778adbfcde48fa0f88cd4e698ae425428159abca38e8c7980da1d
ca2e474372 Fix a compiler warning: unused GetDevURandom() (Vasil Dimov)
Pull request description:
~~Only define GetDevURandom() if it is going to be used.~~
Silence by planting a dummy reference to the `GetDevURandom` symbol
in the places where we don't call the function.
ACKs for top commit:
practicalswift:
ACK ca2e474372 -- increased signal to noise in compiler diagnostics is good
sipa:
utACK ca2e474372
hebasto:
re-ACK ca2e474372, tested on macOS 10.15.6 + llvm clang 10.0.0
Tree-SHA512: 03c98f00dad5d9a3c5c9f68553d72ad5489ec02f18b9769108a22003ec7be7819a731b1eab6a9f64dafb5be0efddccf6980de7e3bb90cd20d4f4d72f74124675
33a84e8f40 build: Update and sort package list in gitian-linux.yml (Hennadii Stepanov)
95051682be build: Drop old hack which is unneeded now (Hennadii Stepanov)
Pull request description:
The hack was aimed to fix an issue in Ubuntu Trusty 14.04 (see #8188).
The current hack implementation was added in #8315.
On master (8db23349fe) this hack is effectively noop, and it is no longer needed.
I see this PR as a step to removing `libfaketime` from gitian builds.
ACKs for top commit:
dongcarl:
tACK 33a84e8f40
laanwj:
Code review ACK 33a84e8f40
Tree-SHA512: 90036c555a500649ccc3d108bf11f09a9cfd2c92c0b598f7e0c0df63a713ae7abaf78f350b68c025470619c967223f45f6a235ad37a6ce1d1a0341ed34963ba0
9f88ded82b test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7e Add txids with non-standard inputs to reject filter (Suhas Daftuar)
Pull request description:
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
ACKs for top commit:
ajtowns:
ACK 9f88ded82b - code review
jnewbery:
Code review ACK 9f88ded82b
ariard:
Code Review/Tested ACK 9f88ded
naumenkogs:
utACK 9f88ded82b
jonatack:
ACK 9f88ded82b
Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148
70452a070b build: set minimum required Boost to 1.58 (fanquake)
Pull request description:
Any systems which only have an older installable Boost can use depends.
1.58.0 retains compatibility with the packages [installable on Ubuntu 16.04](https://packages.ubuntu.com/xenial/libboost-dev).
The projects usage of Boost wont be going away any time soon, if ever (i.e #15382), and our usage of the test framework.
Fixes: #19506
ACKs for top commit:
practicalswift:
ACK 70452a070b -- patch looks correct
laanwj:
ACK 70452a070b
hebasto:
ACK 70452a070b, tested on Linux Mint 20 (x86_64).
Tree-SHA512: d290415e3c70a394b3d7659c0480a35b4082bdce8d48b1c64a0025f7ad6e21567b4dc85813869513ad246d27f950706930410587c11c1aa3693ae6245084765c
fa5288cf5c contrib: Fixup valgrind suppressions file (MarcoFalke)
Pull request description:
I am observing this one on bionic with system boost::fs:
```
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:__wcsnlen_avx2
fun:wcsnrtombs
fun:_ZNKSt7codecvtIwc11__mbstate_tE6do_outERS0_PKwS4_RS4_PcS6_RS6_
fun:_ZN5boost10filesystem11path_traits7convertEPKwS3_RNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt7codecvtIwc11__mbstate_tE
fun:_ZN5boost10filesystem6detail11unique_pathERKNS0_4pathEPNS_6system10error_codeE
...
ACKs for top commit:
practicalswift:
ACK fa5288cf5c -- patch looks correct
Tree-SHA512: 067e10e932a7f5b13e516134e0cfd3030265c1b582cdfde1cea97042e31399aa40c4590710a39429854c68ad703a0ae9f0b06e9af4cdd81e3cacb042939a84b6
10b7a6d532 refactor: make txmempool interface use GenTxid (Pieter Wuille)
5c124e1740 refactor: make FindTxForGetData use GenTxid (Pieter Wuille)
a2bfac8935 refactor: use GenTxid in tx request functions (Pieter Wuille)
e65d115b72 test: request parents of orphan from wtxid relay peer (Anthony Towns)
900d7f6c07 p2p: enable fetching of orphans from wtxid peers (Pieter Wuille)
9efd86a908 refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic (Pieter Wuille)
d362f19355 doc: list support for BIP 339 in doc/bips.md (Pieter Wuille)
Pull request description:
This is based on https://github.com/bitcoin/bitcoin/pull/18044#discussion_r450687076.
A new type `GenTxid` is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted.
Also document BIP339 in doc/bips.md.
ACKs for top commit:
jnewbery:
Code review ACK 10b7a6d532
jonatack:
ACK 10b7a6d532
ajtowns:
ACK 10b7a6d532 -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice.
naumenkogs:
utACK 10b7a6d
Tree-SHA512: d518d13ffd71f8d2b3c175dc905362a7259689e6022a97a0b4f14f1f9fdd87475cf5af70cb12338d1e5d31b52c12e4faaea436114056a2ae9669cb506240758b
e60ef21b8a doc: Clang 8 or later is required with FORCE_USE_SYSTEM_CLANG (fanquake)
Pull request description:
The usage of pragmas within the macOS SDK requires LLVM Clang 8. This is
the same version as our prebuilt Clang, however the minimum is worth noting
here as they may diverge and/or expert users might expect they could use an
earlier version.
If you compile depends using `FORCE_USE_SYSTEM_CLANG=1` and Clang 7 you'll see output like:
```bash
In file included from kernel/qcore_mac_objc.mm:44:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSText.h:9:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:19:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSResponder.h:10:
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h:19:1: error:
expected 'push' or 'pop' after '#pragma clang attribute'
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/os/availability.h:104:273: note: expanded from macro
'API_UNAVAILABLE_BEGIN'
...__API_UNAVAILABLE_BEGIN5, __API_UNAVAILABLE_BEGIN4, __API_UNAVAILABLE_BEGIN3, __API_UNAVAILABLE_BEGIN2, __API_UNAVAILABLE_BEGIN1, 0)(__VA_A...
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
```
I've got a [godbolt here](https://godbolt.org/z/j6r987) that contains a demo of the issue (based off the macOS SDK). It will compile with Clang 8 but not with Clang 7.
ACKs for top commit:
laanwj:
ACK e60ef21b8a
Tree-SHA512: affc082a4b28d9cd102a844e10f63f55f0f20a9f840e5d8037a41470ad1ea83edc3075ce262b1d618c59c9b75445e52ddd9ba9f84c5ac3da9ce20681473c47b1
The usage of pragmas within the macOS SDK requires LLVM Clang 8. This is
the version as our prebuilt Clang, however the minimum is worth noting here
as they may diverge and/or expert users might expect they could use an
earlier version.
If you compile using Clang 7 you'll see output like:
```bash
In file included from kernel/qcore_mac_objc.mm:44:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSText.h:9:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:19:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSResponder.h:10:
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h:19:1: error:
expected 'push' or 'pop' after '#pragma clang attribute'
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/os/availability.h:104:273: note: expanded from macro
'API_UNAVAILABLE_BEGIN'
...__API_UNAVAILABLE_BEGIN5, __API_UNAVAILABLE_BEGIN4, __API_UNAVAILABLE_BEGIN3, __API_UNAVAILABLE_BEGIN2, __API_UNAVAILABLE_BEGIN1, 0)(__VA_A...
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
```
Rather than just using it on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e it is available in newer versions
of FreeBSD (11.1 and later).
This also aligns our code more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
Closes: #19559
While #19559 has been fixed upstream, it makes sense to not only
recommend using `CC_FOR_BUILD`here until the fix is pulled in as
part of our next libsecp update, but after discussing with Cory,
he suggested we should be setting this on OpenBSD (which still has
the an ancient GCC) regardless.
5962522fbc depends: bump native_cctools for fixed lto with external clang (Cory Fields)
00d1ba7aaa depends: enable lto support for Apple's ld64 (Cory Fields)
Pull request description:
This didn't work for a few reasons (various toolchain compatibility issues) the last time I tested it, but after the last round of bumps it works with no apparent issues.
Note that this does not _enable_ LTO by default in any way, only hooks up the machinery for ```-flto``` to work correctly when specified.
Lines were split for an easier rebase after #17919 is merged.
ACKs for top commit:
fanquake:
ACK 5962522fbc. The relevant option upstream is [here](https://github.com/tpoechtrager/cctools-port/blob/master/cctools/m4/llvm.m4#L4).
Tree-SHA512: df2775e74e7bc847e6cef94cb8457d503d6c9e2fdea861e51386fa6ed5a7ba688241db3685561ae1a32f66724c1b3801727252025f00c04b90a3bdc8a4f6f93b
a8865f8b72 [net processing] Tidy up Misbehaving() (John Newbery)
d15b3afb4c [net processing] Always supply debug message to Misbehaving() (John Newbery)
634144a1c2 [net processing] Fixup MaybeDiscourageAndDisconnect() style (John Newbery)
Pull request description:
This PR makes a few minor clean-ups to `Misbehaving()` in preparation to move it out of the cs_main lock.
There are very minor logging changes but otherwise no functional changes.
ACKs for top commit:
troygiorshev:
tACK a8865f8b72
jonatack:
ACK a8865f8
fjahr:
Code review ACK a8865f8b72
promag:
Code review ACK a8865f8b72.
Tree-SHA512: 98fb4f5f76399715545a1ea19290dcebfc8cb4eff72a1d3555dd3de6e184040bb8668c9651dab21db0dfd8e674e53a5977105ef76547146c9f6fa6b4b9d2ba59
fa5979d12f rpc: Avoid useless mempool query in gettxoutproof (MarcoFalke)
fa1f7f28cb rpc: Style fixups in gettxoutproof (MarcoFalke)
Pull request description:
`GetTransaction` implicitly and unconditionally asks the mempool global for a transaction. This is problematic for several reasons:
* `gettxoutproof` is for on-chain txs only and asking the mempool for on-chain txs is confusing and minimally wasteful
* Globals are confusing and make code harder to test with unit tests
Fix both issues by passing in an optional mempool. This also helps with #19556
ACKs for top commit:
hebasto:
re-ACK fa5979d12f
jnewbery:
utACK fa5979d12f
promag:
Code review ACK fa5979d12f.
Tree-SHA512: 048361b82abfcc40481181bd44f70cfc9e97d5d6356549df34bbe30b9de7a0a72d2207a3ad0279b21f06293509b284d8967f58ca7e716263a22b20aa4e7f9c54
This flag was added to binutils/ld in the 2.30 release,
see commit c11c786f0b45617bb8807ab6a57220d5ff50e414:
> The new "-z separate-code" option will generate separate code LOAD
segment which must be in wholly disjoint pages from any other data.
It was made the default for Linux/x86 targets in the 2.31 release, see commit
f6aec96dce1ddbd8961a3aa8a2925db2021719bb:
> This patch adds --enable-separate-code to ld configure to turn on
-z separate-code by default and enables it by default for Linux/x86.
This avoids mixing code pages with data to improve cache performance
as well as security.
> To reduce x86-64 executable and shared object sizes, the maximum page
size is reduced from 2MB to 4KB when -z separate-code is turned on by
default. Note: -z max-page-size= can be used to set the maximum page
size.
> We compared SPEC CPU 2017 performance before and after this change on
Skylake server. There are no any significant performance changes.
Everything is mostly below +/-1%.
Support was also added to LLVMs lld: https://reviews.llvm.org/D64903, however
there is remains off by default.
There were concerns about an increase in binary size, however in our case, the
increase (1 page worth of bytes) would seem negligible, given we are shipping a
multi-megabyte binary, which then downloads 100's of GBs of data.
Also note that most recent versions of distros are shipping a new enough version
of binutils that this is available and/or on by default (assuming the distro has
not turned it off, I haven't checked everywhere):
CentOS 8: 2.30
Debian Buster 2.31.1
Fedora 29: 2.31.1
FreeBSD: 2.33
GNU Guix: 2.33 / 2.34
Ubuntu 18.04: 2.30
Related threads / discussion:
https://bugzilla.redhat.com/show_bug.cgi?id=1623218
ef3d4ce4c3 build: call AC_PATH_TOOL for dsymutil in macOS cross-compile (fanquake)
Pull request description:
While testing #19530 I noticed that we couldn't call [`dsymutil`](https://www.llvm.org/docs/CommandGuide/dsymutil.html) after LTO:
```bash
../libtool: line 10643: x86_64-apple-darwin16-dsymutil: command not found
```
This updates configure to call `AC_PATH_TOOL` so that we end up with the
full path to dsymutil, similar to `otool` and `install_name_tool`, ie:
`/bitcoin/depends/x86_64-apple-darwin16/share/../native/bin/x86_64-apple-darwin16-dsymutil`.
ACKs for top commit:
laanwj:
Code review ACK ef3d4ce4c3
theuni:
ACK ef3d4ce4c3.
Tree-SHA512: e4fa93e7f9f7945289143dfe2a6645ad8ee7f3bee0793412b3509901a30566d6f952e3b39e0e525a54f8dbd0c480f8da70fc6cb80b07800d11b0c6071fbb7466
While testing #19530 I noticed that we couldn't call dsymutil after LTO:
```bash
../libtool: line 10643: x86_64-apple-darwin16-dsymutil: command not found
```
This updates configure to call `AC_PATH_TOOL` so that we end up with the
full path to dsymutil, similar to `otool` and `install_name_tool`, ie:
`/bitcoin/depends/x86_64-apple-darwin16/share/../native/bin/x86_64-apple-darwin16-otool`.
c8992e8959 test: Fix fuzzer compilation on macOS fixes#19557 (freenancial)
Pull request description:
fixes#19557
Before the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) make
Making all in src
CXX test/fuzz/addition_overflow-addition_overflow.o
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow(fuzzed_file->m_offset, n)) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
```
After the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm && make clean && make -j5
...
...
CXXLD test/fuzz/uint256_deserialize
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
```
ACKs for top commit:
fanquake:
ACK c8992e8959 - tested that compiling works on macOS.
MarcoFalke:
review ACK c8992e8959
Tree-SHA512: 965cdc61b30db0e2209c91b29f0d42de927a9a5b85e1e70f22d1452e0955f876726c7a8c1d1a5f448f12bf24eec3000802071cd4ae28d8605343fd43d174ca84
c0f09c2c9d fuzz: add missing overrides to signature_checker (Jon Atack)
Pull request description:
These functions in `fuzz/signature_checker.cpp` override virtual member functions and should be marked `override` instead of `virtual`, which is for introducing a new virtual function. The overridden virtual functions are in `script/interpreter.h:151/156/161`.
Also, per MarcoFalke suggestion, add missing parentheses in `fuzz/scriptnum_ops.cpp` and remove useless `unsigned int >= 0` conditional in `fuzz/script.cpp`.
These changes fix 5 compile warnings in gcc 10 and 3 in clang 11/12.
ACKs for top commit:
vasild:
ACK c0f09c2
MarcoFalke:
review ACK c0f09c2c9d
Tree-SHA512: 76ce73ec577c1f23cf8646c31d44dcd6c6303732c47187d041a8921d0d24a50163989a375352ebc221abf2ac337bc0902149be46b6f9eebc071d2f364c407f71
facdf530c7 contrib: Clean up previous_releases.py (MarcoFalke)
Pull request description:
ACKs for top commit:
fjahr:
tACK facdf530c7
Sjors:
tACK facdf53
hebasto:
ACK facdf530c7, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: c3543320572267035aa342dd170128bbdeb83ca4e2e36a8e46596dd76c8ff1b26ed6759a8073884228b133c45b06ec48889cd0ec83a13bef276b48073d8248e4
6be3eebd74 doc: Update macOS cross compilation dependencies for Focal (Hennadii Stepanov)
Pull request description:
The [`libtinfo5`](https://packages.ubuntu.com/focal/libtinfo5) package is required on Ubuntu Focal for macOS cross compilation.
Fixes#19546.
ACKs for top commit:
fanquake:
ACK 6be3eebd74 I have been [installing the libtinfo5 package](https://github.com/fanquake/core-review/blob/master/docker/debian9.dockerfile#L18) into my cross-compiling images, so mostly forgot about this.
Tree-SHA512: f40ab1efae549276e520f9cc3200438e70297c3081a3f9acc3075ec11d494fd6176ed3f1d4889b39eb2a5706eac835844a75b79335bb7f3c21c985e0779a7562
55057ffc51 rpc: deduplicate WriteHDKeypath() used in decodepsbt (Sebastian Falbesoner)
Pull request description:
The functionality is already provided in the BIP32 utility library `util/bip32.h` with the exact same name and function signature.
ACKs for top commit:
achow101:
ACK 55057ffc51
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/19555/commits/55057ffc51697daafac1224664d5e3258ae0b116
jonatack:
ACK 55057ffc51
Tree-SHA512: 074c1a71ffb32908926bf07f0c5428a46309f6e0d21e7c20b1008197c820b97776a441736d0b6fd8ab0c0852522a0b5a5ddb26a1e4a1100ca02aabc65a07a018
Starting with the 10.1 release, GCC defaults to -fno-common. This causes
linking issues when building genisoimage:
```bash
[ 98%] Building C object genisoimage/CMakeFiles/genisoimage.dir/checksum.o
[100%] Linking C executable genisoimage
/usr/bin/ld: CMakeFiles/genisoimage.dir/apple.o:(.bss+0x0): multiple definition of `outfile'; CMakeFiles/genisoimage.dir/genisoimage.o:(.bss+0x0): first defined here
/usr/bin/ld: CMakeFiles/genisoimage.dir/boot.o:(.bss+0x0): multiple definition of `outfile'; CMakeFiles/genisoimage.dir/genisoimage.o:(.bss+0x0): first defined here
/usr/bin/ld: CMakeFiles/genisoimage.dir/desktop.o:(.bss+0x0): multiple definition of `outfile'; CMakeFiles/genisoimage.dir/genisoimage.o:(.bss+0x0): first defined here
/usr/bin/ld: CMakeFiles/genisoimage.dir/dvd_file.o:(.bss+0x0): multiple definition of `outfile'; CMakeFiles/genisoimage.dir/genisoimage.o:(.bss+0x0): first defined here
```
Rather than patching genisoimage further, pass -fcommon to preserve the
legacy GCC behaviour.
6457361e90 qt: Fix QFileDialog for static builds (Hennadii Stepanov)
Pull request description:
This change partially reverts 248e22bbc0 (#16386) and makes `QFileDialog`s work again for static builds.
Fixes https://github.com/bitcoin-core/gui/issues/32.
ACKs for top commit:
fanquake:
ACK 6457361e90. Although it would be good to know exactly _why_ this fixes the issue. At this stage I also don't think this should be a blocker for 0.20.1.
theuni:
ACK 6457361e90
Tree-SHA512: 8ad27e0bcae6debd02f73b7c374743e37d4edd806922b103a2fe494cf2d9930fe9ef3107b5a6c61f3c466cf7462de2641171880398954e7f2c4f417f5bb820d7
de4fedb6c3 depends: Add documentation for FORCE_USE_SYSTEM_CLANG make flag (Carl Dong)
fe98999dcf depends: Reformat make options as definition list (Carl Dong)
60c55b1b9b depends: Add justifications for macOS clang flags (Carl Dong)
6b8e497eea depends: specify libc++ header location for darwin (Cory Fields)
156b604203 depends: force a new host id string if FORCE_USE_SYSTEM_CLANG is in use (Cory Fields)
c9c572a367 depends: Allow building with system clang (Carl Dong)
e6e5c8d6ca depends: Decouple toolchain + binutils (Carl Dong)
Pull request description:
This replaces: #17099
-----
This patchset allows us to force depends to use system clang.
Previously, #17099 removes our dependency on a specific clang we download from llvm.org, but theuni pointed out that since OSX builds are only ever built with a version of clang that is chosen and "blessed" by Apple, it is more likely that the user will encounter problems if they use their system clang. This patchset forces the user to set `FORCE_USE_SYSTEM_CLANG=1` in order to use their system clang (when they know what they're doing)
ACKs for top commit:
theuni:
ACK de4fedb6c3.
Tree-SHA512: 8774121e035f90c27030bcce06e1b79f7729b5e17802c718e49652ab06e19780632db974df47423c1d1b04f1ab1b7a763554fb922fec05d1cd6445b26578be1d
Debian GCC ignores -Wformat-security, without -Wformat, which
means when we test for it, it currently fails:
```bash
checking whether C++ compiler accepts -Wformat-security... no
...
configure:15907: checking whether C++ compiler accepts -Wformat-security
configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5
cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1plus: all warnings being treated as errors
```
Fix this by just combining the -Wformat and -Wformat-security checks
together.
75122780e2 Increment input value sum only once per UTXO in decodepsbt (Andrew Chow)
Pull request description:
Refactors the UTXO processing of `decodepsbt` to extract the relevant `CTxOut` and handle the input amounts from that. This avoids double counting the input value.
Fixes#19516
ACKs for top commit:
sipa:
utACK 75122780e2
ryanofsky:
Code review ACK 75122780e2
Tree-SHA512: 004ec1597790a88a98098f1a26534d10ab0130a438dec0913522a529a8d7f18ad679948617dbcad6e541fbab5bcb2682aeed386b67746807c03b64d76ce5441d
f32c408f3a Make sure unconfirmed parents are requestable (Pieter Wuille)
c4626bcd21 Drop setInventoryTxToSend based filtering (Pieter Wuille)
43f02ccbff Only respond to requests for recently announced transactions (Pieter Wuille)
b24a17f039 Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille)
a9bc563803 Swap relay pool and mempool lookup (Pieter Wuille)
Pull request description:
This implements the follow-up suggested here: https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627630111 . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.
This:
* Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627627010).
* Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see https://github.com/bitcoin/bitcoin/pull/18861#discussion_r419695831).
It adds 37 KiB of filter per peer.
This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).
ACKs for top commit:
jnewbery:
reACK f32c408f3
jonatack:
re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review.
ajtowns:
re-ACK f32c408f3a
Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
41d55d3057 doc: getpeerinfo banscore deprecation release note (Jon Atack)
dd54e3796e test: getpeerinfo banscore deprecation test (Jon Atack)
8c7647b3fb rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack)
Pull request description:
Per https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592, this PR deprecates returning the `banscore` field in the `getpeerinfo` RPC, updates the help, adds a test, and updates the release notes. Related to #19464.
ACKs for top commit:
fanquake:
ACK 41d55d3057
Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc
fc6a637a01 qt: increase console command max length (10xcryptodev)
Pull request description:
fix#17618
Tested the examples https://github.com/bitcoin/bitcoin/issues/17618#issuecomment-559538070 and works
ACKs for top commit:
MarcoFalke:
Approach ACK fc6a637a01
hebasto:
ACK fc6a637a01, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4975d7fa4c13a6b0f50f5754c3e04eb5a42b1411c385dc883d9948b6fc0dee38900ba2a418218a9a30ce39988a27d22f3ff3a02f0fa44f4136f01eef473efeca
2b78a11b48 doc: afl fuzzing comment about afl-gcc and afl-g++ (nsa)
Pull request description:
When trying to build the fuzz tests with `--enable-lcov` on a Ubuntu machine, noticed that the documentation was lacking with regards to the afl-gcc and afl-g++ options. `afl-clang-fast` and `afl-clang-fast++` in the examples just need to be replaced with `afl-gcc` and `afl-g++`. I also had to set the `-m` flag as well to get the fuzzers to run.
ACKs for top commit:
practicalswift:
ACK 2b78a11b48
MarcoFalke:
Concept ACK 2b78a11b48, haven't tested
Tree-SHA512: d8151afd79de949e8c6da49b69bbbf1470eb478c8ddcbc69b30e86bf9396c0f13835a655d4ae658f7dc4f36c35b02cd23b08358fb73a71e15bf14e76c1f365a4
1e9bfd4926 qt: Reset toolbar after all wallets are closed (Hennadii Stepanov)
Pull request description:
If the last open wallet is closed from the non-"Overview" tab, that tab remains active when a new wallet is opened:
This PR fixes this bug.
ACKs for top commit:
promag:
Code review ACK 1e9bfd4926.
luke-jr:
utACK 1e9bfd4926
jonasschnelli:
utACK 1e9bfd4926
Tree-SHA512: a8dfd7591267e9544ad40c87581d554e5cfaad4b2a5bbfdbaf2596dc6869d2ac6cf7877adfef3d528fc61b081d40c6e30d787bbd7280ef7946aa7f7d9bc8b18e
0b8ba84659 banlist: log post-swept banlist size at startup (fanquake)
Pull request description:
We are currently logging the size of the banlist from before `SweepBanned()` has been called, meaning the value may be incorrect.
i.e banlist.dat had `1`ban. That ban is swept on startup. We log "loaded 1 banned node..". Actual banlist size is `0`.
ACKs for top commit:
jonatack:
Code review ACK 0b8ba84659 `m_banned` is set in SetBanned and is updated by SweepBanned before the logging.
laanwj:
Code review ACK 0b8ba84659
jnewbery:
Code review ACK 0b8ba84659
Tree-SHA512: 1d6e363d6c68d7cc214dd685df3d2d27572f6a58a4c0e43c03cfbb03bc01badb6a10ecae403d137094bb316d27f33feb6be15b4e23ef1e9496cd0b3c23c21698
b9253c7d20 tools: clang-format 6 compatibility (Jon Atack)
Pull request description:
Our `.clang-format` settings inadvertently lost compatibility with Clang versions < 9 in #19095, including for Debian stable. This patch returns compatibility in the interim until the distros update. See discussion from https://github.com/bitcoin/bitcoin/pull/19095#issuecomment-651926138.
ACKs for top commit:
MarcoFalke:
Approach ACK b9253c7d20 , haven't tested
Tree-SHA512: 4af541a195f48d84ffb80e23aaefb624c66bc78f087c8d92b4af5a654420b69fedf25272c6e4fde2688ff88412d306b7a990ce1e15d8b24180374c625a253fb6
a8d39b8840 doc: explain why passing -mlinker-version is required (fanquake)
Pull request description:
I have been down a 🐇 hole. Closes#19359.
When Clang is compiled, [a check is run](https://github.com/llvm/llvm-project/blob/release/8.x/clang/CMakeLists.txt#L353) to define `HOST_LINK_VERSION` as the output of `$CMAKE_LINKER -v`. Note the this is the version of the linker being used to compile Clang itself.. and this check is only run when compiling Clang for macOS.
In the Clang driver, if `HOST_LINK_VERSION` has been defined, there is some additional runtime functionality. An `-mlinker-version` argument, with the value of `HOST_LINK_VERSION` [will be added to the linker arguments](https://github.com/llvm/llvm-project/blob/89de0d8dfbb9a6ff1f8b141ed70b563ecc094878/clang/lib/Driver/Driver.cpp#L382), if `-mlinker-version` has not been passed in by the user.
This is a bit weird, as by default, you are setting `-mlinker-version` to the version of the linker that was used to build the Clang binary, not the linker which will be used when compiling. The commit which introduced the functionality, https://github.com/llvm/llvm-project/commit/628fcf4e3b79ba9f41dd1b49b1aba7a20b68fc0e, described it as a "hack", that should be replaced. However, that was 10 years ago, and the behaviour is still here.
In the Darwin driver, [a check is done](https://github.com/llvm/llvm-project/blob/89de0d8dfbb9a6ff1f8b141ed70b563ecc094878/clang/lib/Driver/ToolChains/Darwin.cpp#L208) for the `-mlinker-version` argument. If there is no argument, the version will default to `0`. Given the above, this should never happen when using Clang for macOS. A series of comparisons are then performed, to check whether the linker version is modern enough to enable certain features, like [`-demangle`](https://github.com/llvm/llvm-project/blob/89de0d8dfbb9a6ff1f8b141ed70b563ecc094878/clang/lib/Driver/ToolChains/Darwin.cpp#L215).
### What this means
#### macOS
A Clang compiled for macOS, i.e `clang+llvm-8.0.0-x86_64-apple-darwin`, will have `HOST_LINKER_VERSION` set to the version of the linker used to compile Clang itself.
At runtime, `-mlinker-version=HOST_LINKER_VERSION` will be added to the linker args, if `-mlinker-version` wasn't passed in. In the Darwin driver, additional arguments, like `-demangle`, will be added to the linker arguments, because `HOST_LINKER_VERSION` was likely some very modern version of `lld` or `ld64`.
#### Linux (cross compilation in depends)
A Clang compiled for Linux, i.e `clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-14.04`, which we now use for macOS builds in depends, will behave differently. As it's built for Linux, `HOST_LINKER_VERSION` was not defined at compile time, and there will be no default behaviour of appending `-mlinker-version=HOST_LINKER_VERSION` to the linker args. Thus, unless you pass in `-mlinker-version` yourself, when the version checks are done in the Darwin driver, no modern linker features will be enabled, as the version will have defaulted to `0`.
Therefore, it's important that we continue to pass `-mlinker-version="our LD64 version"` as part of our compilation flags, if we want to have "modern" linker features enabled for our macOS builds.
#### Summary
[Clang 8](https://releases.llvm.org/download.html#8.0.0). Building a macOS binary. Link line with path arguments trimmed.
| | default behaviour | `-mlinker-version=100` (`-demangle threshold`) | `-mlinker-version=530` |
| - | --------------- | --------------------- | ---------------------- |
| macOS Clang | `-demangle -lto_library ../libLTO.dylib -no_deduplicate -dynamic -arch x86_64 -macosx_version_min 10.15.0 -o a.out ../test-b8b9b3.o -lc++ -lSystem ../libclang_rt.osx.a` | `-demangle -dynamic -arch x86_64 -macosx_version_min 10.15.0 -o a.out ../test-a66966.o -lc++ -lSystem ../libclang_rt.osx.a` | same as default |
| Linux Clang | `-dynamic -arch x86_64 -macosx_version_min 10.12.0 -o a.out ../test-bfce57.o -lc++ -lSystem` | `-demangle -dynamic -arch x86_64 -macosx_version_min 10.12.0 -o a.out ../test-a846a3.o -lc++ -lSystem` | `-demangle -lto_library ../libLTO.dylib -no_deduplicate -dynamic -arch x86_64 -macosx_version_min 10.12.0 -o a.out ../test-de0280.o -lc++ -lSystem` |
Note: Most links here are pointing to the 8.x branch of LLVM/Clang, as we are using that version in depends.
Note: To add a little more confusion, you wont see `-mlinker-version X` in your compile flags, you'll see [`-target-linker-version X`](https://github.com/llvm/llvm-project/blob/431daedee4dcce0c096c400dbf8e64dfe7254fb6/clang/lib/Driver/ToolChains/Clang.cpp#L4777).
ACKs for top commit:
laanwj:
ACK a8d39b8840
Tree-SHA512: 92f93079a5e59a0d561e74336b5cb03e3bf5a34437f5850283b9128c7624494b8285ec16290b1fa8103fe87f8789a53ce44b17902b8c1db5fde24d74b76fb168
92bc268e4a build: Detect missed pkg-config early (Hennadii Stepanov)
1739eb23d8 build: Drop unused use_pkgconfig variable (Hennadii Stepanov)
a661449a2e build: Drop use_pkgconfig check for libmultiprocess check (Hennadii Stepanov)
90b95e7929 build: Drop dead non-pkg-config code for libevent check (Hennadii Stepanov)
44a14afbb8 build: Drop dead non-pkg-config code for qrencode check (Hennadii Stepanov)
10cbae0c39 build: Drop dead non-pkg-config code for ZMQ check (Hennadii Stepanov)
06cfc9cadf build: Fix indentation in UNIVALUE check (Hennadii Stepanov)
6fd2118e77 build: Drop dead non-pkg-config code for UNIVALUE check (Hennadii Stepanov)
e9edbe4dbd build: Always use pkg-config (Hennadii Stepanov)
9e2e753b06 build: Always define ZMQ_STATIC for MinGW (Hennadii Stepanov)
Pull request description:
This PR:
- is based on #18297 (already merged)
- drops all of the non-pkg-config paths from the `configure` script
Ref: #17768
ACKs for top commit:
fanquake:
ACK 92bc268e4a. I re-gitian-built. There are a couple follow-ups that I'll PR shortly. Thanks for addressing my feedback above. I took too long to get back to this.
laanwj:
ACK 92bc268e4a
Tree-SHA512: 83c2d9cf03518867a1ebf7e26a8fc5b6dd8962ef983fe0d84e0c7eb74717f4c36a834da02faf0e503ffd87167005351671cf040c0d4ddae57ee152a6ff84012b
54b5eb2b14 tests: Add std::locale::global to list of locale dependent functions in lint-locale-dependence.sh (practicalswift)
Pull request description:
Add `std::locale::global` to list of locale dependent functions in `lint-locale-dependence.sh`.
We currently flag `setlocale(...)` as locale dependent, but prior to this commit we didn't flag
`std::locale::global(...)` as such.
In addition to setting the global C++ locale `std::locale::global(...)` also does the equivalent of `std::setlocale(LC_ALL, ...);`.
Thus the functionality of `std::locale::global(...)` is a superset of `setlocale(...)` :)
ACKs for top commit:
MarcoFalke:
ACK 54b5eb2b14, fine with me
Tree-SHA512: bcf2f1c765add6ed09c3debca968b75eeea81602503f109c0f76ec98635911d453f4834a39e741703c3d470f123178e8952191a9b1a3429394b99c07765dcf1f
This enables of the use of AI_* definitions in the Windows headers,
specifically AI_ADDRCONFIG, which fixes an issue with libevent and
ipv6 on Windows.
It also aligns with what we define in configure when building Core.
fa74a54fad ci: Increase test timeout for sanitizer configs (MarcoFalke)
Pull request description:
Hopefully fixes#19369
ACKs for top commit:
practicalswift:
ACK fa74a54fad -- patch looks correct!
fanquake:
ACK fa74a54fad - the test failure here is a different issue, and the problem referenced by this PR hasn't occurred, so I think this can be merged. It's also fixing the use of `--factor` which was replaced in #18986.
Tree-SHA512: bec44fff454f20b7c5f8a461560d2496765dea61186027cc0cdce5ac55be0488b6f7f172fec49b89fe59a75b455501e2b4ae91a98c4a17d5c1a722846d2b3b60
8578c6fccd build: Fix search for brew-installed BDB 4 on OS X (Glenn Willen)
Pull request description:
~~NOTE: This PR contains one important fix that I need (to make Bitcoin Core build cleanly on my system without shenanigans), plus some related general cleanup that is not really necessary, and could be annoying. (I am prepared to defend my argument that BDB_CFLAGS is wrong here, and BDB_CPPFLAGS is right, but this could bite anybody who has gotten in the habit of -- or scripted -- setting the former.)~~
Ok, I have been convinced that I was too clever with the refactor and I have removed it. Now it's just the tiny change to fix the build on my local machine.
---
On OS X, when searching Homebrew keg-only packages for BDB 4.8, if we find it,
use BDB_CPPFLAGS and BDB_LIBS instead of CFLAGS and LIBS for the result. This
is (1) more correct, and (2) necessary in order to give this location
priority over other directories in the include search path, which may include
system include directories with other versions of BDB.
ACKs for top commit:
theuni:
ACK 8578c6fccd.
Tree-SHA512: a28f48fc81a25736f7e77c663f21cd9a6ae1cd115682031c5aa695c94cb5afa11920330a60cd6a54832822a2aec1eb23123ac2e2dcd4f0b3835aef9c9339ac97
The way we currently test with AC_CHECK_DECLS do not work with Clang:
```bash
configure:21492: clang++-10 -std=c++11 -c -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
conftest.cpp:100:10: error: builtin functions must be directly called
(void) __builtin_clz;
^
1 error generated.
```
This also removes the __builtin_clz() check, as we don't actually use
it anywhere, and it's trvial to re-add detection if we do start using
it at some point.
adf543d714 darwin: pass mlinker-version so that clang enables new features (Cory Fields)
2418f739f7 macos: Bump to xcode 11.3.1 and 10.15 SDK (Cory Fields)
5c2c835433 depends: bump MacOS toolchain (Cory Fields)
85b5e42088 contrib: macdeploy: Remove historical extraction notes (Carl Dong)
351beb5c9a contrib: macdeploy: Use apple-sdk-tools instead of xar+pbzx (Carl Dong)
fbcfcf6954 native_cctools: Don't use libc++ from pinned clang (Carl Dong)
3381e4a189 Adapt rest of tooling to new SDK naming scheme (Carl Dong)
b3394ab235 contrib: macdeploy: Correctly generate macOS SDK (Carl Dong)
Pull request description:
This PR achieves 3 main things:
1. It simplifies the macOS SDK generation by putting the logic inside a (semi-)portable python3 script `gen-sdk`
2. It transitions us to using `libc++` headers extracted from the `Xcode.app`, which is more correct as those headers better match the `.tbd` library stubs we use from the `MacOSX.sdk` (located under the same `Xcode.app`). Previously, we used `libc++` headers copied from our downloaded, pinned clang (see `native_cctools.mk`).
3. It bumps the macOS toolchain in a way that fulfills all of the following constraints:
1. The new SDK should support compiling with C++17 (our current one doesn't)
2. The new toolchain should not change our minimum supported macOS version (`-mmacosx-version-min`)
3. The new toolchain should expect to use a version of `cctools` that is supported by https://github.com/tpoechtrager/cctools-port
For the constraints in (3), you can reference [this chart](https://en.wikipedia.org/wiki/Xcode#Xcode_7.0_-_11.x_(since_Free_On-Device_Development)) to see that the newest toolchain we can use with our `cctools-port` is `11.3.1`, and the rest of the constraints were tested with local builds.
#### But [the other Wikipedia chart](https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_(since_SwiftUI_framework)) says that the "min macOS to run" for Xcode 11.3.1 is 10.14.4, doesn't that violate constraint (ii)?
This confused me at first too, but the "min macOS to run" is for the Xcode.app App itself. The SDK still supports 10.12, as evident in a few plist files and as proven through local builds.
#### Why bundle all of this together in a single PR?
We need (1) and (2) together, because if we don't, manually adding the `libc++` headers and writing that out in a `README.md` is going to result in a lot of user error, so it's great to have these together to be more correct and also make it easier on the user at the same time.
We need (3) together with everything else because bumping (or in the case of (1), renaming) the SDK requires some human coordination and may break some builds. And since it's not that complicated a change, it makes sense to do it together with the rest.
ACKs for top commit:
theuni:
ACK adf543d714.
fanquake:
ACK adf543d714 - I'll take a look at the linker issue.
Tree-SHA512: 3813b69ebfe9610bee14100f26296fb5438d9bf0dd184ea55e6c38f5ebd94f7c171d98b150fc9e52fde626533f347f7ec51a2b72b79859d946284f578c1084a3
fa1904e5f0 net: Remove dead logging code (MarcoFalke)
fac12ebf4f net: Avoid redundant and confusing FAILED log (MarcoFalke)
Pull request description:
Remove a redundant and confusing "FAILED" log message and gets rid of the unused return type in `ProcessMessage`
ACKs for top commit:
jnewbery:
utACK fa1904e5f0
gzhao408:
utACK https://github.com/bitcoin/bitcoin/commit/fa1904e5f0d164fbcf41398f9ebbaafe82c28419
troygiorshev:
ACK fa1904e5f0
naumenkogs:
utACK fa1904e
Tree-SHA512: bfa553d5efa022727ed17877fb7c08c14849d804fe6d6a7ce172d513857beba35de41ea40b27ff1aedf68b81e2cda7b2a948ac985fcaaf1b6cfb96cce4837c90
f52d403b81 [net] split PushInventory() (John Newbery)
Pull request description:
PushInventory() is currently called with a CInv object, which can be a
MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine
whether to add the hash to setInventoryTxToSend or
vInventoryBlockToSend.
Since the caller always knows what type of inventory they're pushing,
the CInv is wastefully constructed and thrown away, and tx/block relay
is being split out, we split the function into PushTxInventory() and
PushBlockInventory().
ACKs for top commit:
amitiuttarwar:
utACK f52d403b81. nice cleanup, this has bothered me :)
naumenkogs:
utACK f52d403
sipa:
utACK f52d403b81
Tree-SHA512: 331495199a3b1a2620e6a62beb336e494291b725d8fd64bb44726c02e80807f3974ff4f329bb0f059088e65cd7d41eff276c1065806d2dd6e72c5a9f368e82cd
83fd3a6d73 init: use std::thread for ThreadImport() (fanquake)
Pull request description:
[Mentioned](https://github.com/bitcoin/bitcoin/pull/19142#issuecomment-638090759) in #19142, which removed the `boost::interruption_point()`
in `ThreadImport()`.
ACKs for top commit:
hebasto:
ACK 83fd3a6d73, I have reviewed the code and it looks OK, I agree it can be merged.
donaloconnor:
ACK 83fd3a6
laanwj:
Code review ACK 83fd3a6d73
MarcoFalke:
ACK 83fd3a6d73
Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821
fa84edb93c build: don't warn when doxygen isn't found (fanquake)
Pull request description:
Doxygen isn't so important that we need to warn when it is missing. I'd
assume it might even be missing more often than not for most builds.
ACKs for top commit:
MarcoFalke:
Fine with me ACK fa84edb93c
hebasto:
ACK fa84edb93c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 793ebf01a8a5d48b78a70fdef0022633fca59b30074c960ebb21589e3bd98992b8304621a2d999195d12172ed30fe9eefeeb2a952d58853cf58e8d9902b0090c
47b49a05ea contrib: Fix SyntaxWarning in Python base58 implementation (Alex Willmer)
Pull request description:
In Python integers should be compared for equality (`i == j`), not identity (`i is j`). Recent versions of CPython 3.x emit a SyntaxWarning when they encounter this incorrect usage, e.g.
```
$ python3 base58.py
base58.py:110: SyntaxWarning: "is" with a literal. Did you mean "=="?
assert get_bcaddress_version('15VjRaDX9zpbA8LVnbrCAFzrVzN7ixHNsC') is 0
Tests passed
```
ACKs for top commit:
MarcoFalke:
ACK 47b49a05ea
Tree-SHA512: 9f8962025dcdfa062c0515c68a1864f5bbeb86bd0510c0ec0e413a5edb6afbfd5f41b4c0255784e53db8eaf39c68b7cfa7cc8a33a2e5214aae463fda374f8719
fa193c6b1b Add missing includes to fix compile errors (MarcoFalke)
fa09ec83f3 Remove unused variables (MarcoFalke)
Pull request description:
This is required for #19183, but seems like good cleanup that can go in upfront.
ACKs for top commit:
practicalswift:
ACK fa193c6b1b -- patch looks correct
hebasto:
ACK fa193c6b1b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 79b94e7f7ee3a1a8a8fb2ea1ecdf61f130f8b133a37865894da3dbbbf311979e7d1fc013b923fdd7dbf19a221e0232f664defbdb57aa44e0b8c45bfff3c71dcb
3a10d935ac [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430c65 [test] test disconnect for filterclear (gzhao408)
1c6b787e03 [netprocessing] disconnect node that sends filterclear (gzhao408)
Pull request description:
Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.
Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204).
Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.
ACKs for top commit:
jnewbery:
Code review ACK 3a10d935ac
naumenkogs:
utACK 3a10d93
gillichu:
tested ACK: quick test_runner on macOS [`3a10d93`](https://github.com/bitcoin/bitcoin/commit/3a10d935ac8ebabdfd336569d943f042ff84b13e)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
49236be099 [tests] Don't import asyncio to test magic bytes (John Newbery)
Pull request description:
Simplify the test for invalid start bytes. No need to import asyncio and the Network thread.
ACKs for top commit:
MarcoFalke:
review ACK 49236be099
jonatack:
ACK 49236be099
troygiorshev:
ACK 49236be. +0.1 on the additional `cut_len` reformat.
Tree-SHA512: 75cb695603cdc1be7035d7b5117dbef2a1fdb29fd4414a73d75b53d563d6fa800c31bfa9475004622c8bdea4978b51b2055d6fa7be0fe47c7ae34ccc2b0e89a0
8a26848c46 build: Fix m4 escaping (Hennadii Stepanov)
9123ec15db build: Remove extra tokens warning (Hennadii Stepanov)
fded4f48c3 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov)
05a93d5d96 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov)
ddbb419310 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov)
492971de35 build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov)
Pull request description:
This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](https://github.com/bitcoin/bitcoin/pull/8314#issue-76644643) by Cory Fields:
> I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG)
There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt:
- for macOS host (similar to, but not the same as #16391)
- for Windows host (regression)
The fix is ~on its way~ submitted in #18298 (as a followup).
Also this PR picks some small improvements from #17820.
ACKs for top commit:
theuni:
Code review ACK 8a26848c46
dongcarl:
Code Review ACK 8a26848c46
laanwj:
Code review ACK 8a26848c46
Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
This was left in after #17165, so that anyone who had been compiling
with (already disabled by default) BIP70 would realise that support
had been completely removed in 0.20.0. However we should be able to
remove it for 0.21.0.
fa98e10d5e test: Remove leftover comment in mining_basic (MarcoFalke)
faedb50d89 test: pep-8 mining_basic (MarcoFalke)
Pull request description:
Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing
ACKs for top commit:
adamjonas:
code review ACK fa98e10
Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40
948f1134bc gui: add missing translation.h include to fix build (fanquake)
Pull request description:
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
ACKs for top commit:
hebasto:
ACK 948f1134bc, tested on Linux Mint 19.3 (x86_64): it fixes compiling error with the `--disable-wallet` configure option.
Tree-SHA512: db0197b110b3a7d05af2ceb29fbe9eeb6521d28f53b6267aa6d07a975886adb5c6485af79506ab6c66ed101e32292feeaff3707cdbc11432e5b97400953d5631
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
cb38b069b0 util: Don't reference errno when pthread fails. (MIZUTA Takeshi)
Pull request description:
Pthread library does not set errno.
Pthread library's errno is returned by return value.
ACKs for top commit:
practicalswift:
ACK cb38b069b0 -- patch looks correct
MarcoFalke:
review ACK cb38b069b0
hebasto:
ACK cb38b069b0, only squashed commits since the [previous](https://github.com/bitcoin/bitcoin/pull/19194#pullrequestreview-425831739) review.
Tree-SHA512: e6c950e30726e5031db97a7b84c8a9215da5ad3e5d233bcc349f812ad15957ddfe378e26d18339b9e0a5dcac2f50b47a687b87a6a6beaf6139df84f31531321e
fac6b9b938 test: Avoid overwriting the NodeContext member of the testing setup (MarcoFalke)
fa16e7816b build: Add -Wshadow-field (MarcoFalke)
Pull request description:
Adding this warning will eliminate unexpected test failures and hard to review code. Moreover, there shouldn't be a use case in Bitcoin Core that relies on fields to be shadowed.
ACKs for top commit:
fanquake:
ACK fac6b9b938 - Warnings compiling fa16e7816b are below. No warnings with fac6b9b938. The `-Wshadow-field` diagnostic has been available in Clang since 5.0.0. It's not available for GCC.
practicalswift:
ACK fac6b9b938 -- patch looks correct
hebasto:
ACK fac6b9b938, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: 824874ca10877efea7463cf934a2953147f3f99c486f04679426c14ff968975d8652cbba5729bfb7cb2c86c637ade5d1e5b873d611c06bad013a7cad8427e2bf
5d77549d8b doc: Add mypy to test dependencies (Hennadii Stepanov)
7dda912e1c test: Do not swallow flake8 exit code (Hennadii Stepanov)
Pull request description:
After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken.
This PR:
- combines exit codes of `flake8` and `mypy` into the `test/lint/lint-python.sh` exit code
- documents `mypy` as the test dependency
ACKs for top commit:
MarcoFalke:
Approach ACK 5d77549d8b, fine with me
practicalswift:
ACK 5d77549d8b
Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
fa7e002d52 ci: tsan with wallet (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK fa7e002d52 -- patch looks correct and Travis is happy
hebasto:
ACK fa7e002d52, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 1138459bbef72f402f32dae1e28d96f174901d4248d959b538b973747c8b06f221ecd81a386a1f915c0a2faaefb9fb8f11e3be39e6c5e2468bf9ae43d9f97757
89f9fef1f7 refactor: Specify boost/thread/thread.hpp explicitly (Hennadii Stepanov)
fad8c890f5 txdb: Remove unused boost/thread (MarcoFalke)
faa958bc28 txindex: Remove unused boost/thread (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, non-boost threads such as `std::thread` or the `main()` thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a `boost::thread`.
Most of them were accompanied by a `ShutdownRequested` anyway. So even if the current thread was a `boost::thread`, the interruption point would be redundant. (We only interrupt threads during shutdown)
ACKs for top commit:
fanquake:
ACK 89f9fef1f7
hebasto:
ACK 89f9fef1f7, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios.
Tree-SHA512: 17221dadedf2d107e5bda9e4f371cc4f8ffce6ad27cae41aa2b8f1150d8f1adf23d396585ca4a2dd25b1dc6f0d5c81fecd950d8557966ccb45a6d4a85a331d90
fab7d954b2 test: Make valgrind.supp work on aarch64 (MarcoFalke)
Pull request description:
Was easy to fix by simply removing a line
ACKs for top commit:
practicalswift:
ACK fab7d954b2 -- diff looks correct
Tree-SHA512: d2d7c6cac453a3177c20e256ec50a03066f8dbf5ae45299077ccf4a2b45a3a40252b1b5fcaf9224a59bb5c3df5bd90ac58af27eb0f47dc87c2640df5b2b460ca
0fef60c63d build: improved output of configure for build OS (sachinkm77)
Pull request description:
The purpose of this fix is to improve output of the configure script by providing the build OS. This is done by leveraging the build_os set by the script config.sub / config.guess. #18966
ACKs for top commit:
fanquake:
ACK 0fef60c63d - thanks for following up.
Tree-SHA512: b9f49df901a9d37eb16c67c063bb3611602a84391aa54d097a52b740f474c2785c24bf405522d15d724fde25070d354bf20b885add2ee4405a71cbe9ebab5ff3
bd7e530f01 This PR adds initial support for type hints checking in python scripts. (Kiminuo)
Pull request description:
This PR adds initial support for type hints checking in python scripts.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
[Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
**Notes:**
* [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful.
* We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
```python
_opcode_instances = [] # type: List[CScriptOp]
```
to
```python
_opcode_instances:List[CScriptOp] = []
```
for type hints that are **not** function parameters and function return types.
**Useful resources:**
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
ACKs for top commit:
fanquake:
ACK bd7e530f01 - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).
MarcoFalke:
ACK bd7e530f01 fine with me
Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
faf62e6ed0 ci: Remove unused workaround (MarcoFalke)
fa7c850915 ci: Install llvm to get llvm symbolizer (MarcoFalke)
fa563cef61 test: Add more tsan suppressions (MarcoFalke)
fa0cc02c0a ci: Mute depends logs completely (MarcoFalke)
fa906bf298 test: Extend tsan suppressions for clang stdlib (MarcoFalke)
fa10d85079 ci: Use libc++ instead of libstdc++ for tsan (MarcoFalke)
fa0d5ee112 ci: Set halt_on_error=1 for tsan (MarcoFalke)
fa2ffe87f7 ci: Deduplicate DOCKER_EXEC (MarcoFalke)
fac2eeeb9d cirrus: Remove no longer needed install step (MarcoFalke)
Pull request description:
According to the [ThreadSanitizer docs](https://clang.llvm.org/docs/ThreadSanitizer.html#current-status):
> C++11 threading is supported with **llvm libc++**.
For example, the thread sanitizer build is currently not checking for double lock of mutexes.
Fixes (partially) https://github.com/bitcoin/bitcoin/issues/19038#issuecomment-632138003
ACKs for top commit:
practicalswift:
ACK faf62e6ed0
fanquake:
ACK faf62e6ed0
hebasto:
ACK faf62e6ed0, maybe re-organize commits to modify suppressions in a single one?
Tree-SHA512: 98ce5154b4736dfb811ffdb6e6f63a7bc25fe50d3b73134404a8f3715ad53626c31f9c8132dbacf85de47b9409f1e17a4399e35f78b1da30b1577167ea2982ad
fanquake
