From 60fc6ed0987ae60d3825e9b36c56559b58504aff Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 15 Jul 2024 20:05:43 +0200 Subject: [PATCH] [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) --- pkg/relayer/proxy/proxy_test.go | 2 +- pkg/relayer/proxy/relay_verifier.go | 11 ++++------- x/session/keeper/session_hydrator.go | 5 ----- x/shared/types/params.go | 23 ++++++++++++++++++++--- x/tokenomics/keeper/msg_update_params.go | 4 ---- 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/pkg/relayer/proxy/proxy_test.go b/pkg/relayer/proxy/proxy_test.go index f355efc8c..40a541a83 100644 --- a/pkg/relayer/proxy/proxy_test.go +++ b/pkg/relayer/proxy/proxy_test.go @@ -439,7 +439,7 @@ func TestRelayerProxy_Relays(t *testing.T) { inputScenario: sendRequestWithDifferentSession, expectedErrCode: testproxy.JSONRPCInternalErrorCode, - expectedErrMsg: "session mismatch", + expectedErrMsg: "session ID mismatch", }, { desc: "Invalid relay supplier", diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index bed559ec1..598494021 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -65,15 +65,12 @@ func (rp *relayerProxy) VerifyRelayRequest( // Since the retrieved sessionId was in terms of: // - the current block height and sessionGracePeriod (which are not provided by the relayRequest) // - serviceId (which is not provided by the relayRequest) - // - applicationAddress (which is used to to verify the relayRequest signature) - // - // TODO_BLOCKER(@Olshansk): Revisit the assumptions above and updated this if - // structure if necessary. + // - applicationAddress (which is used to verify the relayRequest signature) if session.SessionId != sessionHeader.GetSessionId() { return ErrRelayerProxyInvalidSession.Wrapf( - "session mismatch, expecting: %+v, got: %+v", - session.Header, - relayRequest.Meta.SessionHeader, + "session ID mismatch, expecting: %+v, got: %+v", + session.GetSessionId(), + relayRequest.Meta.GetSessionHeader().GetSessionId(), ) } diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go index 9d07626f5..84336ce27 100644 --- a/x/session/keeper/session_hydrator.go +++ b/x/session/keeper/session_hydrator.go @@ -163,11 +163,6 @@ func (k Keeper) hydrateSessionApplication(ctx context.Context, sh *sessionHydrat func (k Keeper) hydrateSessionSuppliers(ctx context.Context, sh *sessionHydrator) error { logger := k.Logger().With("method", "hydrateSessionSuppliers") - // TODO_BLOCKER(@Olshansk): Retrieve the suppliers at SessionStartBlockHeight, - // NOT THE CURRENT ONE which is what's provided by the context. For now, for - // simplicity, only retrieving the suppliers at the current block height which - // could create a discrepancy if new suppliers were staked mid session. - // TODO(@bryanchriswhite): Investigate if `BlockClient` + `ReplayObservable` where `N = SessionLength` could be used here.` suppliers := k.supplierKeeper.GetAllSuppliers(ctx) candidateSuppliers := make([]*sharedtypes.Supplier, 0) diff --git a/x/shared/types/params.go b/x/shared/types/params.go index 39415ea41..ef692f210 100644 --- a/x/shared/types/params.go +++ b/x/shared/types/params.go @@ -89,9 +89,6 @@ func (params *Params) ParamSetPairs() paramtypes.ParamSetPairs { // Validate validates the set of params func (params *Params) ValidateBasic() error { - // TODO_BLOCKER(@bryanchriswhite): Add `SessionGracePeriodBlocks` as a shared param, - // introduce validation, and ensure it is strictly less than num blocks per session. - if err := ValidateNumBlocksPerSession(params.NumBlocksPerSession); err != nil { return err } @@ -116,6 +113,10 @@ func (params *Params) ValidateBasic() error { return err } + if err := validateGracePeriodOffsetBlocksIsLessThanNumBlocksPerSession(params); err != nil { + return err + } + if err := validateClaimWindowOpenOffsetIsAtLeastGracePeriodEndOffset(params); err != nil { return err } @@ -178,6 +179,10 @@ func validateIsUint64(value any) error { return nil } +// validateClaimWindowOpenOffsetIsAtLeastGracePeriodEndOffset validates that the ClaimWindowOpenOffsetBlocks +// is at least as big as GracePeriodEndOffsetBlocks. The claim window cannot open until the grace period ends +// to ensure that the seed for the earliest supplier claim commit height is only observed after the last relay +// for a given session could be serviced. func validateClaimWindowOpenOffsetIsAtLeastGracePeriodEndOffset(params *Params) error { if params.ClaimWindowOpenOffsetBlocks < params.GracePeriodEndOffsetBlocks { return ErrSharedParamInvalid.Wrapf( @@ -186,6 +191,18 @@ func validateClaimWindowOpenOffsetIsAtLeastGracePeriodEndOffset(params *Params) params.GracePeriodEndOffsetBlocks, ) } + return nil +} +// validateGracePeriodOffsetBlocksIsLessThanNumBlocksPerSession validates that the +// GracePeriodEndOffsetBlocks is less than NumBlocksPerSession; i.e., one session. +func validateGracePeriodOffsetBlocksIsLessThanNumBlocksPerSession(params *Params) error { + if params.GracePeriodEndOffsetBlocks >= params.NumBlocksPerSession { + return ErrSharedParamInvalid.Wrapf( + "GracePeriodEndOffsetBlocks (%v) must be less than NumBlocksPerSession (%v)", + params.GracePeriodEndOffsetBlocks, + params.NumBlocksPerSession, + ) + } return nil } diff --git a/x/tokenomics/keeper/msg_update_params.go b/x/tokenomics/keeper/msg_update_params.go index ae7da9139..88e496344 100644 --- a/x/tokenomics/keeper/msg_update_params.go +++ b/x/tokenomics/keeper/msg_update_params.go @@ -14,10 +14,6 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) return nil, err } - // TODO_BLOCKER(@Olshansk): How do we validate this is the same address that signed the request? - // Do we have to use `msg.GetSigners()` explicitly during the check/validation or - // does the `cosmos.msg.v1.signer` tag in the protobuf definition enforce - // this somewhere in the Cosmos SDK? if msg.Authority != k.GetAuthority() { return nil, types.ErrTokenomicsInvalidSigner.Wrapf( "invalid authority; expected %s, got %s",