fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332325604e8ca6080262543e10de4e46c refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743a0637d16812d26430d99464cab0cee refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e76d4def59dff92bfb2ebbfc6cd4c195 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499ca611b513d9d1986d79c5e3a58cd0f2 refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341fa59b5b0b05f974105104fb2a0df9c3 refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke)
Pull request description:
Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues:
* It is always hard-coded to the empty string.
* This is confusing and tedious when reading or maintaining the code.
* It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`.
* The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used.
Fix all issues by removing it.
ACKs for top commit:
hebasto:
ACK fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
The caption was empty for all call-sites, so this refactor does not
change any behavior.
Note that noui_ThreadSafeMessageBoxRedirect is test-only, so no end-user
behavior is changed here.
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "libevent:".
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
These methods in the Sock class wrap corresponding syscalls,
accepting void* arguments and casting to char* internally, which is
needed for Windows support and ignored on other platforms because
the syscall itself accepts void*:
Send()
Recv()
GetSockOpt()
SetSockOpt()
12ff4be9c724c752fe67835419be3ff4d0e65990 test: ensure -rpcallowip is compatible with RFC4193 (Matthew Zipkin)
c02bd3c1875abd877a0dc73fb8866c883b7fcd32 config: Explain RFC4193 and CJDNS interaction in help and init error (Matthew Zipkin)
f728b6b11100fae1e27f7a0ef92a5930fa8cffb3 init: Configure reachable networks before we start the RPC server (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/32433
`MaybeFlipIPv6toCJDNS()` relies on `g_reachable_nets` to distinguish between CJDNS addresses and other IPv6 addresses. In particular, [RFC4193](https://www.rfc-editor.org/rfc/rfc4193#section-3.1) address or "Unique Local Address" with the L-bit unset also begins with the `fc` prefix. #32433 highlights a use case for these addresses that have nothing to do with CJDNS.
On master we don't parse init flags like `-cjdnsreachable` until *after* the HTTP server has started, causing conflicts with `-rpcallowip` because CJDNS doesn't support subnets.
This PR ensures that `NET_CJDNS` is only present in the reachable networks list if set by `-cjdnsreachable` *before* `-rpcallowip` is checked. If it is set all `fc` addresses are assumed to be CJDNS, can not have subnets, and can't be set for `-rpcallowip`.
I also noted this specific parameter interaction in the init help as well as the error message if configured incorrectly.
This can be tested locally:
`bitcoind -regtest -rpcallowip=fc00:dead:beef::/64 -rpcuser=u -rpcpassword=p`
On master this will just throw an error that doesn't even mention IPv6 at all.
On the branch, this will succeed and can be tested by adding the ULA to a local interface.
On linux: `sudo ip -6 addr add fc00:dead:beef::1/64 dev lo`
On macos: `sudo ifconfig lo0 inet6 fc00:dead:beef::1/128 add`
then: `curl -v -g -6 --interface fc00:dead:beef::1 u:p@[::1]:18443 --data '{"method":"getblockcount"}'`
If the `rpcallowip` option is removed, the RPC request will fail to authorize.
Finally, adding `-cjdnsreachable` to the start up command will throw an error and specify the incompatibility:
> RFC4193 is allowed only if -cjdnsreachable=0.
ACKs for top commit:
achow101:
ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
tapcrafter:
tACK 12ff4be9c724c752fe67835419be3ff4d0e65990
ryanofsky:
Code review ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
willcl-ark:
ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
Tree-SHA512: a4dd70ca2bb9f6ec2c0a9463fd73985d1ed80552c674a9067ac9a86662d1c018cc275ba757cebb2993c5f3971ecf4778b95d35fe7a7178fb41b1d18b601c9960
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.
Also add code comment to clarify what intent of existing code is.
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
Adds invalid rpcbind port checking to
`HTTPBindAddresses()`. While movement of
`CheckHostPortOptions()` in the previous
commit handles rcpbind port errors, updating
`HTTPBindAddresses()` port checking adds
a defensive measure for potential future
changes.
03d49d0f25ab5660524d5ddd171de677a808b984 http: set TCP_NODELAY when creating HTTP server (Roman Zeyde)
Pull request description:
Otherwise, the default HTTP server config may result in high latency, due to Nagle's algorithm (on the server) and delayed ACK (on the client):
[1] https://www.extrahop.com/blog/tcp-nodelay-nagle-quickack-best-practices
[2] https://eklitzke.org/the-caveats-of-tcp-nodelay
Without the fix, fetching a small block takes ~40ms (when connection keep-alive is enabled):
```
$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Server Software:
Server Hostname: localhost
Server Port: 8332
Document Path: /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Document Length: 25086 bytes
Concurrency Level: 1
Time taken for tests: 4.075 seconds
Complete requests: 100
Failed requests: 0
Keep-Alive requests: 100
Total transferred: 2519200 bytes
HTML transferred: 2508600 bytes
Requests per second: 24.54 [#/sec] (mean)
Time per request: 40.747 [ms] (mean)
Time per request: 40.747 [ms] (mean, across all concurrent requests)
Transfer rate: 603.76 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 0 41 4.1 41 42
Waiting: 0 0 0.1 0 1
Total: 0 41 4.1 41 42
Percentage of the requests served within a certain time (ms)
50% 41
66% 41
75% 41
80% 41
90% 42
95% 42
98% 42
99% 42
100% 42 (longest request)
```
With the fix, it takes ~0.2ms:
```
$ ab -k -c 1 -n 1000 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests
Server Software:
Server Hostname: localhost
Server Port: 8332
Document Path: /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Document Length: 25086 bytes
Concurrency Level: 1
Time taken for tests: 0.194 seconds
Complete requests: 1000
Failed requests: 0
Keep-Alive requests: 1000
Total transferred: 25192000 bytes
HTML transferred: 25086000 bytes
Requests per second: 5147.05 [#/sec] (mean)
Time per request: 0.194 [ms] (mean)
Time per request: 0.194 [ms] (mean, across all concurrent requests)
Transfer rate: 126625.50 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 0 0 0.0 0 0
Waiting: 0 0 0.0 0 0
Total: 0 0 0.0 0 0
Percentage of the requests served within a certain time (ms)
50% 0
66% 0
75% 0
80% 0
90% 0
95% 0
98% 0
99% 0
100% 0 (longest request)
```
ACKs for top commit:
achow101:
ACK 03d49d0f25ab5660524d5ddd171de677a808b984
theStack:
re-ACK 03d49d0f25ab5660524d5ddd171de677a808b984
tdb3:
ACK 03d49d0f25ab5660524d5ddd171de677a808b984
Tree-SHA512: bbf3d78b8521f569430850ec4315a75711303547df1a3de213a4ad34c9700105e374e0a649352fd05f8e4badb5b59debd3720e1c5d392c5113d7816648f7fcaa
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.
There is no change in behavior in this commit.
fb3e812277041f239b97b88689a5076796d75b9b p2p: return `CSubNet` in `LookupSubNet` (brunoerg)
Pull request description:
Analyzing the usage of `LookupSubNet`, noticed that most cases uses check if the subnet is valid by calling `subnet.IsValid()`, and the boolean returned by `LookupSubNet` hasn't been used so much, see:
29d540b7ad/src/httpserver.cpp (L172-L174)29d540b7ad/src/net_permissions.cpp (L114-L116)
It makes sense to return `CSubNet` instead of `bool`.
ACKs for top commit:
achow101:
ACK fb3e812277041f239b97b88689a5076796d75b9b
vasild:
ACK fb3e812277041f239b97b88689a5076796d75b9b
theStack:
Code-review ACK fb3e812277041f239b97b88689a5076796d75b9b
stickies-v:
Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812277041f239b97b88689a5076796d75b9b) and it all looks good to me.
Tree-SHA512: ba50d6bd5d58dfdbe1ce1faebd80dd8cf8c92ac53ef33519860b83399afffab482d5658cb6921b849d7a3df6d5cea911412850e08f3f4e27f7af510fbde4b254
0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af net: remove unused CConnman::FindNode(const CSubNet&) (Vasil Dimov)
9482cb780fe04c1f1d9050edd1b8e549e52c86ce netbase: possibly change the result of LookupSubNet() to CJDNS (Vasil Dimov)
53afa68026ffa1313ae4aba3664de7791d23b1c8 net: move MaybeFlipIPv6toCJDNS() from net to netbase (Vasil Dimov)
6e308651c441cbf8763c67cc099c538c333c2872 net: move IsReachable() code to netbase and encapsulate it (Vasil Dimov)
c42ded3d9bda8b273780a4a81490bbf1b9e9c261 fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS (Vasil Dimov)
64d6f77907afd461d9b14ee10ab32335f4454734 net: put CJDNS prefix byte in a constant (Vasil Dimov)
Pull request description:
`LookupSubNet()` would treat addresses that start with `fc` as IPv6 even if `-cjdnsreachable` is set. This creates the following problems where it is called:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS).
* `RPCConsole::unbanSelectedNode()`: in the GUI the ban entries go through `CSubNet::ToString()` and back via `LookupSubNet()`. Then it must match whatever is stored in `BanMan`, otherwise it is impossible to unban via the GUI.
These were uncovered by https://github.com/bitcoin/bitcoin/pull/26859.
Thus, flip the result of `LookupSubNet()` to CJDNS if the network base address starts with `fc` and `-cjdnsreachable` is set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treat `fc.../mask` as an invalid `CSubNet`.
To achieve that, `MaybeFlipIPv6toCJDNS()` has to be moved from `net` to `netbase` and thus also `IsReachable()`. In the process of moving `IsReachable()`, `SetReachable()` and `vfLimited[]` encapsulate those in a class.
ACKs for top commit:
jonatack:
Code review ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
achow101:
ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
mzumsande:
re-ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
Tree-SHA512: 4767a60dc882916de4c8b110ce8de208ff3f58daaa0b560e6547d72e604d07c4157e72cf98b237228310fc05c0a3922f446674492e2ba02e990a272d288bd566
All callers of `LookupSubNet()` need the result to be of CJDNS type if
`-cjdnsreachable` is set and the address begins with `fc`:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails
to white list CJDNS addresses: when a CJDNS peer connects to us, it
will be matched against IPv6 `fc...` subnet and the match will never
succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in
`banlist.json`. Upon reading from disk they have to be converted back
to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6)
would not match a peer (`fc00::1`, CJDNS).
* `setban()` (in `rpc/net.cpp`): otherwise `setban fc.../mask add` would
add an IPv6 entry to BanMan. Subnetting does not make sense for CJDNS
addresses, thus treat `fc.../mask` as invalid `CSubNet`. The result of
`LookupHost()` has to be converted for the case of banning a single
host.
* `InitHTTPAllowList()`: not necessary since before this change
`-rpcallowip=fc...` would match IPv6 subnets against IPv6 peers even
if they started with `fc`. But because it is necessary for the above,
`HTTPRequest::GetPeer()` also has to be adjusted to return CJDNS peer,
so that now CJDNS peers are matched against CJDNS subnets.
It is possible that the client disconnects before the request is
handled. In those cases, evhttp_request_set_on_complete_cb is never
called, which means that on shutdown the server we'll keep waiting
endlessly.
By adding evhttp_connection_set_closecb, libevent automatically
cleans up those dead connections at latest when we shutdown, and
depending on the libevent version already at the moment of remote
client disconnect. In both cases, the bug is fixed.
Introduces and uses a HTTPRequestTracker class to keep track of
how many HTTP requests are currently active, so we don't stop the
server before they're all handled.
This has two purposes:
1. In a next commit, allows us to untrack all requests associated
with a connection without running into lifetime issues of the
connection living longer than the request
(see https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783)
2. Improve encapsulation by making the mutex and cv internal members,
and exposing just the WaitUntilEmpty() method that can be safely
used.
79d343a642f985801da463b03a0627a59a095238 http: update libevent workaround to correct version (stickies-v)
Pull request description:
The libevent bug described in 5ff8eb2637 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](5b40744d15) and [b25813800f97179b2355a7b4b3557e6a7f568df2](b25813800f).
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 5ff8eb2637
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/24303https://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.
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](60978c8080)
hebasto:
ACK 60978c8080ec13ff4571c8a89e742517b2aca692
Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
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.
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()`.
