Currently it was not possible to run just the BlockToJsonVerboes benchmarsk because it did not set up everything it needed, running `bench_bitcoin -filter=BlockToJsonVerbose` caused this assert to fail:
```
bench_bitcoin: chainparams.cpp:506: const CChainParams& Params(): Assertion `globalChainParams' failed.
```
Initializing TestingSetup fixes this.
This commit addresses #20809.
We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid.
This commit adds the CaptureMessage function. This will later be called
when any message is sent or received. The capture directory is fixed,
in a new folder "message_capture" in the datadir. Peers will then have
their own subfolders, named with their IP address and port, replacing
colons with underscores to keep compatibility with Windows. Inside,
received and sent messages will be captured into two binary files,
msgs_recv.dat and msgs_sent.dat.
e.g.
message_capture/203.0.113.7_56072/msgs_recv.dat
message_capture/203.0.113.7_56072/msgs_sent.dat
The format has been designed as to result in a minimal performance
impact. A parsing script is added in a later commit.
bf100f8170 [net] Cleanup InactivityChecks() and add commenting about time (John Newbery)
06fa85cd50 [net] InactivityCheck() takes a CNode reference (John Newbery)
Pull request description:
This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function
This makes #20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing #20721.
#20721 doesn't require this change, so if others don't agree that it's useful and makes review easier, then I'm happy to close this and just do #20721 directly.
ACKs for top commit:
fanquake:
ACK bf100f8170
MarcoFalke:
review ACK bf100f8170💫
Tree-SHA512: 7b001de2a5fbe8a6dc37baeae930db5775290afb2e8a6aecdf13161f1e5b06ef813bc6291d8ee5cefcf1e430c955ea702833a8db84192eebe6e6acf0b9304cb2
962444295d zmq: deduplicate 'sequence' publisher message creation/sending (Sebastian Falbesoner)
Pull request description:
This small PR deduplicates common low-level creation and sending code for the 'sequence' zmq publisher message (methods `NotifyBlock{Connect,Disconnect}()`, `NotifyTransaction{Acceptance,Removal}()` in the class `CZMQPublishSequenceNotifier`) by introducing a helper function.
ACKs for top commit:
jonatack:
Code review re-ACK 962444295d per `git diff f231ffd 9624442`
instagibbs:
utACK 962444295d
Tree-SHA512: de0750d923f36d1a5751331e88eec8a1605cb88c93318830913210485e2bff712310484f18a0fb626df6ef32ce0b0cf57f4421ce62656e05fce7011a0c3c2d0e
f1694757dd guix: Fix typo (Carl Dong)
771c4b98a8 guix: README: Add darwin HOSTS entry (Carl Dong)
8dbf18cb1d guix: Check for macOS SDK before building anything (Carl Dong)
34b23f597e guix: Set ZERO_AR_DATE for darwin build determinism (Carl Dong)
f3835dc6a3 build: Make xorrisofs reproducible with -volume_date (Carl Dong)
c9eb4cf3a0 guix: Add support for darwin builds (Carl Dong)
37fe73a092 build: Add var printing target to src/Makefile.am (Carl Dong)
Pull request description:
This PR brings our Guix builds on par with Gitian in terms of supported architectures.
Reviewers: if you run a build, please submit:
```
find output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
So that we can compare hashes and ensure reproducibility!
ACKs for top commit:
fanquake:
ACK f1694757dd - I think we can make some small usability improvements, but this is ok to merge now.
Tree-SHA512: 4af2b71654a9736467dcc681d10601c6eee37800d7847011a50585455b67b55d61742ca5604585f310a2fd75335b674e5e27dfb5169cb2f26e112aa4c411d8be
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
b396467053 locks: Annotate CTxMemPool::check to require cs_main (Carl Dong)
Pull request description:
```
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.
This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.
However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.
NOTE: After all review-only assertions are removed in "#20158 |
tree-wide: De-globalize ChainstateManager", and assuming that we
keep the changes in "validation: Pass in spendheight to
CTxMemPool::check", we can re-evaluate to see if this annotation
is still necessary.
```
-----
Previous discussions:
1. https://github.com/bitcoin/bitcoin/pull/20158#discussion_r520639845
2. https://github.com/bitcoin/bitcoin/pull/20158#pullrequestreview-557117202
3. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559425521
ACKs for top commit:
jnewbery:
Code review ACK b396467053
MarcoFalke:
ACK b396467053
jonatack:
ACK b396467053 review and debug built, verified that `cs_main` is held by callers of `CTxMemPool::check()` in `PeerManagerImpl::ProcessOrphanTx()`, `PeerManagerImpl::ProcessMessage()`, and `CChainState::ActivateBestChainStep()`
Tree-SHA512: 4635cddb4aa1af9532bb657b2f9c4deec4568d16ba28c574eae91bb77368cd40e23c3c720a9de11cec78e7ad678a44a5e25af67f13214b86b56e777e0c35a026
abb6fa7285 fuzz: Initialize a full TestingSetup where appropriate (Carl Dong)
713314abfa fuzz: Consolidate fuzzing TestingSetup initialization (Carl Dong)
Pull request description:
```
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
```
~~Question for fuzzing people: was it intentional that `src/test/fuzz/net.cpp` would directly instantiate the `BasicTestingSetup` and thus omit the `"-nodebuglogfile"` flag?~~ [Answered](https://github.com/bitcoin/bitcoin/pull/20946#issuecomment-761537108)
ACKs for top commit:
MarcoFalke:
ACK abb6fa7285
Tree-SHA512: 96a5ca6f4cd5ea0e9483b60165b31ae3e9003918c700a7f6ade48010f419f2a6312e10b816b3187f1d263798827571866e4c4ac0bbfb2e0c79dfad254cda68e7
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.
This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.
However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.
NOTE: After all review-only assertions are removed in "#20158 |
tree-wide: De-globalize ChainstateManager", and assuming that we
keep the changes in "validation: Pass in spendheight to
CTxMemPool::check", we can re-evaluate to see if this annotation
is still necessary.
ad57fb756b wallet: Add BerkeleyDB version sanity check at init time (Wladimir J. van der Laan)
Pull request description:
Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation.
This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected.
ACKs for top commit:
decryp2kanon:
utACK ad57fb7
achow101:
ACK ad57fb756b
theStack:
utACK ad57fb756b
meshcollider:
Code review ACK ad57fb756b
Tree-SHA512: 99cd7d836bffbdeb3d4e14053f7139cc85a6d42e631a3f9a3058a848042446b364faee127500f5acb374616e6a61ab2bedebfac1ba9bc993b4d6227114c2a6c2
ea0a7ec949 Remove deprecated bumpfee behavior (Andrew Chow)
Pull request description:
Removes the deprecation message, behavior, and test.
This was marked for removal in 22.0.
ACKs for top commit:
promag:
ACK ea0a7ec949, maybe add need release notes tag.
Tree-SHA512: d1626906849f6ee37213c32e5f8c1433ad8fb7beabcd88f5801b1964b322171a2341bdfbd9a3a5ab39b2fd9d9c6a05f73298583423a73cab1275653105c03e8e
Also clean up and better comment the function. InactivityChecks() uses a
mixture of (non-mockable) system time and mockable time. Make sure
that's well documented.
Despite being marked as const in CConnman before this commit, the
function did mutate the state of the passed in CNode, which is contained
in vNodes, which is a member of CConnman. To make the function truly
const in CConnman and all its data, instead make InactivityChecks() a
pure function, return whether the peer should be disconnected, and let
the calling function (SocketHandler()) update the CNode object. Also
make the CNode& argument const.
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
54ce4fac80 build: improve macro for testing -latomic requirement (fanquake)
2c010b9c56 add std::atomic include to bitcoin-util.cpp (fanquake)
Pull request description:
Since the merge of #19937, riscv builds have been failing, due to a link issue with [`std::atomic_exchange`](https://en.cppreference.com/w/cpp/atomic/atomic_exchange) in `bitcoin-util`:
```bash
CXXLD bitcoin-util
bitcoin_util-bitcoin-util.o: In function `grind_task':
/home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
collect2: error: ld returned 1 exit status
```
We have a [macro](https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/l_atomic.m4) that tries to determine when `-latomic` is required, however it doesn't quite work well enough, as it's currently determining it isn't needed:
```bash
./autogen.sh
./configure --prefix=/home/ubuntu/bitcoin/depends/riscv64-linux-gnu
...
checking whether std::atomic can be used without link library... yes
```
This PR adds a call to `std::atomic_exchange` to the macro, which will get us properly linked against `-latomic` on riscv:
```bash
checking whether std::atomic can be used without link library... no
checking whether std::atomic needs -latomic... yes
```
Also adds an `<atomic>` include to `bitcoin-util.cpp`.
ACKs for top commit:
laanwj:
Tested ACK 54ce4fac80
Tree-SHA512: 963c875097ee96b131163ae8109bcf8fecf4451d20faa2f3d223f9938ea3d8d1ed5604e12ad82c2b4b1c605fd293a9b6b08fefc00dd3e68d09c49e95029c6f50
Only rebucket if the asmap checksum has changed, not if the file format
has changed but no asmap is provided.
Also, don't try to add an entry to another bucket if it already appears
in ADDRMAN_NEW_BUCKETS_PER_ADDRESS buckets.
Version implies that higher numbers take precendence. This is really a
checksum, to check whether the provided asmap is the same as the one
used when the peers.dat file was serialized.
Also update the comments to explain where/why this is used.
An addrman entry can appear in up to 8 new table buckets. We store this
entry->bucket indexing during shutdown so that on restart we can restore
the entries to their correct buckets.
Commit ec45646de9 broke the
deserialization code so that each entry could only be put in up to one
new bucket. Fix that.
This brings PushMessage and ProcessMessages further in line with the
style guide by fixing their if statements.
LogMessage is later called, inside an if statement, inside both of these
methods.
c061800bb1 build: fix RELOC_SECTION security check for bitcoin-util (fanquake)
Pull request description:
The binutils we use for gitian builds strips the reloc section from
Windows binaries, which breaks ASLR. As a temporary workaround, export
main(). This is the same workaround as #18702 (bitcoin-cli), and will
fix the currently failing security check:
```bash
+ make -j1 -C src check-security
make: Entering directory '/home/ubuntu/build/bitcoin/distsrc-x86_64-w64-mingw32/src'
Checking binary security...
bitcoin-util.exe: failed RELOC_SECTION
make: *** [check-security] Error 1
```
Relevant upstream issue:
https://sourceware.org/bugzilla/show_bug.cgi?id=19011
ACKs for top commit:
dongcarl:
ACK c061800bb1
laanwj:
ACK c061800bb1
Tree-SHA512: a1a4da0b2cddfc377190b9044a04f42a859ca79f11ce2c2ab4c3d066a2786c34d5446d75f8eec634f308d2d3349ebbd7c9f76dcaebeeb28e471c829851592367
Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation.
This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected.
It is not documented that if you attempt to send a transaction
which already exists in a block, an RPC_TRANSACTION_ALREADY_IN_CHAIN
exception will be raised. This should be documented so that developers
are aware that this exception is raised.
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.
Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.
fa0aa87071 rpc: Return wtxid from testmempoolaccept (MarcoFalke)
Pull request description:
It would be nice if `testmempoolaccept` returned the unique wtxid directly to avoid a costly `decoderawtransaction` roundtrip
ACKs for top commit:
mjdietzx:
utACK fa0aa87071
stackman27:
utACK [`fa0aa87`](fa0aa87071)
glozow:
cr ACK fa0aa87071
Tree-SHA512: 05dbaf46d93e47e9eedb725c2f57175e6d4e1722da0531fe4f80e74fc2518911da87634f25f61fa2bc8d87a3017e82fd0684b09a0a9706d71deed970035c2e7a
fa0a864b38 fuzz: Use mocktime in process_message* fuzz targets (MarcoFalke)
Pull request description:
Use mocktime to allow time to advance deterministically during execution of a fuzz input. This also allows to drop the call to `JumpOutOfIbd`.
ACKs for top commit:
practicalswift:
cr ACK fa0a864b38
Tree-SHA512: e92fc70ec6bd49760173cb202549f364304e22b3f7127b9a4da8447cf9341008e477ad42c2599c2fde167bbcbc0e2d139709b4ef6371788bc2c1c3b7f589e11d
The binutils we use for gitian builds strips the reloc section from
Windows binaries, which breaks ASLR. As a temporary workaround, export
main(). This is the same workaround as #18702 (bitcoin-cli), and will
fix the currently failing security check:
```bash
+ make -j1 -C src check-security
make: Entering directory '/home/ubuntu/build/bitcoin/distsrc-x86_64-w64-mingw32/src'
Checking binary security...
bitcoin-util.exe: failed RELOC_SECTION
make: *** [check-security] Error 1
```
Relevant upstream issue:
https://sourceware.org/bugzilla/show_bug.cgi?id=19011
2f463f57e3 [doc] for CheckInputsFromMempoolAndCache (gzhao408)
85cc6bed64 lock annotations for MemPoolAccept functions (gzhao408)
Pull request description:
This is a very small PR that adds some lock annotations to clarify that, now, the `pool.cs` lock is held throughout tx validation for mempool. The comments in `CheckInputsFromMempoolAndCache` were unclear/outdated so I updated those as well.
~This PR is a cleanup. It removes unnecessary code that doesn't do much.~
ACKs for top commit:
sdaftuar:
utACK 2f463f57e3
jnewbery:
utACK 2f463f57e3
MarcoFalke:
cr ACK 2f463f57e3
fanquake:
ACK 2f463f57e3
Tree-SHA512: 5863a546b00ef02eba8f208c2c04c32f64671f17c967a5e3c51332fc0f472e5e9addddae075d0b91c77caebe1be9331317646b0bec802e86d9550773fd9621a9
fa75d40ef8 fuzz: Introduce CallOneOf helper to replace switch-case (MarcoFalke)
Pull request description:
The current `switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, nn)) { case 0: ... case 1: ... case nn: ...` has several problems:
* It makes it hard to review newly added targets, because it requires manual counting of cases
* It makes it hard to update a target, because updating all case labels is trivial, but tedious to review and causes merge conflicts
* ~~Updating the target raises the question whether the case labels should be preserved to not invalidate the existing fuzz inputs format. Fuzz input format might already change implicitly on every commit, so this isn't something worthwhile to pursue.~~ Edit: This pull doesn't fix this problem.
Fix all issues by adding a new `CallOneOf` helper
ACKs for top commit:
ajtowns:
ACK fa75d40ef8 - code review only
jnewbery:
utACK fa75d40ef8
Tree-SHA512: 2daa602b240b86c8e85a024e008f03a57ba60349377eed771f4d21a97a9dba9b66e93fff16ff1992018d4330be7a1a276944c3dfdf698748ce135626c380e563
8775691383 Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" (Luke Dashjr)
Pull request description:
The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense
Originally https://github.com/bitcoin/bitcoin/pull/17463, but rewritten here much simpler based on other merged changes.
ACKs for top commit:
hebasto:
ACK 8775691383, tested on Linux Mint 20.1 (x86_64, Qt 5.12.8):
Tree-SHA512: 3953cc9c09613c9a629def8b4dc061b537f148ddcb378430645602e0be0f3a9f1cff083aa685b94b2e9372300d02ec97e0d9ea89db6e3c6feec86795090f0f77
fc726e0138 doc, rpc: add missing signet mentions in network name lists (Sebastian Falbesoner)
Pull request description:
This small PR adds a few missing mentions of signet w.r.t. chain enumerations:
- RPC `getblockchaininfo`: result description for `"chain"`
- RPC `getmininginfo`: result description for `"chain"`
- REST interface documentation:
- default ports listing for each chain
- `"chain"` description for `chaininfo` endpoint result
The instances were identified via `git grep -i "main.*test.*reg"`.
ACKs for top commit:
ajtowns:
ACK fc726e0138 -- quick code review only
benthecarman:
ACK fc726e0138
Tree-SHA512: 62cdc6ef74fa10db75cc04b9eaf7367183f726b3fee3d21fdf741b3816669dd21508735e89da389ddac980f49773ab229263748d1399553375fefe4526361846
This removes a conditional that checks if pindex is equal to nullptr.
This check is redundant because the branch where pindex is set returns at an earlier time. Additionaly, The independence of the earlier and later pindex is made clearer.
