[Proof Validation] Move proof validation to session settlement #703
Conversation
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
…get-hash * pokt/main: [TODO] chore: update `smt.MerkleRoot#Sum()` error handling (#672)
Co-authored-by: Bryan White <[email protected]>
Olshansk
added
protocol
Olshansk
changed the base branch from
refactor/difficulty/target-hash-review
to
refactor/difficulty/target-hash
| isProofValid, err = k.proofKeeper.IsProofValid(ctx, &proof) | ||
| if !isProofValid || err != nil { |
There was a problem hiding this comment.
It seems that IsProofValid always returns false when there's an error. What about renaming this to EnsureValidProof and only return err?
| isProofValid, err = k.proofKeeper.IsProofValid(ctx, &proof) | |
| if !isProofValid || err != nil { | |
| if err := k.proofKeeper.EnsureValidProof(ctx, &proof); err != nil { |
There was a problem hiding this comment.
Great suggestion! Done!!
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (10)
WalkthroughThe recent changes enhance several components within the codebase, primarily focusing on improving error handling and validation processes. Key modifications include the introduction of new fields and methods for managing claim expiration reasons, refining account management, and restructuring session handling logic. Additionally, test suites have been updated to improve reliability and clarity, ensuring that the overall functionality of claim settlements, proof validations, and account operations remain robust and effective. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (11)
x/tokenomics/keeper/settle_pending_claims.go (3)
83-83: Clarify the purpose ofproofIsRequired.The new variable
proofIsRequiredencapsulates the condition of whether a proof is necessary. This improves code readability.
94-95: Clarify the expiration reason for missing proofs.The comment clarifies that the claim should expire if a proof is required but unavailable.
128-131: Clarify the conditions for settling claims.The comments clarify the conditions under which claims are settled, improving code readability.
api/poktroll/tokenomics/event.pulsar.go (1)
23-23: Add a comment to describe the new field.The
expiration_reasonfield is newly added and should be documented for clarity.fd_EventClaimExpired_expiration_reason protoreflect.FieldDescriptor // Reason for the claim expirationx/proof/keeper/proof_validation.go (7)
57-57: Consider using a more descriptive logger message.The logger message "ValidateProof" could be more descriptive to reflect the function name
IsProofValid.- logger := k.Logger().With("method", "ValidateProof") + logger := k.Logger().With("method", "IsProofValid")
211-212: Typographical error in the comment.There is a typographical error in the comment: "to to" should be "to".
- // The RelayMiner has to wait until the submit claim and proof windows is are open - // in order to to create the claim and submit claims and proofs, respectively. + // The RelayMiner has to wait until the submit claim and proof windows are open + // in order to create the claim and submit claims and proofs, respectively.
238-239: Clarify TODO comment.The TODO comment should be more specific about what needs to be investigated.
- // TODO_BETA: Investigate "proof for the path provided does not match one expected by the on-chain protocol" - // error that may occur due to block height differing from the off-chain part. + // TODO_BETA: Investigate the error "proof for the path provided does not match one expected by the on-chain protocol" + // which may occur due to block height differing from the off-chain part.
263-263: Clarify NB comment.The NB comment should be more specific about what is being asserted.
- // NB: no need to assert the testSessionId or supplier address as it is retrieved - // by respective values of the given proof. I.e., if the claim exists, then these - // values are guaranteed to match. + // NB: No need to assert the session ID or supplier address as they are retrieved + // by the respective values of the given proof. If the claim exists, then these + // values are guaranteed to match.
279-279: Improve error message consistency.The error message for
ErrProofInvalidSessionStartHeightshould be consistent with other error messages.- return nil, types.ErrProofInvalidSessionStartHeight.Wrapf( - "claim session start height %d does not match proof session start height %d", - claimSessionHeader.GetSessionStartBlockHeight(), - proofSessionHeader.GetSessionStartBlockHeight(), - ) + return nil, types.ErrProofInvalidSessionStartHeight.Wrapf( + "claim session start height (%d) does not match proof session start height (%d)", + claimSessionHeader.GetSessionStartBlockHeight(), + proofSessionHeader.GetSessionStartBlockHeight(), + )
342-342: Typographical error in the error message.There is a typographical error in the error message: "sessionHeaders" should be "session headers".
- "sessionHeaders service names mismatch expect: %q, got: %q", + "session headers service names mismatch; expected: %q, got: %q",
378-378: Typographical error in the comment.There is a typographical error in the comment: "the the" should be "the".
- // verifyClosestProof verifies the the correctness of the ClosestMerkleProof - // against the root hash committed to when creating the claim. + // verifyClosestProof verifies the correctness of the ClosestMerkleProof + // against the root hash committed to when creating the claim.
| // TODO_TECHDEBT: add a test case such that we can distinguish between early | ||
| // & late session end block heights. |
There was a problem hiding this comment.
Reminder: Add test cases for early & late session end block heights.
The TODO comment indicates missing test cases for distinguishing between early and late session end block heights.
Do you want me to generate the test cases or open a GitHub issue to track this task?
| // TODO_TEST(community): expand: test case to cover each session header field. | ||
| desc: "relay request session header must match proof session header", |
There was a problem hiding this comment.
Reminder: Expand test case to cover each session header field.
The TODO comment indicates missing test cases for each session header field.
Do you want me to generate the test cases or open a GitHub issue to track this task?
| // TODO_TEST: expand: test case to cover each session header field. | ||
| desc: "relay response session header must match proof session header", |
There was a problem hiding this comment.
Reminder: Expand test case to cover each session header field.
The TODO comment indicates missing test cases for each session header field.
Do you want me to generate the test cases or open a GitHub issue to track this task?
| desc: "relay request signature is valid but signed by an incorrect application", | ||
| newProof: func(t *testing.T) *prooftypes.Proof { | ||
| t.Skip("TODO_TECHDEBT(@bryanchriswhite): Implement this") |
There was a problem hiding this comment.
Implement the test case for invalid relay request signature signed by an incorrect application.
The test case is marked as TODO and needs to be implemented.
Do you want me to generate the test case or open a GitHub issue to track this task?
| desc: "relay response signature is valid but signed by an incorrect supplier", | ||
| newProof: func(t *testing.T) *prooftypes.Proof { | ||
| t.Skip("TODO_TECHDEBT(@bryanchriswhite): Implement this") |
There was a problem hiding this comment.
Implement the test case for valid relay response signature signed by an incorrect supplier.
The test case is marked as TODO and needs to be implemented.
Do you want me to generate the test case or open a GitHub issue to track this task?
x/proof/keeper/proof_validation.go
Outdated
| // TODO_FOLLOWUP(@olshansk, #690): Get the difficulty associated with the service | ||
| params := k.GetParams(ctx) | ||
| relayDifficultyTargetHash := params.RelayDifficultyTargetHash | ||
| if relayDifficultyTargetHash == nil || len(relayDifficultyTargetHash) == 0 { |
There was a problem hiding this comment.
Omit nil check; len() for []byte is defined as zero.
The nil check for relayDifficultyTargetHash is redundant as len() for []byte is defined as zero.
- if relayDifficultyTargetHash == nil || len(relayDifficultyTargetHash) == 0 {
+ if len(relayDifficultyTargetHash) == 0 {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if relayDifficultyTargetHash == nil || len(relayDifficultyTargetHash) == 0 { | |
| if len(relayDifficultyTargetHash) == 0 { |
Tools
GitHub Check: go-test
[failure] 157-157:
S1009: should omit nil check; len() for []byte is defined as zero (gosimple)
Summary
What was changed?
SubmitProofin the msgServer handler toSessionSettlementduring the EndBlocker.Why was it changed?
they can keep retrying if it takes place in the SubmitProof handler.
Other:
Issue
Type of change
Select one or more:
Testing
make go_develop_and_testmake test_e2edevnet-test-e2elabel to the PR.Sanity Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation