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
da7a83c5ee Remove WalletDatabase::Create, CreateMock, and CreateDummy (Andrew Chow)
d6045d0ac6 scripted-diff: Replace WalletDatabase::Create* with CreateWalletDatabase (Andrew Chow)
45c08f8a7b Add Create*WalletDatabase functions (Andrew Chow)
Pull request description:
Instead of having `Create`, `CreateMock`, and `CreateDummy` being static functions in `BerkeleyDatabase`, move these to standalone functions in `walletdb.cpp`. This prepares us for having different `WalletDatabase` classes.
Part of #18971. This was originally one commit but has been split into 3 to make it (hopefully) easier to review.
ACKs for top commit:
MarcoFalke:
ACK da7a83c5ee🎂
ryanofsky:
Code review ACK da7a83c5ee. Easy review, nice scripted-diff
Tree-SHA512: 1feb7cb3889168c555154bf3701a49095fd6b8cab911d44b7f7efbf6fcee2280ccb3d4afec8a83755b39a592ecd13b90a318faa655c321f87bdabdf1e2312327
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().
a389ed52e8 walletdb: refactor Read, Write, Erase, and Exists into non-template func (Andrew Chow)
Pull request description:
In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.
The functions `ReadKey`, `WriteKey`, `EraseKey`, and `HasKey` are introduced to handle the actual interaction with the database.
This is mostly a moveonly.
Based on #19290
ACKs for top commit:
ryanofsky:
Code review ACK a389ed52e8. No changes since last review, just non-conflicting rebase
Sjors:
utACK a389ed52e8
MarcoFalke:
ACK a389ed52e8🔳
Tree-SHA512: 73bd2fe9ddc4a132d4db6b97e77f5d5f8aa68b8cb25192384f3bacd826365947763a9eee73672331d34578e3f5ade85ee6aa550ff4d89eb62e482250dd5973e4
26acc8dd9b Add sanity check asserts to span when -DDEBUG (Pieter Wuille)
2676aeadfa Simplify usage of Span in several places (Pieter Wuille)
ab303a16d1 Add Span constructors for arrays and vectors (Pieter Wuille)
bb3d38fc06 Make pointer-based Span construction safer (Pieter Wuille)
1f790a1147 Make Span size type unsigned (Pieter Wuille)
Pull request description:
This improves our Span class by making it closer to the C++20 `std::span` one:
* ~~Support conversion between compatible Spans (e.g. `Span<char>` to `Span<const char>`).~~ (done in #18591)
* Make the size type `std::size_t` rather than `std::ptrdiff_t` (the C++20 one underwent the same change).
* Support construction of Spans directly from arrays, `std::string`s, `std::array`s, `std::vector`s, `prevector`s, ... (for all but arrays, this only works for const containers to prevent surprises).
And then make use of those improvements in various call sites.
I realize the template magic used looks scary, but it's only needed to make overload resultion make the right choices. Note that the operations done on values are all extremely simple: no casts, explicit conversions, or warning-silencing constructions. That should hopefully make it simpler to review.
ACKs for top commit:
laanwj:
Code review ACK 26acc8dd9b
promag:
Code review ACK 26acc8dd9b.
Tree-SHA512: 5a5bd346a140edf782b5b3b3f04d9160c7b9e9def35159814a07780ab1dd352545b88d3cc491e0f80d161f829c49ebfb952fddc9180f1a56f1257aa51f38788a
In order to override these later, the specific details of how the Read,
Write, Erase, and Exists functions interact with the actual database
file need to go into functions that are not templated.
fa02b47313 refactor: Use AbortError in FatalError (MarcoFalke)
Pull request description:
`FatalError` has been copied from `AbortNode`, so the two should use the same style to avoid confusion.
Follow-up to #18927
ACKs for top commit:
hebasto:
ACK fa02b47313, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2cf6d18a6ffb5c2e5cf54f0a072a7cef6dc7e924152b2fee44e6ff2c6c53bad962afd364eda30d8a73883d656429ea68391090e6a27057e69eaefd7c4dad0a33
f8213c05f0 Add means to handle negative capabilities in thread safety annotations (Hennadii Stepanov)
Pull request description:
This commit is separated from #19238, and it adds support of [Negative Capabilities](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) in the Clang Thread Safety Analysis attributes.
> Negative requirements are an alternative `EXCLUDES` [`LOCKS_EXCLUDED`] that provide a stronger safety guarantee. A negative requirement uses the `REQUIRES` [`EXCLUSIVE_LOCKS_REQUIRED`] attribute, in conjunction with the ! operator, to indicate that a capability should not be held.
Examples of usage:
- #19238 (for a class)
- https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base)
ACKs for top commit:
MarcoFalke:
Approach ACK f8213c05f0
vasild:
ACK f8213c05
Tree-SHA512: 86d992826b87579661bd228712ae5ee6acca6f70b885ef7e96458974eac184e4874a525c669607ba6b6c861aa4806409a8792d100e6914c858bcab43d31cfb1b
61c16339da walletdb: Move BDB specific things into bdb.{cpp/h} (Andrew Chow)
8f033642a8 walletdb: moveonly: Move BerkeleyBatch Cursor and Txn funcs to cpp (Andrew Chow)
25a655794a walletdb: move IsWalletLoaded to walletdb.cpp (Andrew Chow)
f6fc5f3849 walletdb: Add IsBDBWalletLoaded to look for BDB wallets specifically (Andrew Chow)
c3538f435a walletdb: Make SpliWalletFilePath non-static (Andrew Chow)
Pull request description:
Moves the BDB specific classes from db.{cpp/h} to bdb.{cpp/h}.
To do this, `SplitWalletFilePath` is first made non-static. Then `IsWalletLoaded` functionality is moved to `IsBDBWalletLoaded` which is called by `IsWalletLoaded`. Then the bulk of db.{cpp/h} is moved to a new file bdb.{cpp/h}.
While doing some moveonly stuff, an additional commit moves the `*Cursor` and `Txn*` implementations out of the header file and into the cpp file.
Part of #18971
ACKs for top commit:
laanwj:
Code review ACK 61c16339da
promag:
Code review ACK 61c16339da.
meshcollider:
utACK 61c16339da
Tree-SHA512: cb676cd34c9cd3c838a4fef230d84711efe4cf0d2eefa64ebfd7f787ddc6f7379db0b29454874ddc46ca7ffee0f18f6f3fb96a85513cd10164048948fd03a80c
fs.cpp:35:17: error: no member named 'strerror' in namespace 'std'
return std::strerror(errno);
~~~~~^
fs.cpp:49:9: error: use of undeclared identifier 'close'
close(fd);
^
2 errors generated.
./interfaces/chain.h:265:55: error: ‘std::function’ has not been declared
virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;
^~~
44cc75f80e wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80e (rebased)
fjahr:
re-ACK 44cc75f80e
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
313a081b90 [net] Add seed.bitcoin.wiz.biz to DNS seeds (wiz)
Pull request description:
I've created the `seed.bitcoin.wiz.biz` DNS seed for the benefit of the Bitcoin community, and will operate it in accordance with the [Bitcoin DNS seed operator policy](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md). Since this is my first PR to the Bitcoin Core project, I also ACK the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).
The data for this DNS seed is generated using redundant instances of TheBlueMatt's [dnsseed-rust implementation](https://github.com/TheBlueMatt/dnsseed-rust), which connects to all discoverable Bitcoin nodes to verify their capabilities and speed, and utilizes the full AS-MAP data from my network's BGP tables to select Bitcoin nodes which are fairly distributed across different networks.
As for my qualifications, I currently operate Bitcoin nodes for the [mempool.space](https://mempool.space/) open-source block explorer project (mempool) and the [Bisq Network](https://bisq.network/) open-source P2P trading community (bisq-network). I have 20 years experience as a network engineer, and all of [my Bitcoin nodes](https://bitnodes.io/nodes/?q=AS54415) are hosted on [my own network](https://ipinfo.io/AS54415) across multiple datacenters. For personal references, the current Bitcoin DNS seed operators Emzy and TheBlueMatt can probably vouch for me.
The DNS responses served from this instance are currently served with a TTL of 60 seconds, and the DNS resolvers do not log queries from users. Any inquiries related to the operation of this DNS seed can be sent to <noc@wiz.biz>.
Here is a rough diagram of the `seed.bitcoin.wiz.biz` DNS seed architecture:
ACKs for top commit:
jonasschnelli:
Tested ACK 313a081b90.
laanwj:
ACK 313a081b90
Tree-SHA512: 9e4ea7a929b7888eba748933c1581328aefcba4de503af96f99630d797d794859b22c99999c25c3fc90f6efaed2598f32784d3acea3e428d84bae3aa37f92a25
5527be0627 refactor: Add AbortError alias (Hennadii Stepanov)
d924f2a596 Drop MSG_NOPREFIX flag (Hennadii Stepanov)
083daf7fba Pass bilingual_str argument to AbortNode() (Hennadii Stepanov)
d1cca129b4 refactor: Use bilingual_str::empty() (Hennadii Stepanov)
Pull request description:
This PR is a [followup](https://github.com/bitcoin/bitcoin/issues/16218#issuecomment-625919724) of #16224, and it adds `bilingual_str` type argument support to the `AbortNode()` functions.
ACKs for top commit:
MarcoFalke:
ACK 5527be0627👟
Tree-SHA512: bf8b15b14912b1f672e6e588fffa1e6eb6f00b4b23d15d0ced7f18fbdf76919244427feb7217007fe29617049308e13def893a03a87358db819cca9692f59905
Every `return false` is preceeded by a detailed debug log message to
explain that a disconnect or misbehavior happened. Logging another
generic "FAILED" message seems redundant.
Also, the size of the message and the message type has already been
logged and is thus redundant as well.
Finally, claiming that message processing FAILED seems odd, because the
message was fully processed to the point where it was concluded that the
peer should be either disconnected or marked as misbehaving.
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`](19e919217e/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`](3a10d935ac)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
The utility 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. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
-Increasing the banscore and/or banning is too harsh,
just disconnecting is enough.
-Return true from ProcessMessage because we already log
receipt of filterclear and disconnect.
-nodes not serving bloomfilters should disconnect peers
that send filterclear, just like filteradd and filterload
-nodes that want to enable/disable txrelay should use
feefilter
ccf1f6ea24 refactor: Drop ::HasWallets() (João Barbosa)
Pull request description:
Minor follow-up of #19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded.
ACKs for top commit:
MarcoFalke:
ACK ccf1f6ea24
hebasto:
ACK ccf1f6ea24, I have reviewed the changes and they look OK, I agree they can be merged.
Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb
This replaces the current benchmarking framework with nanobench [1], an
MIT licensed single-header benchmarking library, of which I am the
autor. This has in my opinion several advantages, especially on Linux:
* fast: Running all benchmarks takes ~6 seconds instead of 4m13s on
an Intel i7-8700 CPU @ 3.20GHz.
* accurate: I ran e.g. the benchmark for SipHash_32b 10 times and
calculate standard deviation / mean = coefficient of variation:
* 0.57% CV for old benchmarking framework
* 0.20% CV for nanobench
So the benchmark results with nanobench seem to vary less than with
the old framework.
* It automatically determines runtime based on clock precision, no need
to specify number of evaluations.
* measure instructions, cycles, branches, instructions per cycle,
branch misses (only Linux, when performance counters are available)
* output in markdown table format.
* Warn about unstable environment (frequency scaling, turbo, ...)
* For better profiling, it is possible to set the environment variable
NANOBENCH_ENDLESS to force endless running of a particular benchmark
without the need to recompile. This makes it to e.g. run "perf top"
and look at hotspots.
Here is an example copy & pasted from the terminal output:
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 2.52 | 396,529,415.94 | 0.6% | 25.42 | 8.02 | 3.169 | 0.06 | 0.0% | 0.03 | `bench/crypto_hash.cpp RIPEMD160`
| 1.87 | 535,161,444.83 | 0.3% | 21.36 | 5.95 | 3.589 | 0.06 | 0.0% | 0.02 | `bench/crypto_hash.cpp SHA1`
| 3.22 | 310,344,174.79 | 1.1% | 36.80 | 10.22 | 3.601 | 0.09 | 0.0% | 0.04 | `bench/crypto_hash.cpp SHA256`
| 2.01 | 496,375,796.23 | 0.0% | 18.72 | 6.43 | 2.911 | 0.01 | 1.0% | 0.00 | `bench/crypto_hash.cpp SHA256D64_1024`
| 7.23 | 138,263,519.35 | 0.1% | 82.66 | 23.11 | 3.577 | 1.63 | 0.1% | 0.00 | `bench/crypto_hash.cpp SHA256_32b`
| 3.04 | 328,780,166.40 | 0.3% | 35.82 | 9.69 | 3.696 | 0.03 | 0.0% | 0.03 | `bench/crypto_hash.cpp SHA512`
[1] https://github.com/martinus/nanobench
* Adds support for asymptotes
This adds support to calculate asymptotic complexity of a benchmark.
This is similar to #17375, but currently only one asymptote is
supported, and I have added support in the benchmark `ComplexMemPool`
as an example.
Usage is e.g. like this:
```
./bench_bitcoin -filter=ComplexMemPool -asymptote=25,50,100,200,400,600,800
```
This runs the benchmark `ComplexMemPool` several times but with
different complexityN settings. The benchmark can extract that number
and use it accordingly. Here, it's used for `childTxs`. The output is
this:
| complexityN | ns/op | op/s | err% | ins/op | cyc/op | IPC | total | benchmark
|------------:|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|----------:|:----------
| 25 | 1,064,241.00 | 939.64 | 1.4% | 3,960,279.00 | 2,829,708.00 | 1.400 | 0.01 | `ComplexMemPool`
| 50 | 1,579,530.00 | 633.10 | 1.0% | 6,231,810.00 | 4,412,674.00 | 1.412 | 0.02 | `ComplexMemPool`
| 100 | 4,022,774.00 | 248.58 | 0.6% | 16,544,406.00 | 11,889,535.00 | 1.392 | 0.04 | `ComplexMemPool`
| 200 | 15,390,986.00 | 64.97 | 0.2% | 63,904,254.00 | 47,731,705.00 | 1.339 | 0.17 | `ComplexMemPool`
| 400 | 69,394,711.00 | 14.41 | 0.1% | 272,602,461.00 | 219,014,691.00 | 1.245 | 0.76 | `ComplexMemPool`
| 600 | 168,977,165.00 | 5.92 | 0.1% | 639,108,082.00 | 535,316,887.00 | 1.194 | 1.86 | `ComplexMemPool`
| 800 | 310,109,077.00 | 3.22 | 0.1% |1,149,134,246.00 | 984,620,812.00 | 1.167 | 3.41 | `ComplexMemPool`
| coefficient | err% | complexity
|--------------:|-------:|------------
| 4.78486e-07 | 4.5% | O(n^2)
| 6.38557e-10 | 21.7% | O(n^3)
| 3.42338e-05 | 38.0% | O(n log n)
| 0.000313914 | 46.9% | O(n)
| 0.0129823 | 114.4% | O(log n)
| 0.0815055 | 133.8% | O(1)
The best fitting curve is O(n^2), so the algorithm seems to scale
quadratic with `childTxs` in the range 25 to 800.
fadf6bd04f refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0 wallet: Make RPC help compile-time static (MarcoFalke)
Pull request description:
Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.
The fix is split into two commits:
* First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test.
* Second, a commit that removes the complicated and confusing code.
ACKs for top commit:
achow101:
re-ACK fadf6bd04f
promag:
Tested ACK fadf6bd04f.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
cf5b8f64b3 tests: Add fuzzing harness for {Read,Write}{LE,BE}{16,32,64} (crypto/common.h) (practicalswift)
4a8181b303 tests: Add std::vector<uint8_t> ConsumeFixedLengthByteVector(FuzzedDataProvider& fuzzed_data_provider, const size_t length) (practicalswift)
Pull request description:
Add fuzzing harness for `{Read,Write}{LE,BE}{16,32,64}` (`crypto/common.h`).
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:
ACK cf5b8f64b3
Tree-SHA512: 26412daa6987add1c721ad0348a5a894d68a646e724f328f2db6d9c9358a533481d8888b89d4b0743e9d1c11aa4e0e5341eb4c0d05a4da77b15ab75489327749
