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

Stream sync protocol #3535

Closed
wants to merge 22 commits into from
Closed

Conversation

JackyWYX
Copy link
Contributor

@JackyWYX JackyWYX commented Feb 8, 2021

Important notice

Since the size of this PR is too large for review, will split this PR to a set of small PRs for review. The breakdown PR will follow a bottom-up sequence for review.

  1. Fix p2p base stream read/write
  2. Stream manager
  3. Rate limiter
  4. Protobuf message
  5. Request manager

Description

  1. Added stream sync protocol running on P2P Stream.
  2. Added downloader module to manage the synchronization task.
  3. Stream manager module to manage over streams.
  4. Request manager module to manage over stream requests.
  5. Flags, commands and misc.

Tests

  1. 1-1 sync pair test - Done
  2. 1-3 sync test - Done
  3. 1-3 with 1 bad peer, 2 good peer (negative test) (Almost finished)
  4. Large scale test (STN test) - Not started.

Current known issues

  1. In very rare cases got error when establishing stream connection failed to negotiate security protocol: error reading handshake message: noise: message is too short (Peers Cannot Successfully Handshake libp2p/go-libp2p-noise#70) This issue is already fixed in libp2p version v0.13.0. Will upgrade libp2p after this release (frozen code).

@JackyWYX JackyWYX force-pushed the stream_downloader branch 4 times, most recently from b9f4800 to 17c94ae Compare February 17, 2021 20:37
cmd/harmony/config.go Outdated Show resolved Hide resolved
cmd/harmony/main.go Outdated Show resolved Hide resolved
@rlan35
Copy link
Contributor

rlan35 commented Feb 19, 2021

Are we ready to test this on a real network?

@JackyWYX
Copy link
Contributor Author

Are we ready to test this on a real network?

Yep, ready! :) @LeoHChen

@JackyWYX JackyWYX changed the title [WIP] Stream sync protocol Stream sync protocol Feb 26, 2021
Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

some quick comments for this huge PR. Only go through a few files.

@@ -7,7 +7,7 @@ import (
"strings"
"testing"

"github.com/harmony-one/harmony/api/service/syncing/downloader"
"github.com/harmony-one/harmony/api/service/legacysync/downloader"
Copy link
Contributor

Choose a reason for hiding this comment

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

the rename of the package from syncing to legacysync should be a separate PR to simplify this PR.

@@ -1269,3 +1282,97 @@ func applyPrometheusFlags(cmd *cobra.Command, config *harmonyConfig) {
config.Prometheus.EnablePush = cli.GetBoolFlagValue(cmd, prometheusEnablePushFlag)
}
}

var (
Copy link
Contributor

Choose a reason for hiding this comment

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

the new flag added can also go to a new PR, can easily be merged.


if consensus.dHelper != nil {
consensus.dHelper.start()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

any error message if dHelper == nil?

return nil
}

func (consensus *Consensus) waitForCommit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment to this function please.

@JackyWYX
Copy link
Contributor Author

Since most of the code in this branch is already done in #3588, closing!

@JackyWYX JackyWYX closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants