Age | Commit message (Collapse) | Author |
|
79d343a642f985801da463b03a0627a59a095238 http: update libevent workaround to correct version (stickies-v)
Pull request description:
The libevent bug described in https://github.com/libevent/libevent/commit/5ff8eb26371c4dc56f384b2de35bea2d87814779 was already patched in [release-2.1.9-beta](https://github.com/libevent/libevent/releases/tag/release-2.1.9-beta), with cherry-picked commits [5b40744d1581447f5b4496ee8d4807383e468e7a](https://github.com/libevent/libevent/commit/5b40744d1581447f5b4496ee8d4807383e468e7a) and [b25813800f97179b2355a7b4b3557e6a7f568df2](https://github.com/libevent/libevent/commit/b25813800f97179b2355a7b4b3557e6a7f568df2).
There should be no side-effects by re-applying the workaround on an already patched version of libevent (as is currently done in master for people running libevent between 2.1.9 and 2.1.12), but it is best to just set the correct version number to avoid confusion.
This will prevent situations like e.g. in https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1238858604, where a reverse workaround was incorrectly applied to the wrong version range.
ACKs for top commit:
fanquake:
ACK 79d343a642f985801da463b03a0627a59a095238
Tree-SHA512: 56d2576411cf38e56d0976523fec951e032a48e35af293ed1ef3af820af940b26f779b9197baaed6d8b79bd1c7f7334646b9d73f80610d63cffbc955958ca8a0
|
|
The libevent bug described in https://github.com/libevent/libevent/commit/5ff8eb26371c4dc56f384b2de35bea2d87814779
was already patched in release-2.1.9-beta, with cherry-picked
commits 5b40744d1581447f5b4496ee8d4807383e468e7a and
b25813800f97179b2355a7b4b3557e6a7f568df2.
There should be no side-effects by re-applying the workaround on
an already patched version of libevent, but it is best to set the
correct version number to avoid confusion.
|
|
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes #24771.
|
|
|
|
be55f545d53d44fdcf2d8ae802e9eae551d120c6 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲
ryanofsky:
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
hebasto:
ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
|
|
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
|
|
`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query parameters are used (e.g. `rest_headers`)
can cause a segfault.
This bugfix is designed to be minimal and without additional behaviour change.
Follow-up work should be done to resolve this in a more general and robust way,
so not every endpoint has to handle it individually.
|
|
to finish - 2nd attempt
60978c8080ec13ff4571c8a89e742517b2aca692 test: Reduce extended timeout on abortnode test (Fabian Jahr)
660bdbf785a32024f0694915fa043968a0afb573 http: Release server before waiting for event base loop exit (João Barbosa)
8c6d007c80dc3fec5ce6c0196381444a5ed7e424 http: Track active requests and wait for last to finish (João Barbosa)
Pull request description:
This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged.
The PR is rebased and comments by jonatack have been addressed.
Once this is merged, I will also reopen #19434.
ACKs for top commit:
achow101:
ACK 60978c8080ec13ff4571c8a89e742517b2aca692
stickies-v:
re-ACK [60978c8](https://github.com/bitcoin/bitcoin/commit/60978c8080ec13ff4571c8a89e742517b2aca692)
hebasto:
ACK 60978c8080ec13ff4571c8a89e742517b2aca692
Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
|
|
CService and use better naming
c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91fb12fba516dee896f1f97692cfa2104
achow101:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
jonatack:
re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
|
|
|
|
|
|
The `bool` return value was introduced in 755aa05174e06effd758eeb78c5af9fb465e9611.
It has been not used since 8d3f46ec3938e2ba17654fecacd1d2629f9915fd.
No behavior change.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
|
|
Removed the default case in the switch statement in order to comply with the Developer Notes
|
|
Removed all break statements from both RequestMethodString and GetRequestMethod functions as they were unreachable
|
|
9d14f27bddab351fe98a2ae197bd4cf8a092c4f3 log: log RPC port on startup (James O'Beirne)
Pull request description:
I just spent a few hours trying to figure out why "18444" wasn't getting me to regtest's RPC server. I'm not the sharpest tool in the shed, but I was maybe understandably confused because "Bound to 127.0.0.1:18445" appears in the logs, which I assumed was the P2P port.
This change logs the RPC listening address by default on startup, which seems like a basic piece of information that shouldn't be buried under `-debug`.
ACKs for top commit:
dergoegge:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
jarolrod:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
aureleoules:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
Tree-SHA512: 5c86f018c0b8d6264abf878c921afe53033b23ab4cf289276bb1ed28fdf591c9d8871a4baa4098c363cb2aa9a637d2e4e18e56b14dfc7d767ee40757d7ff2e7c
|
|
We currently use both. Consolidate on the former.
|
|
We currently use both. Consolidate on the former.
|
|
I just spent a few hours trying to figure out why "18444" wasn't getting
me to regtest's RPC server. I'm not the sharpest tool in the shed, but I
was maybe understandably confused because "Bound to
127.0.0.1:18445" appears in the logs, which I assumed was the P2P port.
This change logs the RPC listening address, which seems like a basic
piece of information that shouldn't be buried in debug logs.
|
|
|
|
|
|
AUTOUIC feature
018d70b58726b361b6951e0e6de04f13eb97a89d scripted-diff: Avoid incompatibility with CMake AUTOUIC feature (Hennadii Stepanov)
Pull request description:
Working on [migration](https://github.com/hebasto/bitcoin/pull/3) from Autotools to CMake build system, I found that our current code base needs to be adjusted.
CMake [allows](https://cmake.org/cmake/help/latest/prop_tgt/AUTOUIC.html) to
> handle the Qt `uic` code generator automatically
When using this feature, statements like `#include "ui_<ui_base>.h"` are processed in a special way.
The `node/ui_interface.h` unintentionally breaks this feature. Of course, it is possible to provide a list of source files to be excluded from `AUTOUIC`. But, unfortunately, this approach does not work for the `qt/sendcoinsdialog.cpp` source file, where there are both https://github.com/bitcoin/bitcoin/blob/b71d37da2c8c8d2a9cef020731767a6929db54b4/src/qt/sendcoinsdialog.cpp#L10 and https://github.com/bitcoin/bitcoin/blob/b71d37da2c8c8d2a9cef020731767a6929db54b4/src/qt/sendcoinsdialog.cpp#L24
ACKs for top commit:
MarcoFalke:
cr ACK 018d70b58726b361b6951e0e6de04f13eb97a89d
ryanofsky:
Code review ACK 018d70b58726b361b6951e0e6de04f13eb97a89d
furszy:
Code review ACK 018d70b5
Tree-SHA512: 4fc83f2e5a82c8ab15c3c3d68f48b9863c47b96c0a66b6276b9b4dfc6063abffd73a16382acfe116553487b3ac697dbde2d9ada1b92010c5d8f8c6aa06f56428
|
|
-BEGIN VERIFY SCRIPT-
sed -i "s|node/ui_interface|node/interface_ui|g" $(git grep -l "node/ui_interface" ./src)
git mv src/node/ui_interface.cpp src/node/interface_ui.cpp
git mv src/node/ui_interface.h src/node/interface_ui.h
sed -i "s|BITCOIN_NODE_UI_INTERFACE_H|BITCOIN_NODE_INTERFACE_UI_H|g" src/node/interface_ui.h
-END VERIFY SCRIPT-
|
|
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
|
|
This is more consistent with the other functions, as well as with the
logging output itself. If we want to make this change, we should do it
before it's all over the place.
|
|
Map libevent's severity to our own severity level for logging.
|
|
|
|
|
|
e40779a4fee03c6c455149bd8e9d1a7ccd991450 refactor: Remove outdated libevent logging code (Fabian Jahr)
0598f36852199d0cee8fe9e676a2e0bec3ebf624 refactor: account for requiring libevent 2.1.8+ (fanquake)
aaf72d62c18f9cb325c150cf0cc21abb201607c8 build: Bump libevent minimum version up to 2.1.8 (Hennadii Stepanov)
Pull request description:
Required to support new functionality in bitcoin/bitcoin#19420.
`libevent` availability: https://repology.org/project/libevent/versions
ACKs for top commit:
laanwj:
Code review ACK e40779a4fee03c6c455149bd8e9d1a7ccd991450
fanquake:
ACK e40779a4fee03c6c455149bd8e9d1a7ccd991450
Tree-SHA512: ccb14ea2f591484a3df5bc4a19f4f5400ef6b1cfb7dc45dd99f96cb948748215ed3b5debc34869763c91b8c7a26993fdb9b870950c0743c4d01038ab27c5e4e2
|
|
The removed code was intended to catch issues with event_enable_debug_logging which was not available prior to libevent 2.1.1. This is not necessary since the minimum libevent version was bumped to 2.1.8.
|
|
|
|
Easily get the query parameter from the URI, with optional default value.
|
|
evhttp_connection_get_peer for new libevent
c62d763fc313585d79ad833c9d729f6acf2652aa Necessary improvements to make configure work without libevent installed (Perlover)
091ccc38c2e589b649648cbcc99aca4802f98775 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover)
Pull request description:
The second parameter of evhttp_connection_get_peer in libevent already has type as `const char **`
The compilation of bitcoind with the fresh libevent occurs errors
Details: https://github.com/bitcoin/bitcoin/issues/23606
ACKs for top commit:
laanwj:
Code review ACK c62d763fc313585d79ad833c9d729f6acf2652aa
luke-jr:
tACK c62d763fc313585d79ad833c9d729f6acf2652aa
Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
|
|
the second parameter. Fixing the problem.
|
|
secure computing mode)
4747da3a5b639b5a336b737e7e3cbf060cf2efcf 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 4747da3a5b639b5a336b737e7e3cbf060cf2efcf
Tree-SHA512: e1c28e323eb4409a46157b7cc0fc29a057ba58d1ee2de268962e2ade28ebd4421b5c2536c64a3af6e9bd3f54016600fec88d016adb49864b63edea51ad838e17
|
|
|
|
Now that we require libevent >=2.0.21, remove backwards compatibility
code for older versions.
|
|
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
|
|
4e353cb618745cdb5d98e58e7dcd400ded01299a http: Release work queue after event base finish (João Barbosa)
Pull request description:
This fixes a race between `http_request_cb` and `StopHTTPServer` where
the work queue is used after release.
Fixes #18856.
ACKs for top commit:
fjahr:
Code review ACK 4e353cb618745cdb5d98e58e7dcd400ded01299a
achow101:
ACK 4e353cb618745cdb5d98e58e7dcd400ded01299a
LarryRuane:
ACK 4e353cb618745cdb5d98e58e7dcd400ded01299a
hebasto:
ACK 4e353cb618745cdb5d98e58e7dcd400ded01299a, tested (rebased on top of master 9313c4e6aa4b707c06a86b33d5d2753cd8383340) on Linux Mint 20.1 (x86_64) using MarcoFalke's [patch](https://github.com/bitcoin/bitcoin/pull/19033#issuecomment-640106647), including different `-rpcthreads`/`-rpcworkqueue` cases. The bug is fixed. The code is correct.
Tree-SHA512: 185d2a9744d0d5134d782bf321ac9958ba17b11a5b3d70b4897c8243e6b146dfd3f23c57aef8e10ae9484374120b64389c1949a9cf0a21dccc47ffc934c20930
|
|
|
|
exceeded
8dd5946c0b7aa8f3976b14a5de4ce84b80a9c32a add functional test (Larry Ruane)
b5a80fa7e487c37b7ac0e3874a2fabade41b9ca8 util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)
Pull request description:
If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.
ACKs for top commit:
fjahr:
tACK 8dd5946c0b7aa8f3976b14a5de4ce84b80a9c32a
luke-jr:
utACK 8dd5946c0b7aa8f3976b14a5de4ce84b80a9c32a (no changes since previous utACK)
hebasto:
re-ACK 8dd5946c0b7aa8f3976b14a5de4ce84b80a9c32a, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/18335#pullrequestreview-460621350) review.
darosior:
ACK 8dd5946c0b7aa8f3976b14a5de4ce84b80a9c32a
Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
|
|
|
|
|
|
|
|
This fixes a race between http_request_cb and StopHTTPServer where
the work queue is used after release.
|
|
-BEGIN VERIFY SCRIPT-
# Move files
git mv src/ui_interface.h src/node/ui_interface.h
git mv src/ui_interface.cpp src/node/ui_interface.cpp
sed -i -e 's/BITCOIN_UI_INTERFACE_H/BITCOIN_NODE_UI_INTERFACE_H/g' src/node/ui_interface.h
# Adjust includes and makefile
sed -i -e 's|ui_interface|node/ui_interface|g' $(git grep -l ui_interface)
# Sort includes
git diff -U0 | clang-format-diff -p1 -i -v
-END VERIFY SCRIPT-
|