feat: enforce max chains during new session #1582

h5law · 2023-10-18T22:30:04Z

Description

Human Summary

This PR addresses the blocker for the Gandalf proposal. It does not implement any of the changes proposed in this proposal but instead enforces the MaxChains parameter which is currently unforced to an extent. It would prevent a node from joining a session if they are staked to more chains than allowed. This enables the future changes to this parameter to be enforceable on-chain by making sure any node joining a session is abiding by this parameter.

Fixes #1584

Summary generated by Reviewpad on 06 Dec 23 01:50 UTC

This pull request includes several changes that affect multiple files:

In the "claim.go" file:
- An import statement has been added for "github.com/tendermint/tendermint/rpc/client".
- The function "SendClaimTx" has been modified to include multiple arguments on separate lines for better readability.
- An additional condition has been added to the if statement inside the "SendClaimTx" function.
- Several log statements inside the "SendClaimTx" function have been modified to use chaining for better readability.
- The function "ValidateClaim" has been modified to add a new error condition for checking the maximum number of chains an app can be staked to.
- The "ValidateClaim" function now checks the length of the app's chains and compares it to the maximum allowed chains.
- Formatting changes have been made to the log statements inside the "ValidateClaim" function for better readability.
- The variable declaration for "msg" inside the "DeleteExpiredClaims" function has been modified to use the shorthand syntax.
In the "run" job:
- The test command timeout has been increased to 15 minutes by adding the argument "-timeout=15m". This change ensures that the tests have enough time to complete.
In the "go.mod" file:
- The version of the "github.com/google/go-cmp" package has been updated from "v0.5.6" to "v0.5.8".
- The version of the "golang.org/x/sys" package has been updated from "v0.1.0" to "v0.14.0".
In the "x/pocketcore/types/service.go" file:
- The import statements have been rearranged and reorganized.
- The "Validate" function of the "Relay" struct has been modified to accept additional parameters and has been formatted with proper line breaks for better readability.
- An additional validation has been added to check if the number of chains an app is staked to exceeds the permitted limit.
- Some variable assignments and conditional statements have been modified for clarity and consistency.
- The "sessionEndCtx" variable has been corrected to be properly assigned based on the block height.
- Some comments within the code have been updated for better clarity.
In the "codec.go" file:
- A new constant "EnforceMaxChainsUpdateKey" has been added.
- A new method "IsAfterEnforceMaxChainsUpgrade" has been added to check if the height is after a specific upgrade.
- The "IsOnNonCustodialUpgrade" method has been updated to include the height in the check.
- The "IsAfterNamedFeatureActivationHeight" method has been updated to include the height in the check.
- The "SliceToExistingMap" function has been updated to use shorthand declaration for the "fmap" variable.
- The "SliceToMap" function has been updated to use shorthand declaration for the "fmap" variable.
- The "MapToSlice" function has been updated to use shorthand declaration for the "fslice" variable.
In the "expectedKeepers.go" file:
- The function "MaxChains" has been added to the "PosKeeper" interface.
- The function "MaxChains" has been added to the "AppsKeeper" interface.
In the "error_codes.go" file:
- A new error code "CodeChainsOverLimitError" with a value of 91 has been added.
- A new error "ChainsOverLimitError" with a message "the number of staked chains is over the limit" has been added.
- A new function "NewChainsOverLimitError" has been added that creates an error with the "CodeChainsOverLimitError" code and a formatted error message that includes the number of chains that were received and the maximum allowed chains.
In the "session.go" file:
- In the "Session" struct, a new import statement for the "log" package has been added. Additionally, the "HashString" method has been modified to return "s.SessionHeader.HashString()" instead of "s.HashString()".
- The "NewSession" function has been modified to add new parameters and adjust the parameter alignment. The added parameters are "sessionNodesCount" and "blockHash".
- The "NewSessionNodes" function has been modified to add new parameters and adjust the parameter alignment. The added parameters are "sessionKey" and "sessionNodesCount".
- The "NewSessionNodes" function also includes additional code related to retrieving values from the "ctx" and "keeper", as well as checking node conditions and adding nodes to the session.
- The "MaxPossibleRelays" function includes a comment correction and a rounding operation for the returned value.
In the "service_test.go" file:
- Lines 26 and 27: The variables "ethereum" and "bitcoin" are modified to use octal escape sequences instead of hexadecimal ones. The value "01" is now "0o1" and the value "02" is now "0o2".
- Lines 148 and 152: The variable "ethereum" is modified to use an octal escape sequence instead of a hexadecimal one. The value "01" is now "0o1".
- Lines 197 and 201: The variable "ethereum" is modified to use an octal escape sequence instead of a hexadecimal one. The value "01" is now "0o1".
- Lines 326 and 330: A new method "MaxChains" is added to the "MockAppsKeeper" struct. This method returns the value 15.
- Lines 397 and 401: A new method "MaxChains" is added to the "MockPosKeeper" struct. This method returns the value 15.

These changes aim to improve various aspects of the code, such as readability, maintainability, error handling, and usage of newer package versions. Please review these changes thoroughly.

reviewpad · 2023-10-18T22:30:14Z

Thank you @h5law for this first contribution!

Olshansk · 2023-10-19T14:30:49Z

@msmania Could you take a look at this review when you have a chance?

msmania · 2023-10-20T17:19:28Z

Can you add some description to explain why we need this? Reviewpad covers what this patch does, but it doesn't cover why part. And if this is to unblock Gandalf, please mention it too.

x/pocketcore/types/session.go

x/nodes/keeper/validator.go

x/nodes/types/validator.go

msmania · 2023-10-23T22:51:52Z

Thank you for adding the description. Given that this upgrade is EnforceMaxChains, probably we may want to enforce app's MaxChains too as an opportunistic change? The change would be to add a similar check in Relay.Validate and ValidateClaim.

RossiNYC · 2023-10-24T15:06:49Z

@h5law - can you link this PR to https://github.com/orgs/pokt-network/projects/143?pane=issue&itemId=38406491

I think you're solving for it.

POKT-Discourse · 2023-10-24T19:23:56Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/pip-32-fix-maxchains-paramater-fmp/4805/1

h5law · 2023-10-24T22:19:52Z

@msmania I have updated the PR to check the app has not exceeded max chains when validating relays and during claim validation. Is this what you had in mind?

msmania · 2023-10-24T22:42:35Z

@msmania I have updated the PR to check the app has not exceeded max chains when validating relays and during claim validation. Is this what you had in mind?

Yes, the logic is something like that, but you need to use MaxChains in the AppKeeper (here). There are two parameters pos/MaximumChains and application/MaximumChains.

msmania

The change looks good.

One more ask. Can you exclude files if the change there is only for linting. Upon quick look, I think the following files have no actual logic change.

app/cmd/rpc/rpc_test.go
app/tx_test.go
x/apps/keeper/application.go
x/nodes/keeper/keeper.go
x/nodes/keeper/reward.go
x/nodes/keeper/validator.go
x/nodes/types/validator.go

x/pocketcore/types/session.go

Olshansk

@h5law Can you add a unit test that verifies this before and after the feature flag is enabled?

@msmania has a great example in #1580 if you need a reference

x/pocketcore/types/service.go

x/pocketcore/types/session.go

h5law · 2023-12-02T00:11:41Z

Added new tests but CI fails for another reason, I have no idea why this error occurs and its fully out of scope of this PR to attend to it. Expanding the timeout fixed it previously but this behaviour is strange and should not occur. Besides that @Olshansk @msmania this PR is ready to go

x/pocketcore/types/session_test.go

msmania · 2023-12-05T15:28:03Z

Added new tests but CI fails for another reason, I have no idea why this error occurs and its fully out of scope of this PR to attend to it. Expanding the timeout fixed it previously but this behaviour is strange and should not occur. Besides that @Olshansk @msmania this PR is ready to go

Hmm, what failed is TestRPC_QueryUnconfirmedTxs, where I think one transaction was processed before the test ran UnconfirmedTxs, which has nothing to do with this change anyway. I'm fine with ignoring irrelevant test failures.

x/pocketcore/types/session.go

In `NewSessionNodes`, the variable `node` can be `nil` if the node is unstaked between `sessionCtx` and `ctx`. In such a case, `node.GetChains()` causes a panic. This patch makes sure we don't call it if the `node` is `nil`. This is a regression from #1582.

In `NewSessionNodes`, the variable `node` can be `nil` if the node is unstaked between `sessionCtx` and `ctx`. In such a case, `node.GetChains()` causes a panic. This patch makes sure we don't call it if the `node` is `nil`. This is a regression from #1582. ## Description  ### Summary generated by Reviewpad on 21 Dec 23 10:23 UTC This pull request fixes a regression introduced in #1582. The patch ensures that the function `node.GetChains()` is not called if the `node` is `nil`, preventing a panic in the `NewSessionNodes` function.

POKT-Discourse · 2024-01-31T19:00:12Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/rc-0-11-1-upgrade-and-hi/5012/1

feat: enforce max chains during new session

23c2a82

h5law added the enhancement New feature or request label Oct 18, 2023

h5law requested review from Olshansk and RossiNYC October 18, 2023 22:30

h5law self-assigned this Oct 18, 2023

reviewpad bot added small Pull request is small waiting-for-review labels Oct 18, 2023

h5law added 2 commits October 18, 2023 23:31

chore: negate max chains check

6bbf7b3

chore: update comment

a6eeabb

Olshansk requested a review from msmania October 19, 2023 14:30

msmania suggested changes Oct 20, 2023

View reviewed changes

x/pocketcore/types/session.go Outdated Show resolved Hide resolved

x/nodes/keeper/validator.go Outdated Show resolved Hide resolved

x/nodes/types/validator.go Outdated Show resolved Hide resolved

h5law added 4 commits October 20, 2023 20:31

chore: address comments

7042817

chore: add mock method

d691298

feat: add feature flag check

428a2e6

chore: remove flag height

f644791

h5law requested a review from msmania October 21, 2023 16:37

h5law added 2 commits October 24, 2023 21:49

chore: use module wide codec

e436e8c

feat: expand enforce to apps

940a686

reviewpad bot added medium Pull request is medium and removed small Pull request is small labels Oct 24, 2023

chore: use app max chains for app enforce

e375b26

Olshansk added this to the Network Scalability milestone Oct 26, 2023

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Nov 26, 2023

msmania suggested changes Nov 29, 2023

View reviewed changes

x/pocketcore/types/session.go Show resolved Hide resolved

chore: revert formatting only changes

b539bb1

reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Nov 29, 2023

h5law requested a review from msmania November 29, 2023 16:57

Olshansk requested changes Dec 1, 2023

View reviewed changes

x/pocketcore/types/service.go Outdated Show resolved Hide resolved

x/pocketcore/types/session.go Outdated Show resolved Hide resolved

h5law added 2 commits December 1, 2023 23:56

chore: address comments

28a1ede

feat: add NewSessionNodes test and NewSessionNodesAfterMaxChains test

d0b904a

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Dec 1, 2023

h5law requested a review from Olshansk December 2, 2023 00:12

Olshansk requested changes Dec 5, 2023

View reviewed changes

x/pocketcore/types/session_test.go Outdated Show resolved Hide resolved

x/pocketcore/types/session_test.go Show resolved Hide resolved

chore: combine tests

d3e970f

h5law requested a review from Olshansk December 5, 2023 17:00

msmania suggested changes Dec 5, 2023

View reviewed changes

x/pocketcore/types/session.go Outdated Show resolved Hide resolved

chore: address final comments

d0463b3

msmania approved these changes Dec 6, 2023

View reviewed changes

Olshansk approved these changes Dec 14, 2023

View reviewed changes

h5law merged commit 02a4e3a into staging Dec 15, 2023
3 checks passed

h5law deleted the enforce_max_chains branch December 15, 2023 16:09

msmania mentioned this pull request Dec 21, 2023

Do not call node.GetChains() if node is nil #1588

Merged

msmania mentioned this pull request Dec 22, 2023

[v0.11 feature] Built-in Reward Share #1581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enforce max chains during new session #1582

feat: enforce max chains during new session #1582

h5law commented Oct 18, 2023 •

edited by reviewpad bot

Loading

reviewpad bot commented Oct 18, 2023

Olshansk commented Oct 19, 2023

msmania commented Oct 20, 2023 •

edited

Loading

msmania commented Oct 23, 2023

RossiNYC commented Oct 24, 2023

POKT-Discourse commented Oct 24, 2023

h5law commented Oct 24, 2023

msmania commented Oct 24, 2023

msmania left a comment

Olshansk left a comment

h5law commented Dec 2, 2023

msmania commented Dec 5, 2023

POKT-Discourse commented Jan 31, 2024

feat: enforce max chains during new session #1582

feat: enforce max chains during new session #1582

Conversation

h5law commented Oct 18, 2023 • edited by reviewpad bot Loading

Description

Human Summary

Summary generated by Reviewpad on 06 Dec 23 01:50 UTC

reviewpad bot commented Oct 18, 2023

Olshansk commented Oct 19, 2023

msmania commented Oct 20, 2023 • edited Loading

msmania commented Oct 23, 2023

RossiNYC commented Oct 24, 2023

POKT-Discourse commented Oct 24, 2023

h5law commented Oct 24, 2023

msmania commented Oct 24, 2023

msmania left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

h5law commented Dec 2, 2023

msmania commented Dec 5, 2023

POKT-Discourse commented Jan 31, 2024

h5law commented Oct 18, 2023 •

edited by reviewpad bot

Loading

msmania commented Oct 20, 2023 •

edited

Loading