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

[WIP] Block by Block Synchronous Sate Sync #812

Open
wants to merge 126 commits into
base: main
Choose a base branch
from

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Jun 8, 2023

Description

Summary generated by Reviewpad on 07 Aug 23 18:22 UTC

This pull request includes changes in multiple files. Here is a summary of the modifications:

  • The "consensus_module's state synchronization" file was completely deleted.
  • The "bus_module.go" file added a new type and a new method.
  • The "state_machine/module.go" file had comments added and a line modified.
  • The "state_machine/fsm.go" file had comments added and new states added.
  • The "hotstuff.proto" file added a comment related to a circular dependency.
  • The "charts/pocket/README.md" file added a new configuration option.
  • The "PROTOCOL_STATE_SYNC.md" file had several updates to the content.
  • The "hotstuff_leader.go" file had modifications in various functions.
  • The "actor.go" file had changes related to the Output Address design.
  • The "events.proto" file added two new messages.
  • The "consensus_module.go" file had several changes and suggestions in the code.
  • The "state_sync_handler.go" file had modifications in two functions.
  • The "types.go" file added a new function.
  • The "config.validator1.json" file had changes in the "servicer" and "ibc" sections.
  • The "charts/pocket/values.yaml" file added a new key-value pair.
  • The "p2p/utils_test.go" file had a constant commented as technical debt.
  • The "defaults.go" file added a new constant.
  • The "events.go" file had updates in comments.
  • The "Tiltfile" had changes in the dependencies list.
  • The "consensus/state_sync/helpers.go" file had changes in three functions.
  • The "event_handler.go" file had a comment removed.
  • The "block.proto" file had modifications in the "BlockHeader" message.

Please review these changes and let me know if you need further assistance.

Issue

Fixes #579

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

What is this PR in its current form?

  • Basic synchronous block-by-block state sync
  • Not the perfect/cleanest code, but something that's somewhat understandable
  • Some code cleanup related to consensus tended to along the way

What is this PR NOT intended to be

  • Fully asynchronous background state sync
  • State sync independent of FSM
  • Any sort of optimization in state sync

Blockers / further improvements needed in this PR

  • Tickets to track future work (tech debt, improvements, cleanup, optimizations)
  • An E2E test as part of this PR
  • A video showing that it works on localnet

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

Copy link

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Added few questions, but nothing jumps to my eyes. I have yet to run the tests under debugging mode. Some of them are failing for me, even on a new repo.

# github.com/pokt-network/pocket/consensus/e2e_tests [github.com/pokt-network/pocket/consensus/e2e_tests.test]
consensus/e2e_tests/utils_test.go:136:28: undefined: eventsChannel
consensus/e2e_tests/utils_test.go:137:36: undefined: eventsChannel
consensus/e2e_tests/utils_test.go:138:40: undefined: eventsChannel
consensus/e2e_tests/utils_test.go:139:34: undefined: eventsChannel
consensus/e2e_tests/utils_test.go:140:28: undefined: eventsChannel
consensus/e2e_tests/utils_test.go:738:9: too many return values
        have (nil)
        want ()
consensus/e2e_tests/utils_test.go:747:11: undefined: IdToNodeMapping
consensus/e2e_tests/utils_test.go:761:11: undefined: IdToNodeMapping
FAIL    github.com/pokt-network/pocket/consensus/e2e_tests [build failed]

I need to look into it though.

@Olshansk
Copy link
Member Author

@red-0ne Will look into the broken tests tomorrow.

@Olshansk Olshansk requested a review from red-0ne August 1, 2023 22:18
@Olshansk Olshansk changed the base branch from main to issues/829/state_sync_test August 3, 2023 19:18
@Olshansk Olshansk changed the base branch from issues/829/state_sync_test to main August 3, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus Consensus specific changes large Pull request is large waiting-for-review
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[DOCUMENT][DEMO] Block by block state sync
4 participants