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

[Protocol] Implement Session Rollovers #275

Closed
7 tasks
Olshansk opened this issue Dec 15, 2023 · 4 comments
Closed
7 tasks

[Protocol] Implement Session Rollovers #275

Olshansk opened this issue Dec 15, 2023 · 4 comments
Assignees
Labels
protocol General core protocol related changes

Comments

@Olshansk
Copy link
Member

Objective

Resolve the infamous session rollover issue once and for all

Origin Document

Screenshot 2023-12-14 at 3 54 21 PM

While working on #252, we had to add a temporary hack below due to:

  • Very fast block times
  • Small number of blocks per session

Overall, this is us hitting the session rollover issue in Shannon

	// TODO_IN_THIS_PR: Either slow down blocks, or increase numBlocksPerSession,
	// or create a ticket related to session rollovers and link to it here.
	// currentBlock := rp.blockClient.LastNBlocks(ctx, 1)[0]
	// session, err := rp.sessionQuerier.GetSession(ctx, appAddress, service.Id, currentBlock.Height())
	// session, err := rp.sessionQuerier.GetSession(ctx, appAddress, service.Id, relayRequest.Meta.SessionHeader.SessionStartBlockHeight)
	currentBlock := rp.blockClient.LastNBlocks(ctx, 1)[0]
	session, err := rp.sessionQuerier.GetSession(ctx, appAddress, service.Id, currentBlock.Height())

Goals

  • Design & Implement an on-chain solution for session rollovers in Shannon

Deliverables

  • A PR that implements and solves session rollovers
  • A governance parameter that modulates how much leeway session rollovers have
  • Document how session rollovers are solved in Shannon

Non-goals / Non-deliverables

  • Using the same solution for session rollovers as in Morse (i.e. client side)

General deliverables

  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.
  • Testing: Add new tests (unit and/or E2E) to the test suite.
  • Makefile: Add new targets to the Makefile to make the new functionality easier to use.
  • Documentation: Update architectural or development READMEs; use mermaid diagrams where appropriate.

Creator: @Olshansk
Co-Owners: @red-0ne

@Olshansk
Copy link
Member Author

tl;dr In pkg/relayer/proxy/relay_verifier.go:

+  session, err := rp.sessionQuerier.GetSession(ctx, appAddress, service.Id, relayRequest.Meta.SessionHeader.SessionStartBlockHeight)
- currentBlock := rp.blockClient.LastNBlocks(ctx, 1)[0]
- session, err := rp.sessionQuerier.GetSession(ctx, appAddress, service.Id, currentBlock.Height())

While implementing the quickstart guide in #252, the change above was necessary to make things work E2E because the session was ending too quickly.

Until session rollovers are implemented, we can potentially run into bugs / issues in local development as well. The solution is to either:

  1. Add the diff above (the easier temp workaround)
  2. Locally increase blocks per session (not gov param at the time of writing this)
  3. Reducing block time (a rollkit flag)

@red-0ne red-0ne moved this from 🔖 Ready to 🏗 In progress in Shannon Jan 8, 2024
@red-0ne
Copy link
Contributor

red-0ne commented Jan 10, 2024

Initial investigation and solution proposal https://www.notion.so/buildwithgrove/Sessions-rollover-f2845bd812c24b9c98934ca510bb70e9

@Olshansk
Copy link
Member Author

Initial investigation and solution proposal notion.so/buildwithgrove/Sessions-rollover-f2845bd812c24b9c98934ca510bb70e9

Reviewed!

Overall, the idea I had in mind was a little different. Only copy-pasting the mermaid diagram here for reference:

Screenshot 2024-01-12 at 3 39 29 PM

CCing @bryanchriswhite since this may be dependant on the work in #141 (comment)

@red-0ne red-0ne moved this from 🏗 In progress to 👀 In review in Shannon Jan 22, 2024
@red-0ne
Copy link
Contributor

red-0ne commented Jan 29, 2024

Closing as per #326 and #332

@red-0ne red-0ne closed this as completed Jan 29, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Shannon Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol General core protocol related changes
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants