Skip to content
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

[Proof] Implement scalable proof validation #1031

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Jan 17, 2025

Summary

This PR refactors proof validation by distributing it across the proof submission workflow for better scalability.

The validation process is divided into two stages:

  1. Proof Submission: Ensures the proof is well-formed and adheres to the current on-chain state.
  2. Proof Module Endblocker: Handles computationally intensive tasks using self-contained parallel verification. It includes:
    • Relay request and response signature verification.
    • Validation of the ClosestMerkleProof.

Additionally, the proof module endblocker:

  • Deletes all the processed proofs
  • Reflects the verification result in the corresponding claim's ProofStatus field.

The potential supplier slashing is retained within the SettlePendingClaims flow, ensuring a clear separation of concerns.

Issue

Type of change

Select one or more from the following:

Testing

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

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

@red-0ne red-0ne added scalability proof Claim & Proof life cycle labels Jan 17, 2025
@red-0ne red-0ne added this to the Beta TestNet Iteration milestone Jan 17, 2025
@red-0ne red-0ne self-assigned this Jan 17, 2025
@okdas okdas added the push-image CI related - pushes images to ghcr.io label Jan 22, 2025
Copy link

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 E2E tests, please add devnet-test-e2e label.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@red-0ne I did a first pass of this, but I plan to do a COMPLETE second pass as well.

This is very critical and going to need some work.

x/proof/keeper/proof.go Show resolved Hide resolved
x/proof/module/abci.go Outdated Show resolved Hide resolved
x/proof/module/abci.go Outdated Show resolved Hide resolved
x/proof/module/abci.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Show resolved Hide resolved
x/proof/keeper/validate_proofs.go Outdated Show resolved Hide resolved
x/proof/keeper/validate_proofs.go Outdated Show resolved Hide resolved
x/proof/keeper/validate_proofs.go Outdated Show resolved Hide resolved
x/proof/keeper/validate_proofs.go Outdated Show resolved Hide resolved
x/proof/keeper/validate_proofs.go Outdated Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk January 24, 2025 05:51
@@ -43,3 +43,11 @@ message EventProofUpdated {
uint64 num_estimated_compute_units = 5 [(gogoproto.jsontag) = "num_estimated_compute_units"];
cosmos.base.v1beta1.Coin claimed_upokt = 6 [(gogoproto.jsontag) = "claimed_upokt"];
}

// Event emitted after a proof has been checked for validity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you #PUC on when/where this is happening (i.e. end blocker)

@@ -43,3 +43,11 @@ message EventProofUpdated {
uint64 num_estimated_compute_units = 5 [(gogoproto.jsontag) = "num_estimated_compute_units"];
cosmos.base.v1beta1.Coin claimed_upokt = 6 [(gogoproto.jsontag) = "claimed_upokt"];
}

// Event emitted after a proof has been checked for validity.
message EventProofValidityChecked {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on EventProofValidated?

poktroll.proof.Proof proof = 1 [(gogoproto.jsontag) = "proof"];
uint64 block_height = 2 [(gogoproto.jsontag) = "block_height"];
poktroll.proof.ClaimProofStatus proof_status = 3 [(gogoproto.jsontag) = "proof_status"];
string reason = 4 [(gogoproto.jsontag) = "reason"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#PUC what kind of values reason could hold

@@ -29,6 +29,9 @@ message Claim {
poktroll.session.SessionHeader session_header = 2;
// Root hash returned from smt.SMST#Root().
bytes root_hash = 3;
// Claim proof status captures the status of the proof for this claim.
// WARNING: This field MUST only be set by proofKeeper#EnsureValidProofSignaturesAndClosestPath
ClaimProofStatus proof_status = 4;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message Claim {
 // Address of the supplier's operator that submitted this claim
 string supplier_operator_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];

 // Session header this claim is for
 poktroll.session.SessionHeader session_header = 2;

 // Root hash from smt.SMST#Root()
 bytes root_hash = 3;

 // IMPORTANT: This field MUST only be set by proofKeeper#EnsureValidProofSignaturesAndClosestPath
 ProofValidationState proof_validation_state = 4;
}

Comment on lines +50 to +56
// ClaimProofStatus defines the status of the proof for a claim.
// The default value is NOT_FOUND, whether the proof is required or not.
enum ClaimProofStatus {
NOT_FOUND = 0;
VALID = 1;
INVALID = 2;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ClaimProofStatus defines the status of the proof for a claim.
// The default value is NOT_FOUND, whether the proof is required or not.
enum ClaimProofStatus {
NOT_FOUND = 0;
VALID = 1;
INVALID = 2;
}
// Status of proof validation for a claim
// Default is PENDING_VALIDATION regardless of proof requirement
enum ProofValidationState {
PENDING_VALIDATION = 0;
VALIDATED = 1;
INVALID = 2;
}


// Retrieve the corresponding claim for the proof submitted so it can be
// used in the proof validation below.
// EnsureWellFormedProof has already validated that the claim referenced by the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#PUC when this was called

coordinator.coordinatorMu.Lock()
defer coordinator.coordinatorMu.Unlock()

// Update the claim to reflect its corresponding the proof validation result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Update the claim to reflect its corresponding the proof validation result.
// Update the claim to reflect the validation result of the associated proof.


logger := k.Logger().With("method", "EndBlocker")

// Iterates through all proofs submitted in this block and removes invalid ones.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we remove

Suggested change
// Iterates through all proofs submitted in this block and removes invalid ones.
// Iterates through all proofs submitted in this block and:
// 1. Updates the proof validation status in the associated claim
// 2. Removes all processed proofs from onchain state

}

logger.Info(fmt.Sprintf(
"validated %d proofs: %d valid, %d invalid",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"validated %d proofs: %d valid, %d invalid",
"checked %d proofs: %d valid, %d invalid",

Comment on lines +137 to +142
// The tokenomics end blocker, which calls SettlePendingClaims, is ALWAYS executed
// AFTER the proof submission window closes. In contrast, the proof end blocker,
// which handles proof validation, is ALWAYS executed WITHIN the proof submission
// window of the same session number.
// This ensures that proof validation is completed before claims settlement,
// as they occur at different block heights.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The tokenomics end blocker, which calls SettlePendingClaims, is ALWAYS executed
// AFTER the proof submission window closes. In contrast, the proof end blocker,
// which handles proof validation, is ALWAYS executed WITHIN the proof submission
// window of the same session number.
// This ensures that proof validation is completed before claims settlement,
// as they occur at different block heights.
// IMPORTANT: Proof validation and claims settlement timing:
// - Proof validation (proof end blocker): Executes WITHIN proof submission window
// - Claims settlement (tokenomics end blocker): Executes AFTER window closes
// This ensures proofs are validated before claims are settled

@Olshansk
Copy link
Member

@red-0ne Ty for taking all of my feedback in the iteration. Looking much better!

I think it should. be g2g after another round of review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proof Claim & Proof life cycle push-image CI related - pushes images to ghcr.io scalability
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants