Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Towards alternative transports first-class support #18989

Closed
ariard opened this issue May 16, 2020 · 1 comment
Closed

Towards alternative transports first-class support #18989

ariard opened this issue May 16, 2020 · 1 comment

Comments

@ariard
Copy link
Member

ariard commented May 16, 2020

See first #18987 and #18988 for code PoCs

The current networking approach suffers from a wide range of issues with regards to transaction origin inference, counter-measures against key infrastructure attackers, and harder security assumptions of higher-level protocols. Being heavily optimized and at same time trying to solve diverse goals like reasonable network security , tx-relay privacy, hindering block-topology, peer diversity, ... a functional networking stack is likely unable to address aforementioned issues without compromising its robustness.

Ideally you want to address scenarios with higher security requirements like an exchange receiving headers-over-DNS to detect tip pinning. A conscientious user always relying on tx-over-radio for each of its coins sends. You can also think about a LN hub willingly to use a HTTPS connection to a block explorer for emergency tx broadcast or an SPV wallet receiving filters-headers-over-obfs4 to defeat local Internet censorship. Of course, you can still rely on external modules or project, but better integrating them with Core to ease deployment and combine them for increased benefits.

Combining may make sense with regards to:

  • Solving first-hop transaction relay by easing identityless broadcast as a best-practice through wireless communications or anonymous network without relying on any specific one.
  • Increasing block mesh distribution, if you have few power users receiving block from space or long-range radios and beaming them through any link to geographical-near other full-nodes may considerably increase network security by decreasing the chance of network infrastructure disruption.
  • Reactive behavior for LN node, you may detect an abnormal block delay by receiving headers-over-LN and use any link available to close your channels

There are more variables at play like user capacities, disruption length, user block-time sensitiveness, disruption amplitude, information leakage. Building a comprehensive model that cover everything is hard, even if there have been few attempts. There are trade-offs, urgency filter-headers fetching from centralized service may prevent your node from being eclipsed at the cost of privacy.

Keeping these in mind, introducing an alternative networking stack as PoC'ed in AltNet may be a long-term solution. While designing such stack, flexibility and modularity should be end-goals while minding per-user requirement, technical capabilities and unknown real-world topology. A generic "drivers" framework, abstracting all of the link oddities seems like an interesting direction to explore. Use of these drivers should never be the default to avoid massive topology modification and be well-documented to let user pick up what suits his/her needs. If we have a robust, fail-safe higher half in the codebase, driver code may be in another repo within the bitcoin-core.org to avoid painful review process.

Some may object that we can rely on Tor to further our goals with regards to anonymity and network censorship-resistant. However, a) Tor DoS is an issue b) bridge dissemination may not suit bitcoin security assumptions c) Tor doesn't seem to explore wireless communications d) we may still reuse pluggable transports like Snowflake or meek without being forced to pas through monitorable exit nodes.

If this proposal is evaluated as worthy for the project, there are a lot of design issues to address, like Rust vs CPP, architecture integration (new threads vs new process), the fanciness of new fetching logic, how to avoid a review iceberg like #17376 as a first step but instead privileging incremental steps, how driver devs may contribute to the project without understanding of other components allowing them to experiment with the protocol's unique flow semantic...

Such subject are hard to encompass so it's likely there are a lot of aspects not raised here. I'm eager to hear people feedback.

@ariard ariard added the Feature label May 16, 2020
@maflcko maflcko added the P2P label May 16, 2020
maflcko pushed a commit that referenced this issue Jun 12, 2020
af2a145 Refactor resource exhaustion test (Troy Giorshev)
5c4648d Fix "invalid message size" test (Troy Giorshev)
ff1e7b8 Move size limits to module-global (Troy Giorshev)
57890ab Remove two unneeded tests (Troy Giorshev)

Pull request description:

  This PR touches only the p2p_invalid_messages.py functional test module.  There are two main goals accomplished here.  First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons.  Second, it refactors the file into a single consistent style.  This file appears to have originally had two authors, with different styles and some test duplication.

  It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](#18242) and [AltNet](#18989) changes.

  This should probably go in ahead of #19107, but the two are not strictly related.

ACKs for top commit:
  jnewbery:
    ACK af2a145
  MarcoFalke:
    re-ACK af2a145 🍦

Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jun 13, 2020
…nal tests

af2a145 Refactor resource exhaustion test (Troy Giorshev)
5c4648d Fix "invalid message size" test (Troy Giorshev)
ff1e7b8 Move size limits to module-global (Troy Giorshev)
57890ab Remove two unneeded tests (Troy Giorshev)

Pull request description:

  This PR touches only the p2p_invalid_messages.py functional test module.  There are two main goals accomplished here.  First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons.  Second, it refactors the file into a single consistent style.  This file appears to have originally had two authors, with different styles and some test duplication.

  It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](bitcoin#18242) and [AltNet](bitcoin#18989) changes.

  This should probably go in ahead of bitcoin#19107, but the two are not strictly related.

ACKs for top commit:
  jnewbery:
    ACK af2a145
  MarcoFalke:
    re-ACK af2a145 🍦

Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
fanquake added a commit that referenced this issue Sep 29, 2020
…r, extend logging

deb5271 Remove header checks out of net_processing (Troy Giorshev)
52d4ae4 Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7 Move checksum check from net_processing to net (Troy Giorshev)
2716647 Give V1TransportDeserializer an m_node_id member (Troy Giorshev)

Pull request description:

  Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.

  In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check.  In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.

  For more context, see https://bitcoincore.reviews/15206.html#l-81.

  This PR improves the separation between the p2p layers, helping improvements like [BIP324](#18242) and #18989.

ACKs for top commit:
  ryanofsky:
    Code review ACK deb5271 just rebase due to conflict on adjacent line
  jnewbery:
    Code review ACK deb5271.

Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Sep 29, 2020
…rk layer, extend logging

deb5271 Remove header checks out of net_processing (Troy Giorshev)
52d4ae4 Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7 Move checksum check from net_processing to net (Troy Giorshev)
2716647 Give V1TransportDeserializer an m_node_id member (Troy Giorshev)

Pull request description:

  Inspired by bitcoin#15206 and bitcoin#15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.

  In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check.  In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.

  For more context, see https://bitcoincore.reviews/15206.html#l-81.

  This PR improves the separation between the p2p layers, helping improvements like [BIP324](bitcoin#18242) and bitcoin#18989.

ACKs for top commit:
  ryanofsky:
    Code review ACK deb5271 just rebase due to conflict on adjacent line
  jnewbery:
    Code review ACK deb5271.

Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue May 13, 2021
…tests

Summary:
af2a145e575f23c64909e6cf1fb323c603bda7ca Refactor resource exhaustion test (Troy Giorshev)
5c4648d17ba18e4194959963994cc6b37053f127 Fix "invalid message size" test (Troy Giorshev)
ff1e7b884447a5ba10553b2d964625f94e255bdc Move size limits to module-global (Troy Giorshev)
57890abf2c7919eddfec36178b1136cd44ffe883 Remove two unneeded tests (Troy Giorshev)

Pull request description:

  This PR touches only the p2p_invalid_messages.py functional test module.  There are two main goals accomplished here.  First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons.  Second, it refactors the file into a single consistent style.  This file appears to have originally had two authors, with different styles and some test duplication.

  It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](bitcoin/bitcoin#18242) and [AltNet](bitcoin/bitcoin#18989) changes.

  This should probably go in ahead of #19107, but the two are not strictly related.

---

Depends on D9511

Backport of [[bitcoin/bitcoin#19177 | core#19177]]

Test Plan:
  ninja all && ./test/functional/test_runner.py p2p_invalid_messages

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Subscribers: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9519
@ariard
Copy link
Member Author

ariard commented Aug 3, 2022

Closing the issue, I'll take future feedbacks on AltNet repository itself: https://github.com/ariard/altnet

I'm following the approach recommended by Russ to rely on Cap'n Proto-over-unix-sockets instead to rewrite libmultiprocess in Rust. Turns out as far less work.

@ariard ariard closed this as completed Aug 3, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants