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

BlockSync provider #724

Merged
merged 8 commits into from
Oct 8, 2020
Merged

Conversation

snaumov
Copy link
Contributor

@snaumov snaumov commented Oct 1, 2020

Summary of changes
Changes introduced in this pull request:

BlockSync "provider" - the logic to run for an incoming BlockSync libp2p request.

Test notes

It was somewhat hard to test this MR. I've added a unit test prepopulating MemoryDB with some chain data (thanks @austinabell for generating file for me), and compared the response with FilFox, see comments in the test.

Reference issue to close (if applicable)

Closes #673

Other information and links

@snaumov snaumov force-pushed the snaumov/blocksync-provider branch 2 times, most recently from a655574 to ae8e9a6 Compare October 1, 2020 00:40
@austinabell austinabell added Network Libp2p and PubSub stuff Status: Needs Review labels Oct 1, 2020
@snaumov snaumov force-pushed the snaumov/blocksync-provider branch 3 times, most recently from c11581f to 909e90e Compare October 1, 2020 12:28
@snaumov snaumov force-pushed the snaumov/blocksync-provider branch 2 times, most recently from 19b2452 to 61c4300 Compare October 1, 2020 13:03
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/service.rs Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/message.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@austinabell
Copy link
Contributor

Thanks so much for this contribution! Looks generally good, and happy to help with any of the changes or suggestions if you would like :)

@snaumov snaumov force-pushed the snaumov/blocksync-provider branch 3 times, most recently from c0bd0b8 to 54a9cd1 Compare October 5, 2020 01:14
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
Comment on lines +29 to +36
pub fn include_blocks(&self) -> bool {
self.options == BLOCKS || self.options == BLOCKS_MESSAGES
}

/// If a request expects messages to be included in response.
pub fn include_messages(&self) -> bool {
self.options == MESSAGES || self.options == BLOCKS_MESSAGES
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be bit operations, BLOCKS is the least significant bit, and MESSAGES is the second least significant bit.

Can be something like:

    pub fn include_blocks(&self) -> bool {
        self.options & BLOCKS != 0
    }

    /// If a request expects messages to be included in response.
    pub fn include_messages(&self) -> bool {
        self.options & MESSAGES != 0
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feels a little less readable imo 🤔

use std::convert::TryFrom;

/// Blocksync request options
pub const BLOCKS: u64 = 1;
pub const MESSAGES: u64 = 2;
pub const BLOCKS_MESSAGES: u64 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this

Copy link
Contributor Author

@snaumov snaumov Oct 6, 2020

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.

yeah it's not necessary though, that only exists because it wasn't bit flags before. 0b01 | 0b10 == 3. There is no need to use this constant itself.

Edit: Also that blocksync spec is wildly incorrect, don't use it for anything meaningful

Copy link
Contributor Author

@snaumov snaumov Oct 8, 2020

Choose a reason for hiding this comment

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

yeah it's not necessary though, that only exists because it wasn't bit flags before

Sorry, when you say before are you referring to your suggestion here: #724 (comment), or these are bit flags in a spec now too?

node/forest_libp2p/src/blocksync/message.rs Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/service.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.

LGTM, thanks so much for the contribution. The only comments left are opinionated and I'm good with this :)

node/forest_libp2p/src/blocksync/provider.rs Outdated Show resolved Hide resolved
node/forest_libp2p/src/blocksync/message.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.

Sorry was just waiting for branch to update before pulling in, didn't want to merge into your branch if you were making changes. I just updated the branch and will pull in when CI passes. Thanks!

@austinabell austinabell merged commit 9de9e35 into ChainSafe:main Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Network Libp2p and PubSub stuff Status: Needs Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement blocksync provider
3 participants