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

Libp2p RPC protocol and Blocksync #229

Merged
merged 46 commits into from
Feb 26, 2020
Merged

Conversation

ec2
Copy link
Member

@ec2 ec2 commented Feb 13, 2020

Summary of changes
Changes introduced in this pull request:

  • Created the framework for libp2p RPC
  • Created the libp2p protocol for Blocksync

Reference issue to close (if applicable)

Closes

Other information and links

Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

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

🔥🔥 Great job getting this in!

node/forest_libp2p/src/rpc/codec.rs Outdated Show resolved Hide resolved
node/forest_libp2p/tests/rpc_test.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Only going to make this quick pass because I don't want to influence other reviews

node/forest_libp2p/src/rpc/blocksync_message.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/behaviour.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/rpc/blocksync_message.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/rpc/blocksync_message.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/rpc/blocksync_message.rs Outdated Show resolved Hide resolved
Copy link
Member

@ansermino ansermino left a comment

Choose a reason for hiding this comment

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

Generally I feel like this could use more docs.

node/forest_libp2p/src/rpc/blocksync_message.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/rpc/codec.rs Outdated Show resolved Hide resolved
Comment on lines 50 to 52
/// Error code
pub status: u64,
pub message: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

If commenting on each field include a comment for message for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note about this, the message isn't in the spec so it's possible it will be removed in the future, but it exists in Lotus so we are including for now

};

/// The time (in seconds) before a substream that is awaiting a response from the user times out.
pub const RESPONSE_TIMEOUT: u64 = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming this was arbitrarily set to 10 seconds? Just trying to understand if that was for a reason or if its a standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it was arbitrary, can tweak it later but 10 seconds seemed like a reasonable timeout for an RPC response


/// Contains the blocks and messages in a particular tipset
#[derive(Clone, Debug, PartialEq)]
pub struct TipSetBundle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly confused with this type...could we not utilize the pre-existing FullTipSet which has a Vec<Blocks> that contains the bls and secp messages and would describe the block each message belongs to?

Further, based on the spec and our existing implementation blocks should only contain a single header not a Vec<BlockHeader>?

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, this type defines specifically what is sent via the blocksync protocol. They use the tipsetbundle to remove redundant data passed over the wire.

Also what type are you saying to only include one header? for even a single tipset there should always be a vector of headers (or blocks).

Edit: what this type represents is essentially a compressed full Tipset, as in you can use the message vectors and includes arrays to put the messages in each block for the tipset

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that makes sense, although not completely clear how its a compressed Full Tipset as it contains the same data fields (e.g. blockheader fields and messages) and even adds an additional field with msg_includes? Could you not still use message vectors from a FullTipset?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's "compressed" because a tipset could have duplicate messages in the tipset in different blocks, the FullTipset is a vector of full blocks with messages, and the TipSetBundle has a dump of all headers, unique messages, and indexes for headers to messages

@austinabell austinabell merged commit 5a18b49 into master Feb 26, 2020
@austinabell austinabell deleted the ec2/fil-blocksync-stable-futures branch February 26, 2020 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants