Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.
In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
1a6323bdbe doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7e scripted-diff: remove MakeUnique<T>() (fanquake)
Pull request description:
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.
ACKs for top commit:
jnewbery:
utACK 1a6323bdbe
practicalswift:
cr ACK 1a6323bdbe: patch looks correct
ajtowns:
ACK 1a6323bdbe -- code review only
glozow:
ACK 1a6323bdbe looks correct
Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
e017a913d0 bitcoind: Add -daemonwait option to wait for initialization (Wladimir J. van der Laan)
c3e6fdee6d shutdown: Use RAII TokenPipe in shutdown (Wladimir J. van der Laan)
612f746a8f util: Add RAII TokenPipe (Wladimir J. van der Laan)
Pull request description:
This adds a `-daemonwait` flag that does the same as `-daemon` except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.
This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.
The use of the libc function `daemon()` is replaced by a custom implementation which is inspired by the [glibc implementation](https://github.com/lattera/glibc/blob/master/misc/daemon.c#L44), but which also creates a pipe from the child to the parent process for communication.
An additional advantage of having our own `daemon()` implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.
TODO:
- [x] Factor out `token_read` and `token_write` to an utility, and use them in `shutdown.cpp` as well—this is exactly the same kind of communication mechanism.
- [x] RAII-ify pipe endpoints.
- [x] Improve granularity of the `configure.ac` checks. This currently still checks for the function `daemon()` which makes no sense as it's not used. It should check for individual functions such as
`fork()` and `setsid()` etc—the former being required, the second optional.
- [-] ~~Signal propagation during initialization: if say, pressing Ctrl-C during `-daemonwait` it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free.~~ This is not necessary, see https://github.com/bitcoin/bitcoin/pull/21007#issuecomment-769007341.
Future:
- Consider if it makes sense to use this in the RPC tests (there would be no more need for "is RPC ready" polling loops). I think this is out of scope for this PR.
ACKs for top commit:
jonatack:
Tested ACK e017a913d0 checked change since previous review is move-only
Tree-SHA512: 53369b8ca2247e4cf3af8cb2cfd5b3399e8e0e3296423d64be987004758162a7ddc1287b01a92d7692328edcb2da4cf05d279b1b4ef61a665b71440ab6a6dbe2
0eaea66e8b Make tx relay data structure use std::chrono types (Pieter Wuille)
55e82881a1 Make all Poisson delays use std::chrono types (Pieter Wuille)
c733ac4d8a Convert block/header sync timeouts to std::chrono types (Pieter Wuille)
4d98b401fb Change all ping times to std::chrono types (Pieter Wuille)
Pull request description:
(Picking up #20044. Rebased against master.)
This changes various uses of integers to represent timestamps and durations to `std::chrono` duration types with type-safe conversions, getting rid of various `.count()`, constructors, and conversion factors.
ACKs for top commit:
jnewbery:
utACK 0eaea66e8b
vasild:
ACK 0eaea66e8b
MarcoFalke:
re-ACK 0eaea66e8b, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
ajtowns:
utACK 0eaea66e8b
Tree-SHA512: 2dbd8d53bf82e98f9b4611e61dc14c448e8957d1a02575b837fadfd59f80e98614d0ccf890fc351f960ade76a6fb8051b282e252e81675a8ee753dba8b1d7f57
Introduce two high level, convenience methods in the `Sock` class:
* `SendComplete()`: keep trying to send the specified data until either
successfully sent all of it, timeout or interrupted.
* `RecvUntilTerminator()`: read until a terminator is encountered (never
after it), timeout or interrupted.
These will be convenient in the I2P SAM implementation.
`SendComplete()` can also be used in the SOCKS5 implementation instead
of calling `send()` directly.
Previously `Sock::Wait()` would not have signaled to the caller whether
a timeout or one of the requested events occurred since that was not
needed by any of the callers.
Such functionality will be needed in the I2P implementation, thus extend
the `Sock::Wait()` method.
`fclose()` is flushing any buffered data to disk, so if it fails then
that could mean that the data was not completely written to disk.
Thus, check if `fclose()` succeeds and only then claim success from
`WriteBinaryFile()`.
If an error occurs and `fread()` returns `0` (nothing was read) then the
code before this patch would have returned "success" with a partially
read contents of the file.
Extract `ReadBinaryFile()` and `WriteBinaryFile()` from `torcontrol.cpp`
to its own `readwritefile.{h,cpp}` files, so that it can be reused from
other modules.
fd6580e405 [refactor] txmempool: split epoch logic into class (Anthony Towns)
Pull request description:
Splits the epoch logic introduced in #17925 into a separate class.
Uses clang's thread safety annotations and encapsulates the data more strongly to reduce chances of bugs from API misuse.
ACKs for top commit:
jonatack:
ACK fd6580e405 using clang thread safety annotations looks like a very good idea, and the encapsulation this change adds should improve robustness (and possible unit test-ability) of the code. Verified that changing some of the locking duly provoked build-time warnings with Clang 9 on Debian and that small changes in the new `Epoch` class were covered by failing functional test assertions in `mempool_updatefromblock.py`, `mempool_resurrect.py`, and `mempool_reorg.py`
hebasto:
re-ACK fd6580e405, since my [previous](https://github.com/bitcoin/bitcoin/pull/18017#pullrequestreview-569619362) review:
Tree-SHA512: 7004623faa02b56639aa05ab7a078320a6d8d54ec62d8022876221e33f350f47df51ddff056c0de5be798f8eb39b5c03c2d3f035698555d70abc218e950f2f8c
This option replaces --with-boost-process
This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.
This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini
b4511e2e2e log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2e
MarcoFalke:
review ACK b4511e2e2e🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
e829c9afbf refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17) (Sebastian Falbesoner)
365539c846 refactor: init vectors via std::{begin,end} to avoid pointer arithmetic (Sebastian Falbesoner)
63d4ee1968 refactor: iterate arrays via C++11 range-based for loops if idx is not needed (Sebastian Falbesoner)
Pull request description:
This refactoring PR picks up the idea of #19626 and replaces all occurences of `sizeof(x)/sizeof(x[0])` (or `sizeof(x)/sizeof(*x)`, respectively) with the now-available C++17 [`std::size`](https://en.cppreference.com/w/cpp/iterator/size) (as [suggested by sipa](https://github.com/bitcoin/bitcoin/pull/19626#issuecomment-666487228)), making the macro `ARRAYLEN` obsolete.
As preparation for this, two other changes are done to eliminate `sizeof(x)/sizeof(x[0])` usage:
* all places where arrays are iterated via an index are changed to use C++11 range-based for loops If the index' only purpose is to access the array element (as [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/19626#discussion_r463404541)).
* `std::vector` initializations are done via `std::begin` and `std::end` rather than using pointer arithmetic to calculate the end (also [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/20429#discussion_r567418821)).
ACKs for top commit:
practicalswift:
cr ACK e829c9afbf: patch looks correct
fanquake:
ACK e829c9afbf
MarcoFalke:
review ACK e829c9afbf 🌩
Tree-SHA512: b01d32c04b9e04d562b7717cae00a651ec9a718645047a90761be6959e0cc2adbd67494e058fe894641076711bb09c3b47a047d0275c736f0b2218e1ce0d193d
9266f7497f util: Use std::chrono for time getters (MarcoFalke)
3c2e16be22 time: add runtime sanity check (Cory Fields)
Pull request description:
I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.
ACKs for top commit:
practicalswift:
Tested ACK 9266f7497f
laanwj:
Code review ACK 9266f7497f
Tree-SHA512: 5471a60e65e9fa8ef48320743ef637f1d162724e717e0f5509118e1e5732fc0844656a9c09d3d1300eb657dcc7a1e1e67305d8c9ef959c63be67393607dd4ceb
std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed
to use the Unix epoch timestamp, but in practice they almost certainly will.
Any differing behavior will be assumed to be an error, unless certain
platforms prove to consistently deviate, at which point we'll cope with it
by adding offsets.
Do a quick runtime check to verify that
time_t(0) == std::chrono::system_clock's epoch time == unix epoch.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
`std::string`'s push_back is rather slow because it needs to check & update the string size. For
`HexStr` the output string size is already easily know, so we can initially create the string with
the correct size and then just assign the data.
`HexStr` is heavily usd in `blockToJSON`, so this change is a noticeable benefit. Benchmark on an i7-8700 @3.2GHz:
* 71,315,461.00 ns/op master
* 62,842,490.00 ns/op this commit
So this little change makes `blockToJSON` about ~13% faster.
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
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.
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.
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
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
da9caa1ced Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd5 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo)
Pull request description:
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
ACKs for top commit:
jonatack:
Code review ACK da9caa1ced only doxygen improvements since my last review per `git diff d867d7a da9caa1`
MarcoFalke:
review ACK da9caa1ced📯
ryanofsky:
Code review ACK da9caa1ced. Just comment and test tweaks since previous review.
Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
22eb7930a6 tracing: add tracing framework (William Casarin)
933ab8a720 build: detect sys/sdt.h for eBPF tracing (William Casarin)
Pull request description:
Instead of writing ad-hoc logging everywhere (eg: #19509), we can take advantage of linux user static defined traces, aka. USDTs ( not the stablecoin 😅 )
The linux kernel can hook into these tracepoints at runtime, but otherwise they have little to no performance impact. Traces can pass data which can be printed externally via tools such as bpftrace. For example, here's one that prints incoming and outgoing network messages:
# Examples
## Network Messages
```
#!/usr/bin/env bpftrace
BEGIN
{
printf("bitcoin net msgs\n");
@start = nsecs;
}
usdt:./src/bitcoind:net:push_message
{
$ip = str(arg0);
$peer_id = (int64)arg1;
$command = str(arg2);
$data_len = arg3;
$data = buf(arg3,arg4);
$t = (nsecs - @start) / 100000;
printf("%zu outbound %s %s %zu %d %r\n", $t, $command, $ip, $peer_id, $data_len, $data);
@outbound[$command]++;
}
usdt:./src/bitcoind:net:process_message
{
$ip = str(arg0);
$peer_id = (int64)arg1;
$command = str(arg2);
$data_len = arg3;
$data = buf(arg3,arg4);
$t = (nsecs - @start) / 100000;
printf("%zu inbound %s %s %zu %d %r\n", $t, $command, $ip, $peer_id, $data_len, $data);
@inbound[$ip, $command]++;
}
```
$ sudo bpftrace netmsg.bt
output: https://jb55.com/s/b11312484b601fb3.txt
if you look at the bottom of the output you can see a histogram of all the messages grouped by message type and IP. nice!
## IBD Benchmarking
```
#!/usr/bin/env bpftrace
BEGIN
{
printf("IBD to 500,000 bench\n");
}
usdt:./src/bitcoind:CChainState:ConnectBlock
{
$height = (uint32)arg0;
if ($height == 1) {
printf("block 1 found, starting benchmark\n");
@start = nsecs;
}
if ($height >= 500000) {
@end = nsecs;
@duration = @end - @start;
exit();
}
}
END {
printf("duration %d ms\n", @duration / 1000000)
}
```
This one hooks into ConnectBlock and prints the IBD time to height 500,000 starting from the first call to ConnectBlock
Userspace static tracepoints give lots of flexibility without invasive logging code. It's also more flexible than ad-hoc logging code, allowing you to instrument many different aspects of the system without having to enable per-subsystem logging.
Other ideas: tracepoints for lock contention, threads, what else?
Let me know what ya'll think and if this is worth adding to bitcoin.
## TODO
- [ ] docs?
- [x] Integrate systemtap-std-dev/libsystemtap into build (provides the <sys/sdt.h> header)
- [x] ~dtrace macos support? (is this still a thing?)~ going to focus on linux for now
ACKs for top commit:
laanwj:
Tested ACK 22eb7930a6
0xB10C:
Tested ACK 22eb7930a6
Tree-SHA512: 69242242112b679c8a12a22b3bc50252c305894fb3055ae6e13d5f56221d858e58af1d698af55e23b69bdb7abedb5565ac6b45fa5144087b77a17acd04646a75
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
281fd1a4a0 Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db6 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)
Pull request description:
There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.
`KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.
Split from #19602 (and a few other PRs/branches I have).
ACKs for top commit:
laanwj:
Code review ACK 281fd1a4a0
jonatack:
ACK 281fd1a4a0, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753
fjahr:
utACK 281fd1a4a0
Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
faa8f68943 Replace boost::variant with std::variant (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency
ACKs for top commit:
fjahr:
Code review ACK faa8f68943
fanquake:
ACK faa8f68943
Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
ef712298c3 util: Check for file being NULL in DirectoryCommit (Luke Dashjr)
4574904038 Fix possible data race when committing block files (Evan Klitzke)
220bb16cbe util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke)
ce5cbaea63 util.h: Document FileCommit function (Evan Klitzke)
844d650eea util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke)
f6cec0bcaf 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 ef712298c3
Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce
faa05854f8 util: Remove probably misleading TODO (MarcoFalke)
fac5efe730 util: Add Assume() identity function (MarcoFalke)
fa861569dc util: Allow Assert(...) to be used in all contexts (practicalswift)
Pull request description:
This is needed for #20138. Please refer to the added documentation for motivation.
ACKs for top commit:
practicalswift:
cr ACK faa05854f8
jnewbery:
utACK faa05854f8
hebasto:
ACK faa05854f8, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 72165fbd898b92ab9a79b070993fa1faa86c2e3545b6645e72c652bda295d5107bc298d0482bf3aaf0926fc0c3e6418a445c0e073b08568c44231f547f76a688
ce9dd45422 Add [[nodiscard]] to RenameOver(...) (practicalswift)
9429a398e2 Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift)
Pull request description:
Handle rename failure in `DumpMempool(...)` by using the `RenameOver(...)` return value.
Add `[[nodiscard]]` to `RenameOver(...)` to reduce the risk of similar rename issues in the future.
ACKs for top commit:
vasild:
ACK ce9dd454
theStack:
ACK ce9dd45422🏷️
Tree-SHA512: 1e63d7f3061e1f6ea2df5750dbc1547a39bd50b6c529812a0c8a0c11d3100c241afdf14094e69b69a38bade7e54a12b2a42888545874398eaf5d02421b57e874
Fixes the compile error when used inside operator[]:
./chain.h:404:23: error: C++11 only allows consecutive left square brackets when introducing an attribute
return (*this)[Assert(pindex)->nHeight] == pindex;
^
Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
Move the hashers that we use for hash tables to a common place.
Moved hashers:
- SaltedTxidHasher
- SaltedOutpointHasher
- FilterHeaderHasher
- SignatureCacheHasher
- BlockHasher
ad5cef5dfd doc: Update data directory path comments (Hennadii Stepanov)
b19e88230f util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
Pull request description:
Wallet names in `listwalletdir` RPC are correct now, even if the `-datadir` path has any number of trailing `/`.
This PR is an alternative to #19933.
Fixes#19928.
ACKs for top commit:
MarcoFalke:
review ACK ad5cef5dfd🔙
promag:
Code review ACK ad5cef5dfd.
meshcollider:
Code review + test run ACK ad5cef5dfd
Tree-SHA512: bccabbd6c18243d48d15b2b27201cc0f5984623dcbc635c8740cf74523f359844c36eadd40391142874fcf452a43880bb6afbf89815ae736e499f9a98143a661
fa48405ef8 Warn on unknown rw_settings (MarcoFalke)
Pull request description:
Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.
Something similar is already done for the command line and config file. See:
* test: Add test for unknown args #16234 (commit fa7dd88b71)
ACKs for top commit:
ryanofsky:
Code review ACK fa48405ef8. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
Tree-SHA512: cec7d88adf84fa0a842f56b26245157736eb50df433db951e622ea07fd145b899822b24cdab1d8b36c066415ce4f0ef09b493fa8a8d691532822a59c573aafa7
d103484fe8 util: Do not use gArgs global in ArgsManager member functions (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
practicalswift:
ACK d103484fe8: patch looks correct
Tree-SHA512: dda7a5062363170c6995f2fd8fda48c0a919e5ca67be9faa8f0fa66f9d3b535f134eb6f4860a0859bc5457c02230b34a8d1264045f22bed8d30668158ac2271f
-BEGIN VERIFY SCRIPT-
sed -i 's/Fee exceeds maximum configured by \-\maxtxfee/Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)/g' src/util/error.cpp test/functional/rpc_fundrawtransaction.py test/functional/rpc_psbt.py test/functional/rpc_rawtransaction.py test/functional/wallet_bumpfee.py test/functional/wallet_create_tx.py
-END VERIFY SCRIPT-
7be6ff6187 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov)
e0d73573a3 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov)
fe42411b4b test: move HasReason so it can be reused (Vasil Dimov)
d2bb681f96 util: move HasPrefix() so it can be reused (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Add an optional support to serialize/unserialize `CNetAddr` in ADDRv2 format (BIP155). The new serialization is engaged by ORing a flag into the stream version.
So far this is only used in tests to ensure the new code works as expected.
ACKs for top commit:
Sjors:
re-tACK 7be6ff6187
sipa:
re-utACK 7be6ff6187
eriknylund:
ACK 7be6ff6187 I built the PR on macOS Catalina 10.15.6, ran both tests and functional tests. I've reviewed the code and think the changes look good and according to BIP155. I verified that the added Base32 encoding test looks as proposed and working. I've run a node for a week only with Onion addresses `-onlynet=onion` without issues and I can connect to other peer reviewers running TorV3 on their nodes and I can connect both of my test nodes to each other.
jonatack:
re-ACK 7be6ff6187 per `git diff b9c46e0 7be6ff6`, debug build, ran/running bitcoind with this change and observed the log and `-netinfo` peer connections while connected as a tor v2 service to both tor v2 peers and also five tor v3 peers.
hebasto:
ACK 7be6ff6187, tested on Linux Mint 20 (x86_64): on top of this pull and #19031 I'm able to connect to onion v3 addresses, and jonatack is able to connect to my created onion v3 address.
Tree-SHA512: dc621411ac4393993aa3ccad10991717ec5f9f2643cae46a24a89802df0a33d6042994fc8ff2f0f397a3dbcd1c0e58fe4724305a2f9eb64d9342c3bdf784d9be
Recognizing addresses from those networks allows us to accept and gossip
them, even though we don't know how to connect to them (yet).
Co-authored-by: eriknylund <erik@daychanged.com>
c4b85ba704 Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr)
Pull request description:
Fixes a bug introduced in #19614
The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined.
This fixes both issues by defining it and checking its value instead of whether it is merely defined.
Pulled out of #14501 by fanquake's request
ACKs for top commit:
fanquake:
ACK c4b85ba704 - thanks for catching and fixing my mistake.
laanwj:
Code review ACK c4b85ba704
Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9
102867c587 net: change CNetAddr::ip to have flexible size (Vasil Dimov)
1ea57ad674 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Do not accept
invalid netmasks that have 0-bits followed by 1-bits and only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
ACKs for top commit:
sipa:
utACK 102867c587
MarcoFalke:
Concept ACK 102867c587
ryanofsky:
Code review ACK 102867c587. Just many suggested updates since last review. Thanks for following up on everything!
jonatack:
re-ACK 102867c587 diff review, code review, build/tests/running bitcoind with ipv4/ipv6/onion peers
kallewoof:
ACK 102867c587
Tree-SHA512: d60bf716cecf8d3e8146d2f90f897ebe956befb16f711a24cfe680024c5afc758fb9e4a0a22066b42f7630d52cf916318bedbcbc069ae07092d5250a11e8f762
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
0a8aa626dd refactor: Make HexStr take a span (Wladimir J. van der Laan)
Pull request description:
Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
ACKs for top commit:
elichai:
Code review ACK 0a8aa626dd
hebasto:
re-ACK 0a8aa626dd
jonatack:
re-ACK 0a8aa626dd
Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992
1d8338d6b7 util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake)
Pull request description:
Rather than just using on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD.
This also aligns more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
ACKs for top commit:
practicalswift:
ACK 1d8338d6b7 -- patch looks correct
laanwj:
ACK 1d8338d6b7
hebasto:
ACK 1d8338d6b7
Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17
