From 5913252a42eeaf2922cb713564b3b1eddbc88370 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Tue, 17 Sep 2024 09:39:29 +0200 Subject: [PATCH] [Proof] Prevent proof submission when not required (#822) ## Summary Prevent the `SubmitProof` keeper from upserting a proof if the `Proof` is not required. ## Issue `Proof`s are among the most block space-consuming primitives in Poktroll. In some cases, a `Proof` submission is unnecessary, but the current `SubmitProof` keeper still allows non-required proofs to be submitted. The `SubmitProof` keeper has enough information to determine whether the `Proof` corresponding to a claim is necessary. The protocol could leverage this information to avoid saving non-required `Proof`s on-chain, thereby optimizing block space usage. ## 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) ## Testing - [ ] **Documentation**: `make docusaurus_start`; only needed if you make doc changes - [x] **Unit Tests**: `make go_develop_and_test` - [x] **LocalNet E2E Tests**: `make test_e2e` - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. ## Sanity Checklist - [x] I have tested my changes using the available tooling - [x] I have commented my code - [x] 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 --- x/proof/keeper/msg_server_submit_proof.go | 120 +++++++++++++++ .../keeper/msg_server_submit_proof_test.go | 137 ++++++++++++++++++ .../keeper/proof_requirement_test.go | 13 +- x/proof/types/errors.go | 1 + x/tokenomics/keeper/keeper_exports_test.go | 13 -- x/tokenomics/keeper/settle_pending_claims.go | 112 +------------- x/tokenomics/types/expected_keepers.go | 1 + 7 files changed, 266 insertions(+), 131 deletions(-) rename x/{tokenomics => proof}/keeper/proof_requirement_test.go (81%) delete mode 100644 x/tokenomics/keeper/keeper_exports_test.go diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 84b52c1f2..1a5353ebb 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -15,6 +15,7 @@ import ( "github.com/pokt-network/poktroll/telemetry" "github.com/pokt-network/poktroll/x/proof/types" + "github.com/pokt-network/poktroll/x/shared" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -103,6 +104,16 @@ func (k msgServer) SubmitProof( return nil, status.Error(codes.Internal, types.ErrProofClaimNotFound.Wrap(err.Error()).Error()) } + // Check if a proof is required for the claim. + proofRequirement, err := k.ProofRequirementForClaim(ctx, claim) + if err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } + if proofRequirement == types.ProofRequirementReason_NOT_REQUIRED { + logger.Warn("trying to submit a proof for a claim that does not require one") + return nil, status.Error(codes.FailedPrecondition, types.ErrProofNotRequired.Error()) + } + // Get metadata for the event we want to emit numRelays, err = claim.GetNumRelays() if err != nil { @@ -191,3 +202,112 @@ func (k Keeper) deductProofSubmissionFee(ctx context.Context, supplierOperatorAd return nil } + +// ProofRequirementForClaim checks if a proof is required for a claim. +// If it is not, the claim will be settled without a proof. +// If it is, the claim will only be settled if a valid proof is available. +// TODO_BLOCKER(@olshansk, #419): Document safety assumptions of the probabilistic proofs mechanism. +func (k Keeper) ProofRequirementForClaim(ctx context.Context, claim *types.Claim) (_ types.ProofRequirementReason, err error) { + logger := k.logger.With("method", "proofRequirementForClaim") + + var requirementReason = types.ProofRequirementReason_NOT_REQUIRED + + // Defer telemetry calls so that they reference the final values the relevant variables. + defer func() { + telemetry.ProofRequirementCounter(requirementReason, err) + }() + + // NB: Assumption that claim is non-nil and has a valid root sum because it + // is retrieved from the store and validated, on-chain, at time of creation. + var numClaimComputeUnits uint64 + numClaimComputeUnits, err = claim.GetNumComputeUnits() + if err != nil { + return requirementReason, err + } + + proofParams := k.GetParams(ctx) + + // Require a proof if the claim's compute units meets or exceeds the threshold. + // + // TODO_BLOCKER(@Olshansk, #419): This is just VERY BASIC placeholder logic to have something + // in place while we implement proper probabilistic proofs. If you're reading it, + // do not overthink it and look at the documents linked in #419. + // + // TODO_IMPROVE(@bryanchriswhite, @red-0ne): It might make sense to include + // whether there was a proof submission error downstream from here. This would + // require a more comprehensive metrics API. + if numClaimComputeUnits >= proofParams.GetProofRequirementThreshold() { + requirementReason = types.ProofRequirementReason_THRESHOLD + + logger.Info(fmt.Sprintf( + "claim requires proof due to compute units (%d) exceeding threshold (%d)", + numClaimComputeUnits, + proofParams.GetProofRequirementThreshold(), + )) + return requirementReason, nil + } + + // Hash of block when proof submission is allowed. + earliestProofCommitBlockHash, err := k.getEarliestSupplierProofCommitBlockHash(ctx, claim) + if err != nil { + return requirementReason, err + } + + // The probability that a proof is required. + proofRequirementSampleValue, err := claim.GetProofRequirementSampleValue(earliestProofCommitBlockHash) + if err != nil { + return requirementReason, err + } + + // Require a proof probabilistically based on the proof_request_probability param. + // NB: A random value between 0 and 1 will be less than or equal to proof_request_probability + // with probability equal to the proof_request_probability. + if proofRequirementSampleValue <= proofParams.GetProofRequestProbability() { + requirementReason = types.ProofRequirementReason_PROBABILISTIC + + logger.Info(fmt.Sprintf( + "claim requires proof due to random sample (%.2f) being less than or equal to probability (%.2f)", + proofRequirementSampleValue, + proofParams.GetProofRequestProbability(), + )) + return requirementReason, nil + } + + logger.Info(fmt.Sprintf( + "claim does not require proof due to compute units (%d) being less than the threshold (%d) and random sample (%.2f) being greater than probability (%.2f)", + numClaimComputeUnits, + proofParams.GetProofRequirementThreshold(), + proofRequirementSampleValue, + proofParams.GetProofRequestProbability(), + )) + return requirementReason, nil +} + +// getEarliestSupplierProofCommitBlockHash returns the block hash of the earliest +// block at which a claim may have its proof committed. +func (k Keeper) getEarliestSupplierProofCommitBlockHash( + ctx context.Context, + claim *types.Claim, +) (blockHash []byte, err error) { + sharedParams, err := k.sharedQuerier.GetParams(ctx) + if err != nil { + return nil, err + } + + sessionEndHeight := claim.GetSessionHeader().GetSessionEndBlockHeight() + supplierOperatorAddress := claim.GetSupplierOperatorAddress() + + proofWindowOpenHeight := shared.GetProofWindowOpenHeight(sharedParams, sessionEndHeight) + proofWindowOpenBlockHash := k.sessionKeeper.GetBlockHash(ctx, proofWindowOpenHeight) + + // TODO_TECHDEBT(@red-0ne): Update the method header of this function to accept (sharedParams, Claim, BlockHash). + // After doing so, please review all calling sites and simplify them accordingly. + earliestSupplierProofCommitHeight := shared.GetEarliestSupplierProofCommitHeight( + sharedParams, + sessionEndHeight, + proofWindowOpenBlockHash, + supplierOperatorAddress, + ) + + return k.sessionKeeper.GetBlockHash(ctx, earliestSupplierProofCommitHeight), nil +} diff --git a/x/proof/keeper/msg_server_submit_proof_test.go b/x/proof/keeper/msg_server_submit_proof_test.go index 7827a701e..11eafd54d 100644 --- a/x/proof/keeper/msg_server_submit_proof_test.go +++ b/x/proof/keeper/msg_server_submit_proof_test.go @@ -692,6 +692,143 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { } } +func TestMsgServer_SubmitProof_FailSubmittingNonRequiredProof(t *testing.T) { + opts := []keepertest.ProofKeepersOpt{ + // Set block hash so we can have a deterministic expected on-chain proof requested by the protocol. + keepertest.WithBlockHash(blockHeaderHash), + // Set block height to 1 so there is a valid session on-chain. + keepertest.WithBlockHeight(1), + } + keepers, ctx := keepertest.NewProofModuleKeepers(t, opts...) + sharedParams := keepers.SharedKeeper.GetParams(ctx) + + // Set proof keeper params to disable relay mining but never require a proof. + proofParams := keepers.Keeper.GetParams(ctx) + proofParams.ProofRequestProbability = 0 + proofParams.RelayDifficultyTargetHash = testProofParams.RelayDifficultyTargetHash + err := keepers.Keeper.SetParams(ctx, proofParams) + require.NoError(t, err) + + // Construct a keyring to hold the keypairs for the accounts used in the test. + keyRing := keyring.NewInMemory(keepers.Codec) + + // Create a pre-generated account iterator to create accounts for the test. + preGeneratedAccts := testkeyring.PreGeneratedAccounts() + + // Create accounts in the account keeper with corresponding keys in the + // keyring for the application and supplier. + supplierOperatorAddr := testkeyring.CreateOnChainAccount( + ctx, t, + supplierOperatorUid, + keyRing, + keepers, + preGeneratedAccts, + ).String() + appAddr := testkeyring.CreateOnChainAccount( + ctx, t, + "app", + keyRing, + keepers, + preGeneratedAccts, + ).String() + + fundSupplierOperatorAccount(t, ctx, keepers, supplierOperatorAddr) + + service := &sharedtypes.Service{ + Id: testServiceId, + ComputeUnitsPerRelay: computeUnitsPerRelay, + OwnerAddress: sample.AccAddress(), + } + + // Add a supplier and application pair that are expected to be in the session. + keepers.AddServiceActors(ctx, t, service, supplierOperatorAddr, appAddr) + + // Get the session for the application/supplier pair which is expected + // to be claimed and for which a valid proof would be accepted. + // Given the setup above, it is guaranteed that the supplier created + // will be part of the session. + sessionHeader := keepers.GetSessionHeader(ctx, t, appAddr, service, 1) + + // Construct a proof message server from the proof keeper. + srv := keeper.NewMsgServerImpl(*keepers.Keeper) + + // Prepare a ring client to sign & validate relays. + ringClient, err := rings.NewRingClient(depinject.Supply( + polyzero.NewLogger(), + prooftypes.NewAppKeeperQueryClient(keepers.ApplicationKeeper), + prooftypes.NewAccountKeeperQueryClient(keepers.AccountKeeper), + prooftypes.NewSharedKeeperQueryClient(keepers.SharedKeeper, keepers.SessionKeeper), + )) + require.NoError(t, err) + + // Submit the corresponding proof. + numRelays := uint64(5) + sessionTree := testtree.NewFilledSessionTree( + ctx, t, + numRelays, service.ComputeUnitsPerRelay, + supplierOperatorUid, supplierOperatorAddr, + sessionHeader, sessionHeader, sessionHeader, + keyRing, + ringClient, + ) + + // Advance the block height to the test claim msg height. + claimMsgHeight := shared.GetEarliestSupplierClaimCommitHeight( + &sharedParams, + sessionHeader.GetSessionEndBlockHeight(), + blockHeaderHash, + supplierOperatorAddr, + ) + ctx = keepertest.SetBlockHeight(ctx, claimMsgHeight) + + // Create a valid claim. + createClaimAndStoreBlockHash( + ctx, t, 1, + supplierOperatorAddr, + appAddr, + service, + sessionTree, + sessionHeader, + srv, + keepers, + ) + + // Advance the block height to the proof path seed height. + earliestSupplierProofCommitHeight := shared.GetEarliestSupplierProofCommitHeight( + &sharedParams, + sessionHeader.GetSessionEndBlockHeight(), + blockHeaderHash, + supplierOperatorAddr, + ) + ctx = keepertest.SetBlockHeight(ctx, earliestSupplierProofCommitHeight-1) + + // Store proof path seed block hash in the session keeper so that it can + // look it up during proof validation. + keepers.StoreBlockHash(ctx) + + // Compute expected proof path. + expectedMerkleProofPath = protocol.GetPathForProof(blockHeaderHash, sessionHeader.GetSessionId()) + + // Advance the block height to the test proof msg height. + proofMsgHeight := shared.GetEarliestSupplierProofCommitHeight( + &sharedParams, + sessionHeader.GetSessionEndBlockHeight(), + blockHeaderHash, + supplierOperatorAddr, + ) + ctx = keepertest.SetBlockHeight(ctx, proofMsgHeight) + + proofMsg := newTestProofMsg(t, + supplierOperatorAddr, + sessionHeader, + sessionTree, + expectedMerkleProofPath, + ) + submitProofRes, err := srv.SubmitProof(ctx, proofMsg) + require.Nil(t, submitProofRes) + require.ErrorIs(t, err, status.Error(codes.FailedPrecondition, prooftypes.ErrProofNotRequired.Error())) +} + // newTestProofMsg creates a new submit proof message that can be submitted // to be validated and stored on-chain. func newTestProofMsg( diff --git a/x/tokenomics/keeper/proof_requirement_test.go b/x/proof/keeper/proof_requirement_test.go similarity index 81% rename from x/tokenomics/keeper/proof_requirement_test.go rename to x/proof/keeper/proof_requirement_test.go index c77c205f4..82c5a404d 100644 --- a/x/tokenomics/keeper/proof_requirement_test.go +++ b/x/proof/keeper/proof_requirement_test.go @@ -4,7 +4,6 @@ import ( "sync/atomic" "testing" - "cosmossdk.io/log" cosmostypes "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" @@ -12,18 +11,18 @@ import ( "github.com/pokt-network/poktroll/testutil/keeper" tetsproof "github.com/pokt-network/poktroll/testutil/proof" "github.com/pokt-network/poktroll/testutil/sample" - prooftypes "github.com/pokt-network/poktroll/x/proof/types" + "github.com/pokt-network/poktroll/x/proof/types" ) func TestKeeper_IsProofRequired(t *testing.T) { // Set expectedCompute units to be below the proof requirement threshold to only // exercise the probabilistic branch of the #isProofRequired() logic. - expectedComputeUnits := prooftypes.DefaultProofRequirementThreshold - 1 - keepers, ctx := keeper.NewTokenomicsModuleKeepers(t, log.NewNopLogger()) + expectedComputeUnits := types.DefaultProofRequirementThreshold - 1 + keepers, ctx := keeper.NewProofModuleKeepers(t) sdkCtx := cosmostypes.UnwrapSDKContext(ctx) var ( - probability = prooftypes.DefaultProofRequestProbability + probability = types.DefaultProofRequestProbability // This was empirically determined to avoid false negatives in unit tests. // As a maintainer of the codebase, you may need to adjust these. tolerance = 0.10 @@ -40,10 +39,10 @@ func TestKeeper_IsProofRequired(t *testing.T) { for i := int64(0); i < sampleSize; i++ { claim := tetsproof.ClaimWithRandomHash(t, sample.AccAddress(), sample.AccAddress(), expectedComputeUnits) - proofRequirementReason, err := keepers.Keeper.ProofRequirementForClaim(sdkCtx, &claim) + proofRequirementReason, err := keepers.ProofRequirementForClaim(sdkCtx, &claim) require.NoError(t, err) - if proofRequirementReason != prooftypes.ProofRequirementReason_NOT_REQUIRED { + if proofRequirementReason != types.ProofRequirementReason_NOT_REQUIRED { numTrueSamples.Add(1) } } diff --git a/x/proof/types/errors.go b/x/proof/types/errors.go index f15a99b47..966500730 100644 --- a/x/proof/types/errors.go +++ b/x/proof/types/errors.go @@ -37,4 +37,5 @@ var ( ErrProofComputeUnitsMismatch = sdkerrors.Register(ModuleName, 1126, "mismatch: claim compute units != number of relays * service compute units per relay") ErrProofNotEnoughFunds = sdkerrors.Register(ModuleName, 1127, "not enough funds to submit proof") ErrProofFailedToDeductFee = sdkerrors.Register(ModuleName, 1128, "failed to deduct proof submission fee") + ErrProofNotRequired = sdkerrors.Register(ModuleName, 1129, "proof not required") ) diff --git a/x/tokenomics/keeper/keeper_exports_test.go b/x/tokenomics/keeper/keeper_exports_test.go deleted file mode 100644 index 2f4d7f46f..000000000 --- a/x/tokenomics/keeper/keeper_exports_test.go +++ /dev/null @@ -1,13 +0,0 @@ -// NB: This file contains exports of unexported members for testing purposes only. -package keeper - -import ( - cosmostypes "github.com/cosmos/cosmos-sdk/types" - - prooftypes "github.com/pokt-network/poktroll/x/proof/types" -) - -// ProofRequirementForClaim wraps the unexported proofRequirementForClaim function for testing purposes. -func (k Keeper) ProofRequirementForClaim(ctx cosmostypes.Context, claim *prooftypes.Claim) (prooftypes.ProofRequirementReason, error) { - return k.proofRequirementForClaim(ctx, claim) -} diff --git a/x/tokenomics/keeper/settle_pending_claims.go b/x/tokenomics/keeper/settle_pending_claims.go index d378095f8..7d307eaf4 100644 --- a/x/tokenomics/keeper/settle_pending_claims.go +++ b/x/tokenomics/keeper/settle_pending_claims.go @@ -1,15 +1,12 @@ package keeper import ( - "context" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" - "github.com/pokt-network/poktroll/telemetry" prooftypes "github.com/pokt-network/poktroll/x/proof/types" - "github.com/pokt-network/poktroll/x/shared" "github.com/pokt-network/poktroll/x/tokenomics/types" ) @@ -68,7 +65,7 @@ func (k Keeper) SettlePendingClaims(ctx sdk.Context) ( proof, isProofFound := k.proofKeeper.GetProof(ctx, sessionId, claim.SupplierOperatorAddress) // Using the probabilistic proofs approach, determine if this expiring // claim required an on-chain proof - proofRequirement, err = k.proofRequirementForClaim(ctx, &claim) + proofRequirement, err = k.proofKeeper.ProofRequirementForClaim(ctx, &claim) if err != nil { return settledResult, expiredResult, err } @@ -235,110 +232,3 @@ func (k Keeper) getExpiringClaims(ctx sdk.Context) (expiringClaims []prooftypes. // Return the actually expiring claims return expiringClaims, nil } - -// proofRequirementForClaim checks if a proof is required for a claim. -// If it is not, the claim will be settled without a proof. -// If it is, the claim will only be settled if a valid proof is available. -// TODO_BLOCKER(@bryanchriswhite, #419): Document safety assumptions of the probabilistic proofs mechanism. -func (k Keeper) proofRequirementForClaim(ctx sdk.Context, claim *prooftypes.Claim) (_ prooftypes.ProofRequirementReason, err error) { - logger := k.logger.With("method", "proofRequirementForClaim") - - var requirementReason = prooftypes.ProofRequirementReason_NOT_REQUIRED - - // Defer telemetry calls so that they reference the final values the relevant variables. - defer func() { - telemetry.ProofRequirementCounter(requirementReason, err) - }() - - // NB: Assumption that claim is non-nil and has a valid root sum because it - // is retrieved from the store and validated, on-chain, at time of creation. - var numClaimComputeUnits uint64 - numClaimComputeUnits, err = claim.GetNumComputeUnits() - if err != nil { - return requirementReason, err - } - - proofParams := k.proofKeeper.GetParams(ctx) - - // Require a proof if the claim's compute units meets or exceeds the threshold. - // - // TODO_BLOCKER(@bryanchriswhite, #419): This is just VERY BASIC placeholder logic to have something - // in place while we implement proper probabilistic proofs. If you're reading it, - // do not overthink it and look at the documents linked in #419. - // - // TODO_IMPROVE(@bryanchriswhite, @red-0ne): It might make sense to include - // whether there was a proof submission error downstream from here. This would - // require a more comprehensive metrics API. - if numClaimComputeUnits >= proofParams.GetProofRequirementThreshold() { - requirementReason = prooftypes.ProofRequirementReason_THRESHOLD - - logger.Info(fmt.Sprintf( - "claim requires proof due to compute units (%d) exceeding threshold (%d)", - numClaimComputeUnits, - proofParams.GetProofRequirementThreshold(), - )) - return requirementReason, nil - } - - earliestProofCommitBlockHash, err := k.getEarliestSupplierProofCommitBlockHash(ctx, claim) - if err != nil { - return requirementReason, err - } - - proofRequirementSampleValue, err := claim.GetProofRequirementSampleValue(earliestProofCommitBlockHash) - if err != nil { - return requirementReason, err - } - - // Require a proof probabilistically based on the proof_request_probability param. - // NB: A random value between 0 and 1 will be less than or equal to proof_request_probability - // with probability equal to the proof_request_probability. - if proofRequirementSampleValue <= proofParams.GetProofRequestProbability() { - requirementReason = prooftypes.ProofRequirementReason_PROBABILISTIC - - logger.Info(fmt.Sprintf( - "claim requires proof due to random sample (%.2f) being less than or equal to probability (%.2f)", - proofRequirementSampleValue, - proofParams.GetProofRequestProbability(), - )) - return requirementReason, nil - } - - logger.Info(fmt.Sprintf( - "claim does not require proof due to compute units (%d) being less than the threshold (%d) and random sample (%.2f) being greater than probability (%.2f)", - numClaimComputeUnits, - proofParams.GetProofRequirementThreshold(), - proofRequirementSampleValue, - proofParams.GetProofRequestProbability(), - )) - return requirementReason, nil -} - -// getEarliestSupplierProofCommitBlockHash returns the block hash of the earliest -// block at which a claim might have its proof committed. -func (k Keeper) getEarliestSupplierProofCommitBlockHash( - ctx context.Context, - claim *prooftypes.Claim, -) (blockHash []byte, err error) { - sharedParams, err := k.sharedQuerier.GetParams(ctx) - if err != nil { - return nil, err - } - - sessionEndHeight := claim.GetSessionHeader().GetSessionEndBlockHeight() - supplierOperatorAddress := claim.GetSupplierOperatorAddress() - - proofWindowOpenHeight := shared.GetProofWindowOpenHeight(sharedParams, sessionEndHeight) - proofWindowOpenBlockHash := k.sessionKeeper.GetBlockHash(ctx, proofWindowOpenHeight) - - // TODO_TECHDEBT: Update the method header of this function to accept (sharedParams, Claim, BlockHash). - // After doing so, please review all calling sites and simplify them accordingly. - earliestSupplierProofCommitHeight := shared.GetEarliestSupplierProofCommitHeight( - sharedParams, - sessionEndHeight, - proofWindowOpenBlockHash, - supplierOperatorAddress, - ) - - return k.sessionKeeper.GetBlockHash(ctx, earliestSupplierProofCommitHeight), nil -} diff --git a/x/tokenomics/types/expected_keepers.go b/x/tokenomics/types/expected_keepers.go index b6e07314a..c8a2aa9e9 100644 --- a/x/tokenomics/types/expected_keepers.go +++ b/x/tokenomics/types/expected_keepers.go @@ -53,6 +53,7 @@ type ProofKeeper interface { AllClaims(ctx context.Context, req *prooftypes.QueryAllClaimsRequest) (*prooftypes.QueryAllClaimsResponse, error) EnsureValidProof(ctx context.Context, proof *prooftypes.Proof) error + ProofRequirementForClaim(ctx context.Context, claim *prooftypes.Claim) (prooftypes.ProofRequirementReason, error) // Only used for testing & simulation GetAllProofs(ctx context.Context) []prooftypes.Proof