542e13b293 rpc: Enhance metadata of the dumptxoutset output (Fabian Jahr)
4d8e5edbaa assumeutxo: Add documentation on dumptxoutset serialization format (Fabian Jahr)
c14ed7f384 assumeutxo: Add test for changed coin size value (Fabian Jahr)
de95953d87 rpc: Optimize serialization disk space of dumptxoutset (Fabian Jahr)
Pull request description:
The second attempt at implementing the `dumptxoutset` space optimization as suggested in #25675. Closes #25675.
This builds on the work done in #26045, addresses open feedback, adds some further improvements (most importantly usage of compact size), documentation, and an additional test.
The [original snapshot at height 830,000](https://github.com/bitcoin/bitcoin/pull/29551) came in at 10.82 GB. With this change, the same snapshot is 8.94 GB, a reduction of 17.4%.
This also enhances the metadata of the output file and adds the following data to allow for better error handling and make future upgrades easier:
- A newly introduced utxo set magic
- A version number
- The network magic
- The block height
ACKs for top commit:
achow101:
ACK 542e13b293
TheCharlatan:
Re-ACK 542e13b293
theStack:
ACK 542e13b293
Tree-SHA512: 0825d30e5c3c364062db3c6cbca4e3c680e6e6d3e259fa70c0c2b2a7020f24a47406a623582040988d5c7745b08649c31110df4c10656aa25f3f27eb35843d99
d7707d9843 rpc: avoid copying into UniValue (Cory Fields)
Pull request description:
These are the simple (and hopefully obviously correct) copies that can be moves instead.
This is a follow-up from https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108751842
As it turns out, there are hundreds of places where we copy UniValues needlessly. It should be the case that moves are always preferred over copies, so there should be no downside to these changes.
willcl-ark, however, noticed that memory usage may increase in some cases. Logically this makes no sense to me. The only plausible explanation imo is that because the moves are faster, more ops/second occur in some cases.
This list of moves was obtained by changing the function signatures of the UniValue functions to accept only rvalues, then compiling and fixing them up one by one. There still exist many places where copies are being made. These can/should be fixed up, but weren't done here for the sake of doing the easy ones first.
I ran these changes through clang-tidy with `performance-move-const-arg` and `bugprone-use-after-move` and no bugs were detected (though that's obviously not to say it can be trusted 100%).
As stated above, there are still lots of other less trivial fixups to do after these including:
- Using non-const UniValues where possible so that moves can happen
- Refactoring code in order to be able to move a UniValue without introducing a use-after-move
- Refactoring functions to accept UniValues by value rather than by const reference
ACKs for top commit:
achow101:
ACK d7707d9843
ryanofsky:
Code review ACK d7707d9843. No changes since last review other than rebase. I agree benchmarks showing increased peak memory usage and RSS are surprising, but number of allocations is down as expected, and runtime is also decreased.
willcl-ark:
ACK d7707d9843
Tree-SHA512: 7f511be73984553c278186286a7d161a34b2574c7f5f1a0edc87c2913b4c025a0af5241ef9af2df17547f2e4ef79710aa5bbb762fc9472435781c0488dba3435
1e54d61c46 test: add coverage for `mapped_as` from `getrawaddrman` (brunoerg)
8c2714907d net: rpc: return peer's mapped AS in getrawaddrman (brunoerg)
Pull request description:
This PR adds two new fields in `getrawaddrman` RPC: "mapped_as" and "source_mapped_as". These fields are used to return the ASN (Autonomous System Number) mapped to the peer and its source. With these informations we can have a better view of the bucketing logic with ASMap specially in projects like [addrman-observer](https://github.com/0xb10c/addrman-observer).
ACKs for top commit:
fjahr:
Code review ACK 1e54d61c46
virtu:
ACK [1e54d61](1e54d61c46)
0xB10C:
ACK 1e54d61c46
glozow:
ACK 1e54d61c46
Tree-SHA512: af86bcc7a2e69bebd3fa9eaa2e527e0758c44c0a958de7292514d5f99f8f01f5df3bae11400451268e0255f738ff3acccc77f48fe129937512f1e9d9963c4c5e
cbc6c440e3 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3 rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e0 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6 rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec4 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da test: refactor interface_rpc.py (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2960
Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found:
- returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error)
- returning both `"error"` and `"result"` fields together in a response object.
- different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors)
https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility.
https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned.
The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially:
- always return HTTP 200 "OK" unless there really is a server error or malformed request
- either return `"error"` OR `"result"` but never both
- same behavior for single and batch requests
If this is merged next steps can be:
- Refactor bitcoin-cli to always use strict 2.0
- Refactor the python test framework to always use strict 2.0 for everything
- Begin deprecation process for 1.0/1.1 behavior (?)
If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.
ACKs for top commit:
cbergqvist:
re ACK cbc6c440e3
ryanofsky:
Code review ACK cbc6c440e3. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.
tdb3:
re ACK for cbc6c440e3
Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
cc67d33fda refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition (Luke Dashjr)
Pull request description:
Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool
ACKs for top commit:
achow101:
ACK cc67d33fda
kristapsk:
cr utACK cc67d33fda
jonatack:
ACK cc67d33fda
Tree-SHA512: 9deb5ea6f85eeb1c7e04536cded65303b0ec459936a97e4f257aff2c50b0984a4ddbf69a4651f48455b9c80200a1fd24e9c74926874fdd9be436bbbe406251ce
For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional<UniValue> with a default value of
UniValue::VNULL.
A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.
b77bad309e rpc: move UniValue in blockToJSON (willcl-ark)
Pull request description:
Fixes: #24542
Fixes: #30052
Without explicitly declaring the move, these `UniValues` get copied, causing increased memory usage. Fix this by explicitly moving the `UniValue` objects.
Used by `rest_block` and `getblock` RPC.
ACKs for top commit:
maflcko:
review ACK b77bad309e
ismaelsadeeq:
ACK b77bad309e
TheCharlatan:
ACK b77bad309e
theuni:
utACK b77bad309e
hebasto:
ACK b77bad309e, I have reviewed the code and it looks OK.
BrandonOdiwuor:
ACK b77bad309e
Tree-SHA512: 767608331040f9cfe5c3568ed0e3c338920633472a1a50d4bbb47d1dc69d2bb11466d611f050ac8ad1a894b47fe1ea4d968cf34cbd44d4bb8d479fc5c7475f6d
Without explicitly declaring the move, these UniValues get copied,
causing increased memory usage. Fix this by explicitly moving the
UniValue objects.
Used by `rest_block` and `getblock` RPC.
The key module's functionality is not used by the kernel library, but
currently kernel users are still required to initialize the key module's
`secp256k1_context_sign` global as part of the `kernel::Context` through
`ECC_Start`.
78e52f663f doc: rpc: fix submitpackage examples (stickies-v)
1a875d4049 rpc: update min package size error message in submitpackage (stickies-v)
f9ece258aa doc: rpc: submitpackage takes sorted array (stickies-v)
17f74512f0 test: add bounds checking for submitpackage RPC (stickies-v)
Pull request description:
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.
Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplication and sending a confusing error message.
- fixups to the `submitpackage` examples
ACKs for top commit:
fjahr:
re-ACK 78e52f663f
instagibbs:
ACK 78e52f663f
achow101:
ACK 78e52f663f
glozow:
utACK 78e52f663f
Tree-SHA512: a8845621bb1cbf784167fc7c82cb8ceb105868b65b26d3465f072d1c04ef3699e85a21a524ade805d423bcecbc34f7d5bff12f2c21cbd902ae1fb154193ebdc9
98570fe29b test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d1 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK 98570fe29b
Eunovo:
Tested ACK 98570fe29b
achow101:
ACK 98570fe29b
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
fa09451f8e Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba82 scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e
TheCharlatan:
ACK fa09451f8e
hebasto:
re-ACK fa09451f8e, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
42fb5311b1 rpc: return warnings as an array instead of just a single one (stickies-v)
Pull request description:
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.
Fix that by returning all warnings as an array.
As a side benefit, clean up the GetWarnings() logic.
Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.
---
Some historical context from git log:
- when `GetWarnings` was introduced in 401926283a, it was used in the `getinfo` RPC, where only a [single error/warning was returned](401926283a (diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250)) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c, with the description [stating](ef2a3de25c (diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411)) that it returned "any network warnings" but in practice still only a single warning was returned
ACKs for top commit:
achow101:
re-ACK 42fb5311b1
tdb3:
Re ACK for 42fb5311b1
TheCharlatan:
ACK 42fb5311b1
maflcko:
ACK 42fb5311b1🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
The RPC documentation for `getblockchaininfo`, `getmininginfo` and
`getnetworkinfo` states that "warnings" returns "any network and
blockchain warnings". In practice, only a single warning is returned.
Fix that by returning all warnings as an array.
As a side benefit, cleans up the GetWarnings() logic.
c6be144c4b Remove timedata (stickies-v)
92e72b5d0d [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge)
7d9c3ec622 [net processing] Introduce PeerManagerInfo (dergoegge)
ee178dfcc1 Add TimeOffsets helper class (stickies-v)
55361a15d1 [net processing] Use std::chrono for type-safe time offsets (stickies-v)
038fd979ef [net processing] Move nTimeOffset to net_processing (dergoegge)
Pull request description:
[An earlier approach](1d226ae1f9/) in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually [left out](https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370) of the PR to make it easier for reviewers to focus on consensus logic changes.
Since network-adjusted time is now only used for warning/informational purposes, cleaning up the logic (building on @dergoegge's approach in #28956) should be quite straightforward and uncontroversial. The main changes are:
- Previously, we would only calculate the time offset from the first 199 outbound peers that we connected to. This limitation is now removed, and we have a proper rolling calculation. I've reduced the set to 50 outbound peers, which seems plenty.
- Previously, we would automatically use the network-adjusted time if the difference was < 70 mins, and warn the user if the difference was larger than that. Since there is no longer any automated time adjustment, I've changed the warning threshold to ~~20~~ 10 minutes (which is an arbitrary number).
- Previously, a warning would only be raised once, and then never again until node restart. This behaviour is now updated to 1) warn to log for every new outbound peer for as long as we appear out of sync, 2) have the RPC warning toggled on/off whenever we go in/out of sync, and 3) have the GUI warn whenever we are out of sync (again), but limited to 1 messagebox per 60 minutes
- no more globals
- remove the `-maxtimeadjustment` startup arg
Closes #4521
ACKs for top commit:
sr-gi:
Re-ACK [c6be144](c6be144c4b)
achow101:
reACK c6be144c4b
dergoegge:
utACK c6be144c4b
Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96
30a6c99935 rpc: access some args by name (stickies-v)
bbb31269bf rpc: add named arg helper (stickies-v)
13525e0c24 rpc: add arg helper unit test (stickies-v)
Pull request description:
Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.
Example usage:
```cpp
const auto action{self.Arg<std::string>("action")};
```
Most of the LoC is adding test coverage and documentation updates. No behaviour change.
An alternative approach to #27788 with significantly less overhaul.
ACKs for top commit:
fjahr:
Code review ACK 30a6c99935
maflcko:
ACK 30a6c99935🥑
ryanofsky:
Code review ACK 30a6c99935. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too.
Tree-SHA512: 4904f5f914fe1d421d32f60edb7c5a028c8ea0f140a2f207a106b4752d441164e073066a6bf2e17693f859fe847815a96609d3cf521e0ac4178d8cd09362ea3d
746b6d8839 test: Add test for createwalletdescriptor (Ava Chow)
2402b63062 wallet: Test upgrade of pre-taproot wallet to have tr() descriptors (Ava Chow)
460ae1bf67 wallet, rpc: Add createwalletdescriptor RPC (Ava Chow)
8e1a475062 wallet: Be able to retrieve single key from descriptors (Ava Chow)
85b1fb19dd wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors (Ava Chow)
73926f2d31 wallet, descspkm: Refactor wallet descriptor generation to standalone func (Andrew Chow)
54e74f46ea wallet: Refactor function for single DescSPKM setup (Andrew Chow)
3b09d0eb7f tests: Test for gethdkeys (Ava Chow)
5febe28c9e wallet, rpc: Add gethdkeys RPC (Ava Chow)
66632e5c24 wallet: Add IsActiveScriptPubKeyMan (Ava Chow)
fa6a259985 desc spkm: Add functions to retrieve specific private keys (Ava Chow)
fe67841464 descriptor: Be able to get the pubkeys involved in a descriptor (Ava Chow)
ef6745879d key: Add constructor for CExtKey that takes CExtPubKey and CKey (Ava Chow)
Pull request description:
This PR adds a `createwalletdescriptor` RPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use.
Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know, `gethdkeys` is also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to use `createwalletdescriptor`, they can easily get the keys that they can specify to it.
See also https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
ACKs for top commit:
Sjors:
re-utACK 746b6d8839
furszy:
ACK 746b6d8
ryanofsky:
Code review ACK 746b6d8839, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching `gethdkeys` output to use normalized descriptors (removing hardened path components).
Tree-SHA512: f2849101e6fbf1f59cb031eaaaee97af5b1ae92aaab54c5716940d210f08ab4fc952df2725b636596cd5747b8f5beb1a7a533425bc10d09da02659473516fbda
When trying to add an address to the IP address manager tried table,
it's first added to the new table and then moved to the tried table.
Previously, adding a conflicting address to the address manager's
tried table with test-only `addpeeraddress tried=true` RPC would
return `{ "success": true }`. However, the address would not be added
to the tried table, but would remain in the new table. This caused,
e.g., issue 28964.
This is fixed by returning `{ "success": false, "error":
"failed-adding-to-tried" }` for failed tried table additions. Since
the address remaining in the new table can't be removed (the address
manager interface does not support removing addresses at the moment
and adding this seems to be a bigger effort), an error message is
returned. This indicates to a user why the RPC failed and allows
accounting for the extra address in the new table.
Also:
To check the number of addresses in each addrman table,
the addrman checks were re-run and the log output of this check
was asserted. Ideally, logs shouldn't be used as an interface
in automated tests. To avoid asserting the logs, use the getaddrmaninfo
and getrawaddrman RPCs (which weren't implemented when the test was added).
Removing the "getnodeaddress" calls would also remove the addrman checks
from the test, which could reduce the test coverage. To avoid this,
these are kept.
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/28949
I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.
The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.
ACKs for top commit:
ismaelsadeeq:
Re-ACK 38f70ba6ac👍🏾
glozow:
ACK 38f70ba6ac with some non-blocking nits
murchandamus:
LGTM, code review ACK 38f70ba6ac
Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
567cec9a05 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe5192891 test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d0163 init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142e gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182 proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548ef net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6f netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d31 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51d configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05
tdb3:
re ACK for 567cec9a05.
achow101:
ACK 567cec9a05
vasild:
ACK 567cec9a05
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
Note that for speed this commit also removes the proof of work and
signet signature checks before returning the block in getblock.
It is assumed if a block is stored it will be valid.
d5228efb53 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913 scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97a [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5dee [refactor] Make signals optional in mempool and chainman (TheCharlatan)
Pull request description:
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.
Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.
ACKs for top commit:
maflcko:
re-ACK d5228efb53🌄
ryanofsky:
Code review ACK d5228efb53. Just comment change since last review.
vasild:
ACK d5228efb53
furszy:
diff ACK d5228ef
Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
