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

x/sync -- Add proto for P2P messages #1472

Merged
merged 22 commits into from
May 25, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 71 additions & 7 deletions proto/sync/sync.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,90 @@ package sync;

option go_package = "github.com/ava-labs/avalanchego/proto/pb/sync";

// Request represents a request for information during syncing.
message Request {
oneof message {
RangeProofRequest range_proof_request = 1;
ChangeProofRequest change_proof_request = 2;
}
}

// A RangeProofRequest requests the key-value pairs in a given key range
// at a specific revision.

Choose a reason for hiding this comment

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

Can you also document key_limit and bytes_limit? And what are they used for?

message RangeProofRequest {
bytes root = 1;
bytes start = 2;
bytes end = 3;
bytes root_hash = 1;
bytes start_key = 2;
bytes end_key = 3;
uint32 key_limit = 4;
uint32 bytes_limit = 5;
}

// A ChangeProofRequest requests the changes between two revisions.
message ChangeProofRequest {
bytes start_root = 1;
bytes end_root = 2;
bytes start = 3;
bytes end = 4;
bytes start_root_hash = 1;
bytes end_root_hash = 2;
bytes start_key = 3;
bytes end_key = 4;
uint32 key_limit = 5;
uint32 bytes_limit = 6;
}

// KeyValue represents a single key and its value.
message KeyValue {
bytes key = 1;
bytes value = 2;
}

// KeyChange is a change for a key from one revision to another.
// If the value is None, the key was deleted.
message KeyChange {

Choose a reason for hiding this comment

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

This is to track a key whose value is changed? If so, KeyChange is the value change of a key from one revision to another. seems more clear. And do we need to distinguish an insert v.s update?

Choose a reason for hiding this comment

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

This can either be a new key-value pair, or a value change for an existing key-value pair

bytes key = 1;
MaybeBytes value = 2;
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 use the optional keyword.

Choose a reason for hiding this comment

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

Discussed offline. optional means the []byte will be nil if not provided, which is annoying because we want to distinguish None from []byte{}/nil, and we treat the two as being equivalent in other places. Using optional would mean that we need to explicitly use nil, not []byte{}, when encoding.

}

// SerializedPath is the serialized representation of a path.
message SerializedPath {
uint32 nibble_length = 1;
bytes value = 2;
}

// MaybeBytes is an option wrapping bytes.
message MaybeBytes {
bytes value = 1;
// If false, this is None.
bool is_nothing = 2;
}
Comment on lines +54 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should exist

Choose a reason for hiding this comment

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


// ProofNode is a node in a merkle proof.
message ProofNode {
SerializedPath key = 1;

Choose a reason for hiding this comment

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

Is the goal to be wire compatible between merkle DB and firewood? I think currently how we represent a proof is different.

Choose a reason for hiding this comment

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

Is the goal to be wire compatible between merkle DB and firewood?

Yes.

I think currently how we represent a proof is different.

Ok let's discuss at the meeting later how to make this compatible with firewood

MaybeBytes value_or_hash = 2;
map<uint32, bytes> children = 3;
}

// RangeProof is the response to a RangeProofRequest.
message RangeProof {
repeated ProofNode start = 1;
repeated ProofNode end = 2;
repeated KeyValue key_values = 3;
}

// ChangeProof is a possible response to a ChangeProofRequest.
// It only consists of a proof of the smallest changed key,
// the highest changed key, and the keys that have changed
// between those. Some keys may be deleted (hence
// the use of KeyChange instead of KeyValue).
message ChangeProof {
bool had_roots_in_history = 1; // TODO remove
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? Is this used as a flag saying that we didn't have the requested roots? If so I wonder if we would be better off having an Error in the Response message instead.

Choose a reason for hiding this comment

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

Currently, if you request a change proof, and the node receiving the request doesn't have sufficient history to serve it, it will set this field to false. We plan to change this in the future so that in that case, the node will instead reply with a range proof. So this flag will be removed. Changing the logic to serve/expect a range proof in response to a change proof is a bunch of work so I just left this for this PR and put a TODO to remove it later. In follow up PRs I've added additional comments wrt this.

repeated ProofNode start_proof = 2;
repeated ProofNode end_proof = 3;
repeated KeyChange key_changes = 4;
}

// ChangeProofResponse is the response for a ChangeProofRequest.
message ChangeProofResponse {
oneof response {
ChangeProof change_proof = 1;
RangeProof range_proof = 2;
}
}