pokt-network / poktroll

The official Shannon upgrade implementation of the Pocket Network Protocol implemented using Rollkit.dev
MIT License
15 stars 6 forks source link

[Relayminer] Query for on-chain session param `num_blocks_per_session` #538

Closed bryanchriswhite closed 1 month ago

bryanchriswhite commented 1 month ago

Summary

Refactors the relayminer to query for the current num_blocks_per_session each (or even multiple) time(s) it's used. This is a naive implementation to just get the relayminer using the on-chain param instead of the const that's still defined in the session keeper pkg but which will be removed in forthcoming work.

Next Steps

  1. Implement ModuleParamsClient[P any] (see: notion - ModuleParamsClient).
  2. Refactor sessionQuerier to use ModuleParamsClient[sessiontypes.Params] to avoid excessive querying.
  3. ...

Issue

Type of change

Select one or more:

Testing

Documentation changes (only if making doc changes)

Local Testing (only if making code changes)

PR Testing (only if making code changes)

Sanity Checklist


:rocket: This description was created by Ellipsis for commit bf24e6c6d0e5199387598c891eacdf4c02d35e6b

Summary:

Refactors the relayminer to dynamically query the num_blocks_per_session parameter from the blockchain, updating relevant classes to utilize this new approach.

Key points:


Generated with :heart: by ellipsis.dev

github-actions[bot] commented 1 month ago

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

bryanchriswhite commented 1 month ago
  1. I'm not a fan of how many times GracePeriod is coming up in so many functions in the SessionQueryClient. The Session is a first class citizen, the Grace period is a hack for an edge case, but it's becoming a "first class citizen", which is NOT what we want.

Currently, SessionGracePeriod is in terms of sessions. It's being used to calculate various block durations / heights / offsets. If we rewrite it to be in terms of blocks, the need for #GetSessionGracePeriodBlockCount() goes away. However, I would also expect this to become a param at some point. If it does, then it will have to be queried and/or cached off-chain & retrieved from the multistore on-chain.

I would propose renaming it to SessionGracePeriodBlocks in the interim.

bryanchriswhite commented 1 month ago

@Olshansk I'm marking this as a draft for now. I think this will change now that we've decided to add the shared module. I may close and re-open this PR on a new branch or rebase this one.

red-0ne commented 1 month ago

@Olshansk

  1. I'm not a fan of how many times GracePeriod is coming up in so many functions in the SessionQueryClient. The Session is a first class citizen, the Grace period is a hack for an edge case, but it's becoming a "first class citizen", which is NOT what we want.

I disagree on that, I see a healthy ratio of SessionEndBlockHeight/GracePeriod in this diff.

It is, one of the key times of the C&P life cycle just like Claim/Proof window submissions that will be almost as visible as soon as we start relying on them.

Although, I agree that the grace period is a bit special in the sense that it indicates when a session is no longer accepting new relays to it.

bryanchriswhite commented 1 month ago

:rotating_light: Do not delete this branch until #555 changes bases! :rotating_light: