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
2894e94d17 Updates msvc build to use ISO standard C++17. (Aaron Clauson)
Pull request description:
This PR adds a compiler option to the msvc build to specify ISO C++17 support as discussed in #16684.
In order to allow Bitcoin Core to compile with the new option two pre-processor defines are also necessary to avoid an warning (treated as an error) for C++17 deprecated features:
- _SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING
- _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
For anyone interested a sample compiler message for each of the warnings is shown below:
````
c:\Dev\github\sipsorcery_bitcoin\src\support\allocators\zeroafterfree.h(21,5): error C4996: 'std::allocator<_Ty>::const_pointer': warning STL4010: Various members of std::allocator are deprecated in C++17. Use std::allocator_traits instead of accessing these members directly. You can define _SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. [c:\Dev\github\sipsorcery_bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
````
````
c:\Dev\github\sipsorcery_bitcoin\src\fs.cpp(29,31): error C4996: 'std::codecvt_utf8_utf16<wchar_t,1114111,(std::codecvt_mode)0>': warning STL4017: std::wbuffer_convert, std::wstring_convert, and the <codecvt> header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class template is NOT deprecated.) The C++ Standard doesn't provide equivalent non-deprecated functionality; consider using MultiByteToWideChar() and WideCharToMultiByte() from <Windows.h> instead. You can define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. [c:\Dev\github\sipsorcery_bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
````
ACKs for top commit:
MarcoFalke:
Approach ACK 2894e94d17
laanwj:
ACK 2894e94d17
Tree-SHA512: aff14726e05cb52f81dee32eafbd5b9ec829d3ed032ed4a03345458d6a22875a307ee8543952298a95d30d80720158586d33dbc8bf1970a99c403c3f1510d7fd
4b5ac25881 Drop unused CDBWrapper methods (Hennadii Stepanov)
Pull request description:
`CDBWrapper::Flush()` and `CDBWrapper::Sync()` are not used in the code.
ACKs for top commit:
promag:
ACK 4b5ac25881.
laanwj:
ACK 4b5ac25881
Tree-SHA512: 06115c59e75995d496173a64ceea1b9bb1b4fe3eac8bf4f59df68b87b112b5b3e8065298dcd5c4c7408544f76ee62922325acc2208619d830fd5dbb420cdda5c
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
e846a2a1d9 refactor: clean up PeriodicFlush() (John Newbery)
Pull request description:
`PeriodicFlush()` is much more convoluted than it needs to be: it has triple nesting, local variables counting refs and return values, and increments the `mapFileUseCount` iterator unnecessarily. Removing all of that makes the function much easier to understand.
ACKs for top commit:
MarcoFalke:
ACK e846a2a1d9🎁
jonatack:
re-ACK e846a2a per `git range-diff f7c19e8 7c10020 e846a2a`
promag:
ACK e846a2a1d9.
Tree-SHA512: 22bc600a5268b139c0a2c16b5a9f14837b262670ec24aef00643fcedd1c3ebcbf46dea1633e76adc8acf78e8840b776e17127307c5ee95308caa94239dad5b88
... unless they're UNCONDITIONAL_RELAY_DELAY old, or there has been
a response to a MEMPOOL request in the mean time.
This is accomplished using a rolling Bloom filter for the last
3500 announced transactions. The probability of seeing more than 100
broadcast events (which can be up to 35 txids each) in 2 minutes for
an outbound peer (where the average frequency is one per minute), is
less than 1 in a million.
This constant is set to 2 minutes, rather than 15. This is still many times
larger than the transaction broadcast interval (2s for outbound, 5s for
inbound), so it should be acceptable for peers to know what our contents of
the mempool was that long ago.
fa8a341b88 wallet: Replace CDataStream& with CDataStream&& where appropriate (MarcoFalke)
fa021e9a5b wallet: Remove confusing double return value ret+success (MarcoFalke)
Pull request description:
The keys and values are only to be used once because their memory is set
to zero. Make that explicit by moving the bytes into the lower level
methods.
ACKs for top commit:
sipa:
utACK fa8a341b88
ryanofsky:
Code review ACK fa8a341b88. Nice changes.
Tree-SHA512: 5c0218bae0f3cd2a07346f1bbf4ad232e5dde7ef2f807d82cc6cfd208d11fe60c8b0f37e7986087b52fbfc79cdfd33c3c8a5822b3d4d9a44d1c6b09e354fc424
e8a2822119 [net] Don't try to take cs_inventory before deleting CNode (John Newbery)
3556227ddd [net] Make cs_inventory a non-recursive mutex (John Newbery)
344e831de5 [net processing] Remove PushBlockInventory and PushBlockHash (John Newbery)
Pull request description:
- Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
- Make cs_inventory a nonrecursive mutex
- Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.
ACKs for top commit:
sipa:
utACK e8a2822119
MarcoFalke:
ACK e8a2822119🍬
hebasto:
re-ACK e8a2822119
Tree-SHA512: dbc721d102cdef7b5827a8f2549daf8b54f543050266999a7ea56c9f36618565b71e31ce0beb1209ba2db43d15388be173355a03fb6db8ad24e2475b145050bd
2ad58381ff Clean up separated ban/discourage interface (Pieter Wuille)
b691f2df5f Replace automatic bans with discouragement filter (Pieter Wuille)
Pull request description:
This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full.
Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to "discouraged" to better reflect reality.
ACKs for top commit:
naumenkogs:
utACK 2ad58381ff
amitiuttarwar:
code review ACK 2ad58381ff
jonatack:
ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838`
jnewbery:
Code review ACK 2ad58381ff
Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5
40506bf93f test: Test gettxouttsetinfo hash_type option (Fabian Jahr)
f17a4d1c4d rpc: Add hash_type NONE to gettxoutsetinfo (Fabian Jahr)
a712cf6f68 rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) (Fabian Jahr)
605884ef21 refactor: Extract GetBogoSize function (Fabian Jahr)
Pull request description:
This is another intermediate part of the Coinstats Index (tracked in #18000).
Sjors suggested [here](https://github.com/bitcoin/bitcoin/pull/18000#issuecomment-641423019) that the part of the changes in #19145 that don't rely on the new `hash_type` muhash, i.e. that are for `hash_type=none`, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements.
Building the index with no UTXO set hash is still valuable because `gettxoutsetinfo` can still be used to audit the `total_amount` for example. By itself this PR is not a huge improvement, `hash_type=none` is speeding up `gettxoutsetinfo` by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer.
ACKs for top commit:
MarcoFalke:
ACK 40506bf93f 🖨
Sjors:
tACK 40506bf93f
Tree-SHA512: 3964c2b8eed427511b1aa9b2ef285dff27dc4d1537d72c3911e435b6e6b40912232da4acb3a09bd19a0372ddffa44103388d8a650169d95a4a727b970d210add
This commit includes a short comment in doc/fuzzing.md that gives
guidance on compiling Bitcoin Core with AFL instrumentation using
afl-gcc and afl-g++.
a76dafa51d ci: Add tsan suppression for race in BerkeleyBatch (Hennadii Stepanov)
Pull request description:
A temporary workaround for #19448.
Top commit has no ACKs.
Tree-SHA512: 47b83ff373e710bc9ba8c3661f9850a14417436028c42eb7765d21337ef25faaac4cf8cf93be844ae592d40264934d7d2f6b7ba0ab6c7209fc0da8fc13067769
d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d1, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
961e667600 Remove cached directories and associated script blocks from appveyor CI configuration. (Aaron Clauson)
Pull request description:
Appveyor CI jobs have been failing in the last 24 hours due to a seemingly corrupted cache, see #19440.
It's possible that the appveyor cache issue is related to the[ recent update](https://www.appveyor.com/updates/2020/07/03/) of the Visual Studio 2019 image
PR #19431 changes the "save cache or error" to false in an attempt to avoid a failing CI job from potentially corrupting the cache. In theory the only way a PR could affect the cache is if the `vcpkg` install list changed. That happens very rarely and did not happen in the last 24 hours and so was not the cause of the current cache problems.
I have done some testing with appveyor build jobs on my own fork and found that installing the `vcpkg` dependencies from scratch and doing a full build can now be done in just under 60 minutes. This is the first time in over 5 months I have been able to build Bitcoin Core on appveyor. Either the new Visual Studio 2019 image has dramatically reduced the build time or appveyor images have had their CPU increased.
This PR removes all use of dependency caching from the appveyor CI config. The trade-off is the 15 minutes saved on each build from having the dependencies cached versus the hours maintainers need to spend investigating when the CI jobs start failing.
ACKs for top commit:
MarcoFalke:
ACK 961e667600
Tree-SHA512: 788c7efbfe6e044739ec41b08df30e24e26bfe0f31d1f5695e7243222a2eb649a2b5fd0254a9238fd416661dc05f737b0545d39feea7aa0da2236fffd7683a1b
fab80fef61 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701ad util: Add Assert identity function (MarcoFalke)
fa457fbd33 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61.
ryanofsky:
Code review ACK fab80fef61
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.
Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.
Contains release notes and several interface improvements by
John Newbery.
When loading descriptor caches, we would accidentally reinitialize the
descriptor cache when seeing that one already exists. This should have
only been initializing the cache when one does not exist. However this
code itself is unnecessary as the act of looking up the cache to add to
it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a
multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case.
Another test case has also been added to check that loading a wallet
with only single key descriptors works.
