[Session] add `num_blocks_per_session` param #530

bryanchriswhite · 2024-05-14T12:55:13Z

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:

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

Issue

[Protocol] Add a NumBlocksPerSession governance parameter #517

Type of change

Select one or more:

Testing

Documentation changes (only if making doc changes)

make docusaurus_start; only needed if you make doc changes

Local Testing (only if making code changes)

Unit Tests: make go_develop_and_test
LocalNet E2E Tests: make test_e2e
See quickstart guide for instructions

PR Testing (only if making code changes)

DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
- THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
- Optionally run make trigger_ci if you want to re-trigger tests without any code changes
- If tests fail, try re-running failed tests only using the GitHub UI as shown here

Sanity Checklist

I have tested my changes using the available tooling
I have commented my code
I have performed a self-review of my own code; both comments & source code
I create and reference any new tickets, if applicable
I have left TODOs throughout the codebase, if applicable

gitguardian · 2024-05-14T12:55:24Z

️✅ 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 · 2024-05-15T10:02:37Z

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 · 2024-05-15T10:02:39Z

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.

tools/scripts/params/tokenomics_compute_units_to_tokens_multiplier.json

x/session/types/params.go

x/application/types/params.go

x/session/types/params.go

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk

This is setting some really great standards that others will just copy-pasta to add more params. Love it!

Olshansk · 2024-05-17T18:47:50Z

x/service/types/params.go

@@ -9,7 +9,10 @@ import (
 // DefaultAddServiceFee is the default value for the add service fee
 // parameter in the genesis state of the service module.
 // TODO_BLOCKER: Revisit default param values for service fee
-const DefaultAddServiceFee = 1000000000 // 1000 POKT
+const (
+ ParamAddServiceFee = "add_service_fee"


Excited to add the champion business logic here soon!

cc @moatus

okdas · 2024-05-21T16:54:27Z

@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 · 2024-05-21T23:46:40Z

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:

@bryanchriswhite is this error expected?

bryanchriswhite · 2024-05-22T06:37:51Z

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:

@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. 🤔

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. 👍 Thanks!

…n E2E test

bryanchriswhite · 2024-05-22T11:32:01Z

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 6c6ec62.

bryanchriswhite · 2024-05-22T11:50:41Z

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

…sion-params_relayminer * pokt/main: [SDK] Track pb.go files (#544) [Session] add `num_blocks_per_session` param (#530)

bryanchriswhite added session Changes related to Session management on-chain On-chain business logic labels May 14, 2024

bryanchriswhite added this to the Shannon MainNet milestone May 14, 2024

bryanchriswhite self-assigned this May 14, 2024

bryanchriswhite changed the base branch from main to issues/322/proof-of-concept May 14, 2024 12:56

bryanchriswhite linked an issue May 14, 2024 that may be closed by this pull request

[Protocol] Add a NumBlocksPerSession governance parameter #517

Closed

10 tasks

bryanchriswhite force-pushed the issues/517/param-num_blocks_per_session branch 3 times, most recently from 179ad01 to 8c0604e Compare May 15, 2024 10:00

bryanchriswhite added push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 15, 2024

github-actions bot added the devnet label May 15, 2024

bryanchriswhite force-pushed the issues/517/param-num_blocks_per_session branch 2 times, most recently from 707e002 to 5846990 Compare May 15, 2024 10:41

bryanchriswhite changed the base branch from issues/322/proof-of-concept to main May 15, 2024 10:43

bryanchriswhite added 3 commits May 15, 2024 14:10

feat: add num_blocks_per_session session module param

a3d3d17

feat: add make target to update all session params

99455d5

chore: use default param values in param update msgs

05e631a

bryanchriswhite force-pushed the issues/517/param-num_blocks_per_session branch from f81089a to 05e631a Compare May 15, 2024 12:10

bryanchriswhite added 2 commits May 15, 2024 14:19

Empty commit

bf3bff3

fixup! feat: add num_blocks_per_session session module param

3aa53ec

bryanchriswhite marked this pull request as ready for review May 15, 2024 12:36

bryanchriswhite requested a review from Olshansk May 15, 2024 12:36

Olshansk requested changes May 15, 2024

View reviewed changes

bryanchriswhite and others added 3 commits May 16, 2024 10:20

test: add coverage for sessiontypes.Params#ValidateNumBlocksPerSession()

d7a04dc

fix: #ValidateNumBlocksPerSession()

56ae9a8

chore: review feedback improvements

b291bc1

Co-authored-by: Daniel Olshansky <[email protected]>

Merge branch 'main' into issues/517/param-num_blocks_per_session

e9afc4c

bryanchriswhite requested a review from Olshansk May 16, 2024 08:29

bryanchriswhite added 3 commits May 16, 2024 15:29

refactor: rename param name vars

40e1e7f

fix: unit tests

2c33381

Merge branch 'main' into issues/517/param-num_blocks_per_session

9597876

bryanchriswhite mentioned this pull request May 17, 2024

[Protocol] Add a NumBlocksPerSession governance parameter #517

Closed

10 tasks

Olshansk approved these changes May 17, 2024

View reviewed changes

bryanchriswhite added 2 commits May 21, 2024 08:39

Merge branch 'main' into issues/517/param-num_blocks_per_session

1eae173

Empty commit

86e114d

Olshansk requested a review from okdas May 21, 2024 20:19

bryanchriswhite added 2 commits May 22, 2024 07:59

Empty commit

eaf5f15

Empty commit

a757d5a

bryanchriswhite added 2 commits May 22, 2024 12:57

chore: add "unauthorized" account to config.yml instead of creating i…

6c6ec62

…n E2E test

Empty commit

1b42911

bryanchriswhite merged commit da70c0e into main May 22, 2024
10 checks passed

bryanchriswhite added a commit that referenced this pull request May 23, 2024

Merge remote-tracking branch 'pokt/main' into issues/517/refactor/ses…

8260bab

…sion-params_relayminer * pokt/main: [SDK] Track pb.go files (#544) [Session] add `num_blocks_per_session` param (#530)

bryanchriswhite deleted the issues/517/param-num_blocks_per_session branch May 23, 2024 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Session] add `num_blocks_per_session` param #530

[Session] add `num_blocks_per_session` param #530

bryanchriswhite commented May 14, 2024 •

edited

Loading

gitguardian bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

Olshansk left a comment

Olshansk May 17, 2024

okdas commented May 21, 2024

okdas commented May 21, 2024

bryanchriswhite commented May 22, 2024 •

edited

Loading

bryanchriswhite commented May 22, 2024

bryanchriswhite commented May 22, 2024 •

edited

Loading

[Session] add num_blocks_per_session param #530

[Session] add num_blocks_per_session param #530

Conversation

bryanchriswhite commented May 14, 2024 • edited Loading

Summary

Next steps:

Issue

Type of change

Testing

Sanity Checklist

gitguardian bot commented May 14, 2024 • edited Loading

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

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk May 17, 2024

Choose a reason for hiding this comment

okdas commented May 21, 2024

okdas commented May 21, 2024

bryanchriswhite commented May 22, 2024 • edited Loading

bryanchriswhite commented May 22, 2024

bryanchriswhite commented May 22, 2024 • edited Loading

[Session] add `num_blocks_per_session` param #530

[Session] add `num_blocks_per_session` param #530

bryanchriswhite commented May 14, 2024 •

edited

Loading

gitguardian bot commented May 14, 2024 •

edited

Loading

bryanchriswhite commented May 22, 2024 •

edited

Loading

bryanchriswhite commented May 22, 2024 •

edited

Loading