-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Session,Supplier] Implement deterministic session suppliers #1103
base: main
Are you sure you want to change the base?
Conversation
7b0f759
to
240ad06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
No major blockers, just a handful of comments here and there.
-
This is REALLY great work! I can tell this wasn't easy to implement ;). Great job on just handling everything from start to finish.
-
Do you think this warrants one E2E test (for regression purposes)?
-
Wanted to align on WHY this is not needed for applications. Apps can come in mid-session and just get a new set of suppliers, and begin paying for services right away. Aligned?
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 1103) |
## Summary Implement the `MsgClaimMorseAccount` handler. This enables claiming the of the total tokens (unstaked balance plus any actor stakes) of the Morse account to a Shannon account. ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [x] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [ ] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…Claimable$1`/g (#1076) ## Summary Rename `QueryGetMorseClaimableAccount` and `QueryGetMorseClaimableAccountResponse` to remove the "get". ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…deps (#1079) ## Summary Add application, supplier, and gateway keepers as migration module dependencies. This is in preparation for future work on #1034 which requires staking application, supplier, and gateway actors during Morse account claiming. ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
## Summary Fix the usage of `isTTLEnabled` to properly support zero TTL (mainly for testing). ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable
## Summary Some edits and fixes to the existing Fullnode documentation Changes: - Fixes and tweaks to https://dev.poktroll.com/operate/walkthroughs/full_node_walkthrough - Fixes and tweaks to https://dev.poktroll.com/tools/user_guide/poktrolld_cli ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [ ] Code health or cleanup - [x] Documentation - [ ] Other (specify) ## Sanity Checklist - [ ] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [x] For docs, I have run `make docusaurus_start` - [ ] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]>
Since Go 1.21, the functions in `x/ex`p used here can already be replaced by functions from the standard library.
#1080) ## Summary ```bash scaffold: message claim_morse_application --module migration --signer shannon_dest_address morse_src_address morse_signature stake:coin service_config --response morse_src_address --response claimed_balance:coin --response service_id --response claimed_application_stake:coin --response claimed_at_height:int --response application ``` Changes: - Scaffold `MsgClaimMorseApplication` message ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [x] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [ ] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [x] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…ods (#1091) ## Summary Extract application staking business logic to `Keeper` methods (as opposed to `msgServer`). **Rationale**: 1. The migration module needs to stake applications/suppliers/gateways (this PR focuses on applications). 2. There's precedent for adding module `Keeper`s as dependencies to other modules, but not `msgServer`s. (see: #1080 (comment)) Changes: - Change the receiver of `#createApplication()` and `#updateApplication()` from `msgServer`, to `Keeper`. - Extract `Keeper#StakeApplication()` from `msgServer#StakeApplication()`. ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [x] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [x] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…er (#1082) ## Summary Implement `MsgClaimMorseApplication` message handler. It should stake, or upstake and replace the service config, a Shannon application, given a previously imported (and unclaimed) `MorseClaimableAccount`. ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [x] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [ ] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…`Keeper` (#1093) Extract supplier staking business logic to `Keeper` methods (as opposed to `msgServer`). **Rationale**: 1. The migration module needs to stake applications/suppliers/gateways (this PR focuses on suppliers). 2. There's precedent for adding module `Keeper`s as dependencies to other modules, but not `msgServer`s. (see: #1080 (comment)) Changes: - Change the receiver of `#createSupplier()` and `#updateSupplier()` from `msgServer`, to `Keeper`. - Extract `Keeper#StakeSupplier()` from `msgServer#StakeSupplier()`. - Issue: #1034 Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
## Summary ```bash ignite scaffold message claim_morse_supplier --module migration --signer shannon_dest_address morse_src_address morse_signature stake:coin service_config --response morse_src_address --response claimed_balance:coin --response service_id --response claimed_supplier_stake:coin --response claimed_at_height:int --response supplier ``` Changes: - Scaffold `MsgClaimMorseSupplier` - Add `TODO_UPNEXT` comments ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [x] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [ ] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [x] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [x] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [x] I added TODOs where applicable --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
…1117) ## Summary Remove unused gateway keeper from the migration module. ## Issue - Issue: #1034 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Sanity Checklist - [ ] I have updated the GitHub Issue `assignees`, `reviewers`, `labels`, `project`, `iteration` and `milestone` - [ ] For docs, I have run `make docusaurus_start` - [ ] For code, I have run `make go_develop_and_test` and `make test_e2e` - [ ] For code, I have added the `devnet-test-e2e` label to run E2E tests in CI - [ ] For configurations, I have update the documentation - [ ] I added TODOs where applicable
Yes, apps can join mid-session and start using a service. This is because applications are part of the session existence, before an app joins, the session simply doesn't exist. Which is not the case for suppliers. |
// TODO_POST_MAINNET: Use the number of suppliers per session used as of the query height. | ||
// As of now the session is hydrated with the "current" NumSuppliersPerSession param value. | ||
// We need to account for NumSuppliersPerSession as of the query height to ensure | ||
// that the session is hydrated with the old number of suppliers in case the param | ||
// has changed between the query height and the current height. | ||
numSuppliersPerSession := int(k.GetParams(ctx).NumSuppliersPerSession) | ||
// Get all suppliers instead of filtering by service ID. | ||
// Suppliers may not be active for the session's service ID at "query height", | ||
// so we cannot filter by supplier.Services which represents the services | ||
// the supplier is active for at the "current height". | ||
suppliers := k.supplierKeeper.GetAllSuppliers(ctx) | ||
|
||
// A mapping from the supplier operator address to a random weight. | ||
// This is used for sorting the suppliers. | ||
// If NumCandidateSuppliers > NumSuppliersPerSession, the deterministic sorting order | ||
// is used to determine which ones are available to serve requests to enable fair but | ||
// random opportunity to serve Applications (i.e. do useful work). | ||
candidatesToRandomWeight := make(map[string]int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO_POST_MAINNET: Use the number of suppliers per session used as of the query height. | |
// As of now the session is hydrated with the "current" NumSuppliersPerSession param value. | |
// We need to account for NumSuppliersPerSession as of the query height to ensure | |
// that the session is hydrated with the old number of suppliers in case the param | |
// has changed between the query height and the current height. | |
numSuppliersPerSession := int(k.GetParams(ctx).NumSuppliersPerSession) | |
// Get all suppliers instead of filtering by service ID. | |
// Suppliers may not be active for the session's service ID at "query height", | |
// so we cannot filter by supplier.Services which represents the services | |
// the supplier is active for at the "current height". | |
suppliers := k.supplierKeeper.GetAllSuppliers(ctx) | |
// A mapping from the supplier operator address to a random weight. | |
// This is used for sorting the suppliers. | |
// If NumCandidateSuppliers > NumSuppliersPerSession, the deterministic sorting order | |
// is used to determine which ones are available to serve requests to enable fair but | |
// random opportunity to serve Applications (i.e. do useful work). | |
candidatesToRandomWeight := make(map[string]int) | |
// TODO_POST_MAINNET: Use the number of suppliers per session used at query height (i.e. sh.blockHeight). | |
// Currently, the session is hydrated with the "current" (i.e. latest block) NumSuppliersPerSession param value. | |
// We need to account for the value at query height to ensure: | |
// - The session is hydrated with the correct historical number of suppliers | |
// - Changes between query height and current height are properly handled | |
numSuppliersPerSession := int(k.GetParams(ctx).NumSuppliersPerSession) | |
// Get all suppliers without service ID filtering because: | |
// - Suppliers may not be active for the session's service ID at "query height" | |
// - We cannot filter by supplier.Services which only represents current (i.e. latest) height | |
suppliers := k.supplierKeeper.GetAllSuppliers(ctx) | |
// Map supplier operator addresses to random weights for deterministic sorting. | |
// This ensures fair distribution when: | |
// - NumCandidateSuppliers exceeds NumSuppliersPerSession | |
// - We need to randomly but fairly determine which suppliers can serve Applications | |
candidatesToRandomWeight := make(map[string]int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@red-0ne I cleaned up the comments while reading+thinking through it.
Are you sure this is a POST_MAINNET action item?
I realize this is potentially an edge case, but I'm a bit afraid this can have very important ramifications to the C&P lifecycle, session ID generation, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to slow down or block this PR.
Let's change this to a TODO_MAINNET
so we don't forget to revisit it next week while thinking/talking about it in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be good to get this over the finish line soon. Just a few more questions/comments.
Thank you so much for the high quality PR!
if numIndices > maxIndex { | ||
panic(fmt.Sprintf("uniqueRandomIndices: numIndices (%d) is greater than maxIndex (%d)", numIndices, maxIndex)) | ||
for _, supplier := range candidateSuppliers { | ||
// Generate deterministic random weight for supplier: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into a small local helper that returns int(binary.BigEndian.Uint64(candidateSeedHash[:8]))
?
// TODO_POST_MAINNET: Use the number of suppliers per session used as of the query height. | ||
// As of now the session is hydrated with the "current" NumSuppliersPerSession param value. | ||
// We need to account for NumSuppliersPerSession as of the query height to ensure | ||
// that the session is hydrated with the old number of suppliers in case the param | ||
// has changed between the query height and the current height. | ||
numSuppliersPerSession := int(k.GetParams(ctx).NumSuppliersPerSession) | ||
// Get all suppliers instead of filtering by service ID. | ||
// Suppliers may not be active for the session's service ID at "query height", | ||
// so we cannot filter by supplier.Services which represents the services | ||
// the supplier is active for at the "current height". | ||
suppliers := k.supplierKeeper.GetAllSuppliers(ctx) | ||
|
||
// A mapping from the supplier operator address to a random weight. | ||
// This is used for sorting the suppliers. | ||
// If NumCandidateSuppliers > NumSuppliersPerSession, the deterministic sorting order | ||
// is used to determine which ones are available to serve requests to enable fair but | ||
// random opportunity to serve Applications (i.e. do useful work). | ||
candidatesToRandomWeight := make(map[string]int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to slow down or block this PR.
Let's change this to a TODO_MAINNET
so we don't forget to revisit it next week while thinking/talking about it in parallel.
// Calculate the difference between weights. | ||
weightDiff := weightA - weightB | ||
|
||
// If weights are equal, use operator addresses as a tiebreaker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, this is fun
// Sort based on weight difference | ||
return weightDiff | ||
} | ||
slices.SortFunc(candidateSuppliers, weightedSupplierSortFn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slices.SortFunc(candidateSuppliers, weightedSupplierSortFn) | |
slices.SortFunc(candidateSuppliers, weightedSupplierSortFn) |
) | ||
|
||
// BeginBlockerActivateSupplierServices processes suppliers that have pending service activations | ||
// at the current block height. It returns the number of suppliers whose services were activated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// at the current block height. It returns the number of suppliers whose services were activated. | |
// at the current block height. | |
// It returns the number of suppliers whose services were activated. |
@@ -17,7 +17,10 @@ import ( | |||
) | |||
|
|||
// TODO_BETA(@red-0ne): Update supplier staking documentation to remove the upstaking requirement and introduce the staking fee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO_BETA(@red-0ne): Update supplier staking documentation to remove the upstaking requirement and introduce the staking fee. | |
// TODO_POST_MAINNET(@red-0ne): Update supplier staking documentation to remove the upstaking requirement and introduce the staking fee. |
// Overwrite the latest service configuration if there is already a service | ||
// config update for the same session start height. | ||
// This is to avoid having duplicate service configs with the same activation | ||
// height, which is useless and potentially confusing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great comment
} | ||
|
||
return numSuppliersWithPrunedHistory, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was your comment in the PR:
This is because applications are part of the session existence, before an app joins, the session simply doesn't exist. Which is not the case for suppliers.
I think this is really great.
Can you think of a good place to stuff this in the code somewhere so it's not lost?
|
||
for _, supplier := range k.GetAllSuppliers(ctx) { | ||
// Store the original number of historical service configs. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Special case: if all service historical service config updates would be pruned, | ||
// retain the most recent one. | ||
// This is necessary for the session hydration process that relies on the | ||
// service config history to determine the current active service configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more please? #PUC
Summary
Replace supplier's activation heights map with a service config history for better tracking of service changes over time.
Primary Changes:
ServicesActivationHeightsMap
withServiceConfigHistory
to track service configurations and their activation heightsServiceConfigUpdate
message type to store service configurations and their effective block heightsBeginBlockerActivateSupplierServices
to handle service activations at session start blocksSecondary changes:
Issue
Type of change
Select one or more from the following:
consensus-breaking
label if so. See [Infra] Automatically add theconsensus-breaking
label #791 for detailsSanity Checklist
assignees
,reviewers
,labels
,project
,iteration
andmilestone
make docusaurus_start
make go_develop_and_test
andmake test_e2e
devnet-test-e2e
label to run E2E tests in CI