net, refactor: Kill proxy globals #26590

pull dergoegge wants to merge 4 commits into bitcoin:master from dergoegge:2022-11-socks5-refactor changing 19 files +602 −456

dergoegge commented at 3:14 PM on November 28, 2022: member

This PR moves the proxy info globals into their own component ProxyManager which CConnman is made to hold an instance of.

The alternative here would be to move the proxy info data directly into CConnman but it seems a little nicer for testing to have this be a self contained component.
DrahtBot commented at 3:14 PM on November 28, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26609 (refactor: Move txmempool_entry.h --> kernel/mempool_entry.h by hebasto)
- #26441 (rpc, p2p: add addpermissionflags RPC by brunoerg)
- #26261 (p2p: cleanup LookupIntern, Lookup and LookupHost by brunoerg)
- #26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)
- #25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
- #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
- #25515 (net, test: Virtualise CConnman and add initial PeerManager unit tests by dergoegge)
- #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
- #25268 (refactor: Introduce EvictionManager by dergoegge)
- #25136 (Torcontrol opt check by amadeuszpawlik)
- #20018 (p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified by dhruv)
- #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
- #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
- #10102 (Multiprocess bitcoin by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
[net] Move SOCKS5 proxy code into socks5.{h,cpp} f457efd730
[net] Introduce ProxyManager (kill proxy globals) a93b9341ff
[net] ProxyManager::HasProxy should only return true for valid addresses 9c5887e9d9
[fuzz] Add target for ProxyManager 8df0c4c383
in src/Makefile.am:656 in ad3a745d27 outdated
```
 652 | @@ -651,6 +653,7 @@ libbitcoin_common_a_SOURCES = \
 653 |    policy/feerate.cpp \
 654 |    policy/policy.cpp \
 655 |    protocol.cpp \
 656 | +  proxy.cpp \
```
maflcko commented at 3:27 PM on November 28, 2022:

Any reason to put this in common? I'd expect it can live next to conman, as the cli or wallet shouldn't be using this, no?

Might be better to put into node/proxy.cpp?

dergoegge commented at 4:11 PM on November 28, 2022:

No reason, moved to node/proxy.*.
dergoegge force-pushed on Nov 28, 2022

in src/Makefile.am:667 in 8df0c4c383

 663 | @@ -661,6 +664,7 @@ libbitcoin_common_a_SOURCES = \
 664 |    script/sign.cpp \
 665 |    script/signingprovider.cpp \
 666 |    script/standard.cpp \
 667 | +  socks5.cpp \

maflcko commented at 4:16 PM on November 28, 2022:

Same ;)

DrahtBot added the label Needs rebase on Dec 6, 2022
DrahtBot commented at 8:05 PM on December 6, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
dergoegge closed this on Dec 7, 2022
maflcko commented at 11:57 AM on December 7, 2022: member

I guess it could still make sense to do a move-only build system change to move proxy/socks out of the bins that don't need it (wallet, util, cli, ...)?
hebasto commented at 12:03 PM on December 7, 2022: member

I guess it could still make sense to do a move-only build system change to move proxy/socks out of the bins that don't need it (wallet, util, cli, ...)?

+1
maflcko commented at 3:32 PM on December 9, 2022: member

I did that in https://github.com/bitcoin/bitcoin/commit/fa02d161585d5c9f69a0bdc9677cefc8845e4ab2, but I am not sure how useful it is.

Maybe it would make more sense to create a separate net_utils library?
bitcoin locked this on Dec 9, 2023

Contributors

Labels

net, refactor: Kill proxy globals #26590

Reviews

Conflicts