ef712298c3f8bc2afdad783f05080443b72b3f77 util: Check for file being NULL in DirectoryCommit (Luke Dashjr)
457490403853321d308c6ca6aaa90d6f8f29b4cf Fix possible data race when committing block files (Evan Klitzke)
220bb16cbee5b91d0bc0fcc6c71560d631295fa5 util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke)
ce5cbaea63ad4ea78e533bdb14f47f414061ae7f util.h: Document FileCommit function (Evan Klitzke)
844d650eea3bd809884cc5dd996a388bdc58314e util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke)
f6cec0bcaf560fa310853ad3fe17022602b63d5f util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke)
Pull request description:
Reviving #12696
ACKs for top commit:
laanwj:
Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77
Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce
31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 Don't declare de facto const member functions as non-const (practicalswift)
Pull request description:
_Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_
Changes in this PR:
* Don't declare de facto const member functions as non-const
* Don't declare de facto const reference variables as non-const
Awards for finding candidates for the above changes go to:
* `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html))
* `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/))
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`.
ACKs for top commit:
ajtowns:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
jonatack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
theStack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 ❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
faec0638872798b58b9882ee079014555bc8393e log: Use Join() helper when listing log categories (MarcoFalke)
Pull request description:
This removes the global `ListLogCategories` and replaces it with a one-line member function `LogCategoriesString`, which just calls `Join`.
Should be a straightforward refactor to get rid of a few LOC.
ACKs for top commit:
laanwj:
ACK faec0638872798b58b9882ee079014555bc8393e
promag:
ACK faec0638872798b58b9882ee079014555bc8393e, I also think it's fine as it is (re https://github.com/bitcoin/bitcoin/pull/18669#discussion_r412944724).
Tree-SHA512: 2f51f9ce1246eda5630015f3a869e36953c7eb34f311baad576b92d7829e4e88051c6189436271cd0a13732a49698506345b446b98fd28e58edfb5b62169f1c9
fa6c114ae604571435e8c4d25906a8b6d5b9984c test: Add sanitizer suppressions for AMD EPYC CPUs (MarcoFalke)
Pull request description:
Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs `Using the 'shani(1way,2way)' SHA256 implementation` (excerpt from debug log).
For reference, google cloud CPUs (which is what Cirrus CI uses) print `Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation`
The traceback I got:
```
crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18
#1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9
#2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5
#3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5
#4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27
#5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in
ACKs for top commit:
laanwj:
Anyhow ACK fa6c114ae604571435e8c4d25906a8b6d5b9984c
Tree-SHA512: 968a1d28eedec58c337b1323862f583cb1bcd78c5f03396940b9ab53ded12f8c6652877909aba05ee5586532137418fd817ff979bd7bef6e07856094f9d7f9b1
378aedc45248cea82d9a3e6dc1038d6828008a76 [net] Add cs_vSend lock annotations (John Newbery)
673254515a2f97e53dd8c7335c836b083ba7e31a [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery)
Pull request description:
RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend.
Also correctly annotate the CNode data members that are guarded by cs_vSend.
This is a simpler alternative to #19673.
ACKs for top commit:
jnewbery:
ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs.
troygiorshev:
ACK 378aedc45248cea82d9a3e6dc1038d6828008a76
theStack:
re-ACK 378aedc45248cea82d9a3e6dc1038d6828008a76
MarcoFalke:
review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌
Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07
## Issue being fixed or feature implemented
When building with `-Wreorder-ctor` capture, the build fails with
`error: field 'lastMNListForVotingKeys' will be initialized after field
'votedFundingYesTriggerHash'`
## What was done?
Moved down `votedFundingYesTriggerHash` to last position in
`CGovernanceManager`.
## How Has This Been Tested?
## Breaking Changes
no
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Implementation of issue https://github.com/dashpay/dash-issues/issues/43
## What was done?
Masternode will try to create, sign and submit a Superblock (GovTrigger)
during the `nSuperblockMaturityWindow`.
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
0000a0c7e9e4e7c1afafe6ef75b7624f4c573190 Remove confusing and almost useless "unexpected version" warning (MarcoFalke)
Pull request description:
It is useless because it isn't displayed for most users:
* It isn't displayed in normal operation (because the validation debug category is disabled by default)
* It isn't displayed for users that sync up their nodes intermittently, e.g. once a day or once a week (because it is disabled for IBD)
* It is only displayed in the debug log (as opposed to the versionbits warning, which is displayed more prominently)
It is confusing because it doesn't have a use case:
Despite the above, if a user *did* see the warning, it would most likely be a false positive (like it has been in the past). Even if it wasn't, there is nothing they can do about it. The only thing they could do is to check for updates and hope that a fixed version is available. But why would the user be so scrupulously precise in enabling the warning and reading the log, but then fail to regularly check update channels for updated software?
ACKs for top commit:
practicalswift:
ACK 0000a0c7e9e4e7c1afafe6ef75b7624f4c573190
decryp2kanon:
ACK 0000a0c
LarryRuane:
ACK 0000a0c7e9e4e7c1afafe6ef75b7624f4c573190
Tree-SHA512: 16e069c84be6ab6034baeefdc515d0e5cdf560b2005d2faec5f989d45494bd16cfcb4ffca6a17211d9556ae44f9737a60a476c08b5c2bb5e1bd29724ecd6d5c1
e5faec65bd06a3b14175aca3040290f343bd6e9c doc: Fix doxygen comment silent merge conflict in descriptor.cpp (W. J. van der Laan)
Pull request description:
It looks like #21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.
(please merge only if CI passes)
ACKs for top commit:
ajtowns:
ACK e5faec65bd06a3b14175aca3040290f343bd6e9c -- fixed it for me
meshcollider:
ACK e5faec65bd06a3b14175aca3040290f343bd6e9c modulo CI
Tree-SHA512: b07d50fd12aa7c239a92aad8ef29f4e88583c3ce701ebedba7c426aac4981c79113adc4670b7d055ab9535a28bdc3f9a30e6ca1b1ed0d7b9a333a3d9c4b40d8a
4eca20d6f7d850492d331d89d1cdd77abb3c70c1 [doc] correct comment about ATMPW (glozow)
8fa74aeb5b96419c7d40b40f8e1e1269509278e2 [doc] correct comment in chainparams (glozow)
2f8272c2a4b6fa84c04dfeb4d751bb218f2d4c78 [doc] GetBestBlock() doesn't do nothing (gzhao408)
Pull request description:
Came across a few misleading comments, wanted to fix them
ACKs for top commit:
jnewbery:
ACK 4eca20d6f7
MarcoFalke:
ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1
laanwj:
Code review ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1
Tree-SHA512: 5bef1f1e7703f304128cf0eb8945e139e031580c99062bbbe15bf4db8443c2ba5a8c65844833132e6646c8980c678fc1d2ab0c63e17105585d583570ee350fd0
fab633d2dbfed1efcc3a02061685d56327ae51fd doc: Update fuzzing docs for afl-clang-lto (MarcoFalke)
Pull request description:
Update the docs to default to `afl-clang-lto`. The afl-gcc (and other afl legacy fuzz engines) are still supported, though discouraged.
ACKs for top commit:
fanquake:
ACK fab633d2dbfed1efcc3a02061685d56327ae51fd - seems to work for me. Compiled and ran some fuzzers using Clang 11 on Bionic. Set `llvm-config` so that `clang-11` would be used over `clang` (10).
jarolrod:
ACK fab633d2dbfed1efcc3a02061685d56327ae51fd, tested on Ubuntu Focal
Tree-SHA512: 3d1969c167bea45a9d691f3b757f51213d550c9c1b895bed1fcf3c2f7345791787cfb13c376291b94eb3181caf4ae3126f4d01c7cebda7b2bb1c40a1294e9a68
ebde946a527e50630df180c6565ea5bf8d2ab5aa [doc] Improve comment about protected peers (Amiti Uttarwar)
Pull request description:
The comment currently suggests a long-standing node would infrequently protect peers under normal circumstances. Clarify that we also protect peers that are synced to the same work as our chain tip. [Relevant check here](ee0dc02c6f/src/net_processing.cpp (L1997)).
ACKs for top commit:
Empact:
ACK ebde946a52
jnewbery:
ACK ebde946a527e50630df180c6565ea5bf8d2ab5aa
Tree-SHA512: 3692f4098e95f935d801e0ee6bbd3a7c9480e66ca070a7c68ba79c4fc2e62377f5d37080c7b6a7d15ab617aaf4d3df9b26abc4f1b090d572ba46fdd092a6a64a
590bda79e876d9b959083105b8c7c41dd87706eb scripted-diff: Remove setup_clean_chain if default is not changed (Fabian Jahr)
98892f39e3d079c73bff7f2a5d5420fa95270497 doc: Improve setup_clean_chain documentation (Fabian Jahr)
Pull request description:
The first commit improves documentation on setup_clean_chain which is misunderstood quite frequently. Most importantly it fixes the TestShell docs which are simply incorrect.
The second commit removes the instances of `setup_clean_clain` in functional tests where it is not changing the default.
This used to be part of #19168 which also sought to rename`setup_clean_chain`.
ACKs for top commit:
jonatack:
ACK 590bda79e876d9b959083105b8c7c41dd87706eb
Tree-SHA512: a7881186e65d31160b8f84107fb185973b37c6e50f190a85c6e2906a13a7472bb4efa9440bd37fe0a9ac5cd2d1e8559870a7e4380632d9a249eca8980b945f3e
6f2c4fd0775a9c45eacc4bab8f138528852fdf44 netinfo: add user help documentation (Jon Atack)
Pull request description:
This is the help doc commit of #20764 without the rest of the PR or anything new since the 0.21.0 branch-off in order to target giving users a -netinfo help doc for 0.21.
- to test the new help
```
$ ./src/bitcoin-cli -netinfo help
```
- to see the updated short help
```
$ ./src/bitcoin-cli -help | grep -A4 netinfo
```
<details><summary><code>-netinfo</code> help doc</summary><p>
```
$ ./src/bitcoin-cli -netinfo help
-netinfo level "help"
Returns a network peer connections dashboard with information from the remote server.
Under the hood, -netinfo fetches the data by calling getpeerinfo and getnetworkinfo.
An optional integer argument from 0 to 4 can be passed for different peers listings.
Pass "help" to see this detailed help documentation.
If more than one argument is passed, only the first one is read and parsed.
Suggestion: use with the Linux watch(1) command for a live dashboard; see example below.
Arguments:
1. level (integer 0-4, optional) Specify the info level of the peers dashboard (default 0):
0 - Connection counts and local addresses
1 - Like 0 but with a peers listing (without address or version columns)
2 - Like 1 but with an address column
3 - Like 1 but with a version column
4 - Like 1 but with both address and version columns
2. help (string "help", optional) Print this help documentation instead of the dashboard.
Result:
* The peers listing in levels 1-4 displays all of the peers sorted by direction and minimum ping time:
Column Description
------ -----------
<-> Direction
"in" - inbound connections are those initiated by the peer
"out" - outbound connections are those initiated by us
type Type of peer connection
"full" - full relay, the default
"block" - block relay; like full relay but does not relay transactions or addresses
net Network the peer connected through ("ipv4", "ipv6", "onion", "i2p", or "cjdns")
mping Minimum observed ping time, in milliseconds (ms)
ping Last observed ping time, in milliseconds (ms)
send Time since last message sent to the peer, in seconds
recv Time since last message received from the peer, in seconds
txn Time since last novel transaction received from the peer and accepted into our mempool, in minutes
blk Time since last novel block passing initial validity checks received from the peer, in minutes
age Duration of connection to the peer, in minutes
asmap Mapped AS (Autonomous System) number in the BGP route to the peer, used for diversifying
peer selection (only displayed if the -asmap config option is set)
id Peer index, in increasing order of peer connections since node startup
address IP address and port of the peer
version Peer version and subversion concatenated, e.g. "70016/Satoshi:21.0.0/"
* The connection counts table displays the number of peers by direction, network, and the totals
for each, as well as a column for block relay peers.
* The local addresses table lists each local address broadcast by the node, the port, and the score.
Examples:
Connection counts and local addresses only
> bitcoin-cli -netinfo
Compact peers listing
> bitcoin-cli -netinfo 1
Full dashboard
> bitcoin-cli -netinfo 4
Full live dashboard, adjust --interval or --no-title as needed (Linux)
> watch --interval 1 --no-title ./src/bitcoin-cli -netinfo 4
See this help
> bitcoin-cli -netinfo help
```
</p></details>
ACKs for top commit:
laanwj:
ACK 6f2c4fd0775a9c45eacc4bab8f138528852fdf44
Tree-SHA512: dd49b1ce65546dacfb8ba9f9d57de0eae55560fd05533cf26c0b5d6ec65bf1de789c3287e90a0e2f47707532fab2fe62919a4192a7ffd58ac8eec18293e9aaeb
7b6887e75aec7e0d5c25682c26943073b7a728e5 doc: Convert depends options list from html to markdown (Wladimir J. van der Laan)
Pull request description:
This makes it easier to read in `less`, which is important for install instructions.
Rendered: [before](7ef6b1c51d/depends (dependency-options)) - [after](d97042406f/depends/README.md (dependency-options))
ACKs for top commit:
jonatack:
Code review re-ACK 7b6887e75aec7e0d5c25682c26943073b7a728e5 per `git diff d970424 7b6887e`
hebasto:
re-ACK 7b6887e75aec7e0d5c25682c26943073b7a728e5
Tree-SHA512: 02970b2bb97d2e8fb2d66470f6d70662653fda176bf6f4861742823b361fdc7ab6a2b44143480ac1a525b8d7808b6a068e8b3677dbba16cd783b4cab90470af5
e6fe1c37d0a2f8037996dd80619d6c23ec028729 rpc: Improve avoidpartialspends and avoid_reuse documentation (Fabian Jahr)
8f073076b102b77897e5a025ae555baae3d1f671 wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 (Fabian Jahr)
Pull request description:
Follow-up to #17824.
This increases OUTPUT_GROUP_MAX_ENTRIES to 100 which means that OutputGroups will now be up to 100 outputs large, up from previously 10. The main motivation for this change is that during the PR review club on #17824 [several participants signaled](https://bitcoincore.reviews/17824.html#l-339) that 100 might be a better value here.
I think fees should be manageable for users but more importantly, users should know what they can expect when using the wallet with this configuration, so I also tried to clarify the documentation on `-avoidpartialspends` and `avoid_reuse` a bit. If there are other additional ways how or docs where users can be made aware of the potential consequences of using these parameters, please let me know. Another small upside is that [there seem to be a high number of batching transactions with 100 and 200 inputs](https://miro.medium.com/max/3628/1*sZ5eaBSbsJsHx-J9iztq2g.png)([source](https://medium.com/@hasufly/an-analysis-of-batching-in-bitcoin-9bdf81a394e0)) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.
ACKs for top commit:
jnewbery:
ACK e6fe1c37d0
Xekyo:
retACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
rajarshimaitra:
tACK `e6fe1c3`
achow101:
ACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
glozow:
code review ACK e6fe1c37d0
Tree-SHA512: 79685c58bafa64ed8303b0ecd616fce50fc9a2b758aa79833e4ad9f15760e09ab60c007bc16ab4cbc4222e644cfd154f1fa494b0f3a5d86faede7af33a6f2826
3d552b0d788a7d3102396b32d0de08e57cbfd297 [doc] explain why CheckBlock() is called before AcceptBlock() (Sjors Provoost)
Pull request description:
Based on https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html and its PDF attachment.
ACKs for top commit:
MarcoFalke:
cr ACK 3d552b0d788a7d3102396b32d0de08e57cbfd297
Tree-SHA512: d1ef39855317853e0e7e051ec6015054d0d227fcdf20281c2c1921056537f1f79044aa1bdd35f46475edd17596fbcae79aeb338c4865b1269a01b158f6cb2ac4
fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e doc: Rename fuzz seed_dir to corpus_dir (MarcoFalke)
Pull request description:
The fuzz corpus directory might contain hand-crafted seeds, but generally it is a set of test inputs. See also https://github.com/google/fuzzing/blob/master/docs/glossary.md#corpus
ACKs for top commit:
practicalswift:
cr ACK fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e: patch looks correct and "why not?" :)
fanquake:
ACK fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e - did not test
Tree-SHA512: 38c952feb07aeeeb038b3261a12c824fab9ce5153d75f0ecf6d3f43db4f50998eeb2b14b11b7155f529189c93783fa2c11c81059021a04398c43f3505b31a2d4
8c09c0c1d18885ef94f79b3f2d073f43269bc95d fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() (practicalswift)
Pull request description:
Avoid time-based "non-determinism" in fuzzing harnesses by using mocked `GetTime()`.
Prior to this commit the fuzzing harnesses `banman`, `connman`, `net` and `rbf` had time-based "non-determinism". `addrman` is fixed in #20425. `process_message` and `process_messages` are left to fix: simply using mock time is not enough for them due to interaction with `IsInitialBlockDownload()`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
review ACK 8c09c0c1d18885ef94f79b3f2d073f43269bc95d
practicalswift:
> review ACK [8c09c0c](8c09c0c1d1)
Tree-SHA512: 32dfbead3dfd18cf4ff56dc2ea341aa977441b4e19a54879cf54fa5820c7e2b14b92c7e238d32fd785654f3b28cc82826ae66c03e94c292633c63c41196ba9a8
fabce459bb44e90dc7ae9c44eeedab707435af5b fuzz: version handshake (MarcoFalke)
Pull request description:
Not fuzzing the version handshake will limit fuzz coverage
ACKs for top commit:
practicalswift:
cr ACK fabce459bb44e90dc7ae9c44eeedab707435af5b: patch looks very much correct
Tree-SHA512: 4091d27d39edee781d033e471b352084bb54df250d0890e4821a325926a44dff9b26a2614d67dd0529f73bd366b075d7a0a1a570c2837de286a1b93a59a8fb91
ba7e17e073f833eccd4c7c111ae9058c3f123371 rpc, test: document {previous,next}blockhash as optional (Sebastian Falbesoner)
Pull request description:
This PR updates the result help of the following RPCs w.r.t. the `previousblockhash` and `nextblockhash` fields:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain "previousblockhash") and best block (should not contain "nextblockhash").
Top commit has no ACKs.
Tree-SHA512: ef42c5c773fc436e1b4a67be14e2532e800e1e30e45e54a57431c6abb714d2c069c70d40ea4012d549293b823a1973b3f569484b3273679683b28ed40abf46bb
5e531e6beb5381c0be5efaa24b7e423e593568e4 assumptions: check C++17 assumption with MSVC (fanquake)
c7b46489f8c4d880382248fb47266d81948bbce0 assumptions: assume a C++17 compiler (fanquake)
Pull request description:
This has been the case since #20413.
This should also enable the check for MSVC. From my reading of https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-160 and https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ if we set the `/Zc:__cplusplus` switch in additional options, MSVC will report the correct value for `__cplusplus`. However I have not tested this.
ACKs for top commit:
laanwj:
Code review ACK 5e531e6beb5381c0be5efaa24b7e423e593568e4
hebasto:
ACK 5e531e6beb5381c0be5efaa24b7e423e593568e4, checked the MS docs, and AppVeyor build is green.
practicalswift:
ACK 5e531e6beb5381c0be5efaa24b7e423e593568e4
Tree-SHA512: a4fb525cf5c33abc944c614edb0313a39c8a39a1637a03c09342c15ba0925f4eb037062e65e51b42ade667506b7e554c7159acf86e6b8c35d0a87dd79a6f239b
77772a1b809e443a6861ee49009ff8bc55cff9c3 doc: Rework internal and external links (MarcoFalke)
Pull request description:
Some minor changes:
* Move Bitcoin Core download link to the very top. *Reason:* The download link has nothing to do with the section that explains Bitcoin. Also, anyone quickly looking for the download link will find it faster.
* Add a new link to the doc folder. *Reason*: Apart from the documentation that is shipped with the binary software, the doc folder is the primary location for Bitcoin Core related documentation.
* Remove dead link to pdf. *Reason*: The pdf can be trivially found by asking a search engine.
* Remove reference to "build server". *Reason*: There is no "build server". The CI system is explained in the next sentence in detail.
* Remove dead? link to translation mailing list. *Reason*: The translation process is explained in `doc/translation_process.md`, no need to explain it in detail in the main readme.
ACKs for top commit:
laanwj:
ACK 77772a1b809e443a6861ee49009ff8bc55cff9c3
RiccardoMasutti:
ACK 77772a1
Tree-SHA512: 365824c6da519892ff239842b0c9525f559513eb800afd77b14febdc9d1a79294d9448df6cd25b623f1734ff859d9e20951e78fb8091d19fdadde0dd24eeddd4
ae98aec9c0521cdcec76459c8200bd45ff6a1485 refactor: Make CAddrMan::cs non-recursive (Hennadii Stepanov)
f5d1c7fac70f424114dae3be270fdc31589a8c34 Add AssertLockHeld to CAddrMan private functions (Hennadii Stepanov)
5ef1d0b6982f05f70ff2164ab9af1ac1d2f97f5d Add thread safety annotations to CAddrMan public functions (Hennadii Stepanov)
b138973a8b4bbe061ad97011f278a21e08ea79e6 refactor: Avoid recursive locking in CAddrMan::Clear (Hennadii Stepanov)
f79a664314b88941c1a2796623e846d0a5916c06 refactor: Apply consistent pattern for CAddrMan::Check usage (Hennadii Stepanov)
187b7d2bb36e6de9cd960378021ebe690619a2ef refactor: Avoid recursive locking in CAddrMan::Check (Hennadii Stepanov)
f77d9c79aa41dab4285e95c9432cc6d853be67a3 refactor: Fix CAddrMan::Check style (Hennadii Stepanov)
06703973c758c2c5d0ff916993aa7055f609d2d7 Make CAddrMan::Check private (Hennadii Stepanov)
efc6fac951e75ba913350bb470c3d4e6a4e284b9 refactor: Avoid recursive locking in CAddrMan::size (Hennadii Stepanov)
2da95545ea42f925dbc7703e42e9356908a8c83e test: Drop excessive locking in CAddrManTest::SimConnFail (Hennadii Stepanov)
Pull request description:
This PR replaces `RecursiveMutex CAddrMan::cs` with `Mutex CAddrMan::cs`.
All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.
Related to #19303.
Based on #22025, and first three commits belong to it.
ACKs for top commit:
vasild:
ACK ae98aec9c0521cdcec76459c8200bd45ff6a1485
Tree-SHA512: c3a2d3d955a75befd7e497a802b8c10730e393be9111ca263ad0464d32fae6c7edf9bd173ffb6bc9bb61c4b39073a74eba12979d47f26b0b7b4a861d100942df
3e68efa615968e0c9d68a7f197c7852478f6be78 [net] Move checks from GetLocalAddrForPeer to caller (John Newbery)
d21d2b264cd77c027a06f68289cf4c3f177d1ed0 [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery)
Pull request description:
This is the first part of #21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`.
ACKs for top commit:
MarcoFalke:
re-ACK 3e68efa615968e0c9d68a7f197c7852478f6be78 🍅
Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f
## Issue being fixed or feature implemented
Unlike bitcoin we are using PREVIOUS block in `GetBlockSubsidy()`.
That creates special case for genesis block, because it doesn't have
previous block. In this special case instead of calling
`GetBlockSubsidy` should be used pre-calculated value. To avoid
confusion for new code and simplify implementation, there's introduced a
new method `GetBlockSubsidyPrev` that has other interface: it takes
pointer `CBlockIndex* prev` in agruments instead pair of height + nbits.
These changes are follow-up for #5501
## What was done?
Implemented new method `GetBlockSubsidyPrev()` and used instead of
`GetBlockSubsidy` when it is more convenient.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Make CI builds faster.
We have this
https://github.com/dashpay/dash/blob/develop/ci/dash/build_src.sh#L18
but by that time `CCACHE_SIZE` is set to 100M via
https://github.com/dashpay/dash/blob/develop/ci/test/00_setup_env.sh#L51
already.
## What was done?
set `CCACHE_SIZE` variable in `.gitlab-ci.yml`, drop confusing and
useless lines from `ci/dash/build_src.sh` (`CCACHE_SIZE` and
`CCACHE_COMPRESS` defaults are handled by `ci/test/00_setup_env.sh`)
## How Has This Been Tested?
I set `CCACHE_SIZE` to 400M yesterday via Gitlab UI to test it.
results:
before, trivial doc change, 100M:
https://gitlab.com/dashpay/dash/-/jobs/4931233566#L59 (~17 minutes)
after, trivial code change, 400M:
https://gitlab.com/dashpay/dash/-/jobs/4935764148#L80 (~12 minutes)
Removed it from Gitlab UI now but it should still say `Set cache size
limit to 400.0 MB` in logs for this PR.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
fix buid errors like https://gitlab.com/dashpay/dash/-/jobs/4933232262
## What was done?
reorder initializations
## How Has This Been Tested?
local build with `-werror`
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
c7437185589926ec8def2af6bede6a407b3d2e4a test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit fa4c29bc1d, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki):
ad message type `filterload`:
> The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**.
ad message type `filteradd`:
> The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object).
Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit).
Also fixes#18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with #18544) below to also use the more commonly used `assert_debug_log` method.
ACKs for top commit:
MarcoFalke:
ACK c7437185589926ec8def2af6bede6a407b3d2e4a
robot-visions:
ACK c7437185589926ec8def2af6bede6a407b3d2e4a
jonasschnelli:
utACK c7437185589926ec8def2af6bede6a407b3d2e4a. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524
Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be
a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996b340c19cebab9efb6c89d20fe051ef net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:
c0b389b335/src/net.h (L812)
and after a loaded filter is removed again through a `filterclear` message:
c0b389b335/src/net_processing.cpp (L3201)
The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)
This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.
Here is a before/after comparison in behaviour:
| code part / scenario | master branch | PR branch |
| --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
| `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` |
| `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) |
On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
cd543d9193ac1882c1b4a8a84e3ac7356a8b7ce9 test: check misbehavior more independently in p2p_filter.py (Danny Lee)
Pull request description:
This expands on #18672 in two ways:
- Check positive cases (`filterload` accepted, `filteradd` accepted) in addition to the negative cases added in #18672
- Address MarcoFalke 's [suggestion](https://github.com/bitcoin/bitcoin/pull/18672#discussion_r412101752) to successfully load a filter before testing `filteradd`
ACKs for top commit:
theStack:
re-ACK cd543d9193
Tree-SHA512: f82402f6287ccddf08b38b6432d5e2b2b2ef528802a981d04c24bac459022f732d9090d4849d72d3d1eb2c757161dcb18c4c036b6e11dc80114e9cd49f21c3bd
