76c60d7b31 test: validation:block_connected tracepoint test (0xb10c)
260e28ece8 test: utxocache:* tracepoint tests (0xb10c)
34b27bac68 test: net:in/out_message tracepoint tests (0xb10c)
c934087b62 test: checks for tracepoint tests (0xb10c)
Pull request description:
This adds functional tests for the USDT tracepoints added in https://github.com/bitcoin/bitcoin/pull/22006 and https://github.com/bitcoin/bitcoin/pull/22902. This partially fixes#23296. The tests **are probably skipped** on most systems as these tests require:
- a Linux system with a kernel that supports BPF (and available kernel headers)
- that Bitcoin Core is compiled with tracepoints for USDT support (default when compiled with depends)
- [bcc](https://github.com/iovisor/bcc) installed
- the tests are run with a privileged user that is able to e.g. do BPF syscalls and load BPF maps
The tests are not yet run in our CI as the CirrusCI containers lack the required permissions (see https://github.com/bitcoin/bitcoin/issues/23296#issuecomment-1024920845). Running the tests in a VM in the CI could work, but I haven't experimented with this yet. The priority was to get the actual tests done first to ensure the tracepoints work as intended for the v23.0 release. Running the tracepoint tests in the CI is planned as the next step to finish #23296.
The tests can, however, be run against e.g. release candidates by hand. Additionally, they provide a starting point for tests for future tracepoints. PRs adding new tracepoint should include tests. This makes reviewing these PRs easier.
The tests require privileges to execute BPF sycalls (`CAP_SYS_ADMIN` before Linux kernel 5.8 and `CAP_BPF` and `CAP_PERFMON` on 5.8+) and permissions to `/sys/kernel/debug/tracing/`. It's currently recommended to run the tests in a virtual machine (or on a VPS) where it's sensible to use the `root` user to gain these privileges. Never run python scripts you haven't carefully reviewed with `root` permissions! It's unclear if a non-root user can even gain the required privileges. This needs more experimenting.
The goal here is to test the tracepoint interface to make sure the [documented interface](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#tracepoint-documentation) does not break by accident. The tracepoints expose implementation details. This means we also need to rely on implementation details of Bitcoin Core in these functional tests to trigger the tracepoints. An example is the test of the `utxocache:flush` tracepoint: On Bitcoin Core shutdown, the UTXO cache is flushed twice. The corresponding tracepoint test expects two flushes, too - if not, the test fails. Changing implementation details could cause these tests to fail and the tracepoint API to break. However, we purposefully treat the tracepoints only as [**semi-stable**](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#semi-stable-api). The tracepoints should not block refactors or changes to other internals.
ACKs for top commit:
jb55:
tACK 76c60d7b31
laanwj:
Tested ACK 76c60d7b31
Tree-SHA512: 9a63d945c68102e59d751bd8d2805ddd7b37185408fa831d28a9cb6641b701961389b55f216c475df7d4771154e735625067ee957fc74f454ad7a7921255364c
fa9112aac0 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519 (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0c (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac0
laanwj:
Code review ACK fa9112aac0
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
bb84b7145b add tests for no recipient and using send_max while inputs are specified (ishaanam)
49090ec402 Add sendall RPC née sweep (Murch)
902793c777 Extract FinishTransaction from send() (Murch)
6d2208a3f6 Extract interpretation of fee estimation arguments (Murch)
a31d75e5fb Elaborate error messages for outdated options (Murch)
35ed094e4b Extract prevention of outdated option names (Murch)
Pull request description:
Add sendall RPC née sweep
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _given UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _given budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending a given set of
UTXOs such as paying the value from one or more specific UTXOs, emptying
a wallet, or burning dust, we realized that there are some cases in
which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual
computation of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific
subset of the wallet's UTXO pool (by default all of them) and assigns
the funds to one or more receivers. Receivers can either be specified
with a given amount or receive an equal share of the remaining
unassigned funds. At least one recipient must be provided without
assigned amount to collect the remainder. The `sendall` call will
never create change. The call has a `send_max` option that changes the
default behavior of spending all UTXOs ("no UTXO left behind"), to
maximizing the output amount of the transaction by skipping uneconomic
UTXOs. The `send_max` option is incompatible with providing a specific
set of inputs.
---
Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.
ACKs for top commit:
achow101:
re-ACK bb84b7145b
Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
fa76d8d4d7 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2.
ACKs for top commit:
fanquake:
utACK fa76d8d4d7
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
2726b60a3a test: use MiniWallet for rpc_createmultisig.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (rpc_createmultisig.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
ACKs for top commit:
danielabrozzoni:
re-ACK 2726b60a3a
Tree-SHA512: fb0ef22d3f1c161ca5963cb19ce76533ac3941f15102fc0aa2286ef3bec48f219e5934d504b41976f9f295fb6ca582b737e0fea896df4eb964cdaba1b2c91650
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
c4d76c6faa tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
75656adfd2 test: add functional test for `-maxtipage` parameter (Sebastian Falbesoner)
Pull request description:
This PR adds a missing test for the `-maxtipage` parameter which controls what is the allowed maximum tip age for leaving IBD:
792d0d8d51/src/init.cpp (L540)
Relevant code path in the `CChainState::IsInitialBlockDownload` method:
792d0d8d51/src/validation.cpp (L1479-L1480)
The test is pretty simple and should be self-explanatory.
ACKs for top commit:
MarcoFalke:
review ACK 75656adfd2
Tree-SHA512: 0a10dca13cb18c29e64fc8412f4c8f2bcaff1bab8645bd85266c242ba88ce036a150c03cbbe9810c3bb44649810af0aa9cb3584dbae886a7bdb16b72150d08de
975097f424 Let test_runner.py start multiple jobs per timeslot (Pieter Wuille)
Pull request description:
test_runner.py currently only checks every 0.5s whether any job has finished, and if so, starts at most one new job. At higher parallellism it becomes increasingly likely that multiple jobs have finished at the same time. Fix this by always noticing *all* finished jobs every timeslot, and starting as many new ones.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 975097f424
prayank23:
ACK 975097f424
Tree-SHA512: b70c51f05efcde9bc25475c192b86e86b4c399495b42dee20576af3e6b99e8298be8b9e82146abdabbaedb24a13ee158a7c8947518b16fc4f33a3b434935b550
126853214a test: add functional test for -startupnotify (Bruno Garcia)
Pull request description:
This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.
ACKs for top commit:
theStack:
Tested ACK 126853214a
kristapsk:
re-ACK 126853214a
Tree-SHA512: 5bf3e46124ee5c9d609c9993e6465d5a71a8d2275dcf07c8ce0549f013f7f8863d483b46b7164152f566468a689371ccb87f01cf118c3c9cac5b2be673b61a5c
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
2f9515f37a rpc: move fees object to match help (josibake)
07ade7db8f doc: add release note for fee field deprecation (josibake)
2ee406ce3e test: add functional test for deprecatedrpc=fees (josibake)
35d928c632 rpc: deprecate fee fields from mempool entries (josibake)
Pull request description:
per #22682 , top level fee fields for mempool entries have been deprecated since 0.17 but are still returned. this PR properly deprecates them so that they are no longer returned unless `-deprecatedrpc=fees` is passed.
the first commit takes care of deprecation and also updates `test/functional/mempool_packages.py` to only use the `fees` object. the second commit adds a new functional test for `-deprecatedrpc=fees`
closes#22682
## questions for the reviewer
* `-deprecatedrpc=fees` made the most sense to me, but happy to change if there is a name that makes more sense
* #22682 seems to indicate that after some period of time, the fields will be removed all together. if we have a rough idea of when this will be, i can add a `TODO: fully remove in vXX` comment to `entryToJSON`
## testing
to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:
```bash
./src/bitcoind -daemon -deprecatedrpc=fees
```
you should see entries with the deprecated fields like so:
```json
{
"<txid>": {
"fees": {
"base": 0.00000671,
"modified": 0.00000671,
"ancestor": 0.00000671,
"descendant": 0.00000671
},
"fee": 0.00000671,
"modifiedfee": 0.00000671,
"descendantfees": 671,
"ancestorfees": 671,
"vsize": 144,
"weight": 573,
...
},
```
you can also check `getmempoolentry` using any of the txid's from the output above.
next start the node without the deprecated flag, repeat the commands from above and verify that the deprecated fields are no longer present at the top level, but present in the "fees" object
ACKs for top commit:
jnewbery:
reACK 2f9515f37a
glozow:
utACK 2f9515f37a
Tree-SHA512: b175f4d39d26d96dc5bae26717d3ccfa5842d98ab402065880bfdcf4921b14ca692a8919fe4e9969acbb5c4d6e6d07dd6462a7e0a0a7342556279b381e1a004e
e4a54af6b8 test: add wallet_transactiontime_rescan.py --descriptors to test_runner.py (Sebastian Falbesoner)
b60e02e993 test: fix test wallet_transactiontime_rescan.py for descriptor wallets (Sebastian Falbesoner)
a905ed1a61 test: refactor: use `set_node_times` helper in wallet_transactiontime_rescan.py (Sebastian Falbesoner)
Pull request description:
The functional test wallet_transactiontime_rescan.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, the test framework maps the importaddress RPC calls to the importdescriptors RPC (rescan=False -> timestamp='now'), which always rescans blocks of the past 2 hours, based on the current MTP timestamp. In order to avoid importing the last address (wo3), we generate 10 more blocks with advanced time, to ensure that the balance after importing is zero:
681b25e3cd/test/functional/wallet_transactiontime_rescan.py (L125-L134)
Calling this test with descriptor wallets is also added to test runner. Fixes#23562.
ACKs for top commit:
Sjors:
tACK e4a54af
brunoerg:
tACK e4a54af6b8
Tree-SHA512: 9fd8e298d48dd7947b1218d61a1a66c1241b3dbb14451b0ec7cd30caa74ee540e7ee5a7bd10d421b9e3b6e549fa5c3e85bd02496436128b433b328118642f600
d9803f7a0a test: add stress tests for initialization (James O'Beirne)
23f85616a8 test: add node.chain_path and node.debug_log_path (James O'Beirne)
Pull request description:
In the course of coming up with a test plan for #23280, I thought it would be neat to include a Python snippet showing how I tested the initialization process. I quickly realized I was reinventing the functional test framework... so here's a new test.
This change bangs init around like the Fonz hitting a jukebox. It adds some interesting (read: lazy and random) coverage for the initialization process by
- interrupting init with SIGTERM after certain log statements,
- interrupting init at random points, and
- starting init with some essential data missing (block files, block indices, etc.) to test init error paths.
As far as I can tell, some of these code paths are uncovered otherwise (namely the startup errors).
---
Incidentally, I think I may have uncovered some kind of a bug or race condition with indexing initialization based on an intermittent failure in this testcase. This test sometimes fails after shutting down immediately after `loadblk` thread start:
```
2021-10-15T21:14:51.295000Z TestFramework (INFO): Starting node and will exit after line 'loadblk thread start'
36 │ 2021-10-15T21:14:51.296000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
37 │ 2021-10-15T21:14:51.493000Z TestFramework (INFO): terminating node after 110 log lines seen
38 │ 2021-10-15T21:14:51.625000Z TestFramework (INFO): Starting node and will exit after line 'txindex thread start'
39 │ 2021-10-15T21:14:51.625000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
------> [[ FAILURE HERE ]] 2021-10-15T21:15:21.626000Z TestFramework (WARNING): missed line {bail_line}; bailing now after {num_lines} lines
```
and then fails to start up afterwards. Combined logs showing `Error: txindex best block of the index goes beyond pruned data`, when the node under test is not pruned:
```
node0 2021-10-15T21:16:51.848439Z [shutoff] [validationinterface.cpp:244] [ChainStateFlushed] Enqueuing ChainStateFlushed: block hash=1014bc4ff4917602ae53d10e9dfe230af4b7d52a6cdaa8a47798b9c288180907
node0 2021-10-15T21:16:51.848954Z [shutoff] [init.cpp:302] [Shutdown] Shutdown: done
test 2021-10-15T21:16:51.882000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 87, in run_test
check_clean_start()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 60, in check_clean_start
node.wait_for_rpc_connection()
File "/home/james/src/bitcoin/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
test 2021-10-15T21:16:51.882000Z TestFramework (DEBUG): Closing down network thread
test 2021-10-15T21:16:51.933000Z TestFramework (INFO): Stopping nodes
test 2021-10-15T21:16:51.933000Z TestFramework.node0 (DEBUG): Stopping node
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
```
ACKs for top commit:
laanwj:
Code review ACK d9803f7a0a
Tree-SHA512: 4d80dc399daf199a6222e81e47d12d830dc7af07355eddbb7f52479a676a645b8d3d45093ff54a9295f01a163b2f4fe0e038e83fc269969e03d4cfda69eaf111
fadc4c7272 test: Add txindex migration test (MarcoFalke)
Pull request description:
Test for #22626
ACKs for top commit:
theStack:
Tested ACK fadc4c7272🌁
Tree-SHA512: fc7133ef52826bf0d4fa2ac72c3f1bed4a185ff7492396552ff2cbf6531b053238039211a710cbb949379c56875cd7715f1ed49a514dd3b3f1b46554e3d4bef5
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
9de0d94508 doc: add disclaimer highlighting shortcomings of the basic multisig example (Michael Dietz)
f9479e4626 test, doc: basic M-of-N multisig minor cleanup and clarifications (Michael Dietz)
e05cd0546a doc: add another signing flow for multisig with descriptor wallets and PSBTs (Michael Dietz)
17dd657300 doc: M-of-N multisig using descriptor wallets and PSBTs, as well as a signing flow (Michael Dietz)
1f20501efc test: add functional test for multisig flow with descriptor wallets and PSBTs (Michael Dietz)
Pull request description:
Aims to resolve issue https://github.com/bitcoin/bitcoin/issues/21278. I try to follow the steps laanwj outlined there exactly, with the exception of using `combinepsbt` instead of `joinpsbts`. I wrote a functional test to make sure it works as expected before doing the docs, and figured it would also be a good source of documentation. So I kept the test as simple as possible and didn't go crazy with edge-cases and various checks. I do have a lot more test-cases I've written that I will follow up with (either in a separate PR or another commit - lmk if you have a preference), but I want to do it in a way that doesn't bloat this test so it remains useful as a quickstart (unless that's a bad idea)?
ACKs for top commit:
S3RK:
Code review ACK 9de0d94. Rspigler's argument convinced me that we should leave the workflow with two wallets. I assume using multisig with external signers is a popular use-case and it's important to keep compatibility.
laanwj:
Code and documentation review ACK 9de0d94508
Tree-SHA512: 6c76e787c21f09d8be5eaa11f3ca3eaa4868497824050562bdfb2095c73b90f5e8987a8775119891d6bfde586e3f31ad1b13e4b67b0802e1d23ef050227a1211
4747da3a5b Add syscall sandboxing (seccomp-bpf) (practicalswift)
Pull request description:
Add experimental syscall sandboxing using seccomp-bpf (Linux secure computing mode).
Enable filtering of system calls using seccomp-bpf: allow only explicitly allowlisted (expected) syscalls to be called.
The syscall sandboxing implemented in this PR is an experimental feature currently available only under Linux x86-64.
To enable the experimental syscall sandbox the `-sandbox=<mode>` option must be passed to `bitcoind`:
```
-sandbox=<mode>
Use the experimental syscall sandbox in the specified mode
(-sandbox=log-and-abort or -sandbox=abort). Allow only expected
syscalls to be used by bitcoind. Note that this is an
experimental new feature that may cause bitcoind to exit or crash
unexpectedly: use with caution. In the "log-and-abort" mode the
invocation of an unexpected syscall results in a debug handler
being invoked which will log the incident and terminate the
program (without executing the unexpected syscall). In the
"abort" mode the invocation of an unexpected syscall results in
the entire process being killed immediately by the kernel without
executing the unexpected syscall.
```
The allowed syscalls are defined on a per thread basis.
I've used this feature since summer 2020 and I find it to be a helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.
---
Quick start guide:
```
$ ./configure
$ src/bitcoind -regtest -debug=util -sandbox=log-and-abort
…
2021-06-09T12:34:56Z Experimental syscall sandbox enabled (-sandbox=log-and-abort): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.
…
2021-06-09T12:34:56Z Syscall filter installed for thread "addcon"
2021-06-09T12:34:56Z Syscall filter installed for thread "dnsseed"
2021-06-09T12:34:56Z Syscall filter installed for thread "net"
2021-06-09T12:34:56Z Syscall filter installed for thread "msghand"
2021-06-09T12:34:56Z Syscall filter installed for thread "opencon"
2021-06-09T12:34:56Z Syscall filter installed for thread "init"
…
# A simulated execve call to show the sandbox in action:
2021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
…
Aborted (core dumped)
$
```
---
[About seccomp and seccomp-bpf](https://en.wikipedia.org/wiki/Seccomp):
> In computer security, seccomp (short for secure computing mode) is a facility in the Linux kernel. seccomp allows a process to make a one-way transition into a "secure" state where it cannot make any system calls except exit(), sigreturn(), and read() and write() to already-open file descriptors. Should it attempt any other system calls, the kernel will terminate the process with SIGKILL or SIGSYS. In this sense, it does not virtualize the system's resources but isolates the process from them entirely.
>
> […]
>
> seccomp-bpf is an extension to seccomp that allows filtering of system calls using a configurable policy implemented using Berkeley Packet Filter rules. It is used by OpenSSH and vsftpd as well as the Google Chrome/Chromium web browsers on Chrome OS and Linux. (In this regard seccomp-bpf achieves similar functionality, but with more flexibility and higher performance, to the older systrace—which seems to be no longer supported for Linux.)
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 4747da3a5b
Tree-SHA512: e1c28e323eb4409a46157b7cc0fc29a057ba58d1ee2de268962e2ade28ebd4421b5c2536c64a3af6e9bd3f54016600fec88d016adb49864b63edea51ad838e17
Short options should only be a single character. If not, they can't be
concatenated in a single "-word" (from review by luke-jr).
F is chosen instead of f, because f could be reserved to the nested
wallet_hd.py (test_framework/test_framework.py) arguments parser.
18c5b23a0f [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge)
a79ad65fc2 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge)
5e231c116b [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge)
5bf6587457 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge)
0dc8bf5b92 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge)
Pull request description:
A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.
In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.
>**Example:**
>A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`.
## Approach
This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`.
A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.
ACKs for top commit:
naumenkogs:
ACK 18c5b23a0f
rajarshimaitra:
tACK 18c5b23a0f
jnewbery:
reACK 18c5b23a0f
theStack:
re-ACK 18c5b23a0f🥛
Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
