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

feat!: Address agnostic p2p #5176

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Oct 21, 2024

Context

Fixes #5117

How things currently work:

  • Topology of the peers (public keys and addresses) are stored on-chain.
  • There are (Un)RegisterPeer instructions for changing topology.
  • Initial topology is provided in the genesis block (via RegisterPeer instructions)
  • In case genesis block is not provided to the peer, there is sumeragi.trusted_peers config parameter with addresses of initial peers

Solution

Planned changes:

  • Peer addresses will not be stored on-chain (removed from topology)
  • Each peer will store current peer addresses off-chain and do periodic gossiping
  • Peer A will accept connection from peer B if peer B public key is in topology
  • Peer public address will be added to the config.
    • Public address will be sent when connecting to other peers - so that other peers will know our peer's external address and can broadcast this address to other peers (more details in "Use case: New peer added")
    • Note that peer internal address is already in config - network.address
    • Public port can be different from internal port e.g. in case when peer is in docker or behind nginx

Use case: Peer changes address

  • Initially there are three peers: A, B, C
  • C changes its address
  • C knows addresses of other peers and tries to establish connection with them
  • Other peers accept connection because C public key is in topology

Use case: New peer added

  • Initially there are peers A and B with addresses iroha1:8000 and iroha2:8000
  • We want to add peer C with address iroha3:8000
  • First, we invoke RegisterPeer instruction to add C public key to topology
  • Then peer C is started with the following config:
    • sumeragi.trusted_peers - only peer A
    • network.public_address - iroha3:8000
  • C tries to connect to A
    • A receives connection from 9.9.9.9:52143 (52143 is some random port chosen by C for outcoming connection, and 9.9.9.9 is some IP address, e.g. local IP address in kubernetes cluster)
    • A accepts such connection because C public key is in topology
    • C sends its public address (iroha3:8000) to A
    • A gossip address of C to B
    • B receives address of C and connects to C

Migration Guide

  • New parameter was added (network.public_address in the config, or P2P_PUBLIC_ADDRESS env var). This parameter should contain external address of the peer used for p2p (as seen by other peers)
  • genesis.json — changed topology:

Before:

"topology": [
    {
        "address": "irohad0:1001",
        "id": "ed0120A88F67F9D5D94F63DC57661D0B15054298BBB66B39E601FED71E97CD593A7FDB"
    },
    {
        "address": "irohad0:1002",
        "id": "ed0120647E9781F012EF44281A13D673178767465E1622E7BB1094C4C375B98133DBD1"
    },
    ...
]

After:

"topology": [
    "ed0120A88F67F9D5D94F63DC57661D0B15054298BBB66B39E601FED71E97CD593A7FDB",
    "ed0120647E9781F012EF44281A13D673178767465E1622E7BB1094C4C375B98133DBD1",
    ...
]

Review notes

Most meaningful changes are:

  • crates/iroha_core/src/peers_gossiper.rs - logic related to peers gossiping
  • crates/iroha_p2p/src/network.rs
    • Now peer connects to addresses received from PeersGossiper service. (Previously peer connects to addresses of peers from topology)
    • In each pair of peers, both try to establish initial connection (one of two connections will be dropped based on disambiguator rule). This is needed since when adding new peer, other peers might not know its address
  • crates/iroha_p2p/src/peer.rs - after establishing connection, peer now sends its public_address
  • crates/iroha_data_model/src/peer.rs - moved address from PeerId to Peer

Other changes are mostly related to Peer/PeerId


TODO:

  • Fix tests (in particular multiple_networks)
  • Add new tests
  • Fix docker compose

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@dima74 dima74 self-assigned this Oct 21, 2024
@github-actions github-actions bot added api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Oct 21, 2024
Copy link

@BAStos525

@SamHSmith SamHSmith self-assigned this Oct 22, 2024
Copy link
Contributor

@SamHSmith SamHSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

crates/iroha_data_model/src/peer.rs Show resolved Hide resolved
crates/iroha_data_model/src/peer.rs Show resolved Hide resolved
crates/iroha_data_model/src/peer.rs Outdated Show resolved Hide resolved
crates/iroha_data_model/src/peer.rs Outdated Show resolved Hide resolved
crates/iroha_data_model/src/peer.rs Outdated Show resolved Hide resolved
crates/iroha_core/src/peers_gossiper.rs Outdated Show resolved Hide resolved
crates/iroha_core/src/peers_gossiper.rs Show resolved Hide resolved
crates/iroha_core/src/peers_gossiper.rs Show resolved Hide resolved
crates/iroha_core/src/peers_gossiper.rs Outdated Show resolved Hide resolved
crates/iroha_core/src/peers_gossiper.rs Outdated Show resolved Hide resolved
crates/iroha_test_network/src/fslock_ports.rs Outdated Show resolved Hide resolved
crates/iroha_test_network/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 276 to 277
#[config(env = "P2P_EXTERNAL_PORT")]
pub external_port: WithOrigin<u16>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the notion of port, and it being a required parameter, is not the right decision.

Knowing only the port is not sufficient to connect to a peer. They might be hosted on different hosts, especially in environments like Kubernetes cluster, where different peers might have different associated domains. While network.address is used to bind the peer to listen to a specific address in the local environment, there could be an optional mention of network.public_address which other peers could use to connect to this peer. Moreover, in some environments it could make sense for it to be an array, so that peer will broadcast all of them to all peers and each of the peers will use the first reachable one. Being optional, peer will broadcast its local address (network.address) by default.

Maybe I am not right. Could you explain how external_port would work in a context when peers are distributed around multiple machines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain how external_port would work in a context when peers are distributed around multiple machines?

Consider we have network with PeerA and some other peers, and add new PeerX. Initially network doesn't know PeerX address and knows only its public key. Initially PeerX knows only PeerA address.

  • PeerX tries to connect to PeerA
  • PeerA receives connection from some IP 1.2.3.4
  • PeerA accepts the connection because it knows PeerX public key
  • PeerX sends its external_port (5555) to PeerA
  • PeerA knows PeerX IP (1.2.3.4) and its port (5555) and so forms its address (1.2.3.4:5555)
  • PeerA broadcasts PeerX address (1.2.3.4:5555) to other peers

So here we assume that other peers could reach PeerX through the same IP (1.2.3.4) as the PeerA. I agree that this assumption might be incorrect. E.g. if PeerA and PeerX are both in some kubernetes cluster, PeerA might receive connection from PeerX using some local kubernetes IP. Such IP would not work for PeerB outside of the cluster.

So I like the idea of providing full address in the config (that is replace network.external_port with network.public_address). But I don't understand why it should be optional. As I understand, network.address usually is like 0.0.0.0:port, so basically it contains only port to bind and can omit the host.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, network.address usually is like 0.0.0.0:port, so basically it contains only port to bind and can omit the host.

It is not always the case. One could specify some other IP to bind to a specific network interface for security (to limit access) or other reasons. It might be crucial for advanced deployments to be able to alter the host.

But I would agree that in most cases it is 0.0.0.0. Shall we split network.address to required port and optional host defaulting to 0.0.0.0?

So I like the idea of providing full address in the config (that is replace network.external_port with network.public_address). But I don't understand why it should be optional.

Maybe you are right and network.public_address is needed in the majority in cases. My rationale was that "for some setups it might not be needed and peers could broadcast their network.address as if it is network.public_address"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s8sato ideas here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we split network.address to required port and optional host defaulting to 0.0.0.0?

Personally I think current parameter network.address is OK

for some setups it might not be needed and peers could broadcast their network.address as if it is network.public_address

I agree that it may be convenient for some setups, but for complex setups it probably will be hard to understand for users that peers are not connecting to each other because some optional parameter is not specified. So I think it should be required parameter

Copy link
Contributor

@s8sato s8sato Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection from my side.
I think a connecting peer X has to notify its public address only i.e. should't include its private address in the notification, because the private address can conflict with another peer's one in another cluster.

Let's note your decision as doc comments to clarify address public_address responsibilities

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Also, maybe gossip_address would be a clearer name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a connecting peer X has to notify its public address only i.e. should't include its private address in the notification, because the private address can conflict with another peer's one in another cluster.

Could you elaborate on this please? It is already the case that when connected, peer sends only its public address, isn't it?

Let's note your decision as doc comments to clarify address public_address responsibilities

Added comment to iroha_config::parameters::user::Network

@0x009922 0x009922 self-assigned this Oct 25, 2024
@dima74 dima74 force-pushed the diralik/address-agnostic-p2p branch 2 times, most recently from 91e7ea6 to 37209a0 Compare October 29, 2024 18:44
@dima74
Copy link
Contributor Author

dima74 commented Oct 29, 2024

Update:

  • Use full public_address (instead of external_port)
  • Store gossip address from each peer independently and use majority rule when choosing address for specific peer
  • Fixed tests

@dima74 dima74 marked this pull request as ready for review October 29, 2024 19:14
@dima74 dima74 requested a review from s8sato as a code owner October 29, 2024 19:14
@dima74 dima74 force-pushed the diralik/address-agnostic-p2p branch from 37209a0 to bcd2b69 Compare October 30, 2024 12:15
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

crates/iroha_test_network/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
/// Peers received via gossiping from other peers
/// First-level key corresponds to `SocketAddr`
/// Second-level key - peer from which such `SocketAddr` was received
gossip_peers: BTreeMap<PeerId, BTreeMap<PeerId, SocketAddr>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need some eviction policy. This can be done separately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to periodically drop entries which were received long time ago?

Currently we have eviction based on topology

Copy link
Contributor

@mversic mversic Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we have eviction based on topology

I see, I think then it's ok. Can a malicious actor overwhelm other peer's gossip handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a malicious actor overwhelm other peer's gossip handle?

Single malicious actor can't, since we choose address using majority rule

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but a single malicious actor could send lots of different false addresses, couldn't it?

Copy link
Contributor

@mversic mversic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really well documented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address agnostic p2p
5 participants