d4187e4619 [test] Use mocktime in test_seed_peers() (Dhruv Mehta)
015637dd44 [refactor] Correct log message in net.cpp (Dhruv Mehta)
Pull request description:
The test now takes less than 5 seconds instead of more than 2 minutes
Further context: https://github.com/bitcoin/bitcoin/pull/19884/files#r575336503
Before:
```
2021-02-12T17:22:25.980000Z TestFramework (INFO): Test seed peers, this will take about 2 minutes
2021-02-12T17:24:30.472000Z TestFramework (INFO): Test -networkactive option
```
After:
```
2021-02-12T17:33:39.224000Z TestFramework (INFO): Test seed peers
2021-02-12T17:33:43.139000Z TestFramework (INFO): Test -networkactive option
```
Top commit has no ACKs.
Tree-SHA512: 6d8df7d4433c96268694577e4c10a346785e076d45fa220091875e55def200100e7b827fac2a1f7853a2c2c39e9661e06288dca8c645da9e13d4318a4ff2172e
fe3e993968 [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta)
Pull request description:
Closes#19795
Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.
To reproduce:
`rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code
Other changes in the PR:
- `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds.
ACKs for top commit:
LarryRuane:
re-ACK fe3e993968
laanwj:
re-ACK fe3e993968
Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f
060a2a64d4 ci: remove boost thread installation (fanquake)
06e1d7d81d build: don't build or use Boost Thread (fanquake)
7097add83c refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef8 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d4
vasild:
ACK 060a2a64d4
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
de4238f92f build: consolidate reduced export checks (fanquake)
012bdec1b7 build: add building libconsensus to end-of-configure output (fanquake)
8f360e349e build: remove ax_gcc_func_attribute macro (fanquake)
f054a089ec build: remove AX_GCC_FUNC_ATTRIBUTE test for dllimport (fanquake)
7cd0a69664 build: test for __declspec(dllexport) in configure (fanquake)
1624e17b54 build: remove duplicate visibility attribute detection (fanquake)
Pull request description:
Darwin targets do not have a `protected` visibility function attribute, see [LLVM explanation](8e9a505139/clang/lib/Basic/Targets/OSTargets.h (L131)). This means that the `AX_GCC_FUNC_ATTRIBUTE` check for `visibility` fails:
```bash
configure:24513: checking for __attribute__((visibility))
configure:24537: g++ -std=c++11 -o conftest -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -Wl,-headerpad_max_install_names conftest.cpp >&5
conftest.cpp:35:56: warning: target does not support 'protected' visibility; using 'default' [-Wunsupported-visibility]
int foo_pro( void ) __attribute__((visibility("protected")));
^
1 warning generated.
configure:24537: $? = 0
configure:24550: result: no
```
This leads to `EXPORT_SYMBOL` being [defined to nothing](f4de89edfa/src/script/bitcoinconsensus.h (L29)), as `HAVE_FUNC_ATTRIBUTE_VISIBILITY` is not defined, and when building with reduced exports, you end up with a libbitcoinconsensus.dylib that doesn't export any `_bitcoinconsensus_*` symbols.
```bash
➜ git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
➜ git:(master)
```
We do have a [second check](f4de89edfa/configure.ac (L882)) for the `visibility` attribute, which works for Darwin as it's only testing for default visibility, however the result of this check isn't used at all. It was added in #4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in #5235, it used the results of the added `AX_GCC_FUNC_ATTRIBUTE` calls.
This PR removes our usage of the AX_GCC_FUNC_ATTRIBUTE macro entirely, in favour of our own checks in configure. This meant adding a check for `dllexport`, which I've tested as working with both [GCC](https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html) and [Clang](https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#dllexport) when building for Windows. I haven't added an equivalent check for `dllimport`, as we weren't actually using the result of that check, we're just testing that `MSC_VER` was defined before using.
With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected:
```bash
./autogen.sh
./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes --enable-reduce-exports
make -j8
...
nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
000000000000a340 T _bitcoinconsensus_verify_script
00000000000097e0 T _bitcoinconsensus_verify_script_with_amount
000000000000a3c0 T _bitcoinconsensus_version
```
```python
>>> import ctypes
>>> consensus = ctypes.CDLL("src/.libs/libbitcoinconsensus.dylib")
>>> print(consensus.bitcoinconsensus_version())
1
>>> exit()
```
TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib?
ACKs for top commit:
laanwj:
Code review ACK de4238f92f
Tree-SHA512: d148f3c55d14dac6e9e5b718cc65bb557bcf6f663218d24bc9044b86281bd5dd3d931ebea79c336a58e8ed50d683218c0a9e75494f2267b91097665043e252ae
e987ae5a55 test: Add test for deterministic UTXO set hash results (Fabian Jahr)
6ccc8fc067 test: Add test for gettxoutsetinfo RPC with MuHash (Fabian Jahr)
0d3b2f643d rpc: Add hash_type MUHASH to gettxoutsetinfo (Fabian Jahr)
2474645f3b refactor: Separate hash and stats calculation in coinstats (Fabian Jahr)
a1fcceac69 refactor: Improve encapsulation between MuHash3072 and Num3072 (Fabian Jahr)
Pull request description:
This is another Pr in the series PRs for Coinstatsindex (see overview in #18000). This PR adds the `hash_type` option `muhash` to `gettxoutsetinfo` through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet.
ACKs for top commit:
Sjors:
tACK e987ae5
achow101:
ACK e987ae5a55
jonatack:
Tested re-ACK e987ae5a55 per `git diff 3506d90 e987ae5`, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned `hash_serialized_2` of `2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454` and this branch returned `muhash` of `c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c`
ryanofsky:
Code review ACK e987ae5a55. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review.
Tree-SHA512: 9a739ce375e73749fa69a467262b60d3e5314ef384e2d7150b3bbc8e4125cd9fd1db95306623bb9a632fcbaf5d9d2bf2f5cc43bf717d4ff5e2c9c4b52dd9296c
We are already testing for this, and our test works correctly with a Darwin
target, where the macro does not. Darwin targets do not support "protected"
visibility.
25f899cc23 log: Move "Pre-allocating up to position 0x[...] in [...].dat" log message to debug category (practicalswift)
acd7980b37 log: Move "Leaving block file [...]: [...]" log message to debug category (practicalswift)
Pull request description:
Move `Pre-allocating up to position 0x[…] in […].dat` log message to debug category.
After the cleanup of `-debug=net` log messages PR (#20724) was merged recently the console log now has very high signal to noise ratio. That's great! :)
This PR increases the signal to noise ratio slightly more by moving the most common remaining implementation detail log message (`Pre-allocating up to position 0x[…] in […].dat`) to the debug category where it belongs :)
Expected standard output from `bitcoind` (when in steady state) before this patch:
```
$ src/bitcoind
…
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z Pre-allocating up to position 0x0000000 in blk00000.dat
0000-00-00T00:00:00Z Pre-allocating up to position 0x000000 in rev00000.dat
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
```
Expected standard output from `bitcoind` (when in steady state) after this patch:
```
$ src/bitcoind
…
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
```
I find the latter alternative much easier to visually scan for anomalies (and more aesthetically pleasing TBH!).
Non-GUI users deserve nice interfaces too :)
ACKs for top commit:
laanwj:
ACK 25f899cc23
Tree-SHA512: 5970798c41b041527ebdcbd843c5e136c257c28c3b21fc74102da8970406ca5c0c7e406305c5e6e67de5c1708dc1858af07a77a2e05f44159b7103423e8ab32f
fabce459bb fuzz: version handshake (MarcoFalke)
Pull request description:
Not fuzzing the version handshake will limit fuzz coverage
ACKs for top commit:
practicalswift:
cr ACK fabce459bb: patch looks very much correct
Tree-SHA512: 4091d27d39edee781d033e471b352084bb54df250d0890e4821a325926a44dff9b26a2614d67dd0529f73bd366b075d7a0a1a570c2837de286a1b93a59a8fb91
53e716ea11 [refactor] improve style for touched code (gzhao408)
174cb5330a [refactor] const ATMPArgs and non-const Workspace (gzhao408)
f82baf0762 [refactor] return MempoolAcceptResult (gzhao408)
9db10a5506 [refactor] clean up logic in testmempoolaccept (gzhao408)
Pull request description:
This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:
- It makes accessing values that don't make sense (e.g. fee) when the tx is invalid an error.
- Returning `MempoolAcceptResult` from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable "out" param.
- We don't have to be iterating through a bunch of lists for package validation, we can just return a `std::vector<MempoolAcceptResult>`.
- We don't have to refactor all ATMP call sites again if/when we want to return more stuff from it.
ACKs for top commit:
MarcoFalke:
ACK 53e716ea11💿
jnewbery:
Code review ACK 53e716ea11
ariard:
Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn't find holes.
Tree-SHA512: fa6ec324a08ad9e6e55948615cda324cba176255708bf0a0a0f37cedb7a75311aa334ac6f223be7d8df3c7379502b1081102b9589f9a9afa1713ad3d9ab3c24f
615ba0eb96 test: add Sock unit tests (Vasil Dimov)
7bd21ce1ef style: rename hSocket to sock (Vasil Dimov)
04ae846904 net: use Sock in InterruptibleRecv() and Socks5() (Vasil Dimov)
ba9d73268f net: add RAII socket and use it instead of bare SOCKET (Vasil Dimov)
dec9b5e850 net: move CloseSocket() from netbase to util/sock (Vasil Dimov)
aa17a44551 net: move MillisToTimeval() from netbase to util/time (Vasil Dimov)
Pull request description:
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
ACKs for top commit:
laanwj:
Re-ACK 615ba0eb96
jonatack:
re-ACK 615ba0eb96
Tree-SHA512: 3003e6bc0259295ca0265ccdeb1522ee25b4abe66d32e6ceaa51b55e0a999df7ddee765f86ce558a788c1953ee2009bfa149b09d494593f7d799c0d7d930bee8
3ddbf22ed1 util: Disallow negative mocktime (MarcoFalke)
f5f2f97168 net: Avoid UBSan warning in ProcessMessage(...) (practicalswift)
Pull request description:
Avoid UBSan warning in `ProcessMessage(...)`.
Context: https://github.com/bitcoin/bitcoin/pull/20380#issuecomment-770427182 (thanks Crypt-iQ!)
ACKs for top commit:
MarcoFalke:
re-ACK 3ddbf22ed1 only change is adding patch written by me
ajtowns:
ACK 3ddbf22ed1 -- code review only
Tree-SHA512: e8d7af0457ca86872b75a4e406c0a93aafd841c2962e244e147e748cc7ca118c56be0fdafe53765f4b291410030b2c3cc8f76f733b37a955d34fc885ab6037b9
fa650ca7f1 Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f1: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
fa4bc897fc fuzz: Fail if message type is not fuzzed (MarcoFalke)
faefed8cd5 fuzz: Count message type fuzzers before main() (MarcoFalke)
Pull request description:
`process_message_*` is a nice way to quickly fuzz a single message type. However, the offered message types are outdated and all BIPs implemented in the last years are missing.
Fix that by adding them and failing when the number of message types don't add up.
ACKs for top commit:
practicalswift:
cr ACK fa4bc897fc: patch looks correct and touches only `src/test/fuzz/`
Tree-SHA512: 8c98374b50fb4ab2ff2550daeab4c6e9f486bfe847466d217d4bc97d119adc99a82b87b56f47535b1cf8f844232bc7fa1230712a9147cda514ae78851556f988
fabeb5b9c7 fuzz: Disable shuffle when merge=1 (MarcoFalke)
Pull request description:
This should hopefully help make the deletion of fuzz inputs more deterministic.
My tests (N=1) revealed that without this patch 7000 files differ (https://github.com/bitcoin-core/qa-assets/pull/44#issuecomment-768841467). With this patch, "only" 2000 files differ.
ACKs for top commit:
practicalswift:
cr ACK fabeb5b9c7: `-shuffle=0` and `-prefer_small=1` make sense
Tree-SHA512: 21a701f52450d402a91dd6e0b33d564c63a9c3b919738eb9a80c24d48fc5b964088e325470738f39af0d595612c844acc7bf0941590cc2dc8c6f6ee4cb69c861
fac726b1b8 doc: Fixup docs in fuzz/script_assets_test_minimizer.cpp (MarcoFalke)
fafca47adc fuzz: Hide script_assets_test_minimizer (MarcoFalke)
Pull request description:
This is not an actual fuzz target. It is a hack to exploit the built-in capability of fuzz engines to measure coverage.
ACKs for top commit:
practicalswift:
cr ACK fac726b1b8: patch looks correct and touches only `src/test/fuzz/`
Tree-SHA512: 0652dd8d9e95746b0906be4044467435d8204a34a30366ae9bdb75b9cb0788d429db7cedf2760fd543565d9d4f7ee206873ed10a29dd715a792a26337f65b53c
fa2c521115 Deduplicate some block-to-JSON code. (Daniel Kraft)
Pull request description:
Some of the logic converting blocks and block headers to JSON for the blockchain RPC methods (`getblock`, `getlockheader`) was duplicated. Instead of that, the `blockToJSON` RPC method now calls `blockheaderToJSON` first, and then fills in the missing, block-specific bits explicitly.
ACKs for top commit:
laanwj:
Code review ACK fa2c521115
Tree-SHA512: 1b9b269e251d9c8c1056f253cfc2a795170d609f4c26ecc85b1ff9cdd567095a673dd89564e0d587b32dfc152ea01b2682169b018f2c9c3004c511a9998d772e
7a135d57b2 Add EditorConfig file. (Kiminuo)
Pull request description:
### Motivation
Developers are supposed to follow [Coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-general). However, [from time to time](https://github.com/bitcoin/bitcoin/pull/21075#discussion_r570125634) a PR is created and then its author is asked to change tabs to spaces, for example.
Introducing an `.editorconfig` file can mitigate these formatting issues.
### User story
A contributor wants to create a new PR. She clones Bitcoin Core repo, opens her editor, the editor loads `.editorconfig` rules and writes her patch with correct formatting rules. Less Coding Style issues is then discovered in the PR review process and thus less CI runs are needed.
### What is EditorConfig file?
https://editorconfig.org provides very well and concise explanation:
> What is EditorConfig?
> EditorConfig helps maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs. The EditorConfig project consists of a file format for defining coding styles and a collection of text editor plugins that enable editors to read the file format and adhere to defined styles. EditorConfig files are easily readable and they work nicely with version control systems.
### Support
`.editorconfig` is supported by many IDEs and text editors. Sometimes, the support is out of the box and sometimes a plugin is needed. However, for example, VS Code detects `.editorconfig` presence and automatically offers you to install the missing plugin.
See https://editorconfig.org/#pre-installed for details on support. To name a few:
* Visual Studio (out of the box)
* VS Code (plugin)
* JetBrains IDEs (IntelliJ IDEA, PyCharm, etc.) (out of the box)
* Sublime Text (plugin)
* Emacs (plugin)
* Vim (plugin)
Not supported (AFAIK):
* [mcedit](https://github.com/MidnightCommander/mc)
### My editor does not support `.editorconfig`
Then nothing really changes for you.
### `.editorconfig` vs `.clang-format`
As explained [here](https://devblogs.microsoft.com/cppblog/clangformat-support-in-visual-studio-2017-15-7-preview-1/):
> Note that Visual Studio also supports EditorConfig, which works in a similar way. ClangFormat, however, has a [much larger variety of style options](https://clang.llvm.org/docs/ClangFormatStyleOptions.html) than EditorConfig, including some very C++ specific rules that can be set, and it is already used by C++ developers today.
Having both `.editorconfig` and `.clang-format` in a project, may not always work correctly though, I think. As some editors may have a plugin for `.editorconfig` and a plugin for `clang-formatter` which may not work correctly in unison. In VS Code & Visual Studio EditorConfig [takes precedence over other settings](https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/ide/cpp-editorconfig-properties.md#c-editorconfig-formatting-conventions).
### Possible issues
Your editor may change formatting for some 3rd party library if you edit the code. A solution for this would be to make EditorConfig rules more specific (include only certain paths). I'm not sure if it is an issue in practice.
### Testing
Add some trailing whitespace to a Python file and save the file. You should see that the trailing whitespace is removed.
### Possible future work
It would be great to define rules for Makefiles. This would be good start:
```
# Makefiles
[Makefile,*.am]
indent_style = tab
trim_trailing_whitespace = true
```
I don't know makefiles in this project good enough to propose something reasonable. If this PR is well received, it would be great to add it in this PR.
Also, there are actually many different file extensions and so the proposed `.editorconfig` file can be probably improved very much:
```powershell
Get-ChildItem -Recurse | % {$_.Extension.ToLower()} | sort | unique
```
<details><summary>Click to see the output</summary>
```
.1
.ac
.adoc
.am
.bash-completion
.bat
.bmp
.c
.cc
.cert
.cfg
.clang_complete
.clang-format
.cmake
.cmd
.cnf
.com
.conf
.cpp
.css
.csv
.doxyfile
.dtd
.empty
.exe
.exp
.gci
.gitattributes
.github
.gitignore
.gitmodules
.guess
.h
.hex
.hpp
.html
.icns
.ico
.idb
.ilk
.in
.include
.ini
.init
.ipp
.jam
.js
.json
.lastbuildstate
.lib
.list
.log
.m
.m4
.md
.mk
.mm
.moc
.obj
.openrc
.openrcconf
.patch
.pc
.pdb
.pl
.plist
.png
.po
.pro
.py
.python-version
.qbk
.qm
.qml
.qrc
.raw
.rb
.rc
.recipe
.res
.s
.sage
.sass
.scm
.scss
.service
.sgml
.sh
.sln
.spec
.sub
.supp
.svg
.targets
.td
.tlog
.ts
.tx
.txt
.ui
.user
.v2
.vcxproj
.verbatim
.vscode
.xml
.xpm
.xsl
.y
.yapf
.yml
.yy
```
</details>
Fixes#21092
ACKs for top commit:
laanwj:
Tested re-ACK 7a135d57b2
MarcoFalke:
Approach ACK 7a135d57b2
Tree-SHA512: c36a3424ecc751fbdd66101463b0c470f5c7adcdb4795b1cd267ff718eb345a04615fc1182338adf5b7db724469dca00c64815a9ef77064734a6536fba41a2ba
In the arguments of `InterruptibleRecv()`, `Socks5()` and
`ConnectThroughProxy()` the variable `hSocket` was previously of type
`SOCKET`, but has been changed to `Sock`. Thus rename it to `sock` to
imply its type, to distinguish from other `SOCKET` variables and to
abide to the coding style wrt variables' names.
Use the `Sock` class instead of `SOCKET` for `InterruptibleRecv()` and
`Socks5()`.
This way the `Socks5()` function can be tested by giving it a mocked
instance of a socket.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
Move `CloseSocket()` (and `NetworkErrorString()` which it uses) from
`netbase.{h,cpp}` to newly added `src/util/sock.{h,cpp}`.
This is necessary in order to use `CloseSocket()` from a newly
introduced Sock class (which will live in `src/util/sock.{h,cpp}`).
`sock.{h,cpp}` cannot depend on netbase because netbase will depend
on it.
faa8afe70b ci: Re-run wine tests once if they fail (MarcoFalke)
Pull request description:
Works around the intermittent wine issue: https://github.com/bitcoin/bitcoin/issues/21122#issuecomment-776517563
ACKs for top commit:
fanquake:
ACK faa8afe70b - thanks for following up with this.
Tree-SHA512: cb377a8e62c7fcf38fb5bdd6203af82c6c302c86f7fc219cb756eb7b6d3d4fe4ebf30a36ebba739199ff7814b26256a3701cf38dc5e681b18c2f249cdf6d852e
Move `MillisToTimeval()` from `netbase.{h,cpp}` to
`src/util/system.{h,cpp}`.
This is necessary in order to use `MillisToTimeval()` from a newly
introduced `src/util/sock.{h,cpp}` which cannot depend on netbase
because netbase will depend on it.
059e8ccc1e Change BOOST_CHECK to BOOST_CHECK_EQUAL to see mismatched values when a check fails. (Kiminuo)
Pull request description:
This is useful to see mismatched values when a check fails as specified in the [Boost documentation](https://www.boost.org/doc/libs/1_71_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level.html).
This PR would make #20744 PR's diff smaller by a bit.
ACKs for top commit:
MarcoFalke:
review ACK 059e8ccc1e
theStack:
Code Review ACK 059e8ccc1e
Tree-SHA512: 82359ef38e0d1926f12a34aeff6fde6d1d307c703a080547749b908f873c2a2f894f6f094c33470b32987c229e3a1f17f7d1e877663c53293c023bde0e7272c1
Some of the logic converting blocks and block headers to JSON for
the blockchain RPC methods (getblock, getlockheader) was duplicated.
Instead of that, the blockToJSON RPC method now calls blockheaderToJSON
first, and then fills in the missing, block-specific bits explicitly.
fa272dfdff ci: Properly bump to focal for win cross build (MarcoFalke)
Pull request description:
Fixes#21122
ACKs for top commit:
fanquake:
ACK fa272dfdff - Looks good to me. I can see the wine version output, `wine-5.0 (Ubuntu 5.0-3ubuntu1)`, in the [log](https://cirrus-ci.com/task/5743559502462976), and `make check` is running.
Tree-SHA512: cd37462afc5512e00cef5e9e7fd1bb5c43c600e833b30cdea2c1c443dc7b0e68f5f2cbaf9d7b655892059af0a226478211db1dc97c10fe4dcdfe666a784f3afc
1bca2aa694 Introduce GetUniquePath(base) helper method to replace boost::filesystem::unique_path() which is not available in std::filesystem. (Kiminuo)
Pull request description:
This PR makes it easier in #20744 to remove our dependency on the `boost::filesystem::unique_path()` function which does not have a direct equivalent in C++17.
This PR attempts to re-implement `boost::filesystem::unique_path()` as `GetUniquePath(path)` but the implementations are not meant to be the same.
Note:
* Boost 1.75.0 implementation of `unique_path`: 9cab675b71/src/unique_path.cpp (L235)
* In the previous implementation, I attempted to add:
```cpp
fs::path GetUniquePath(const fs::path& base)
{
FastRandomContext rnd;
fs::path tmpFile = base / HexStr(rnd.randbytes(8));
return tmpFile;
}
```
to `fs.cpp` but this leads to a circular dependency: "fs -> random -> logging -> fs". That is why the modified implementation adds a new file.
ACKs for top commit:
laanwj:
Code review ACK 1bca2aa694
ryanofsky:
Code review ACK 1bca2aa694. It's a simple change and extra test coverage is nice
Tree-SHA512: f324bdf0e254160c616b5033c3ece33d87db23eb0135acee99346ade7b5cf0d30f3ceefe359a25a8e9b53ba8e4419f459c2bdd369e10fc0152ce95031d1f221c
c9095b738f test: remove unnecessary assignment in bdb (Bruno Garcia)
Pull request description:
This PR removes the unnecessary assignment to page_info['entries'] on line 54 since there is another assignment for it in line 59.
I think a lint (#21096) would detect cases like this one.
ACKs for top commit:
achow101:
ACK c9095b738f
theStack:
Code Review ACK c9095b738f
Tree-SHA512: 23377077c015b04361fd416b41bf6806ad0bdd4d264be6760f0fd3bc88d694d2cd52cae250519925c5d3b3c70715772714c3863f8fa181a2eb4883204ccdbf9d
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
