Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Add new peers without restarting a node #32

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
210 changes: 210 additions & 0 deletions text/0000-add-peers-in-runtime.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
- Feature Name: `add_peers_in_runtime`
- Start Date: 2018-11-12
- RFC PR: (leave this empty)
- Sawtooth Issue: (leave this empty)

# Summary
[summary]: #summary This RFC proposes to implement the possibility to add new
peers and remove the existing connections in runtime (through the component

Choose a reason for hiding this comment

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

Recommend: "This RFC proposes functionality to add and remove static peer connections while a validator node is running."

validator endpoint) when a node is working in the `static` peering mode. Along
with that it also adds the corresponding extensions to the off-chain

Choose a reason for hiding this comment

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

Recommend: "This RFC also adds the corresponding extensions to the off-chain permssioning model."

permissioning model.

# Motivation
[motivation]: #motivation

When an administrator adds a new node to an existing Sawtooth network he/she has

Choose a reason for hiding this comment

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

add comma after 'network' and just pick either he or she for the gender

to restart a node with new peering settings. This makes any automation
significantly harder to write than if we had the possibility to add peers in the

Choose a reason for hiding this comment

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

Recommend: "Restarting the process adds substantial complexity to infrastructure automation, and incurs system downtime."

runtime and also decreases the uptime.

To resolve this problem our team proposes to add a method to add new peers to a
agunde406 marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

Add a comma after problem

running node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you also considered removing nodes? The security implications are a little more risky but about the same. The benefit of including a remove function would let us address the limits of maximum-peer-connectivity.

Copy link
Contributor

@agunde406 agunde406 Nov 30, 2018

Choose a reason for hiding this comment

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

I agree that it would be good to include removal functionality for discussion.

Copy link
Author

Choose a reason for hiding this comment

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

I think that this is a good idea.


An example use case is using Sawtooth along with a service discovery system like
[Consul](https://www.consul.io):

- A Consul node is set up along with the Sawtooth node;
- A middleware continuously fetches the changes of the peer list from the Consul
node;
- The middleware adds peers to the Sawtooth node via the validator component
endpoint.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

When an administrator adds new peers to the network you very likely want to add

Choose a reason for hiding this comment

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

Recommend: "When an administrator adds new peers to the network, she likely wants to connect to them without needing to restart existing validators with an updated --peers parameter value."

them without restarting the validator with a newer `--peers` parameter value. To
add a new peer you can send the `ClientAddPeersRequest` to the `component`

Choose a reason for hiding this comment

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

Recommend: "To add a new peer, the administrator can send..." and replace 'your' with 'their' later in the sentence.

endpoint of your Sawtooth validator.

The Protocol Buffers definition for this message is the following:

```protobuf
message ClientAddPeersRequest {
repeated string peers = 1;
}
```

Simple Python example using `sawtooth_sdk.messaging.stream`:

```python
new_peer = 'tcp://192.168.0.100:8008'
add_peer_request = ClientAddPeerRequest(peer_uri=new_peers)
future = stream.send(Message.CLIENT_ADD_PEER_REQUEST,
add_peer_request.SerializeToString())
response_serialized = future.result().content
response = ClientAddPeerResponse()
response.ParseFromString(response_serialized)
```

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

## Protocol Buffers definitions
[protobuf]: #protobuf

I propose to add them to `protos/client_peers.proto`:

Choose a reason for hiding this comment

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

Remove I/we language


```protobuf
message ClientAddPeerRequest {
repeated string peer_uri = 1;
string admin_public_key = 2;
// The signature of `peer_uri`
string signature = 3;
}

message ClientAddPeerResponse {
enum Status {
STATUS_UNSET = 0;
OK = 1;
INTERNAL_ERROR = 2;
ADMIN_AUTHORIZATION_ERROR = 3;
// One or more of peer URIs were malformed. List of malformed URIs is in
// the `invalid_uris` field of this response.
INVALID_PEER_URI = 4;
MAXIMUM_PEERS_CONNECTIVITY_REACHED = 5;
AUTHORIZATION_VIOLATION = 6;
agunde406 marked this conversation as resolved.
Show resolved Hide resolved
CONNECTION_DROPPED = 7;
}
Status status = 1;
}

message ClientRemovePeerRequest {
repeated string peer_uri = 1;
string admin_public_key = 2;
// The signature of `peer_uri`
string signature = 3;
}

message ClientRemovePeerResponse {
enum Status {
STATUS_UNSET = 0;
OK = 1;
INTERNAL_ERROR = 2;
ADMIN_AUTHORIZATION_ERROR = 3;
// The requested peer do not exist
PEER_NOT_FOUND = 4;
}
Status status = 1;
}
```

The rationale behind the `invalid_uris` is to be more precise about what is
wrong and to ease the debugging process for developers.

We should also add new message types to `protos/validator.proto`:

Choose a reason for hiding this comment

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

Remove I/we language


```protobuf
message Message {

enum MessageType {
// ...
CLIENT_PEERS_ADD_REQUEST = 131;
CLIENT_PEERS_ADD_RESPONSE = 132;
CLIENT_PEERS_REMOVE_REQUEST = 131;
eugene-babichenko marked this conversation as resolved.
Show resolved Hide resolved
CLIENT_PEERS_REMOVE_RESPONSE = 132;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 134

Copy link
Author

Choose a reason for hiding this comment

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

Done.

// ...
}
// ...
}
```

## How are the requests processed by the validator
[request-processing]: #request-processing

The requests are received on the `component` endpoint. When the validator
receives a new request for adding peers it:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add what the validator would do to remove peers

Copy link
Author

Choose a reason for hiding this comment

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

Added that, and there is one question: what to do if we go below the minimum peer connectivity limit here? I have two options in my mind:

  • Just allow to do that, but it my bring the node functionality down;
  • Another option is to throw an error here in case we reached the minimum limit.

Wondering which of those two will be a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isnt this proposal only for static peering? minimum peer connectivity is used for dynamic peering.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, sorry


- Validates the format of peer URI which has to be `tcp://ADDRESS:PORT_NUMBER`
- If the validation was successful then the validator tries to connect to a

Choose a reason for hiding this comment

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

Add comma after successful

provided peer. If the connection was successful it returns the `OK` status.

Choose a reason for hiding this comment

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

Add comma after successful

Otherwise the corresponding error status is returned.

Choose a reason for hiding this comment

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

Add comma after Otherwise


Edge cases:

- The peer address format was wrong in one or more of the provided peers. If
that happens then the request fails without adding any new peers to the peers

Choose a reason for hiding this comment

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

Add comma after happens

list and returns the `INVALID_PEER_URI` status along with the list of faulty
peer URIs.
- If the `--maximum-peer-connectivity` parameter was provided to the validator

Choose a reason for hiding this comment

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

Add comma after "provided to the validator"

then the validator checks if it has reached the maximum peer connectivity and

Choose a reason for hiding this comment

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

Add comma after connectivity

fails with an error if so. The validator also fails if it cannot add _all_ of
the peers provided in a request without breaking the provided
`maximum-peer-connectivity`.

## Permissioning

The proposition is to add the `admin` role to off-chain permissioning that will
restrict access to requests that can be malicious. The workflow for the
validation is the following:

- If the `admin` role is not specified then the permissioning module will use

Choose a reason for hiding this comment

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

Add comma after specified

the `default` policy.
- If the `default` policy is not specified then the validation of the

Choose a reason for hiding this comment

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

Add comma after specified

permissions is not performed and fields `admin_public_key` and `signature` can
be omitted.
- If the `default` or the `admin` policy is specified, then the permission
verifier checks:
- If the `admin_public_key` is allowed.
- If the `signature` is correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on this? In particular, it is not clear what is being signed.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The operation should also be covered by the signature. If only the URI is signed then it could be replayed in a ClientRemovePeerRequest. You could look at the batch format for an example of using a signature that covers the entirety of the message. https://github.com/hyperledger/sawtooth-core/blob/master/protos/batch.proto
There may be a simpler approach for this kind of message.

Copy link
Author

Choose a reason for hiding this comment

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

I am still waiting for a reply to this message. I am not really sure whether we should use just message signatures or develop a more complicated system for future use (I believe that such system should appear as a separate and more general RFC).

- If one of the above conditions is not satisfied then the

Choose a reason for hiding this comment

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

Add comma after satisfied

`ADMIN_AUTHORIZATION_ERROR` is returned.

# Drawbacks
[drawbacks]: #drawbacks

- The proposed solution does not specify any connection retry policies leaving
it to the existing peering implementation.

# Rationale and alternatives
[alternatives]: #alternatives

Alternatives were not considered and judging from multiple examples this is the
state-of-the-art solution.

# Prior art
[prior-art]: #prior-art

- [`addnode` in Bitcoin JSON RPC](https://bitcoincore.org/en/doc/0.16.0/rpc/network/addnode/)
- [`admin_addPeer` in Ethereum management API](https://github.com/ethereum/go-ethereum/wiki/Management-APIs#admin_addpeer)

Those two allow adding new peers in their platforms. Interesting points:

- Ethereum can return the connection status;
- Bitcoin allows specifying the connection retry policy.

# Unresolved questions
[unresolved]: #unresolved-questions

- During the pre-RFC discussion on the #sawtooth-core-dev channel, there was no
final solution on "should it be included to the REST API or no?"
- In the same discussion, there was a proposition to resolve security issues by
using this feature along with the permissioning module. If we do not add this
feature to the REST API and hence to the administrative utilities then I do
not see any point in permissioning because the described feature remains an
internal interface of the application. Even if we do then we can restrict the
access to that feature by using a proxy as suggested in the documentation.
- Should our team include the integration example of this solution for Consul?
- Should the permissioning be left as it is or generalized to a structure like
`(admin_public_key, signature, request)`?