5952292133 wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam)
54e07ee22f wallet: track mempool conflicts (ishaanam)
d64922b590 wallet refactor: use CWalletTx member functions to determine tx state (ishaanam)
ffe5ff1fb6 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam)
180973a941 test: Add tests for wallet mempool conflicts (ishaanam)
Pull request description:
The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory.
`IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true.
A txid is added to `mempool_conflicts` during `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during `transactionRemovedFromMempool`.
This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result.
Builds on #27145
Second attempt at #18600
ACKs for top commit:
achow101:
ACK 5952292133
ryanofsky:
Code review ACK 5952292133. Just small suggested changes since last review
furszy:
ACK 59522921
Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29
7295986778 Unit tests for CalculateFeerateDiagramsForRBF (Greg Sanders)
b767e6bd47 test: unit test for ImprovesFeerateDiagram (Greg Sanders)
7e89b659e1 Add fuzz test for FeeFrac (Greg Sanders)
4d6528a3d6 fuzz: fuzz diagram creation and comparison (Greg Sanders)
e9c5aeb11d test: Add tests for CompareFeerateDiagram and CheckConflictTopology (Greg Sanders)
588a98dccc fuzz: Add fuzz target for ImprovesFeerateDiagram (Greg Sanders)
2079b80854 Implement ImprovesFeerateDiagram (Greg Sanders)
66d966dcfa Add FeeFrac unit tests (Greg Sanders)
ce8e22542e Add FeeFrac utils (Greg Sanders)
Pull request description:
This is a smaller piece of https://github.com/bitcoin/bitcoin/pull/28984 broken off for easier review.
Up to date explanation of diagram checks are here: https://delvingbitcoin.org/t/mempool-incentive-compatibility/553
This infrastructure has two near term applications prior to cluster mempool:
1) Limited Package RBF(https://github.com/bitcoin/bitcoin/pull/28984): We want to allow package RBF only when we know it improves the mempool. This narrowly scoped functionality allows use with v3-like topologies, and will be expanded at some point post-cluster mempool when diagram checks can be done efficiently against bounded cluster sizes.
2) Replacement for single tx RBF(in a cluster size of up to two) against conflicts of up to cluster size two. `ImprovesFeerateDiagram` interface will have to change for this use-case, which is a future direction to solve certain pins and improve mempool incentive compatibility: https://delvingbitcoin.org/t/ephemeral-anchors-and-mev/383#diagram-checks-fix-this-3
And longer-term, this would be the proposed way we would compute incentive compatibility for all conflicts, post-cluster mempool.
ACKs for top commit:
sipa:
utACK 7295986778
glozow:
code review ACK 7295986778
murchandamus:
utACK 7295986778
ismaelsadeeq:
Re-ACK 7295986778
willcl-ark:
crACK 7295986778
sdaftuar:
ACK 7295986778
Tree-SHA512: 79593e5a087801c06f06cc8b73aa3e7b96ab938d3b90f5d229c4e4bfca887a77b447605c49aa5eb7ddcead85706c534ac5eb6146ae2396af678f4beaaa5bea8e
824f47294a node: Use log levels in noui_ThreadSafeMessageBox (TheCharlatan)
ddc7872c08 node: Make translations of fatal errors consistent (TheCharlatan)
Pull request description:
The extra `bilingual_str` argument of the fatal error notifications and `node::AbortNode()` is often unused and when used usually contains the same string as the message argument. It also seems to be confusing, since it is not consistently used for errors requiring user action. For example some assumeutxo fatal errors require the user to do something, but are not translated.
So simplify the fatal error and abort node interfaces by only passing a translated string. This slightly changes the fatal errors displayed to the user.
ACKs for top commit:
stickies-v:
re-ACK 824f47294a
maflcko:
ACK 824f47294a🔎
achow101:
ACK 824f47294a
hebasto:
re-ACK 824f47294a.
Tree-SHA512: 2868ee7b045fe7f3ac582ce5039141b398480b7627734976201dafaaef7544b8461635a7292fee4a7f32ff1bfc26f9bd4d0c292dca424ba42fb7fc4483d7ce8d
99954f914f test: fix test to ensure hidden RPC is present in detailed help (stratospher)
0d01f6f0c6 test: remove unused mocktime in test_addpeeraddress (0xb10c)
6205466512 rpc: "addpeeraddress tried" return error on failure (0xb10c)
Pull request description:
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 new returning `{ "success": false, "error": "..." }` 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. This is done in the functional test for the `getrawaddrman` RPC.
Fixes #28964
ACKs for top commit:
achow101:
ACK 99954f914f
stratospher:
reACK 99954f9. 🚀
brunoerg:
utACK 99954f914f
Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76
fa4d98b3c8 Avoid divide-by-zero in header sync logs when NodeClock is behind (MarcoFalke)
fa58550317 refactor: Modernize header sync logs (MarcoFalke)
Pull request description:
The log may be confusing, when the NodeClock is behind the current header tip.
Fix it, by assuming the NodeClock is never behind the current header tip.
ACKs for top commit:
sipa:
utACK fa4d98b3c8
sr-gi:
tACK [fa4d98b](fa4d98b3c8)
achow101:
ACK fa4d98b3c8
tdb3:
ACK fa4d98b3c8
Tree-SHA512: 3c5aee4030af387695918c5238012c972ebf850b52e956b5f74590cd7fd4eff0b3e593d411e3eb2a0bb12294af8dc6fbe320f90e4c261399b65a404ff3c3cbd9
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.
So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.
Also de-duplicate the abort error log since it is repeated in noui.cpp.
f65b0f6401 index: Move last_locator_write_time and logging to end of threadsync loop (Fabian Jahr)
Pull request description:
In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit 7878f97bf1 from https://github.com/bitcoin/bitcoin/pull/25494 and was reported by pstratem in https://github.com/bitcoin/bitcoin/pull/26903 with an alternate fix.
ACKs for top commit:
achow101:
ACK f65b0f6401
ryanofsky:
Code review ACK f65b0f6401. Just moved log "Syncing" log line since last commit to avoid having to call now() twice.
furszy:
ACK f65b0f6401
TheCharlatan:
ACK f65b0f6401
Tree-SHA512: afa8f05786318d36346d167ff53ea0b3bc8abdb0ad04465d199dc3eb91e9f837369e24fcb7e24b5757b02d698ec504e61da6ac365eaf006c874fc07a424a7e20
Behavior changes are:
- if a tx has a mempool conflict, the wallet will not attempt to
rebroadcast it
- if a txo is spent by a mempool-conflicted tx, that txo is no
longer considered spent
9d9a7458a2 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed2 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d76 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662 assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf9 doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6b validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)
Pull request description:
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.
Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.
Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
ACKs for top commit:
Sjors:
re-ACK 9d9a7458a2
achow101:
ACK 9d9a7458a2
mzumsande:
Tested ACK 9d9a7458a2
maflcko:
ACK 9d9a7458a2🎯
Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
dfcef536d0 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2039
When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.
`FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.
The call stack looks like this:
```
init()
ThreadImport() <-- process fReindex flag
LoadExternalBlockFile()
AcceptBlock()
SaveBlockToDisk()
FindBlockPos()
FlushBlockFile() <-- unnecessary if block is already on disk
```
A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.
ACKs for top commit:
sipa:
utACK dfcef536d0
mzumsande:
re-ACK dfcef536d0
achow101:
ACK dfcef536d0
furszy:
Rebase diff ACK dfcef53.
Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
99afb9d15a refactor: init, simplify index shutdown code (furszy)
0faafb57f8 index: decrease ThreadSync cs_main contention (furszy)
f1469eb454 index: cache last block filter header (furszy)
a6756ecdb2 index: blockfilter, decouple header lookup into its own function (furszy)
331f044e3b index: blockfilter, decouple Write into its own function (furszy)
bcbd7eb8d4 bench: basic block filter index initial sync (furszy)
Pull request description:
Work decoupled from #26966 per request.
The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.
Also, reduces `cs_main` lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.
Testing Note:
To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with `-blockfilterindex` and monitor the logs until the syncing process finish.
Local Benchmark Results:
*Master (c252a0fc0f):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 132,042,516.60 | 7.57 | 0.3% | 7.79 | `BlockFilterIndexSync`
*PR (43a212cfdac6c64e82b601c664443d022f191520):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 126,915,841.60 | 7.88 | 0.6% | 7.51 | `BlockFilterIndexSync`
ACKs for top commit:
Sjors:
re-ACK 99afb9d15a
achow101:
ACK 99afb9d15a
TheCharlatan:
Re-ACK 99afb9d15a
andrewtoth:
ACK 99afb9d15a
Tree-SHA512: 927daadd68f4ee1ca781a89519539b895f5185a76ebaf525fbc246ea8dcf40d44a82def00ac34b188640802844b312270067f1b33e65a2479e06be9169c616de
6e873df347 serfloat: improve/simplify tests (Pieter Wuille)
b45f1f5658 serfloat: do not test encode(bits)=bits anymore (Pieter Wuille)
Pull request description:
Closes #28941.
Our current tests for serfloat verify two distinct properties:
1. Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
2. Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.
#28941 seems to show that the second property doesn't always hold, but just for "subnormal" numbers (below $2^{-1021}$). Since we don't care about encoding these numbers, we could exclude such subnormal numbers from the hardware-identical representation test, but this PR goes further and just drops the second property entirely, as I don't think we care about edge-case compatibility with pre-v22.0 code for fee_estimates.dat (the only place it is used).
ACKs for top commit:
glozow:
ACK 6e873df347
fanquake:
ACK 6e873df347 - It's not as much of a priority, but I think we could still backport this.
Tree-SHA512: e18ceee0753a7ee7e999fdfa10b014dc5bb67b6ef79522a0f8c76b889adcfa785772fc26ed7559bcb5a09a9938e243bb54eedd9549bc59080a2c8090155e2267
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.
626f8e398e fuzz: actually test garbage >64b in p2p transport test (Pieter Wuille)
Pull request description:
This fixes an oversight from #28196: in the `p2p_transport_bidirectional_v2` fuzz test, when the desired garbage length is over 64 bytes, the code would actually use garbage length 0. Fix this.
ACKs for top commit:
instagibbs:
ACK 626f8e398e
brunoerg:
crACK 626f8e398e
Tree-SHA512: f6346367adb10464b6c9d20aef43625531d2a4d8110887ad03214b8c1907b83560f2dd5b5415e2180a40b4cd276d51881b32b60c740471b5c6bb218aa19848d8
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
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.
This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.
Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.
636c9862cf ci: Bump `TIDY_LLVM_V` (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.22](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.22), which is compatible with Clang 18.
ACKs for top commit:
fanquake:
ACK 636c9862cf
Tree-SHA512: 78ce89244c5e487dd1be8b4bd2ca6f06d19b04b78289ebc21985110574053545dcce5eb622edf2bede2cf7bb58360170e976d30a4484a127d34dd17b1c604e9c
This new function takes the populated sets of
direct and all conflicts computed in the current
mempool, assuming the replacements are a single
chunk, and computes a diagram check.
The diagram check only works against cluster
sizes of 2 or less, and fails if it encounters
a different topology.
Co-authored-by: Suhas Daftuar <sdaftuar@chaincode.com>
28287cfbe1 test: add script compression coverage for not-on-curve P2PK outputs (Sebastian Falbesoner)
Pull request description:
This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](44b05bf3fe/src/pubkey.cpp (L297-L302)), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't be recovered (see also call-site of `IsToPubKey`):
44b05bf3fe/src/compressor.cpp (L49-L50)
Likewise, for a compressed script of an uncompressed P2PK script (i.e. compression ids 4 and 5) where the x coordinate is not on the curve, decompression fails:
44b05bf3fe/src/compressor.cpp (L122-L129)
Note that the term "compression" is used here in two different meanings (though they are related), which might be a little confusing. The encoding of a pubkey can either be compressed (33-bytes with 0x02/0x03 prefixes) or uncompressed (65-bytes with 0x04 prefix). On the other hand there is also compression for whole output scripts, which is used for storing scriptPubKeys in the UTXO set in a compact way (and also for the `dumptxoutset` result, accordingly). P2PK output scripts with uncompressed pubkeys get compressed by storing only the x-coordinate and the sign as a prefix (0x04 = even, 0x05 = odd). Was diving deeper into the subject while working on https://github.com/bitcoin/bitcoin/pull/27432, where the script decompression of uncompressed P2PK needed special handling (see also https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108798536).
Trivia: as of now (block 801066), there are 13 uncompressed P2PK outputs in the UTXO set with a pubkey not on the curve (which obviously means they are unspendable).
ACKs for top commit:
achow101:
ACK 28287cfbe1
tdb3:
ACK for 28287cfbe1.
cbergqvist:
ACK 28287cf!
marcofleon:
Nicely done, ACK 28287cfbe1. Built the PR branch, ran the unit and functional tests, everything passed.
Tree-SHA512: 777b6c3065654fbfa1ce94926f4cadb91a9ca9dc4dd4af6008ad77bd1da5416f156ad0dfa880d26faab2e168bf9b27e0a068abc9a2be2534d82bee61ee055c65
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.
07cd510ffe [refactor] consolidate invalid MempoolAcceptResult processing (glozow)
9353aa4066 [refactor] consolidate valid MempoolAcceptResult processing (glozow)
Pull request description:
Every time we try to `ProcessTransaction` (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in `m_recent_rejects` so we don't try to download/validate it again.
There are 2 current places and at least 1 future place where we need to process `MempoolAcceptResult`:
- In the `ProcessMessage` logic after receiving and validating a tx
- In `ProcessOrphanTx` where we retry orphans
- With #28970, after processing a package of transactions, we should do these updates for each tx in the package.
Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR.
ACKs for top commit:
instagibbs:
ACK 07cd510ffe
achow101:
ACK 07cd510ffe
dergoegge:
Code review ACK 07cd510ffe
TheCharlatan:
ACK 07cd510ffe
Tree-SHA512: c4e74cb65e4f52882fca52e6682efa5dcf1562d98418454e09be256ffd026caae642a90aa5b9cccaae214be240d6f4be9d87b516953b2ee69a655f16ea569ed9
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
