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
1fca9811e1 lint: Skip whitespace lint for guix patches (Carl Dong)
a91c46c57d guix: Make nsis reproducible by respecting SOURCE-DATE-EPOCH (Carl Dong)
Pull request description:
```
When building nsis, if VERSION is not specified, it defaults to
cvs_version which is non-deterministic as it includes the current date.
This patches nsis to default to SOURCE_DATE_EPOCH if it exists so that
nsis is reproducible.
Upstream change: https://github.com/kichik/nsis/pull/13
```
Sidenote: also a good demonstration of how Guix allows us to flexibly patch our tools!
Note to reviewers: if you want to compare hashes, please build after Jan 16th 2021 without my substitute server enabled!
ACKs for top commit:
fanquake:
ACK 1fca9811e1
Tree-SHA512: b800e0ce5f73827ad353739effb9167ec3a6bdb362c725ae20dd3f025ce78660f85c70ce1d75cd0896facf1e8fe38a9e058459ed13dec71ab3a2fe41e20eaa5d
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
92370033a2 contrib: embed C++11 patch in install_db4.sh (jackielove4u)
Pull request description:
This is a continuation of https://github.com/bitcoin/bitcoin/pull/20665.
Closes#20722.
ACKs for top commit:
laanwj:
ACK 92370033a2
fanquake:
ACK 92370033a2.
Tree-SHA512: ebfd16f5301158de1acc1b8eeca43b3d94f0a6d438832133a30648e5e8a88268b4af983be0bb57f3018e3af8459f32f0de676c1b4e8942e199a4497c776631c5
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
2c403279e2 gitian: Remove codesign_allocate and pagestuff from MacOS build (Andrew Chow)
f55eed2514 gitian: use signapple to create the MacOS code signature (Andrew Chow)
95b06d2185 gitian: use signapple to apply the MacOS code signature (Andrew Chow)
42bb1ea363 gitian: install signapple in gitian-osx-signer.yml (Andrew Chow)
Pull request description:
The MacOS code signing issues that were encountered during the 0.21.0 release cycle have shown that it is necessary for us to use a code signing tool for which the source code is available and modifiable by us. Given that there appears to not be such a tool available, I have written such a tool, [signapple](https://github.com/achow101/signapple), that we can use. This tool is able to create a valid MacOS code signature, detach it in a way that we were doing previously, and attach it to the unsigned binary. This tool can also verify that the signature is correct.
This PR implements the usage of that tool in the gitian build for the code signed MacOS binary. The code signer will use this tool to create the detached signature. Gitian builders will use this tool to apply the detached signature. The `gitian-osx-signer.yml` descriptor has been modified to install this tool so that the `detached-sig-apply.sh` script can use it. Additionally, the `codesign_allocate` and `pagestuff` tools are no longer necessary so they are no longer added to the tarball used in code signing. Lastly, both the `detached-sig-create.sh` and `detached-sig-apply.sh` scripts are made to be significantly less complex and to not do unexpected things such as unpacking an already unpacked tarball.
The detached code signature that signapple creates is almost identical to that which we were previously creating. The only difference is that the cpu architecture name is included in the extension (e.g. we have `bitcoin-qt.x86_64sign` instead of `bitcoin-qt.sign`). This was done in order to support signing universal binaries which we may want to do in the future. However signapple can still apply existing code signatures as it will accept the `.sign` extension. If it is desired, it can be modified to produce signatures with just the `.sign` extension. However I do not think it is necessary to maintain compatibility with the old process.
ACKs for top commit:
laanwj:
Code review ACK 2c403279e2
Tree-SHA512: 2a0e01e9133f8859b9de26e7e8fe1d2610d2cbdee2845e6008b12c083c7e3622cbb2d9b83c50a269e2c3074ab95914a8225d3cd4108017f58b77a62bf10951e0
9d02654677 doc: Fix systemd spelling and link to doc/init.md (Hennadii Stepanov)
601778c310 script: Add Documentation key to bitcoind.service (Hennadii Stepanov)
d9392b724c script: Improve robustness of bitcoind.service on startup (Hennadii Stepanov)
Pull request description:
If network interfaces are not properly up the following happens:
```
...
2021-01-08T10:17:11Z scheduler thread start
2021-01-08T10:17:11Z libevent: getaddrinfo: address family for nodename not supported
2021-01-08T10:17:11Z Binding RPC on address 127.0.0.1 port 8332 failed.
2021-01-08T10:17:11Z HTTP: creating work queue of depth 16
2021-01-08T10:17:11Z Using random cookie authentication.
2021-01-08T10:17:11Z Generated RPC authentication cookie /var/lib/bitcoind/.cookie
2021-01-08T10:17:11Z HTTP: starting 2 worker threads
2021-01-08T10:17:11Z init message: Loading banlist...
2021-01-08T10:17:11Z SetNetworkActive: true
2021-01-08T10:17:11Z Error: Cannot resolve -externalip address: <EDITED>
2021-01-08T10:17:11Z Shutdown: In progress...
2021-01-08T10:17:11Z scheduler thread exit
2021-01-08T10:17:11Z Shutdown: done
```
This PR improves robustness on startup in such cases in documented way:
https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
Also minor doc improvements are added.
ACKs for top commit:
Sjors:
ACK 9d02654
practicalswift:
ACK 9d02654677: patch looks correct
darosior:
ACK 9d02654677 -- been using the first patch too
Tree-SHA512: 38294f5682c09e6ea9008de7d7459098c920cf1b98ad8ef8a5d2ca01f2f781c0fec5591dc40ef36eeb19d94991b0c7fb7cb38c4e716bc7219875c9bcd0a55e1b
fa1d5e5137 test: Fix get_previous_releases.py for aarch64 (MarcoFalke)
Pull request description:
Otherwise it will fail with "Not sure which binary to download..."
ACKs for top commit:
laanwj:
Code review ACK fa1d5e5137
Tree-SHA512: 0db71e898a431665757ce835016a4e05c629a95abc4a2951eac9bd9b5876ec3dc3d6f156d58565e2bcdf918cde4f2649183d4a58038ac13c705a7e914c0094d1
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.
When building nsis, if VERSION is not specified, it defaults to
cvs_version which is non-deterministic as it includes the current date.
This patches nsis to default to SOURCE_DATE_EPOCH if it exists so that
nsis is reproducible.
Upstream change: https://github.com/kichik/nsis/pull/13
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
riscv builds are currently failing because -latomic isn't being linked
against, when it is needed:
```bash
/home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
```
This exteneds our macro to ensure that -latomic is linked against when
required.
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.
bc99ae77e4 scripted-diff: Fix typo in stub manual pages (Wladimir J. van der Laan)
b5e93f873a doc: Add manual page generation for bitcoin-util (Wladimir J. van der Laan)
Pull request description:
- Add `-version` option to `bitcoin-util`
- Add `bitcoin-util` call to `gen-manpages.sh`
- Add stub manual page `bitcoin-util.1`
- Add install of `bitcoin-util.1` to build system
ACKs for top commit:
fanquake:
ACK bc99ae77e4
Tree-SHA512: 948df66c62bbca1cf6da26845dfa63f8f5d036a3d5744add468dd1ce7f442c123d7b0db7011c2e8e3ee6539fd391c7ee2c21b706ec81b21b02821c9501cd077d
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
