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

[Session] add `num_blocks_per_session` param #530

Closed bryanchriswhite closed 1 month ago

bryanchriswhite commented 1 month ago

Summary

This is the first step towards the objective outlined in #517: introduce the NumBlocksPerSession parameter to the session module.

(This PR is dependent on the param related E2E tests added in #486)

Next steps:

  1. Add support for querying for session module params to SessionQueryClient & SupplierClient.
  2. Refactor RelayMiner to query dependent params on demand (naive implementation).
  3. Add session keeper dependency to application keeper.
  4. Remove & refactor outstanding usages of sessionkeeper.NumBlocksPerSession placeholder constant (depends on 1 & 2).
  5. Add support for updating individual params to the session module (scaffold MsgUpdateParam).
  6. Refactor RelayMiner & SessionQueryClient to react to param updates.

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

gitguardian[bot] commented 1 month ago

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them. Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately. Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

github-actions[bot] commented 1 month ago

The image is going to be pushed after the next commit. You can use make trigger_ci to push an empty commit. If you also want to run an E2E test, please add devnet-test-e2e label.

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.

okdas commented 1 month ago

@bryanchriswhite I saw this is blocked due to e2e test failing on CI. Looking at the output of the test, it seems like the reason is the accounts were not fully initialized due to send transactions not working correctly. It appears we've recently introduced some fees. Here's the error:

raw_log: ‘insufficient fees; got:  required: 2000upokt: insufficient fee’

I'll see how to address that, and I'll probably end up pushing a fix in your PR.

okdas commented 1 month ago

We started having this issue because the minimum-gas-prices was set to "0.01upokt" (I put it there while changing this config on TestNet).

On LocalNet, we have this set to 0upokt. Originally, I started fixing the tests to account for the new fee, but it would take more time than I originally thought, so I just rolled back that configuration on DevNets keeping it on TestNet.

E2E tests mostly pass. There's this e2e test error though, which seems to be related to that PR:

Screenshot 2024-05-21 at 4 44 07 PM

@bryanchriswhite is this error expected?

bryanchriswhite commented 1 month ago

Thanks for investigating and for the context @okdas!

E2E tests mostly pass. There's this e2e test error though, which seems to be related to that PR: Screenshot 2024-05-21 at 4 44 07 PM

@bryanchriswhite is this error expected?

This is not expected; however, it makes sense if the DevNet state is not reset between E2E test runs and a previous run failed after altering DevNet state. The test attempts to reset all module params to their default values after each scenario but depending on how the test fails, it seems, this may not happen as expected. :thinking:

It might be better to add the "unauthorized" key to the config.yaml instead of adding it to the keyring during the test. I've noticed this also seems to cause errors locally when running make acc_initialize_pubkeys after running E2E tests; although, I don't think it actually causes any functional issue.

After re-building and -running CI, it's passing. :+1: Thanks!

bryanchriswhite commented 1 month ago

It might be better to add the "unauthorized" key to the config.yaml instead of adding it to the keyring during the test.

Done in 6c6ec62b4aa3c18488c28b62a69d004361585f80.

bryanchriswhite commented 1 month ago

:rotating_light: DO NOT delete this branch until #538 has changed its base branch off of this branch! :rotating_light: