From 61c8dc88310dbadadfb8968cc0b2f3570199f90b Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Fri, 1 Mar 2024 15:08:32 +0100 Subject: [PATCH 01/23] feat: Implement proof's smt and signature validations --- e2e/tests/session.feature | 2 +- testutil/keeper/proof.go | 2 + x/proof/keeper/keeper.go | 10 +- x/proof/keeper/msg_server_submit_proof.go | 454 +++++++++++++++++++++- x/proof/module/module.go | 9 +- x/proof/types/errors.go | 10 +- x/proof/types/expected_keepers.go | 10 +- x/service/types/relay.go | 10 + 8 files changed, 481 insertions(+), 26 deletions(-) diff --git a/e2e/tests/session.feature b/e2e/tests/session.feature index f0626011d..88842ab9f 100644 --- a/e2e/tests/session.feature +++ b/e2e/tests/session.feature @@ -9,7 +9,7 @@ Feature: Session Namespace # The timeout for when a claim can be submitted on-chain depends on `createClaimWindowStartHeight`, which # is a function of `SessionGracePeriod`. The higher this value, the higher this timeout needs to be. Since # this test is not dependant on the grace period, setting it to 0 and having a lower grace period will simplify it. - And the user should wait for "7" seconds + And the user should wait for "10" seconds Then the claim created by supplier "supplier1" for service "svc1" for application "app1" should be persisted on-chain # TODO_IMPROVE: And an event should be emitted... And after the supplier submits a proof for the session for service "svc1" for application "app1" diff --git a/testutil/keeper/proof.go b/testutil/keeper/proof.go index 0189ccd62..4ca4dd76b 100644 --- a/testutil/keeper/proof.go +++ b/testutil/keeper/proof.go @@ -80,6 +80,8 @@ func ProofKeeper( log.NewNopLogger(), authority.String(), mockSessionKeeper, + nil, + nil, ) ctx := sdk.NewContext(stateStore, cmtproto.Header{}, false, log.NewNopLogger()) diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 09e5d465a..e3acb81ff 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -21,7 +21,9 @@ type ( // should be the x/gov module account. authority string - sessionKeeper types.SessionKeeper + sessionKeeper types.SessionKeeper + applicationKeeper types.ApplicationKeeper + accountKeeper types.AccountKeeper } ) @@ -32,6 +34,8 @@ func NewKeeper( authority string, sessionKeeper types.SessionKeeper, + applicationKeeper types.ApplicationKeeper, + accountKeeper types.AccountKeeper, ) Keeper { if _, err := sdk.AccAddressFromBech32(authority); err != nil { panic(fmt.Sprintf("invalid authority address: %s", authority)) @@ -43,7 +47,9 @@ func NewKeeper( authority: authority, logger: logger, - sessionKeeper: sessionKeeper, + sessionKeeper: sessionKeeper, + applicationKeeper: applicationKeeper, + accountKeeper: accountKeeper, } } diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 442098270..f2280afad 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -1,17 +1,40 @@ package keeper import ( + "bytes" "context" + "crypto/sha256" + ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" + ringtypes "github.com/athanorlabs/go-dleq/types" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" + cosmostypes "github.com/cosmos/cosmos-sdk/types" + ring "github.com/noot/ring-go" + "github.com/pokt-network/smt" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "github.com/pokt-network/poktroll/pkg/relayer/protocol" "github.com/pokt-network/poktroll/x/proof/types" + servicetypes "github.com/pokt-network/poktroll/x/service/types" + sessionkeeper "github.com/pokt-network/poktroll/x/session/keeper" + sessiontypes "github.com/pokt-network/poktroll/x/session/types" +) + +const ( + // TODO_TECHDEBT: relayDifficultyBits should be a governance-based parameter + relayDifficultyBits = 0 + + // sumSize is the size of the sum of the relay request and response + // in bytes. This is used to extract the relay request and response + // from the closest merkle proof. + // TODO_TECHDEBT: Have a method on the smst to extract the value hash or export + // sumSize to be used instead of current local value + sumSize = 8 ) func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) (*types.MsgSubmitProofResponse, error) { // TODO_BLOCKER: Prevent Proof upserts after the tokenomics module has processes the respective session. - // TODO_BLOCKER: Validate the signature on the Proof message corresponds to the supplier before Upserting. logger := k.Logger().With("method", "SubmitProof") logger.Debug("submitting proof") @@ -55,15 +78,68 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return nil, status.Error(codes.InvalidArgument, err.Error()) } + // Unmarshal the closest merkle proof from the message. + sparseMerkleClosestProof := &smt.SparseMerkleClosestProof{} + if err := sparseMerkleClosestProof.Unmarshal(msg.GetProof()); err != nil { + return nil, types.ErrProofInvalidProof.Wrapf( + "failed to unmarshal closest merkle proof: %s", + err, + ) + } + + // Get the relay request and response from the proof.GetClosestMerkleProof. + closestValueHash := sparseMerkleClosestProof.ClosestValueHash + relayBz := closestValueHash[:len(closestValueHash)-sumSize] + relay := &servicetypes.Relay{} + if err := k.cdc.Unmarshal(relayBz, relay); err != nil { + return nil, types.ErrProofInvalidRelay.Wrapf( + "failed to unmarshal relay: %s", + err, + ) + } + + // Verify the relay request and response session headers match the proof session header. + if err := validateRelaySessionHeaders(relay, msg.GetSessionHeader()); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Verify the relay response's signature. + supplierAddress := msg.GetSupplierAddress() + if err := k.verifyRelayResponseSignature(ctx, relay.GetRes(), supplierAddress); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Verify the relay request's signature. + appAddress := msg.GetSessionHeader().GetApplicationAddress() + if err := k.verifyRelayRequestSignature(ctx, relay.GetReq(), appAddress); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Validate that proof's path matches the earliest proof submission block hash. + if err := k.validateClosestPath(ctx, sparseMerkleClosestProof, msg.GetSessionHeader()); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Verify the relay's difficulty. + if err := validateMiningDifficulty(relayBz); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + claim, err := k.queryAndValidateClaimForProof(ctx, msg) + if err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Verify the proof's closest merkle proof. + if err := verifyClosestProof(sparseMerkleClosestProof, claim.GetRootHash()); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + // Construct and insert proof after all validation. proof := types.Proof{ SupplierAddress: msg.GetSupplierAddress(), SessionHeader: msg.GetSessionHeader(), - ClosestMerkleProof: msg.Proof, - } - - if err := k.queryAndValidateClaimForProof(ctx, &proof); err != nil { - return nil, status.Error(codes.FailedPrecondition, err.Error()) + ClosestMerkleProof: msg.GetProof(), } // TODO_BLOCKER: check if this proof already exists and return an appropriate error @@ -85,22 +161,28 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( // queryAndValidateClaimForProof ensures that a claim corresponding to the given proof's // session exists & has a matching supplier address and session header. -func (k msgServer) queryAndValidateClaimForProof(ctx context.Context, proof *types.Proof) error { - sessionId := proof.GetSessionHeader().GetSessionId() +func (k msgServer) queryAndValidateClaimForProof( + ctx context.Context, + msg *types.MsgSubmitProof, +) (*types.Claim, error) { + sessionId := msg.GetSessionHeader().GetSessionId() // 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. - foundClaim, found := k.GetClaim(ctx, sessionId, proof.GetSupplierAddress()) + foundClaim, found := k.GetClaim(ctx, sessionId, msg.GetSupplierAddress()) if !found { - return types.ErrProofClaimNotFound.Wrapf("no claim found for session ID %q and supplier %q", sessionId, proof.GetSupplierAddress()) + return nil, types.ErrProofClaimNotFound.Wrapf( + "no claim found for session ID %q and supplier %q", + sessionId, msg.GetSupplierAddress(), + ) } claimSessionHeader := foundClaim.GetSessionHeader() - proofSessionHeader := proof.GetSessionHeader() + proofSessionHeader := msg.GetSessionHeader() // Ensure session start heights match. if claimSessionHeader.GetSessionStartBlockHeight() != proofSessionHeader.GetSessionStartBlockHeight() { - return types.ErrProofInvalidSessionStartHeight.Wrapf( + return nil, types.ErrProofInvalidSessionStartHeight.Wrapf( "claim session start height %d does not match proof session start height %d", claimSessionHeader.GetSessionStartBlockHeight(), proofSessionHeader.GetSessionStartBlockHeight(), @@ -109,7 +191,7 @@ func (k msgServer) queryAndValidateClaimForProof(ctx context.Context, proof *typ // Ensure session end heights match. if claimSessionHeader.GetSessionEndBlockHeight() != proofSessionHeader.GetSessionEndBlockHeight() { - return types.ErrProofInvalidSessionEndHeight.Wrapf( + return nil, types.ErrProofInvalidSessionEndHeight.Wrapf( "claim session end height %d does not match proof session end height %d", claimSessionHeader.GetSessionEndBlockHeight(), proofSessionHeader.GetSessionEndBlockHeight(), @@ -118,7 +200,7 @@ func (k msgServer) queryAndValidateClaimForProof(ctx context.Context, proof *typ // Ensure application addresses match. if claimSessionHeader.GetApplicationAddress() != proofSessionHeader.GetApplicationAddress() { - return types.ErrProofInvalidAddress.Wrapf( + return nil, types.ErrProofInvalidAddress.Wrapf( "claim application address %q does not match proof application address %q", claimSessionHeader.GetApplicationAddress(), proofSessionHeader.GetApplicationAddress(), @@ -127,12 +209,354 @@ func (k msgServer) queryAndValidateClaimForProof(ctx context.Context, proof *typ // Ensure service IDs match. if claimSessionHeader.GetService().GetId() != proofSessionHeader.GetService().GetId() { - return types.ErrProofInvalidService.Wrapf( + return nil, types.ErrProofInvalidService.Wrapf( "claim service ID %q does not match proof service ID %q", claimSessionHeader.GetService().GetId(), proofSessionHeader.GetService().GetId(), ) } + return &foundClaim, nil +} + +// validateRelaySessionHeaders ensures that the relay request and response session headers +// match the proof session header. +func validateRelaySessionHeaders( + relay *servicetypes.Relay, + msgSessHeader *sessiontypes.SessionHeader, +) error { + reqSessHeader := relay.GetReq().GetMeta().GetSessionHeader() + respSessHeader := relay.GetRes().GetMeta().GetSessionHeader() + + // Ensure the relay request and response application addresses match + // the proof application address. + + if reqSessHeader.GetApplicationAddress() != msgSessHeader.GetApplicationAddress() { + return types.ErrProofInvalidRelay.Wrapf( + "relay request application address %s does not match proof application address %s", + reqSessHeader.GetApplicationAddress(), + msgSessHeader.GetApplicationAddress(), + ) + } + + if respSessHeader.GetApplicationAddress() != msgSessHeader.GetApplicationAddress() { + return types.ErrProofInvalidRelay.Wrapf( + "relay response application address %s does not match proof application address %s", + reqSessHeader.GetApplicationAddress(), + msgSessHeader.GetApplicationAddress(), + ) + } + + // Ensure the relay request and response service IDs match the proof service ID. + + if reqSessHeader.GetService().GetId() != msgSessHeader.GetService().GetId() { + return types.ErrProofInvalidRelay.Wrapf( + "relay request service ID %s does not match proof service ID %s", + reqSessHeader.GetService().GetId(), + msgSessHeader.GetService().GetId(), + ) + } + + if respSessHeader.GetService().GetId() != msgSessHeader.GetService().GetId() { + return types.ErrProofInvalidRelay.Wrapf( + "relay response service ID %s does not match proof service ID %s", + respSessHeader.GetService().GetId(), + msgSessHeader.GetService().GetId(), + ) + } + + // Ensure the relay request and response session start block heights + // match the proof session start block height. + + if reqSessHeader.GetSessionStartBlockHeight() != msgSessHeader.GetSessionStartBlockHeight() { + return types.ErrProofInvalidRelay.Wrapf( + "relay request session start height %d does not match proof session start height %d", + reqSessHeader.GetSessionStartBlockHeight(), + msgSessHeader.GetSessionStartBlockHeight(), + ) + } + + if respSessHeader.GetSessionStartBlockHeight() != msgSessHeader.GetSessionStartBlockHeight() { + return types.ErrProofInvalidRelay.Wrapf( + "relay response session start height %d does not match proof session start height %d", + respSessHeader.GetSessionStartBlockHeight(), + msgSessHeader.GetSessionStartBlockHeight(), + ) + } + + // Ensure the relay request and response session end block heights + // match the proof session end block height. + + if reqSessHeader.GetSessionEndBlockHeight() != msgSessHeader.GetSessionEndBlockHeight() { + return types.ErrProofInvalidRelay.Wrapf( + "relay request session end height %d does not match proof session end height %d", + reqSessHeader.GetSessionEndBlockHeight(), + msgSessHeader.GetSessionEndBlockHeight(), + ) + } + + if respSessHeader.GetSessionEndBlockHeight() != msgSessHeader.GetSessionEndBlockHeight() { + return types.ErrProofInvalidRelay.Wrapf( + "relay response session end height %d does not match proof session end height %d", + respSessHeader.GetSessionEndBlockHeight(), + msgSessHeader.GetSessionEndBlockHeight(), + ) + } + + // Ensure the relay request and response session IDs match the proof session ID. + + if reqSessHeader.GetSessionId() != msgSessHeader.GetSessionId() { + return types.ErrProofInvalidRelay.Wrapf( + "relay request session ID %s does not match proof session ID %s", + reqSessHeader.GetSessionId(), + msgSessHeader.GetSessionId(), + ) + } + + if respSessHeader.GetSessionId() != msgSessHeader.GetSessionId() { + return types.ErrProofInvalidRelay.Wrapf( + "relay response session ID %s does not match proof session ID %s", + respSessHeader.GetSessionId(), + msgSessHeader.GetSessionId(), + ) + } + + return nil +} + +// verifyRelayRequestSignature verifies the signature on the relay request. +// TODO_TECHDEBT: Factor out the relay request signature verification into a shared +// function that can be used by both the proof and relayer packages. +func (k msgServer) verifyRelayRequestSignature( + ctx context.Context, + relayRequest *servicetypes.RelayRequest, + appAddress string, +) error { + // Deserialize the ring signature from the relay request. + signature := relayRequest.GetMeta().GetSignature() + ringSig := new(ring.RingSig) + if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { + return types.ErrProofInvalidRelayRequest.Wrapf( + "error deserializing ring signature: %v", + err, + ) + } + + // Get the ring for the application address. + appRing, err := k.getRingForAppAddress(ctx, appAddress) + if err != nil { + return types.ErrProofInvalidRelayRequest.Wrapf( + "error getting ring for application address %s: %v", + appAddress, + err, + ) + } + + // Ensure the ring signature matches the ring for the application address. + if !ringSig.Ring().Equals(appRing) { + return types.ErrProofInvalidRelayRequest.Wrapf( + "ring signature does not match ring for application address %s", + appAddress, + ) + } + + // Get and hash the signable bytes of the relay request + requestSignableBz, err := relayRequest.GetSignableBytesHash() + if err != nil { + return types.ErrProofInvalidRelayRequest.Wrapf( + "error getting signable bytes: %v", + err, + ) + } + + // Verify the relay request's signature + if valid := ringSig.Verify(requestSignableBz); !valid { + return types.ErrProofInvalidRelayRequest.Wrap("invalid ring signature") + } + + return nil +} + +// getRingForAppAddress returns the RingSinger used to sign relays. It does so by fetching +// the latest information from the application module and creating the correct ring. +// This method also caches the ring's public keys for future use. +func (k msgServer) getRingForAppAddress( + ctx context.Context, + appAddress string, +) (*ring.Ring, error) { + foundApp, appFound := k.applicationKeeper.GetApplication(ctx, appAddress) + if !appFound { + return nil, types.ErrProofInvalidRelayRequest.Wrapf( + "application not found for address %s", + appAddress, + ) + } + + // Create a slice of addresses for the ring. + ringAddresses := make([]string, 0) + ringAddresses = append(ringAddresses, appAddress) // app address is index 0 + if len(foundApp.DelegateeGatewayAddresses) == 0 { + // add app address twice to make the ring size of mininmum 2 + // TODO_HACK: We are adding the appAddress twice because a ring + // signature requires AT LEAST two pubKeys. When the Application has + // not delegated to any gateways, we add the application's own address + // twice. This is a HACK and should be investigated as to what is the + // best approach to take in this situation. + ringAddresses = append(ringAddresses, appAddress) + } else { + // add the delegatee gateway addresses + ringAddresses = append(ringAddresses, foundApp.DelegateeGatewayAddresses...) + } + + // Get the points on the secp256k1 curve for the addresses. + points, err := k.addressesToPoints(ctx, ringAddresses) + if err != nil { + return nil, err + } + + // Create a new ring from points on the secp256k1 curve + return ring.NewFixedKeyRingFromPublicKeys(ring_secp256k1.NewCurve(), points) +} + +// addressesToPoints converts a slice of addresses to a slice of points on the +// secp256k1 curve, by querying the account module for the public key for each +// address and converting them to the corresponding points on the secp256k1 curve +func (k msgServer) addressesToPoints( + ctx context.Context, + addresses []string, +) ([]ringtypes.Point, error) { + curve := ring_secp256k1.NewCurve() + points := make([]ringtypes.Point, len(addresses)) + + for i, addr := range addresses { + // Retrieve the account from the auth module + accAddr, err := cosmostypes.AccAddressFromBech32(addr) + if err != nil { + return nil, err + } + + account := k.accountKeeper.GetAccount(ctx, accAddr) + + key := account.GetPubKey() + // Check if the key is a secp256k1 public key + if _, ok := key.(*secp256k1.PubKey); !ok { + return nil, types.ErrProofNotSecp256k1Curve.Wrapf("got %T", key) + } + // Convert the public key to the point on the secp256k1 curve + point, err := curve.DecodeToPoint(key.Bytes()) + if err != nil { + return nil, err + } + // Insert the point into the slice of points + points[i] = point + } + return points, nil +} + +// verifyRelayResponseSignature verifies the signature on the relay response. +// TODO_TECHDEBT: Factor out the relay response signature verification into a shared +// function that can be used by both the proof and the SDK packages. +func (k msgServer) verifyRelayResponseSignature( + ctx context.Context, + relayResponse *servicetypes.RelayResponse, + supplierAddress string, +) error { + // Get the account from the auth module + accAddr, err := cosmostypes.AccAddressFromBech32(supplierAddress) + if err != nil { + return err + } + + supplierAccount := k.accountKeeper.GetAccount(ctx, accAddr) + + // Get the public key from the account + pubKey := supplierAccount.GetPubKey() + if pubKey == nil { + return types.ErrProofInvalidRelayResponse.Wrapf( + "no public key found for supplier address %s", + supplierAddress, + ) + } + + supplierSignature := relayResponse.Meta.SupplierSignature + + // Get the relay response signable bytes and hash them. + responseSignableBz, err := relayResponse.GetSignableBytesHash() + if err != nil { + return err + } + + // Verify the relay response's signature + if valid := pubKey.VerifySignature(responseSignableBz[:], supplierSignature); !valid { + return types.ErrProofInvalidRelayResponse.Wrap("invalid relay response signature") + } + + return nil +} + +// verifyClosestProof verifies the closest merkle proof against the expected root hash. +func verifyClosestProof( + proof *smt.SparseMerkleClosestProof, + expectedRootHash []byte, +) error { + spec := smt.NoPrehashSpec(sha256.New(), true) + + valid, err := smt.VerifyClosestProof(proof, expectedRootHash, spec) + if err != nil { + return err + } + + if !valid { + return types.ErrProofInvalidProof.Wrap("invalid closest merkle proof") + } + + return nil +} + +// validateMiningDifficulty ensures that the relay's mining difficulty meets the required +// difficulty. +// TODO_TECHDEBT: Factor out the relay mining difficulty validation into a shared function +// that can be used by both the proof and the miner packages. +func validateMiningDifficulty(relayBz []byte) error { + hasher := sha256.New() + hasher.Write(relayBz) + realyHash := hasher.Sum(nil) + + difficultyBits, err := protocol.CountDifficultyBits(realyHash) + if err != nil { + return types.ErrProofInvalidRelay.Wrapf( + "error counting difficulty bits: %s", + err, + ) + } + + if difficultyBits < relayDifficultyBits { + return types.ErrProofInvalidRelay.Wrapf( + "relay difficulty %d is less than the required difficulty %d", + difficultyBits, + relayDifficultyBits, + ) + } + + return nil +} + +// validateClosestPath ensures that the proof's path matches the expected path. +func (k msgServer) validateClosestPath( + ctx context.Context, + proof *smt.SparseMerkleClosestProof, + sessionHeader *sessiontypes.SessionHeader, +) error { + blockHeight := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() + blockHash := k.sessionKeeper.GetBlockHash(ctx, blockHeight) + + if !bytes.Equal(proof.Path, blockHash) { + return types.ErrProofInvalidProof.Wrapf( + "proof path %x does not match block hash %x", + proof.Path, + blockHash, + ) + } + return nil } diff --git a/x/proof/module/module.go b/x/proof/module/module.go index 8eb8cacde..c18584733 100644 --- a/x/proof/module/module.go +++ b/x/proof/module/module.go @@ -177,9 +177,10 @@ type ModuleInputs struct { Config *modulev1.Module Logger log.Logger - AccountKeeper types.AccountKeeper - BankKeeper types.BankKeeper - SessionKeeper types.SessionKeeper + AccountKeeper types.AccountKeeper + BankKeeper types.BankKeeper + SessionKeeper types.SessionKeeper + ApplicationKeeper types.ApplicationKeeper } type ModuleOutputs struct { @@ -201,6 +202,8 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { in.Logger, authority.String(), in.SessionKeeper, + in.ApplicationKeeper, + in.AccountKeeper, ) m := NewAppModule( in.Cdc, diff --git a/x/proof/types/errors.go b/x/proof/types/errors.go index 6f686ed68..bc8e45db7 100644 --- a/x/proof/types/errors.go +++ b/x/proof/types/errors.go @@ -18,7 +18,11 @@ var ( ErrProofClaimNotFound = sdkerrors.Register(ModuleName, 1109, "claim not found") ErrProofProofNotFound = sdkerrors.Register(ModuleName, 1110, "proof not found") ErrProofInvalidProof = sdkerrors.Register(ModuleName, 1111, "invalid proof") - //ErrProofUnauthorized = sdkerrors.Register(ModuleName, 1112, "unauthorized supplier signer") - //ErrProofInvalidServiceConfig = sdkerrors.Register(ModuleName, 1113, "invalid service config") - //ErrProofInvalidClosestMerkleProof = sdkerrors.Register(ModuleName, 1114, "invalid closest merkle proof") + ErrProofInvalidRelay = sdkerrors.Register(ModuleName, 1112, "invalid relay") + ErrProofInvalidRelayRequest = sdkerrors.Register(ModuleName, 1113, "invalid relay request") + ErrProofInvalidRelayResponse = sdkerrors.Register(ModuleName, 1114, "invalid relay response") + ErrProofNotSecp256k1Curve = sdkerrors.Register(ModuleName, 1115, "not secp256k1 curve") + //ErrProofUnauthorized = sdkerrors.Register(ModuleName, 1116, "unauthorized supplier signer") + //ErrProofInvalidServiceConfig = sdkerrors.Register(ModuleName, 1117, "invalid service config") + //ErrProofInvalidClosestMerkleProof = sdkerrors.Register(ModuleName, 1118, "invalid closest merkle proof") ) diff --git a/x/proof/types/expected_keepers.go b/x/proof/types/expected_keepers.go index a9d8eaf71..ec62d3069 100644 --- a/x/proof/types/expected_keepers.go +++ b/x/proof/types/expected_keepers.go @@ -7,17 +7,18 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" + apptypes "github.com/pokt-network/poktroll/x/application/types" sessiontypes "github.com/pokt-network/poktroll/x/session/types" ) type SessionKeeper interface { GetSession(context.Context, *sessiontypes.QueryGetSessionRequest) (*sessiontypes.QueryGetSessionResponse, error) + GetBlockHash(ctx context.Context, height int64) []byte } // AccountKeeper defines the expected interface for the Account module. type AccountKeeper interface { - GetAccount(context.Context, sdk.AccAddress) sdk.AccountI // only used for simulation - // Methods imported from account should be defined here + GetAccount(context.Context, sdk.AccAddress) sdk.AccountI } // BankKeeper defines the expected interface for the Bank module. @@ -25,3 +26,8 @@ type BankKeeper interface { SpendableCoins(context.Context, sdk.AccAddress) sdk.Coins // Methods imported from bank should be defined here } + +// ApplicationKeeper defines the expected application keeper to retrieve applications +type ApplicationKeeper interface { + GetApplication(ctx context.Context, address string) (app apptypes.Application, found bool) +} diff --git a/x/service/types/relay.go b/x/service/types/relay.go index 7815fc1ab..96c846001 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -15,7 +15,12 @@ func (req RelayRequest) getSignableBytes() ([]byte, error) { // Hashing the marshaled request message guarantees that the signable bytes are // always of a constant and expected length. func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { + signature := req.Meta.Signature requestBz, err := req.getSignableBytes() + + // Set the signature back to its original value + req.Meta.Signature = signature + if err != nil { return [32]byte{}, err } @@ -38,7 +43,12 @@ func (res RelayResponse) getSignableBytes() ([]byte, error) { // Hashing the marshaled response message guarantees that the signable bytes are // always of a constant and expected length. func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { + signature := res.Meta.SupplierSignature responseBz, err := res.getSignableBytes() + + // Set the signature back to its original value + res.Meta.SupplierSignature = signature + if err != nil { return [32]byte{}, err } From 6239e744d9c376a876d8eafa6fdb6533eebafab4 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Tue, 5 Mar 2024 16:19:02 +0100 Subject: [PATCH 02/23] [Proof] refactor: ring client for on-chain use (#411) Co-authored-by: Redouane Lakrache --- pkg/crypto/interface.go | 28 ++- pkg/crypto/rings/cache.go | 209 ++++++---------------- pkg/crypto/rings/client.go | 205 +++++++++++++++++++++ pkg/crypto/rings/errors.go | 7 +- pkg/crypto/rings/ring.go | 40 +++++ pkg/relayer/proxy/errors.go | 18 +- pkg/relayer/proxy/relay_verifier.go | 84 ++------- testutil/keeper/proof.go | 3 +- x/proof/keeper/keeper.go | 41 ++++- x/proof/keeper/msg_server_submit_proof.go | 136 +------------- x/proof/keeper/option.go | 14 ++ x/proof/module/module.go | 6 +- x/proof/types/account_query_client.go | 32 ++++ x/proof/types/application_query_client.go | 32 ++++ x/proof/types/expected_keepers.go | 1 + 15 files changed, 467 insertions(+), 389 deletions(-) create mode 100644 pkg/crypto/rings/client.go create mode 100644 pkg/crypto/rings/ring.go create mode 100644 x/proof/keeper/option.go create mode 100644 x/proof/types/account_query_client.go create mode 100644 x/proof/types/application_query_client.go diff --git a/pkg/crypto/interface.go b/pkg/crypto/interface.go index 4298a9b6a..cfdd0574f 100644 --- a/pkg/crypto/interface.go +++ b/pkg/crypto/interface.go @@ -5,6 +5,8 @@ import ( "context" "github.com/noot/ring-go" + + "github.com/pokt-network/poktroll/x/service/types" ) // RingCache is used to store rings used for signing and verifying relay requests. @@ -12,19 +14,29 @@ import ( // the addresses of the gateways the application is delegated to, and converting // them into their corresponding public key points on the secp256k1 curve. type RingCache interface { + RingClient + + // GetCachedAddresses returns the addresses of the applications that are + // currently cached in the ring cache. + GetCachedAddresses() []string // Start starts the ring cache, it takes a cancellable context and, in a // separate goroutine, listens for on-chain delegation events and invalidates // the cache if the redelegation event's AppAddress is stored in the cache. Start(ctx context.Context) - // GetCachedAddresses returns the addresses of the applications that are - // currently cached in the ring cache. - GetCachedAddresses() []string - // GetRingForAddress returns the ring for the given application address if - // it exists. If it does not exist in the cache, it follows a lazy approach - // of querying the on-chain state and creating it just-in-time, caching for - // future retrievals. - GetRingForAddress(ctx context.Context, appAddress string) (*ring.Ring, error) // Stop stops the ring cache by unsubscribing from on-chain delegation events. // And clears the cache, so that it no longer contains any rings, Stop() } + +// RingClient is used to construct rings by querying the application module for +// the addresses of the gateways the application is delegated to, and converting +// them into their corresponding public key points on the secp256k1 curve. +type RingClient interface { + // GetRingForAddress returns the ring for the given application address if + // it exists. + GetRingForAddress(ctx context.Context, appAddress string) (*ring.Ring, error) + + // VerifyRelayRequestSignature verifies the relay request signature against the + // ring for the application address in the relay request. + VerifyRelayRequestSignature(ctx context.Context, relayRequest *types.RelayRequest) error +} diff --git a/pkg/crypto/rings/cache.go b/pkg/crypto/rings/cache.go index e1a21ed50..a3c66ad9d 100644 --- a/pkg/crypto/rings/cache.go +++ b/pkg/crypto/rings/cache.go @@ -2,19 +2,16 @@ package rings import ( "context" - "fmt" "sync" "cosmossdk.io/depinject" - ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" - ringtypes "github.com/athanorlabs/go-dleq/types" - "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" "github.com/noot/ring-go" "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" "github.com/pokt-network/poktroll/pkg/observable/channel" "github.com/pokt-network/poktroll/pkg/polylog" + "github.com/pokt-network/poktroll/x/service/types" ) var _ crypto.RingCache = (*ringCache)(nil) @@ -23,23 +20,16 @@ type ringCache struct { // logger is the logger for the ring cache. logger polylog.Logger - // ringPointsCache maintains a map of application addresses to the points - // on the secp256k1 curve that correspond to the public keys of the gateways - // the application is delegated to. These are used to build the app's ring. - ringPointsCache map[string][]ringtypes.Point - ringPointsMu *sync.RWMutex + // ringsByAddr maintains a map of application addresses to the ring composed of + // the public keys of the application and the gateways the application is delegated to. + ringsByAddr map[string]*ring.Ring + ringsByAddrMu *sync.RWMutex // delegationClient is used to listen for on-chain delegation events and - // invalidate cache entries for rings that have been updated on chain. + // invalidate ringsByAddr entries for rings that have been updated on chain. delegationClient client.DelegationClient - // applicationQuerier is the querier for the application module, and is - // used to get the addresses of the gateways an application is delegated to. - applicationQuerier client.ApplicationQueryClient - - // accountQuerier is the querier for the account module, and is used to get - // the public keys of the application and its delegated gateways. - accountQuerier client.AccountQueryClient + ringClient crypto.RingClient } // NewRingCache returns a new RingCache instance. It requires a depinject.Config @@ -50,10 +40,10 @@ type ringCache struct { // - client.DelegationClient // - client.ApplicationQueryClient // - client.AccountQueryClient -func NewRingCache(deps depinject.Config) (crypto.RingCache, error) { +func NewRingCache(deps depinject.Config) (_ crypto.RingCache, err error) { rc := &ringCache{ - ringPointsCache: make(map[string][]ringtypes.Point), - ringPointsMu: &sync.RWMutex{}, + ringsByAddr: make(map[string]*ring.Ring), + ringsByAddrMu: &sync.RWMutex{}, } // Supply the account and application queriers to the RingCache. @@ -61,20 +51,24 @@ func NewRingCache(deps depinject.Config) (crypto.RingCache, error) { deps, &rc.logger, &rc.delegationClient, - &rc.applicationQuerier, - &rc.accountQuerier, ); err != nil { return nil, err } + // Construct and assign underlying ring client. + rc.ringClient, err = NewRingClient(deps) + if err != nil { + return nil, err + } + return rc, nil } // Start starts the ring cache by subscribing to on-chain redelegation events. func (rc *ringCache) Start(ctx context.Context) { - rc.logger.Info().Msg("starting ring cache") - // Listen for redelegation events and invalidate the cache if the - // redelegation event's address is stored in the cache. + rc.logger.Info().Msg("starting ring ringsByAddr") + // Listen for redelegation events and invalidate the cache if contains a ring + // corresponding to the redelegation event's address . go func() { select { case <-ctx.Done(): @@ -86,7 +80,8 @@ func (rc *ringCache) Start(ctx context.Context) { } // goInvalidateCache listens for redelegation events and invalidates the -// cache if the app address in the redelegation event is stored in the cache. +// cache if ring corresponding to the app address in the redelegation event +// exists in the cache. // This function is intended to be run in a goroutine. func (rc *ringCache) goInvalidateCache(ctx context.Context) { // Get the latest redelegation replay observable. @@ -96,16 +91,16 @@ func (rc *ringCache) goInvalidateCache(ctx context.Context) { channel.ForEach[client.Redelegation]( ctx, redelegationObs, func(ctx context.Context, redelegation client.Redelegation) { - // Lock the cache for writing. - rc.ringPointsMu.Lock() - defer rc.ringPointsMu.Unlock() + // Lock ringsByAddr for writing. + rc.ringsByAddrMu.Lock() + defer rc.ringsByAddrMu.Unlock() // Check if the redelegation event's app address is in the cache. - if _, ok := rc.ringPointsCache[redelegation.GetAppAddress()]; ok { + if _, ok := rc.ringsByAddr[redelegation.GetAppAddress()]; ok { rc.logger.Debug(). Str("app_address", redelegation.GetAppAddress()). - Msg("redelegation event received; invalidating cache entry") - // Invalidate the cache entry. - delete(rc.ringPointsCache, redelegation.GetAppAddress()) + Msg("redelegation event received; invalidating ringsByAddr entry") + // Invalidate the ringsByAddr entry. + delete(rc.ringsByAddr, redelegation.GetAppAddress()) } }) } @@ -113,9 +108,9 @@ func (rc *ringCache) goInvalidateCache(ctx context.Context) { // Stop stops the ring cache by unsubscribing from on-chain redelegation events. func (rc *ringCache) Stop() { // Clear the cache. - rc.ringPointsMu.Lock() - rc.ringPointsCache = make(map[string][]ringtypes.Point) - rc.ringPointsMu.Unlock() + rc.ringsByAddrMu.Lock() + rc.ringsByAddr = make(map[string]*ring.Ring) + rc.ringsByAddrMu.Unlock() // Close the delegation client. rc.delegationClient.Close() } @@ -123,10 +118,10 @@ func (rc *ringCache) Stop() { // GetCachedAddresses returns the addresses of the applications that are // currently cached in the ring cache. func (rc *ringCache) GetCachedAddresses() []string { - rc.ringPointsMu.RLock() - defer rc.ringPointsMu.RUnlock() - keys := make([]string, 0, len(rc.ringPointsCache)) - for k := range rc.ringPointsCache { + rc.ringsByAddrMu.RLock() + defer rc.ringsByAddrMu.RUnlock() + keys := make([]string, 0, len(rc.ringsByAddr)) + for k := range rc.ringsByAddr { keys = append(keys, k) } return keys @@ -139,31 +134,28 @@ func (rc *ringCache) GetCachedAddresses() []string { func (rc *ringCache) GetRingForAddress( ctx context.Context, appAddress string, -) (*ring.Ring, error) { - var ( - ring *ring.Ring - err error - ) +) (ring *ring.Ring, err error) { + // Lock the ringsByAddr map. + rc.ringsByAddrMu.Lock() + defer rc.ringsByAddrMu.Unlock() - // Lock the cache for reading. - rc.ringPointsMu.RLock() // Check if the ring is in the cache. - points, ok := rc.ringPointsCache[appAddress] - // Unlock the cache in case it was not cached. - rc.ringPointsMu.RUnlock() + ring, ok := rc.ringsByAddr[appAddress] if !ok { - // If the ring is not in the cache, get it from the application module. + // If the ring is not in the cache, get it from the ring client. rc.logger.Debug(). Str("app_address", appAddress). - Msg("ring cache miss; fetching from application module") - ring, err = rc.getRingForAppAddress(ctx, appAddress) + Msg("ring ringsByAddr miss; fetching from application module") + ring, err = rc.ringClient.GetRingForAddress(ctx, appAddress) + + // Add the address points to the cache. + rc.ringsByAddr[appAddress] = ring } else { // If the ring is in the cache, create it from the points. rc.logger.Debug(). Str("app_address", appAddress). - Msg("ring cache hit; creating from points") - ring, err = newRingFromPoints(points) + Msg("ring ringsByAddr hit; creating from points") } if err != nil { return nil, err @@ -173,106 +165,11 @@ func (rc *ringCache) GetRingForAddress( return ring, nil } -// getRingForAppAddress returns the RingSinger used to sign relays. It does so by fetching -// the latest information from the application module and creating the correct ring. -// This method also caches the ring's public keys for future use. -func (rc *ringCache) getRingForAppAddress( - ctx context.Context, - appAddress string, -) (*ring.Ring, error) { - points, err := rc.getDelegatedPubKeysForAddress(ctx, appAddress) - if err != nil { - return nil, err - } - // Cache the ring's points for future use - rc.logger.Debug(). - Str("app_address", appAddress). - Msg("updating ring cache for app") - rc.ringPointsMu.Lock() - defer rc.ringPointsMu.Unlock() - rc.ringPointsCache[appAddress] = points - return newRingFromPoints(points) -} - -// newRingFromPoints creates a new ring from points on the secp256k1 curve -func newRingFromPoints(points []ringtypes.Point) (*ring.Ring, error) { - return ring.NewFixedKeyRingFromPublicKeys(ring_secp256k1.NewCurve(), points) -} - -// getDelegatedPubKeysForAddress returns the ring used to sign a message for -// the given application address, by querying the application module for it's -// delegated pubkeys and converting them to points on the secp256k1 curve in -// order to create the ring. -func (rc *ringCache) getDelegatedPubKeysForAddress( +// VerifyRelayRequestSignature verifies the relay request signature against the +// ring for the application address in the relay request. +func (rc *ringCache) VerifyRelayRequestSignature( ctx context.Context, - appAddress string, -) ([]ringtypes.Point, error) { - rc.ringPointsMu.Lock() - defer rc.ringPointsMu.Unlock() - - // Get the application's on chain state. - app, err := rc.applicationQuerier.GetApplication(ctx, appAddress) - if err != nil { - return nil, err - } - - // Create a slice of addresses for the ring. - ringAddresses := make([]string, 0) - ringAddresses = append(ringAddresses, appAddress) // app address is index 0 - if len(app.DelegateeGatewayAddresses) == 0 { - // add app address twice to make the ring size of mininmum 2 - // TODO_HACK: We are adding the appAddress twice because a ring - // signature requires AT LEAST two pubKeys. When the Application has - // not delegated to any gateways, we add the application's own address - // twice. This is a HACK and should be investigated as to what is the - // best approach to take in this situation. - ringAddresses = append(ringAddresses, appAddress) - } else { - // add the delegatee gateway addresses - ringAddresses = append(ringAddresses, app.DelegateeGatewayAddresses...) - } - - // Get the points on the secp256k1 curve for the addresses. - points, err := rc.addressesToPoints(ctx, ringAddresses) - if err != nil { - return nil, err - } - - // Return the public key points on the secp256k1 curve. - return points, nil -} - -// addressesToPoints converts a slice of addresses to a slice of points on the -// secp256k1 curve, by querying the account module for the public key for each -// address and converting them to the corresponding points on the secp256k1 curve -func (rc *ringCache) addressesToPoints( - ctx context.Context, - addresses []string, -) ([]ringtypes.Point, error) { - curve := ring_secp256k1.NewCurve() - points := make([]ringtypes.Point, len(addresses)) - rc.logger.Debug(). - // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` instead of formatting here. - Str("addresses", fmt.Sprintf("%v", addresses)). - Msg("converting addresses to points") - for i, addr := range addresses { - // Retrieve the account from the auth module - acc, err := rc.accountQuerier.GetAccount(ctx, addr) - if err != nil { - return nil, err - } - key := acc.GetPubKey() - // Check if the key is a secp256k1 public key - if _, ok := key.(*secp256k1.PubKey); !ok { - return nil, ErrRingsNotSecp256k1Curve.Wrapf("got %T", key) - } - // Convert the public key to the point on the secp256k1 curve - point, err := curve.DecodeToPoint(key.Bytes()) - if err != nil { - return nil, err - } - // Insert the point into the slice of points - points[i] = point - } - return points, nil + relayRequest *types.RelayRequest, +) error { + return rc.ringClient.VerifyRelayRequestSignature(ctx, relayRequest) } diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go new file mode 100644 index 000000000..6942bc29b --- /dev/null +++ b/pkg/crypto/rings/client.go @@ -0,0 +1,205 @@ +package rings + +import ( + "context" + "fmt" + + "cosmossdk.io/depinject" + ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" + ringtypes "github.com/athanorlabs/go-dleq/types" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/noot/ring-go" + + "github.com/pokt-network/poktroll/pkg/client" + "github.com/pokt-network/poktroll/pkg/crypto" + "github.com/pokt-network/poktroll/pkg/polylog" + "github.com/pokt-network/poktroll/x/service/types" +) + +var _ crypto.RingClient = (*ringClient)(nil) + +type ringClient struct { + // logger is the logger for the ring cache. + logger polylog.Logger + + // applicationQuerier is the querier for the application module, and is + // used to get the addresses of the gateways an application is delegated to. + applicationQuerier client.ApplicationQueryClient + + // accountQuerier is the querier for the account module, and is used to get + // the public keys of the application and its delegated gateways. + accountQuerier client.AccountQueryClient +} + +// NewRingClient returns a new ring client constructed from the given dependencies. +// It returns an error if the required dependencies are not supplied. +// +// Required dependencies: +// - polylog.Logger +// - client.ApplicationQueryClient +// - client.AccountQueryClient +func NewRingClient(deps depinject.Config) (crypto.RingClient, error) { + rc := new(ringClient) + + if err := depinject.Inject( + deps, + &rc.logger, + &rc.applicationQuerier, + &rc.accountQuerier, + ); err != nil { + return nil, err + } + + return rc, nil +} + +// GetRingForAddress returns the ring for the address provided. The ring is created by +// querying for the application address and delegated gateways' account public keys and +// converting them to their secp256k1 curve points. +func (rc *ringClient) GetRingForAddress( + ctx context.Context, + appAddress string, +) (*ring.Ring, error) { + points, err := rc.getDelegatedPubKeysForAddress(ctx, appAddress) + if err != nil { + return nil, err + } + // Cache the ring's points for future use + rc.logger.Debug(). + Str("app_address", appAddress). + Msg("updating ring ringsByAddr for app") + return newRingFromPoints(points) +} + +// VerifyRelayRequestSignature verifies the relay request signature against the +// ring for the application address in the relay request. +func (rc *ringClient) VerifyRelayRequestSignature( + ctx context.Context, + relayRequest *types.RelayRequest, +) error { + rc.logger.Debug(). + Fields(map[string]any{ + "session_id": relayRequest.Meta.SessionHeader.SessionId, + "application_address": relayRequest.Meta.SessionHeader.ApplicationAddress, + "service_id": relayRequest.Meta.SessionHeader.Service.Id, + }). + Msg("verifying relay request signature") + + // Extract the relay request's ring signature + if relayRequest.Meta == nil { + return ErrRingClientEmptyRelayRequestSignature.Wrapf( + "request payload: %s", relayRequest.Payload, + ) + } + + signature := relayRequest.Meta.Signature + if signature == nil { + return ErrRingClientInvalidRelayRequest.Wrapf( + "missing signature from relay request: %v", relayRequest, + ) + } + + ringSig := new(ring.RingSig) + if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { + return ErrRingClientInvalidRelayRequestSignature.Wrapf( + "error deserializing ring signature: %v", err, + ) + } + + if relayRequest.Meta.SessionHeader.ApplicationAddress == "" { + return ErrRingClientInvalidRelayRequest.Wrap( + "missing application address from relay request", + ) + } + + // Get the ring for the application address of the relay request + appAddress := relayRequest.Meta.SessionHeader.ApplicationAddress + appRing, err := rc.GetRingForAddress(ctx, appAddress) + if err != nil { + return ErrRingClientInvalidRelayRequest.Wrapf( + "error getting ring for application address %s: %v", appAddress, err, + ) + } + + // Verify the ring signature against the ring + if !ringSig.Ring().Equals(appRing) { + return ErrRingClientInvalidRelayRequestSignature.Wrapf( + "ring signature does not match ring for application address %s", appAddress, + ) + } + + // Get and hash the signable bytes of the relay request + requestSignableBz, err := relayRequest.GetSignableBytesHash() + if err != nil { + return ErrRingClientInvalidRelayRequest.Wrapf("error getting signable bytes: %v", err) + } + + // Verify the relay request's signature + if valid := ringSig.Verify(requestSignableBz); !valid { + return ErrRingClientInvalidRelayRequestSignature.Wrapf("invalid ring signature") + } + return nil +} + +// getDelegatedPubKeysForAddress returns the ring used to sign a message for +// the given application address, by querying the application module for it's +// delegated pubkeys and converting them to points on the secp256k1 curve in +// order to create the ring. +func (rc *ringClient) getDelegatedPubKeysForAddress( + ctx context.Context, + appAddress string, +) ([]ringtypes.Point, error) { + // Get the application's on chain state. + app, err := rc.applicationQuerier.GetApplication(ctx, appAddress) + if err != nil { + return nil, err + } + + // Create a slice of addresses for the ring. + ringAddresses := make([]string, 0) + ringAddresses = append(ringAddresses, appAddress) // app address is index 0 + if len(app.DelegateeGatewayAddresses) == 0 { + // add app address twice to make the ring size of mininmum 2 + // TODO_HACK: We are adding the appAddress twice because a ring + // signature requires AT LEAST two pubKeys. When the Application has + // not delegated to any gateways, we add the application's own address + // twice. This is a HACK and should be investigated as to what is the + // best approach to take in this situation. + ringAddresses = append(ringAddresses, appAddress) + } else { + // add the delegatee gateway addresses + ringAddresses = append(ringAddresses, app.DelegateeGatewayAddresses...) + } + + // Get the points on the secp256k1 curve for the addresses. + points, err := rc.addressesToPoints(ctx, ringAddresses) + if err != nil { + return nil, err + } + + // Return the public key points on the secp256k1 curve. + return points, nil +} + +// addressesToPoints converts a slice of addresses to a slice of points on the +// secp256k1 curve, by querying the account module for the public key for each +// address and converting them to the corresponding points on the secp256k1 curve +func (rc *ringClient) addressesToPoints( + ctx context.Context, + addresses []string, +) ([]ringtypes.Point, error) { + publicKeys := make([]cryptotypes.PubKey, len(addresses)) + rc.logger.Debug(). + // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` instead of formatting here. + Str("addresses", fmt.Sprintf("%v", addresses)). + Msg("converting addresses to points") + for i, addr := range addresses { + acc, err := rc.accountQuerier.GetAccount(ctx, addr) + if err != nil { + return nil, err + } + publicKeys[i] = acc.GetPubKey() + } + + return pointsFromPublicKeys(publicKeys...) +} diff --git a/pkg/crypto/rings/errors.go b/pkg/crypto/rings/errors.go index cd85b519b..588afeee7 100644 --- a/pkg/crypto/rings/errors.go +++ b/pkg/crypto/rings/errors.go @@ -3,6 +3,9 @@ package rings import sdkerrors "cosmossdk.io/errors" var ( - codespace = "rings" - ErrRingsNotSecp256k1Curve = sdkerrors.Register(codespace, 1, "key is not a secp256k1 public key") + codespace = "rings" + ErrRingsNotSecp256k1Curve = sdkerrors.Register(codespace, 1, "key is not a secp256k1 public key") + ErrRingClientEmptyRelayRequestSignature = sdkerrors.Register(codespace, 2, "empty relay request signature") + ErrRingClientInvalidRelayRequest = sdkerrors.Register(codespace, 3, "invalid relay request") + ErrRingClientInvalidRelayRequestSignature = sdkerrors.Register(codespace, 4, "invalid relay request signature") ) diff --git a/pkg/crypto/rings/ring.go b/pkg/crypto/rings/ring.go new file mode 100644 index 000000000..35d023101 --- /dev/null +++ b/pkg/crypto/rings/ring.go @@ -0,0 +1,40 @@ +package rings + +import ( + ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" + ringtypes "github.com/athanorlabs/go-dleq/types" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/noot/ring-go" +) + +// newRingFromPoints creates a new ring from points on the secp256k1 curve +func newRingFromPoints(points []ringtypes.Point) (*ring.Ring, error) { + return ring.NewFixedKeyRingFromPublicKeys(ring_secp256k1.NewCurve(), points) +} + +// pointsFromPublicKeys returns the secp256k1 points for the given public keys. +// It returns an errof if any of the keys is not a secp256k1 key. +func pointsFromPublicKeys( + publicKeys ...cryptotypes.PubKey, +) (points []ringtypes.Point, err error) { + curve := ring_secp256k1.NewCurve() + + for _, key := range publicKeys { + // Check if the key is a secp256k1 public key + if _, ok := key.(*secp256k1.PubKey); !ok { + return nil, ErrRingsNotSecp256k1Curve.Wrapf("got %T", key) + } + + // Convert the public key to the point on the secp256k1 curve + point, err := curve.DecodeToPoint(key.Bytes()) + if err != nil { + return nil, err + } + + // Insert the point into the slice of points + points = append(points, point) + } + + return points, nil +} diff --git a/pkg/relayer/proxy/errors.go b/pkg/relayer/proxy/errors.go index 5a4d81af7..9650ee577 100644 --- a/pkg/relayer/proxy/errors.go +++ b/pkg/relayer/proxy/errors.go @@ -5,14 +5,12 @@ import sdkerrors "cosmossdk.io/errors" var ( codespace = "relayer_proxy" ErrRelayerProxyUnsupportedRPCType = sdkerrors.Register(codespace, 1, "unsupported relayer proxy rpc type") - ErrRelayerProxyInvalidRelayRequestSignature = sdkerrors.Register(codespace, 2, "invalid relay request signature") - ErrRelayerProxyInvalidSession = sdkerrors.Register(codespace, 3, "invalid session in relayer request") - ErrRelayerProxyInvalidSupplier = sdkerrors.Register(codespace, 4, "invalid relayer proxy supplier") - ErrRelayerProxyUndefinedSigningKeyName = sdkerrors.Register(codespace, 5, "undefined relayer proxy signing key name") - ErrRelayerProxyUndefinedProxiedServicesEndpoints = sdkerrors.Register(codespace, 6, "undefined proxied services endpoints for relayer proxy") - ErrRelayerProxyInvalidRelayRequest = sdkerrors.Register(codespace, 7, "invalid relay request") - ErrRelayerProxyInvalidRelayResponse = sdkerrors.Register(codespace, 8, "invalid relay response") - ErrRelayerProxyEmptyRelayRequestSignature = sdkerrors.Register(codespace, 9, "empty relay response signature") - ErrRelayerProxyServiceEndpointNotHandled = sdkerrors.Register(codespace, 10, "service endpoint not handled by relayer proxy") - ErrRelayerProxyUnsupportedTransportType = sdkerrors.Register(codespace, 11, "unsupported proxy transport type") + ErrRelayerProxyInvalidSession = sdkerrors.Register(codespace, 2, "invalid session in relayer request") + ErrRelayerProxyInvalidSupplier = sdkerrors.Register(codespace, 3, "invalid relayer proxy supplier") + ErrRelayerProxyUndefinedSigningKeyName = sdkerrors.Register(codespace, 4, "undefined relayer proxy signing key name") + ErrRelayerProxyUndefinedProxiedServicesEndpoints = sdkerrors.Register(codespace, 5, "undefined proxied services endpoints for relayer proxy") + ErrRelayerProxyInvalidRelayRequest = sdkerrors.Register(codespace, 6, "invalid relay request") + ErrRelayerProxyInvalidRelayResponse = sdkerrors.Register(codespace, 7, "invalid relay response") + ErrRelayerProxyServiceEndpointNotHandled = sdkerrors.Register(codespace, 8, "service endpoint not handled by relayer proxy") + ErrRelayerProxyUnsupportedTransportType = sdkerrors.Register(codespace, 9, "unsupported proxy transport type") ) diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index 29924ee13..e347c9ee5 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -3,10 +3,6 @@ package proxy import ( "context" - sdkerrors "cosmossdk.io/errors" - ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" - ring "github.com/noot/ring-go" - sessiontypes "github.com/pokt-network/poktroll/pkg/relayer/session" "github.com/pokt-network/poktroll/x/service/types" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" @@ -18,81 +14,21 @@ func (rp *relayerProxy) VerifyRelayRequest( relayRequest *types.RelayRequest, service *sharedtypes.Service, ) error { - rp.logger.Debug(). - Fields(map[string]any{ - "session_id": relayRequest.Meta.SessionHeader.SessionId, - "application_address": relayRequest.Meta.SessionHeader.ApplicationAddress, - "service_id": relayRequest.Meta.SessionHeader.Service.Id, - }). - Msg("verifying relay request signature") - - // extract the relay request's ring signature - if relayRequest.Meta == nil { - return ErrRelayerProxyEmptyRelayRequestSignature.Wrapf( - "request payload: %s", relayRequest.Payload, - ) - } - signature := relayRequest.Meta.Signature - if signature == nil { - return sdkerrors.Wrapf( - ErrRelayerProxyInvalidRelayRequest, - "missing signature from relay request: %v", relayRequest, - ) - } - - ringSig := new(ring.RingSig) - if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { - return sdkerrors.Wrapf( - ErrRelayerProxyInvalidRelayRequestSignature, - "error deserializing ring signature: %v", err, - ) - } - - if relayRequest.Meta.SessionHeader.ApplicationAddress == "" { - return sdkerrors.Wrap( - ErrRelayerProxyInvalidRelayRequest, - "missing application address from relay request", - ) - } - - // get the ring for the application address of the relay request - appAddress := relayRequest.Meta.SessionHeader.ApplicationAddress - appRing, err := rp.ringCache.GetRingForAddress(ctx, appAddress) - if err != nil { - return sdkerrors.Wrapf( - ErrRelayerProxyInvalidRelayRequest, - "error getting ring for application address %s: %v", appAddress, err, - ) - } - - // verify the ring signature against the ring - if !ringSig.Ring().Equals(appRing) { - return sdkerrors.Wrapf( - ErrRelayerProxyInvalidRelayRequestSignature, - "ring signature does not match ring for application address %s", appAddress, - ) - } - - // get and hash the signable bytes of the relay request - requestSignableBz, err := relayRequest.GetSignableBytesHash() - if err != nil { - return ErrRelayerProxyInvalidRelayRequest.Wrapf("error getting signable bytes: %v", err) + if err := rp.ringCache.VerifyRelayRequestSignature(ctx, relayRequest); err != nil { + return err } - // verify the relay request's signature - if valid := ringSig.Verify(requestSignableBz); !valid { - return sdkerrors.Wrapf( - ErrRelayerProxyInvalidRelayRequestSignature, - "invalid ring signature", - ) - } + // Application address is used to verify the relayRequest signature, it is + // guaranteed to be present in the relayRequest since the signature has already + // been verified. + appAddress := relayRequest.GetMeta().GetSessionHeader().GetApplicationAddress() // Query for the current session to check if relayRequest sessionId matches the current session. rp.logger.Debug(). Fields(map[string]any{ - "session_id": relayRequest.Meta.SessionHeader.SessionId, - "application_address": relayRequest.Meta.SessionHeader.ApplicationAddress, - "service_id": relayRequest.Meta.SessionHeader.Service.Id, + "session_id": relayRequest.GetMeta().GetSessionHeader().GetSessionId(), + "application_address": relayRequest.GetMeta().GetSessionHeader().GetApplicationAddress(), + "service_id": relayRequest.GetMeta().GetSessionHeader().GetService().GetId(), }). Msg("verifying relay request session") @@ -119,7 +55,7 @@ func (rp *relayerProxy) VerifyRelayRequest( // we can reduce the session validity check to checking if the retrieved session's sessionId // matches the relayRequest sessionId. // TODO_INVESTIGATE: Revisit the assumptions above at some point in the future, but good enough for now. - if session.SessionId != relayRequest.Meta.SessionHeader.SessionId { + if session.SessionId != relayRequest.GetMeta().GetSessionHeader().GetSessionId() { return ErrRelayerProxyInvalidSession.Wrapf( "session mismatch, expecting: %+v, got: %+v", session.Header, diff --git a/testutil/keeper/proof.go b/testutil/keeper/proof.go index 4ca4dd76b..29fa3b0de 100644 --- a/testutil/keeper/proof.go +++ b/testutil/keeper/proof.go @@ -74,7 +74,7 @@ func ProofKeeper( }, ).AnyTimes() - k := keeper.NewKeeper( + k, err := keeper.NewKeeper( cdc, runtime.NewKVStoreService(storeKey), log.NewNopLogger(), @@ -83,6 +83,7 @@ func ProofKeeper( nil, nil, ) + require.NoError(t, err) ctx := sdk.NewContext(stateStore, cmtproto.Header{}, false, log.NewNopLogger()) diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index e3acb81ff..43fbad8b6 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -1,13 +1,20 @@ package keeper import ( + "context" "fmt" "cosmossdk.io/core/store" + "cosmossdk.io/depinject" "cosmossdk.io/log" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/noot/ring-go" + "github.com/pokt-network/poktroll/pkg/crypto" + "github.com/pokt-network/poktroll/pkg/crypto/rings" + "github.com/pokt-network/poktroll/pkg/polylog" + _ "github.com/pokt-network/poktroll/pkg/polylog/polyzero" "github.com/pokt-network/poktroll/x/proof/types" ) @@ -24,6 +31,7 @@ type ( sessionKeeper types.SessionKeeper applicationKeeper types.ApplicationKeeper accountKeeper types.AccountKeeper + ringClient crypto.RingClient } ) @@ -36,12 +44,29 @@ func NewKeeper( sessionKeeper types.SessionKeeper, applicationKeeper types.ApplicationKeeper, accountKeeper types.AccountKeeper, -) Keeper { + opts ...KeeperOption, +) (Keeper, error) { if _, err := sdk.AccAddressFromBech32(authority); err != nil { panic(fmt.Sprintf("invalid authority address: %s", authority)) } - return Keeper{ + applicationQuerier := types.NewAppKeeperQueryClient(applicationKeeper) + accountQuerier := types.NewAccountKeeperQueryClient(accountKeeper) + // TODO_TECHDEBT: Use cosmos-sdk based polylog implementation once available. Also remove polyzero import. + polylogger := polylog.Ctx(context.TODO()) + + ringClientDeps := depinject.Supply( + polylogger, + applicationQuerier, + accountQuerier, + ) + + ringClient, err := rings.NewRingClient(ringClientDeps) + if err != nil { + return Keeper{}, err + } + + k := Keeper{ cdc: cdc, storeService: storeService, authority: authority, @@ -50,7 +75,14 @@ func NewKeeper( sessionKeeper: sessionKeeper, applicationKeeper: applicationKeeper, accountKeeper: accountKeeper, + ringClient: ringClient, } + + for _, opt := range opts { + opt(&k) + } + + return k, nil } // GetAuthority returns the module's authority. @@ -62,3 +94,8 @@ func (k Keeper) GetAuthority() string { func (k Keeper) Logger() log.Logger { return k.logger.With("module", fmt.Sprintf("x/%s", types.ModuleName)) } + +// GetRingForAddress returns a ring for the given application address +func (k Keeper) GetRingForAddress(ctx context.Context, appAddress string) (*ring.Ring, error) { + return k.ringClient.GetRingForAddress(ctx, appAddress) +} diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index f2280afad..b0efa37d2 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -5,11 +5,7 @@ import ( "context" "crypto/sha256" - ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" - ringtypes "github.com/athanorlabs/go-dleq/types" - "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cosmostypes "github.com/cosmos/cosmos-sdk/types" - ring "github.com/noot/ring-go" "github.com/pokt-network/smt" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -110,8 +106,7 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( } // Verify the relay request's signature. - appAddress := msg.GetSessionHeader().GetApplicationAddress() - if err := k.verifyRelayRequestSignature(ctx, relay.GetReq(), appAddress); err != nil { + if err := k.ringClient.VerifyRelayRequestSignature(ctx, relay.GetReq()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } @@ -324,135 +319,6 @@ func validateRelaySessionHeaders( return nil } -// verifyRelayRequestSignature verifies the signature on the relay request. -// TODO_TECHDEBT: Factor out the relay request signature verification into a shared -// function that can be used by both the proof and relayer packages. -func (k msgServer) verifyRelayRequestSignature( - ctx context.Context, - relayRequest *servicetypes.RelayRequest, - appAddress string, -) error { - // Deserialize the ring signature from the relay request. - signature := relayRequest.GetMeta().GetSignature() - ringSig := new(ring.RingSig) - if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { - return types.ErrProofInvalidRelayRequest.Wrapf( - "error deserializing ring signature: %v", - err, - ) - } - - // Get the ring for the application address. - appRing, err := k.getRingForAppAddress(ctx, appAddress) - if err != nil { - return types.ErrProofInvalidRelayRequest.Wrapf( - "error getting ring for application address %s: %v", - appAddress, - err, - ) - } - - // Ensure the ring signature matches the ring for the application address. - if !ringSig.Ring().Equals(appRing) { - return types.ErrProofInvalidRelayRequest.Wrapf( - "ring signature does not match ring for application address %s", - appAddress, - ) - } - - // Get and hash the signable bytes of the relay request - requestSignableBz, err := relayRequest.GetSignableBytesHash() - if err != nil { - return types.ErrProofInvalidRelayRequest.Wrapf( - "error getting signable bytes: %v", - err, - ) - } - - // Verify the relay request's signature - if valid := ringSig.Verify(requestSignableBz); !valid { - return types.ErrProofInvalidRelayRequest.Wrap("invalid ring signature") - } - - return nil -} - -// getRingForAppAddress returns the RingSinger used to sign relays. It does so by fetching -// the latest information from the application module and creating the correct ring. -// This method also caches the ring's public keys for future use. -func (k msgServer) getRingForAppAddress( - ctx context.Context, - appAddress string, -) (*ring.Ring, error) { - foundApp, appFound := k.applicationKeeper.GetApplication(ctx, appAddress) - if !appFound { - return nil, types.ErrProofInvalidRelayRequest.Wrapf( - "application not found for address %s", - appAddress, - ) - } - - // Create a slice of addresses for the ring. - ringAddresses := make([]string, 0) - ringAddresses = append(ringAddresses, appAddress) // app address is index 0 - if len(foundApp.DelegateeGatewayAddresses) == 0 { - // add app address twice to make the ring size of mininmum 2 - // TODO_HACK: We are adding the appAddress twice because a ring - // signature requires AT LEAST two pubKeys. When the Application has - // not delegated to any gateways, we add the application's own address - // twice. This is a HACK and should be investigated as to what is the - // best approach to take in this situation. - ringAddresses = append(ringAddresses, appAddress) - } else { - // add the delegatee gateway addresses - ringAddresses = append(ringAddresses, foundApp.DelegateeGatewayAddresses...) - } - - // Get the points on the secp256k1 curve for the addresses. - points, err := k.addressesToPoints(ctx, ringAddresses) - if err != nil { - return nil, err - } - - // Create a new ring from points on the secp256k1 curve - return ring.NewFixedKeyRingFromPublicKeys(ring_secp256k1.NewCurve(), points) -} - -// addressesToPoints converts a slice of addresses to a slice of points on the -// secp256k1 curve, by querying the account module for the public key for each -// address and converting them to the corresponding points on the secp256k1 curve -func (k msgServer) addressesToPoints( - ctx context.Context, - addresses []string, -) ([]ringtypes.Point, error) { - curve := ring_secp256k1.NewCurve() - points := make([]ringtypes.Point, len(addresses)) - - for i, addr := range addresses { - // Retrieve the account from the auth module - accAddr, err := cosmostypes.AccAddressFromBech32(addr) - if err != nil { - return nil, err - } - - account := k.accountKeeper.GetAccount(ctx, accAddr) - - key := account.GetPubKey() - // Check if the key is a secp256k1 public key - if _, ok := key.(*secp256k1.PubKey); !ok { - return nil, types.ErrProofNotSecp256k1Curve.Wrapf("got %T", key) - } - // Convert the public key to the point on the secp256k1 curve - point, err := curve.DecodeToPoint(key.Bytes()) - if err != nil { - return nil, err - } - // Insert the point into the slice of points - points[i] = point - } - return points, nil -} - // verifyRelayResponseSignature verifies the signature on the relay response. // TODO_TECHDEBT: Factor out the relay response signature verification into a shared // function that can be used by both the proof and the SDK packages. diff --git a/x/proof/keeper/option.go b/x/proof/keeper/option.go new file mode 100644 index 000000000..502e438b1 --- /dev/null +++ b/x/proof/keeper/option.go @@ -0,0 +1,14 @@ +package keeper + +import "github.com/pokt-network/poktroll/pkg/crypto" + +// KeeperOption is a function that can be optionally passed to the keeper constructor +// to modify its initialization behavior. +type KeeperOption func(*Keeper) + +// WithRingClient overrides the RingClient that the keeper will use with the given client. +func WithRingClient(client crypto.RingClient) KeeperOption { + return func(keeper *Keeper) { + keeper.ringClient = client + } +} diff --git a/x/proof/module/module.go b/x/proof/module/module.go index c18584733..a737888a2 100644 --- a/x/proof/module/module.go +++ b/x/proof/module/module.go @@ -196,7 +196,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { if in.Config.Authority != "" { authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) } - k := keeper.NewKeeper( + k, err := keeper.NewKeeper( in.Cdc, in.StoreService, in.Logger, @@ -205,6 +205,10 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { in.ApplicationKeeper, in.AccountKeeper, ) + if err != nil { + panic(err) + } + m := NewAppModule( in.Cdc, k, diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go new file mode 100644 index 000000000..c112e8a0e --- /dev/null +++ b/x/proof/types/account_query_client.go @@ -0,0 +1,32 @@ +package types + +import ( + "context" + + "github.com/cosmos/cosmos-sdk/types" + accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" + + "github.com/pokt-network/poktroll/pkg/client" +) + +var _ client.AccountQueryClient = (*AccountKeeperQueryClient)(nil) + +type AccountKeeperQueryClient struct { + keeper AccountKeeper +} + +func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQueryClient { + return &AccountKeeperQueryClient{keeper: accountKeeper} +} + +func (accountQueryClient *AccountKeeperQueryClient) GetAccount( + ctx context.Context, + addr string, +) (accounttypes.AccountI, error) { + addrBz, err := types.AccAddressFromBech32(addr) + if err != nil { + return nil, err + } + + return accountQueryClient.keeper.GetAccount(ctx, addrBz), nil +} diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go new file mode 100644 index 000000000..b05bd64b5 --- /dev/null +++ b/x/proof/types/application_query_client.go @@ -0,0 +1,32 @@ +package types + +import ( + "context" + + "github.com/pokt-network/poktroll/pkg/client" + apptypes "github.com/pokt-network/poktroll/x/application/types" +) + +var ( + _ client.ApplicationQueryClient = (*AppKeeperQueryClient)(nil) +) + +type AppKeeperQueryClient struct { + keeper ApplicationKeeper +} + +func NewAppKeeperQueryClient(appKeeper ApplicationKeeper) client.ApplicationQueryClient { + return &AppKeeperQueryClient{keeper: appKeeper} +} + +func (appQueryClient *AppKeeperQueryClient) GetApplication( + ctx context.Context, + appAddr string, +) (apptypes.Application, error) { + app, _ := appQueryClient.keeper.GetApplication(ctx, appAddr) + return app, nil +} + +func (appQueryClient *AppKeeperQueryClient) GetAllApplications(ctx context.Context) ([]apptypes.Application, error) { + return appQueryClient.keeper.GetAllApplications(ctx), nil +} diff --git a/x/proof/types/expected_keepers.go b/x/proof/types/expected_keepers.go index ec62d3069..cb6758d32 100644 --- a/x/proof/types/expected_keepers.go +++ b/x/proof/types/expected_keepers.go @@ -30,4 +30,5 @@ type BankKeeper interface { // ApplicationKeeper defines the expected application keeper to retrieve applications type ApplicationKeeper interface { GetApplication(ctx context.Context, address string) (app apptypes.Application, found bool) + GetAllApplications(ctx context.Context) []apptypes.Application } From 835889e5275af0cba2263acf8d699b4de5d5b632 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Wed, 6 Mar 2024 11:52:45 +0100 Subject: [PATCH 03/23] refactor: make session comparaison more generic --- x/proof/keeper/msg_server_submit_proof.go | 114 ++++++---------------- 1 file changed, 31 insertions(+), 83 deletions(-) diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index b0efa37d2..3801741ec 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -94,8 +94,13 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( ) } - // Verify the relay request and response session headers match the proof session header. - if err := validateRelaySessionHeaders(relay, msg.GetSessionHeader()); err != nil { + // Verify that the relay request session header matches the proof session header. + if err := compareSessionHeaders(msg.GetSessionHeader(), relay.GetReq().Meta.GetSessionHeader()); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + // Verify that the relay response session header matches the proof session header. + if err := compareSessionHeaders(msg.GetSessionHeader(), relay.GetRes().Meta.GetSessionHeader()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } @@ -214,105 +219,48 @@ func (k msgServer) queryAndValidateClaimForProof( return &foundClaim, nil } -// validateRelaySessionHeaders ensures that the relay request and response session headers -// match the proof session header. -func validateRelaySessionHeaders( - relay *servicetypes.Relay, - msgSessHeader *sessiontypes.SessionHeader, +// compareSessionHeaders compares a session header against an expected session header. +func compareSessionHeaders( + expectedSessionHeader *sessiontypes.SessionHeader, + sessionHeader *sessiontypes.SessionHeader, ) error { - reqSessHeader := relay.GetReq().GetMeta().GetSessionHeader() - respSessHeader := relay.GetRes().GetMeta().GetSessionHeader() - - // Ensure the relay request and response application addresses match - // the proof application address. - - if reqSessHeader.GetApplicationAddress() != msgSessHeader.GetApplicationAddress() { - return types.ErrProofInvalidRelay.Wrapf( - "relay request application address %s does not match proof application address %s", - reqSessHeader.GetApplicationAddress(), - msgSessHeader.GetApplicationAddress(), - ) - } - - if respSessHeader.GetApplicationAddress() != msgSessHeader.GetApplicationAddress() { - return types.ErrProofInvalidRelay.Wrapf( - "relay response application address %s does not match proof application address %s", - reqSessHeader.GetApplicationAddress(), - msgSessHeader.GetApplicationAddress(), - ) - } - - // Ensure the relay request and response service IDs match the proof service ID. - - if reqSessHeader.GetService().GetId() != msgSessHeader.GetService().GetId() { + if sessionHeader.GetApplicationAddress() != expectedSessionHeader.GetApplicationAddress() { return types.ErrProofInvalidRelay.Wrapf( - "relay request service ID %s does not match proof service ID %s", - reqSessHeader.GetService().GetId(), - msgSessHeader.GetService().GetId(), + "sessionHeaders application addresses mismatch expect: %s, got: %s", + expectedSessionHeader.GetApplicationAddress(), + sessionHeader.GetApplicationAddress(), ) } - if respSessHeader.GetService().GetId() != msgSessHeader.GetService().GetId() { + if sessionHeader.GetService().GetId() != expectedSessionHeader.GetService().GetId() { return types.ErrProofInvalidRelay.Wrapf( - "relay response service ID %s does not match proof service ID %s", - respSessHeader.GetService().GetId(), - msgSessHeader.GetService().GetId(), + "sessionHeaders service IDs mismatch expect: %s, got: %s", + expectedSessionHeader.GetService().GetId(), + sessionHeader.GetService().GetId(), ) } - // Ensure the relay request and response session start block heights - // match the proof session start block height. - - if reqSessHeader.GetSessionStartBlockHeight() != msgSessHeader.GetSessionStartBlockHeight() { - return types.ErrProofInvalidRelay.Wrapf( - "relay request session start height %d does not match proof session start height %d", - reqSessHeader.GetSessionStartBlockHeight(), - msgSessHeader.GetSessionStartBlockHeight(), - ) - } - - if respSessHeader.GetSessionStartBlockHeight() != msgSessHeader.GetSessionStartBlockHeight() { + if sessionHeader.GetSessionStartBlockHeight() != expectedSessionHeader.GetSessionStartBlockHeight() { return types.ErrProofInvalidRelay.Wrapf( - "relay response session start height %d does not match proof session start height %d", - respSessHeader.GetSessionStartBlockHeight(), - msgSessHeader.GetSessionStartBlockHeight(), + "sessionHeaders session start heights mismatch expect: %d, got: %d", + expectedSessionHeader.GetSessionStartBlockHeight(), + sessionHeader.GetSessionStartBlockHeight(), ) } - // Ensure the relay request and response session end block heights - // match the proof session end block height. - - if reqSessHeader.GetSessionEndBlockHeight() != msgSessHeader.GetSessionEndBlockHeight() { - return types.ErrProofInvalidRelay.Wrapf( - "relay request session end height %d does not match proof session end height %d", - reqSessHeader.GetSessionEndBlockHeight(), - msgSessHeader.GetSessionEndBlockHeight(), - ) - } - - if respSessHeader.GetSessionEndBlockHeight() != msgSessHeader.GetSessionEndBlockHeight() { - return types.ErrProofInvalidRelay.Wrapf( - "relay response session end height %d does not match proof session end height %d", - respSessHeader.GetSessionEndBlockHeight(), - msgSessHeader.GetSessionEndBlockHeight(), - ) - } - - // Ensure the relay request and response session IDs match the proof session ID. - - if reqSessHeader.GetSessionId() != msgSessHeader.GetSessionId() { + if sessionHeader.GetSessionEndBlockHeight() != expectedSessionHeader.GetSessionEndBlockHeight() { return types.ErrProofInvalidRelay.Wrapf( - "relay request session ID %s does not match proof session ID %s", - reqSessHeader.GetSessionId(), - msgSessHeader.GetSessionId(), + "sessionHeaders session end heights mismatch expect: %d, got: %d", + expectedSessionHeader.GetSessionEndBlockHeight(), + sessionHeader.GetSessionEndBlockHeight(), ) } - if respSessHeader.GetSessionId() != msgSessHeader.GetSessionId() { + if sessionHeader.GetSessionId() != expectedSessionHeader.GetSessionId() { return types.ErrProofInvalidRelay.Wrapf( - "relay response session ID %s does not match proof session ID %s", - respSessHeader.GetSessionId(), - msgSessHeader.GetSessionId(), + "sessionHeaders session IDs mismatch expect: %s, got: %s", + expectedSessionHeader.GetSessionId(), + sessionHeader.GetSessionId(), ) } From a01cb267f1d7cdb255af6304cf8aeac944099a26 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Wed, 6 Mar 2024 18:05:27 +0100 Subject: [PATCH 04/23] chore: add comments --- x/proof/keeper/msg_server_submit_proof.go | 4 ++++ x/service/types/relay.go | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 3801741ec..80080d264 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -62,6 +62,10 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( 3. verify(claim.Root, proof.ClosestProof); verify the closest proof is correct */ + // Ensure that all validation and verification checks are successful on the + // MsgSubmitProof message before constructing the proof and inserting it into + // the store. + if err := msg.ValidateBasic(); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } diff --git a/x/service/types/relay.go b/x/service/types/relay.go index 96c846001..d686447df 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -15,6 +15,9 @@ func (req RelayRequest) getSignableBytes() ([]byte, error) { // Hashing the marshaled request message guarantees that the signable bytes are // always of a constant and expected length. func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { + // Save the signature and restore it after getting the signable bytes + // since getSignableBytes sets the signature to nil but does not preserve + // its original value. signature := req.Meta.Signature requestBz, err := req.getSignableBytes() @@ -43,6 +46,9 @@ func (res RelayResponse) getSignableBytes() ([]byte, error) { // Hashing the marshaled response message guarantees that the signable bytes are // always of a constant and expected length. func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { + // Save the signature and restore it after getting the signable bytes + // since getSignableBytes sets the signature to nil but does not preserve + // its original value. signature := res.Meta.SupplierSignature responseBz, err := res.getSignableBytes() From 0b82243113e61c844a725376dd851d875771866f Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Thu, 7 Mar 2024 16:42:49 +0100 Subject: [PATCH 05/23] [PubKeyClient] Implement PubKeyClient for on/off-chain usage (#413) Co-authored-by: Bryan White --- pkg/appgateserver/cmd/cmd.go | 1 + pkg/crypto/interface.go | 10 +++ pkg/crypto/pubkey_client/client.go | 58 ++++++++++++++++ pkg/crypto/pubkey_client/errors.go | 8 +++ pkg/crypto/rings/client.go | 60 +++++++++-------- pkg/deps/config/suppliers.go | 19 ++++++ pkg/sdk/deps_builder.go | 12 +++- pkg/sdk/relay_verifier.go | 82 ----------------------- pkg/sdk/sdk.go | 36 +++++++--- pkg/sdk/send_relay.go | 19 +++++- testutil/keeper/proof.go | 3 +- x/proof/keeper/keeper.go | 27 +++----- x/proof/keeper/msg_server_submit_proof.go | 58 ++++------------ x/proof/module/module.go | 6 +- x/service/types/errors.go | 22 +++--- x/service/types/relay.go | 48 ++++++++++++- 16 files changed, 269 insertions(+), 200 deletions(-) create mode 100644 pkg/crypto/pubkey_client/client.go create mode 100644 pkg/crypto/pubkey_client/errors.go delete mode 100644 pkg/sdk/relay_verifier.go diff --git a/pkg/appgateserver/cmd/cmd.go b/pkg/appgateserver/cmd/cmd.go index f8a439716..43213d721 100644 --- a/pkg/appgateserver/cmd/cmd.go +++ b/pkg/appgateserver/cmd/cmd.go @@ -191,6 +191,7 @@ func setupAppGateServerDependencies( config.NewSupplyAccountQuerierFn(), // leaf config.NewSupplyApplicationQuerierFn(), // leaf config.NewSupplySessionQuerierFn(), // leaf + config.NewSupplyPubKeyClientFn(), config.NewSupplyRingCacheFn(), config.NewSupplyPOKTRollSDKFn(appGateConfig.SigningKey), diff --git a/pkg/crypto/interface.go b/pkg/crypto/interface.go index cfdd0574f..36302d277 100644 --- a/pkg/crypto/interface.go +++ b/pkg/crypto/interface.go @@ -4,6 +4,7 @@ package crypto import ( "context" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/noot/ring-go" "github.com/pokt-network/poktroll/x/service/types" @@ -40,3 +41,12 @@ type RingClient interface { // ring for the application address in the relay request. VerifyRelayRequestSignature(ctx context.Context, relayRequest *types.RelayRequest) error } + +// PubKeyClient is used to get the public key given an address. +// On-chain and off-chain implementations should take care of retrieving the +// address' account and returning its public key. +type PubKeyClient interface { + // GetPubKeyFromAddress returns the public key of the given account address if + // it exists. + GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) +} diff --git a/pkg/crypto/pubkey_client/client.go b/pkg/crypto/pubkey_client/client.go new file mode 100644 index 000000000..2ef70c2d3 --- /dev/null +++ b/pkg/crypto/pubkey_client/client.go @@ -0,0 +1,58 @@ +package pubkeyclient + +import ( + "context" + + "cosmossdk.io/depinject" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + + "github.com/pokt-network/poktroll/pkg/client" + "github.com/pokt-network/poktroll/pkg/crypto" +) + +var _ crypto.PubKeyClient = (*pubKeyClient)(nil) + +// pubKeyClient is an implementation of the PubKeyClient that uses an account +// querier to get the public key of an address. +type pubKeyClient struct { + // accountQuerier is the querier for the account module, it is used to get + // the account of an address. + accountQuerier client.AccountQueryClient +} + +// NewPubKeyClient creates a new PubKeyClient with the given dependencies. +// The querier is injected using depinject and has to be specific to the +// environment in which the pubKeyClient is initialized as on-chain and off-chain +// environments may have different queriers. +// +// Required dependencies: +// - client.AccountQueryClient +func NewPubKeyClient(deps depinject.Config) (crypto.PubKeyClient, error) { + pc := new(pubKeyClient) + + if err := depinject.Inject( + deps, + &pc.accountQuerier, + ); err != nil { + return nil, err + } + + return pc, nil +} + +// GetPubKeyFromAddress returns the public key of the given address. +// It uses the accountQuerier to get the account and then returns its public key. +func (pc *pubKeyClient) GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) { + acc, err := pc.accountQuerier.GetAccount(ctx, address) + if err != nil { + return nil, err + } + + // If the account's public key is nil, then return an error. + pubKey := acc.GetPubKey() + if pubKey == nil { + return nil, ErrPubKeyClientEmptyPubKey + } + + return pubKey, nil +} diff --git a/pkg/crypto/pubkey_client/errors.go b/pkg/crypto/pubkey_client/errors.go new file mode 100644 index 000000000..d59d7f0a5 --- /dev/null +++ b/pkg/crypto/pubkey_client/errors.go @@ -0,0 +1,8 @@ +package pubkeyclient + +import sdkerrors "cosmossdk.io/errors" + +var ( + codespace = "pubkeyclient" + ErrPubKeyClientEmptyPubKey = sdkerrors.Register(codespace, 1, "empty public key") +) diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 6942bc29b..03f572e15 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -6,12 +6,12 @@ import ( "cosmossdk.io/depinject" ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" - ringtypes "github.com/athanorlabs/go-dleq/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/noot/ring-go" "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" + pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/polylog" "github.com/pokt-network/poktroll/x/service/types" ) @@ -26,9 +26,8 @@ type ringClient struct { // used to get the addresses of the gateways an application is delegated to. applicationQuerier client.ApplicationQueryClient - // accountQuerier is the querier for the account module, and is used to get - // the public keys of the application and its delegated gateways. - accountQuerier client.AccountQueryClient + // pubKeyClient + pubKeyClient crypto.PubKeyClient } // NewRingClient returns a new ring client constructed from the given dependencies. @@ -38,14 +37,17 @@ type ringClient struct { // - polylog.Logger // - client.ApplicationQueryClient // - client.AccountQueryClient -func NewRingClient(deps depinject.Config) (crypto.RingClient, error) { +func NewRingClient(deps depinject.Config) (_ crypto.RingClient, err error) { rc := new(ringClient) + rc.pubKeyClient, err = pubkeyclient.NewPubKeyClient(deps) + if err != nil { + return nil, err + } if err := depinject.Inject( deps, &rc.logger, &rc.applicationQuerier, - &rc.accountQuerier, ); err != nil { return nil, err } @@ -60,7 +62,7 @@ func (rc *ringClient) GetRingForAddress( ctx context.Context, appAddress string, ) (*ring.Ring, error) { - points, err := rc.getDelegatedPubKeysForAddress(ctx, appAddress) + pubKeys, err := rc.getDelegatedPubKeysForAddress(ctx, appAddress) if err != nil { return nil, err } @@ -68,6 +70,14 @@ func (rc *ringClient) GetRingForAddress( rc.logger.Debug(). Str("app_address", appAddress). Msg("updating ring ringsByAddr for app") + + // Get the points on the secp256k1 curve for the public keys in the ring. + points, err := pointsFromPublicKeys(pubKeys...) + if err != nil { + return nil, err + } + + // Return the ring the constructed from the public key points on the secp256k1 curve. return newRingFromPoints(points) } @@ -148,7 +158,7 @@ func (rc *ringClient) VerifyRelayRequestSignature( func (rc *ringClient) getDelegatedPubKeysForAddress( ctx context.Context, appAddress string, -) ([]ringtypes.Point, error) { +) ([]cryptotypes.PubKey, error) { // Get the application's on chain state. app, err := rc.applicationQuerier.GetApplication(ctx, appAddress) if err != nil { @@ -171,35 +181,27 @@ func (rc *ringClient) getDelegatedPubKeysForAddress( ringAddresses = append(ringAddresses, app.DelegateeGatewayAddresses...) } - // Get the points on the secp256k1 curve for the addresses. - points, err := rc.addressesToPoints(ctx, ringAddresses) - if err != nil { - return nil, err - } + rc.logger.Debug(). + // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` instead of formatting here. + Str("addresses", fmt.Sprintf("%v", ringAddresses)). + Msg("converting addresses to points") - // Return the public key points on the secp256k1 curve. - return points, nil + return rc.addressesToPubKeys(ctx, ringAddresses) } -// addressesToPoints converts a slice of addresses to a slice of points on the -// secp256k1 curve, by querying the account module for the public key for each -// address and converting them to the corresponding points on the secp256k1 curve -func (rc *ringClient) addressesToPoints( +// addressesToPubKeys uses the public key client to query the account module for +// the public key corresponding to each address given. +func (rc *ringClient) addressesToPubKeys( ctx context.Context, addresses []string, -) ([]ringtypes.Point, error) { - publicKeys := make([]cryptotypes.PubKey, len(addresses)) - rc.logger.Debug(). - // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` instead of formatting here. - Str("addresses", fmt.Sprintf("%v", addresses)). - Msg("converting addresses to points") +) ([]cryptotypes.PubKey, error) { + pubKeys := make([]cryptotypes.PubKey, len(addresses)) for i, addr := range addresses { - acc, err := rc.accountQuerier.GetAccount(ctx, addr) + acc, err := rc.pubKeyClient.GetPubKeyFromAddress(ctx, addr) if err != nil { return nil, err } - publicKeys[i] = acc.GetPubKey() + pubKeys[i] = acc } - - return pointsFromPublicKeys(publicKeys...) + return pubKeys, nil } diff --git a/pkg/deps/config/suppliers.go b/pkg/deps/config/suppliers.go index 8f53a0ca6..17c412840 100644 --- a/pkg/deps/config/suppliers.go +++ b/pkg/deps/config/suppliers.go @@ -17,6 +17,7 @@ import ( "github.com/pokt-network/poktroll/pkg/client/query" querytypes "github.com/pokt-network/poktroll/pkg/client/query/types" txtypes "github.com/pokt-network/poktroll/pkg/client/tx/types" + pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" "github.com/pokt-network/poktroll/pkg/sdk" @@ -376,3 +377,21 @@ func NewSupplyPOKTRollSDKFn(signingKeyName string) SupplierFn { return depinject.Configs(deps, depinject.Supply(poktrollSDK)), nil } } + +// NewSupplyPubKeyClientFn supplies a depinject config with a PubKeyClient. +func NewSupplyPubKeyClientFn() SupplierFn { + return func( + _ context.Context, + deps depinject.Config, + _ *cobra.Command, + ) (depinject.Config, error) { + // Create the pubKey client. + pubKeyClient, err := pubkeyclient.NewPubKeyClient(deps) + if err != nil { + return nil, err + } + + // Supply the pubKey client to the provided deps + return depinject.Configs(deps, depinject.Supply(pubKeyClient)), nil + } +} diff --git a/pkg/sdk/deps_builder.go b/pkg/sdk/deps_builder.go index cfd0ff4b3..f1359c5c3 100644 --- a/pkg/sdk/deps_builder.go +++ b/pkg/sdk/deps_builder.go @@ -12,6 +12,7 @@ import ( "github.com/pokt-network/poktroll/pkg/client/delegation" eventsquery "github.com/pokt-network/poktroll/pkg/client/events" "github.com/pokt-network/poktroll/pkg/client/query" + pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" ) @@ -54,12 +55,19 @@ func (sdk *poktrollSDK) buildDeps( } deps = depinject.Configs(deps, depinject.Supply(grpcClient)) - // Create and supply the account querier + // Create and supply the account querier to the pubKey Client accountQuerier, err := query.NewAccountQuerier(deps) if err != nil { return nil, err } - deps = depinject.Configs(deps, depinject.Supply(accountQuerier)) + + // Create and supply the pubKey Client + pubKeyClientDeps := depinject.Supply(accountQuerier) + pubKeyClient, err := pubkeyclient.NewPubKeyClient(pubKeyClientDeps) + if err != nil { + return nil, err + } + deps = depinject.Configs(deps, depinject.Supply(pubKeyClient)) // Create and supply the application querier applicationQuerier, err := query.NewApplicationQuerier(deps) diff --git a/pkg/sdk/relay_verifier.go b/pkg/sdk/relay_verifier.go deleted file mode 100644 index 6b487a9a6..000000000 --- a/pkg/sdk/relay_verifier.go +++ /dev/null @@ -1,82 +0,0 @@ -package sdk - -import ( - "context" - - cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - - "github.com/pokt-network/poktroll/pkg/polylog" - "github.com/pokt-network/poktroll/x/service/types" -) - -// verifyResponse verifies the relay response signature. -func (sdk *poktrollSDK) verifyResponse( - ctx context.Context, - supplierAddress string, - relayResponse *types.RelayResponse, -) error { - logger := polylog.Ctx(context.Background()) - - // Get the supplier's public key. - supplierPubKey, err := sdk.getSupplierPubKeyFromAddress(ctx, supplierAddress) - if err != nil { - return err - } - - // Extract the supplier's signature - if relayResponse.Meta == nil { - return ErrSDKEmptyRelayResponseSignature.Wrapf( - "response payload: %s", relayResponse.Payload, - ) - } - supplierSignature := relayResponse.Meta.SupplierSignature - - // Get the relay response signable bytes and hash them. - responseSignableBz, err := relayResponse.GetSignableBytesHash() - if err != nil { - return err - } - - logger.Debug(). - Str("supplier", supplierAddress). - Str("application", relayResponse.Meta.SessionHeader.ApplicationAddress). - Str("service", relayResponse.Meta.SessionHeader.Service.Id). - Int64("end_height", relayResponse.Meta.SessionHeader.SessionEndBlockHeight). - Msg("About to verify relay response signature.") - - // Verify the relay response signature. - if !supplierPubKey.VerifySignature(responseSignableBz[:], supplierSignature) { - return ErrSDKInvalidRelayResponseSignature - } - - return nil -} - -// getSupplierPubKeyFromAddress gets the supplier's public key from the cache or -// queries if it is not found. The public key is then cached before being returned. -func (sdk *poktrollSDK) getSupplierPubKeyFromAddress( - ctx context.Context, - supplierAddress string, -) (cryptotypes.PubKey, error) { - supplierPubKey, ok := sdk.supplierAccountCache[supplierAddress] - if ok { - return supplierPubKey, nil - } - - // Query for the supplier account to get the application's public key - // to verify the relay request signature. - acc, err := sdk.accountQuerier.GetAccount(ctx, supplierAddress) - if err != nil { - return nil, err - } - - fetchedPubKey := acc.GetPubKey() - if fetchedPubKey == nil { - return nil, ErrSDKEmptySupplierPubKey - } - - // Cache the retrieved public key. - sdk.supplierAccountCache[supplierAddress] = fetchedPubKey - - return fetchedPubKey, nil -} diff --git a/pkg/sdk/sdk.go b/pkg/sdk/sdk.go index c2c836e6b..aa3474a57 100644 --- a/pkg/sdk/sdk.go +++ b/pkg/sdk/sdk.go @@ -53,10 +53,6 @@ type poktrollSDK struct { // for a specific session serviceSessionSuppliers map[string]map[string]*SessionSuppliers - // accountQuerier is the querier for the account module. - // It is used to get the the supplier's public key to verify the relay response signature. - accountQuerier client.AccountQueryClient - // applicationQuerier is the querier for the application module. // It is used to query a specific application or all applications applicationQuerier client.ApplicationQueryClient @@ -65,9 +61,13 @@ type poktrollSDK struct { // It is used to get the current block height to query for the current session. blockClient client.BlockClient - // accountCache is a cache of the supplier accounts that has been queried + // pubKeyClient the client used to get the public key given an account address. + // TODO_TECHDEBT: Add a size limit to the cache. + pubKeyClient crypto.PubKeyClient + + // supplierPubKeyCache is a cache of the suppliers pubKeys that has been queried. // TODO_TECHDEBT: Add a size limit to the cache. - supplierAccountCache map[string]cryptotypes.PubKey + supplierPubKeyCache map[string]cryptotypes.PubKey } // NewPOKTRollSDK creates a new POKTRollSDK instance with the given configuration. @@ -75,7 +75,7 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK sdk := &poktrollSDK{ config: config, serviceSessionSuppliers: make(map[string]map[string]*SessionSuppliers), - supplierAccountCache: make(map[string]cryptotypes.PubKey), + supplierPubKeyCache: make(map[string]cryptotypes.PubKey), } var err error @@ -93,7 +93,7 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK &sdk.logger, &sdk.ringCache, &sdk.sessionQuerier, - &sdk.accountQuerier, + &sdk.pubKeyClient, &sdk.applicationQuerier, &sdk.blockClient, ); err != nil { @@ -113,3 +113,23 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK return sdk, nil } + +// getPubKeyFromAddress returns the public key of the given address. +// It uses the accountQuerier to get the account if it is not already in the cache. +// If the account does not have a public key, it returns an error. +func (sdk *poktrollSDK) getPubKeyFromAddress( + ctx context.Context, + address string, +) (cryptotypes.PubKey, error) { + if pubKey, ok := sdk.supplierPubKeyCache[address]; ok { + return pubKey, nil + } + + pubKey, err := sdk.pubKeyClient.GetPubKeyFromAddress(ctx, address) + if err != nil { + return nil, err + } + + sdk.supplierPubKeyCache[address] = pubKey + return pubKey, nil +} diff --git a/pkg/sdk/send_relay.go b/pkg/sdk/send_relay.go index bb6ece483..16a338344 100644 --- a/pkg/sdk/send_relay.go +++ b/pkg/sdk/send_relay.go @@ -101,13 +101,30 @@ func (sdk *poktrollSDK) SendRelay( return nil, ErrSDKHandleRelay.Wrapf("error unmarshaling relay response: %s", err) } + if err := relayResponse.ValidateBasic(); err != nil { + return nil, ErrSDKHandleRelay.Wrapf("%s", err) + } + + // Get the supplier's public key. + supplierPubKey, err := sdk.getPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) + if err != nil { + return nil, ErrSDKHandleRelay.Wrapf("error getting supplier public key: %s", err) + } + + sdk.logger.Debug(). + Str("supplier", supplierEndpoint.SupplierAddress). + Str("application", relayResponse.GetMeta().GetSessionHeader().GetApplicationAddress()). + Str("service", relayResponse.GetMeta().GetSessionHeader().GetService().GetId()). + Int64("end_height", relayResponse.GetMeta().GetSessionHeader().GetSessionEndBlockHeight()). + Msg("About to verify relay response signature.") + // Verify the response signature. We use the supplier address that we got from // the getRelayerUrl function since this is the address we are expecting to sign the response. // TODO_TECHDEBT: if the RelayResponse is an internal error response, we should not verify the signature // as in some relayer early failures, it may not be signed by the supplier. // TODO_IMPROVE: Add more logging & telemetry so we can get visibility and signal into // failed responses. - if err := sdk.verifyResponse(ctx, supplierEndpoint.SupplierAddress, relayResponse); err != nil { + if err := relayResponse.VerifySignature(supplierPubKey); err != nil { return nil, ErrSDKVerifyResponseSignature.Wrapf("%s", err) } diff --git a/testutil/keeper/proof.go b/testutil/keeper/proof.go index 29fa3b0de..4ca4dd76b 100644 --- a/testutil/keeper/proof.go +++ b/testutil/keeper/proof.go @@ -74,7 +74,7 @@ func ProofKeeper( }, ).AnyTimes() - k, err := keeper.NewKeeper( + k := keeper.NewKeeper( cdc, runtime.NewKVStoreService(storeKey), log.NewNopLogger(), @@ -83,7 +83,6 @@ func ProofKeeper( nil, nil, ) - require.NoError(t, err) ctx := sdk.NewContext(stateStore, cmtproto.Header{}, false, log.NewNopLogger()) diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 43fbad8b6..3db7d25b4 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -9,9 +9,9 @@ import ( "cosmossdk.io/log" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/noot/ring-go" "github.com/pokt-network/poktroll/pkg/crypto" + pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" _ "github.com/pokt-network/poktroll/pkg/polylog/polyzero" @@ -32,6 +32,7 @@ type ( applicationKeeper types.ApplicationKeeper accountKeeper types.AccountKeeper ringClient crypto.RingClient + pubKeyClient crypto.PubKeyClient } ) @@ -44,8 +45,7 @@ func NewKeeper( sessionKeeper types.SessionKeeper, applicationKeeper types.ApplicationKeeper, accountKeeper types.AccountKeeper, - opts ...KeeperOption, -) (Keeper, error) { +) Keeper { if _, err := sdk.AccAddressFromBech32(authority); err != nil { panic(fmt.Sprintf("invalid authority address: %s", authority)) } @@ -63,10 +63,15 @@ func NewKeeper( ringClient, err := rings.NewRingClient(ringClientDeps) if err != nil { - return Keeper{}, err + panic(err) } - k := Keeper{ + pubKeyClient, err := pubkeyclient.NewPubKeyClient(depinject.Supply(accountQuerier)) + if err != nil { + panic(err) + } + + return Keeper{ cdc: cdc, storeService: storeService, authority: authority, @@ -76,13 +81,8 @@ func NewKeeper( applicationKeeper: applicationKeeper, accountKeeper: accountKeeper, ringClient: ringClient, + pubKeyClient: pubKeyClient, } - - for _, opt := range opts { - opt(&k) - } - - return k, nil } // GetAuthority returns the module's authority. @@ -94,8 +94,3 @@ func (k Keeper) GetAuthority() string { func (k Keeper) Logger() log.Logger { return k.logger.With("module", fmt.Sprintf("x/%s", types.ModuleName)) } - -// GetRingForAddress returns a ring for the given application address -func (k Keeper) GetRingForAddress(ctx context.Context, appAddress string) (*ring.Ring, error) { - return k.ringClient.GetRingForAddress(ctx, appAddress) -} diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 80080d264..93c380608 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -5,7 +5,6 @@ import ( "context" "crypto/sha256" - cosmostypes "github.com/cosmos/cosmos-sdk/types" "github.com/pokt-network/smt" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -98,6 +97,14 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( ) } + if err := relay.GetReq().ValidateBasic(); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + + if err := relay.GetRes().ValidateBasic(); err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + // Verify that the relay request session header matches the proof session header. if err := compareSessionHeaders(msg.GetSessionHeader(), relay.GetReq().Meta.GetSessionHeader()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) @@ -108,9 +115,13 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return nil, status.Error(codes.FailedPrecondition, err.Error()) } + supplierPubKey, err := k.pubKeyClient.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) + if err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + // Verify the relay response's signature. - supplierAddress := msg.GetSupplierAddress() - if err := k.verifyRelayResponseSignature(ctx, relay.GetRes(), supplierAddress); err != nil { + if err := relay.GetRes().VerifySignature(supplierPubKey); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } @@ -271,47 +282,6 @@ func compareSessionHeaders( return nil } -// verifyRelayResponseSignature verifies the signature on the relay response. -// TODO_TECHDEBT: Factor out the relay response signature verification into a shared -// function that can be used by both the proof and the SDK packages. -func (k msgServer) verifyRelayResponseSignature( - ctx context.Context, - relayResponse *servicetypes.RelayResponse, - supplierAddress string, -) error { - // Get the account from the auth module - accAddr, err := cosmostypes.AccAddressFromBech32(supplierAddress) - if err != nil { - return err - } - - supplierAccount := k.accountKeeper.GetAccount(ctx, accAddr) - - // Get the public key from the account - pubKey := supplierAccount.GetPubKey() - if pubKey == nil { - return types.ErrProofInvalidRelayResponse.Wrapf( - "no public key found for supplier address %s", - supplierAddress, - ) - } - - supplierSignature := relayResponse.Meta.SupplierSignature - - // Get the relay response signable bytes and hash them. - responseSignableBz, err := relayResponse.GetSignableBytesHash() - if err != nil { - return err - } - - // Verify the relay response's signature - if valid := pubKey.VerifySignature(responseSignableBz[:], supplierSignature); !valid { - return types.ErrProofInvalidRelayResponse.Wrap("invalid relay response signature") - } - - return nil -} - // verifyClosestProof verifies the closest merkle proof against the expected root hash. func verifyClosestProof( proof *smt.SparseMerkleClosestProof, diff --git a/x/proof/module/module.go b/x/proof/module/module.go index a737888a2..c18584733 100644 --- a/x/proof/module/module.go +++ b/x/proof/module/module.go @@ -196,7 +196,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { if in.Config.Authority != "" { authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) } - k, err := keeper.NewKeeper( + k := keeper.NewKeeper( in.Cdc, in.StoreService, in.Logger, @@ -205,10 +205,6 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { in.ApplicationKeeper, in.AccountKeeper, ) - if err != nil { - panic(err) - } - m := NewAppModule( in.Cdc, k, diff --git a/x/service/types/errors.go b/x/service/types/errors.go index cd4ea82b6..d94a48fc8 100644 --- a/x/service/types/errors.go +++ b/x/service/types/errors.go @@ -6,14 +6,16 @@ import sdkerrors "cosmossdk.io/errors" // x/service module sentinel errors var ( - ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") - ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service") - ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service") - ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID") - ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name") - ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists") - ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid service fee") - ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found") - ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service") - ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee") + ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") + ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service") + ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service") + ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID") + ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name") + ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists") + ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid service fee") + ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found") + ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service") + ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee") + ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response") + ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request") ) diff --git a/x/service/types/relay.go b/x/service/types/relay.go index d686447df..e0ef6c2c6 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -1,6 +1,10 @@ package types -import "crypto/sha256" +import ( + "crypto/sha256" + + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" +) // getSignableBytes returns the bytes resulting from marshaling the relay request // A value receiver is used to avoid overwriting any pre-existing signature @@ -33,6 +37,22 @@ func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { return sha256.Sum256(requestBz), nil } +func (req *RelayRequest) ValidateBasic() error { + if req.GetMeta() == nil { + return ErrServiceInvalidRelayRequest.Wrap("missing meta") + } + + if err := req.GetMeta().GetSessionHeader().ValidateBasic(); err != nil { + return ErrServiceInvalidRelayRequest.Wrapf("invalid session header: %s", err) + } + + if len(req.GetMeta().GetSignature()) == 0 { + return ErrServiceInvalidRelayRequest.Wrap("missing signature") + } + + return nil +} + // getSignableBytes returns the bytes resulting from marshaling the relay response // A value receiver is used to avoid overwriting any pre-existing signature func (res RelayResponse) getSignableBytes() ([]byte, error) { @@ -69,5 +89,31 @@ func (res *RelayResponse) ValidateBasic() error { // SessionHeader, consider sending an on-chain challenge, lowering their // QoS, or other future work. + if res.GetMeta() == nil { + return ErrServiceInvalidRelayResponse.Wrap("missing meta") + } + + if err := res.GetMeta().GetSessionHeader().ValidateBasic(); err != nil { + return ErrServiceInvalidRelayResponse.Wrapf("invalid session header: %s", err) + } + + if len(res.GetMeta().GetSupplierSignature()) == 0 { + return ErrServiceInvalidRelayResponse.Wrap("missing supplier signature") + } + + return nil +} + +func (res *RelayResponse) VerifySignature(supplierPubKey cryptotypes.PubKey) error { + // Get the signable bytes hash of the response. + signableBz, err := res.GetSignableBytesHash() + if err != nil { + return err + } + + if ok := supplierPubKey.VerifySignature(signableBz[:], res.GetMeta().GetSupplierSignature()); !ok { + return ErrServiceInvalidRelayResponse.Wrap("invalid signature") + } + return nil } From 74e7a09dd625abe5c25bd0f902ef6a33c1b79e89 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Thu, 7 Mar 2024 21:49:47 +0100 Subject: [PATCH 06/23] chore: Address review change requests --- pkg/crypto/pubkey_client/client.go | 2 +- pkg/crypto/rings/cache.go | 2 ++ pkg/crypto/rings/client.go | 41 +++++++++++------------ x/proof/keeper/msg_server_submit_proof.go | 28 ++++++++++++---- x/proof/keeper/option.go | 14 -------- x/proof/types/account_query_client.go | 2 ++ x/proof/types/application_query_client.go | 6 ++-- 7 files changed, 48 insertions(+), 47 deletions(-) delete mode 100644 x/proof/keeper/option.go diff --git a/pkg/crypto/pubkey_client/client.go b/pkg/crypto/pubkey_client/client.go index 2ef70c2d3..0b753026a 100644 --- a/pkg/crypto/pubkey_client/client.go +++ b/pkg/crypto/pubkey_client/client.go @@ -16,7 +16,7 @@ var _ crypto.PubKeyClient = (*pubKeyClient)(nil) // querier to get the public key of an address. type pubKeyClient struct { // accountQuerier is the querier for the account module, it is used to get - // the account of an address. + // the public key corresponding to an address. accountQuerier client.AccountQueryClient } diff --git a/pkg/crypto/rings/cache.go b/pkg/crypto/rings/cache.go index a3c66ad9d..f4ff001e4 100644 --- a/pkg/crypto/rings/cache.go +++ b/pkg/crypto/rings/cache.go @@ -29,6 +29,8 @@ type ringCache struct { // invalidate ringsByAddr entries for rings that have been updated on chain. delegationClient client.DelegationClient + // ringClient is used to retrieve the rings that are cached and verify relay + // request signatures against the rings. ringClient crypto.RingClient } diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 03f572e15..1070a92f6 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -66,11 +66,6 @@ func (rc *ringClient) GetRingForAddress( if err != nil { return nil, err } - // Cache the ring's points for future use - rc.logger.Debug(). - Str("app_address", appAddress). - Msg("updating ring ringsByAddr for app") - // Get the points on the secp256k1 curve for the public keys in the ring. points, err := pointsFromPublicKeys(pubKeys...) if err != nil { @@ -87,43 +82,45 @@ func (rc *ringClient) VerifyRelayRequestSignature( ctx context.Context, relayRequest *types.RelayRequest, ) error { + if relayRequest.GetMeta() == nil { + return ErrRingClientInvalidRelayRequest.Wrap("missing meta from relay request") + } + + sessionHeader := relayRequest.GetMeta().GetSessionHeader() + if err := sessionHeader.ValidateBasic(); err != nil { + return ErrRingClientInvalidRelayRequest.Wrapf("invalid session header: %q", err) + } + rc.logger.Debug(). Fields(map[string]any{ - "session_id": relayRequest.Meta.SessionHeader.SessionId, - "application_address": relayRequest.Meta.SessionHeader.ApplicationAddress, - "service_id": relayRequest.Meta.SessionHeader.Service.Id, + "session_id": sessionHeader.GetSessionId(), + "application_address": sessionHeader.GetApplicationAddress(), + "service_id": sessionHeader.GetService().GetId(), }). Msg("verifying relay request signature") // Extract the relay request's ring signature - if relayRequest.Meta == nil { - return ErrRingClientEmptyRelayRequestSignature.Wrapf( - "request payload: %s", relayRequest.Payload, - ) - } - - signature := relayRequest.Meta.Signature - if signature == nil { - return ErrRingClientInvalidRelayRequest.Wrapf( - "missing signature from relay request: %v", relayRequest, - ) + if relayRequest.GetMeta().GetSignature() == nil { + return ErrRingClientInvalidRelayRequest.Wrap("missing signature from relay request") } + signature := relayRequest.GetMeta().GetSignature() ringSig := new(ring.RingSig) + if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { return ErrRingClientInvalidRelayRequestSignature.Wrapf( - "error deserializing ring signature: %v", err, + "error deserializing ring signature: %s", err, ) } - if relayRequest.Meta.SessionHeader.ApplicationAddress == "" { + if sessionHeader.GetApplicationAddress() == "" { return ErrRingClientInvalidRelayRequest.Wrap( "missing application address from relay request", ) } // Get the ring for the application address of the relay request - appAddress := relayRequest.Meta.SessionHeader.ApplicationAddress + appAddress := sessionHeader.GetApplicationAddress() appRing, err := rc.GetRingForAddress(ctx, appAddress) if err != nil { return ErrRingClientInvalidRelayRequest.Wrapf( diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 93c380608..d2f4dd897 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -17,8 +17,8 @@ import ( ) const ( - // TODO_TECHDEBT: relayDifficultyBits should be a governance-based parameter - relayDifficultyBits = 0 + // TODO_TECHDEBT: relayMinDifficultyBits should be a governance-based parameter + relayMinDifficultyBits = 0 // sumSize is the size of the sum of the relay request and response // in bytes. This is used to extract the relay request and response @@ -241,7 +241,7 @@ func compareSessionHeaders( ) error { if sessionHeader.GetApplicationAddress() != expectedSessionHeader.GetApplicationAddress() { return types.ErrProofInvalidRelay.Wrapf( - "sessionHeaders application addresses mismatch expect: %s, got: %s", + "sessionHeaders application addresses mismatch expect: %q, got: %q", expectedSessionHeader.GetApplicationAddress(), sessionHeader.GetApplicationAddress(), ) @@ -249,7 +249,7 @@ func compareSessionHeaders( if sessionHeader.GetService().GetId() != expectedSessionHeader.GetService().GetId() { return types.ErrProofInvalidRelay.Wrapf( - "sessionHeaders service IDs mismatch expect: %s, got: %s", + "sessionHeaders service IDs mismatch expect: %q, got: %q", expectedSessionHeader.GetService().GetId(), sessionHeader.GetService().GetId(), ) @@ -273,7 +273,7 @@ func compareSessionHeaders( if sessionHeader.GetSessionId() != expectedSessionHeader.GetSessionId() { return types.ErrProofInvalidRelay.Wrapf( - "sessionHeaders session IDs mismatch expect: %s, got: %s", + "sessionHeaders session IDs mismatch expect: %q, got: %q", expectedSessionHeader.GetSessionId(), sessionHeader.GetSessionId(), ) @@ -318,11 +318,11 @@ func validateMiningDifficulty(relayBz []byte) error { ) } - if difficultyBits < relayDifficultyBits { + if difficultyBits < relayMinDifficultyBits { return types.ErrProofInvalidRelay.Wrapf( "relay difficulty %d is less than the required difficulty %d", difficultyBits, - relayDifficultyBits, + relayMinDifficultyBits, ) } @@ -335,6 +335,20 @@ func (k msgServer) validateClosestPath( proof *smt.SparseMerkleClosestProof, sessionHeader *sessiontypes.SessionHeader, ) error { + // The RelayMiner has to wait until the createClaimWindowStartHeight and the + // submitProofWindowStartHeight are open to respectively create the claim and + // submit the proof. + // These windows are calculated as SessionEndBlockHeight + GracePeriodBlockCount. + // see: relayerSessionsManager.waitForEarliest{CreateClaim,SubmitProof}Height(). + // The RelayMiner hast to wait this long to ensure that late relays are accepted + // and included in the SessionNumber=N tree. + // (i.e. relays initiated by Applications/Gateways in SessionNumber=N but + // arriving to the RelayMiner in SessionNumber=N + 1) + // Otherwise, the RelayMiner would not be able to include the late relays in + // the SessionNumber N claim and the proof. + // Since smt.ProveClosest is in terms of submitProofWindowStartHeight, this + // block's hash needs to be used for validation too. + // TODO_TECHDEBT(#409): Reference the session rollover documentation here. blockHeight := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() blockHash := k.sessionKeeper.GetBlockHash(ctx, blockHeight) diff --git a/x/proof/keeper/option.go b/x/proof/keeper/option.go deleted file mode 100644 index 502e438b1..000000000 --- a/x/proof/keeper/option.go +++ /dev/null @@ -1,14 +0,0 @@ -package keeper - -import "github.com/pokt-network/poktroll/pkg/crypto" - -// KeeperOption is a function that can be optionally passed to the keeper constructor -// to modify its initialization behavior. -type KeeperOption func(*Keeper) - -// WithRingClient overrides the RingClient that the keeper will use with the given client. -func WithRingClient(client crypto.RingClient) KeeperOption { - return func(keeper *Keeper) { - keeper.ringClient = client - } -} diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index c112e8a0e..0262bc158 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -15,6 +15,8 @@ type AccountKeeperQueryClient struct { keeper AccountKeeper } +// NewAccountKeeperQueryClient returns a new AccountQueryClient that is backed +// by an AccountKeeper instance. func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQueryClient { return &AccountKeeperQueryClient{keeper: accountKeeper} } diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go index b05bd64b5..b0164c416 100644 --- a/x/proof/types/application_query_client.go +++ b/x/proof/types/application_query_client.go @@ -7,14 +7,14 @@ import ( apptypes "github.com/pokt-network/poktroll/x/application/types" ) -var ( - _ client.ApplicationQueryClient = (*AppKeeperQueryClient)(nil) -) +var _ client.ApplicationQueryClient = (*AppKeeperQueryClient)(nil) type AppKeeperQueryClient struct { keeper ApplicationKeeper } +// NewAppKeeperQueryClient returns a new ApplicationQueryClient that is backed +// by an ApplicationKeeper instance. func NewAppKeeperQueryClient(appKeeper ApplicationKeeper) client.ApplicationQueryClient { return &AppKeeperQueryClient{keeper: appKeeper} } From e4484bee31a80761f0b60a3d42653d1b78caf858 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Fri, 8 Mar 2024 14:51:40 +0100 Subject: [PATCH 07/23] chore: Update keeper query clients in-code documentation --- pkg/crypto/rings/client.go | 5 +++++ x/proof/types/account_query_client.go | 5 +++++ x/proof/types/application_query_client.go | 6 ++++++ 3 files changed, 16 insertions(+) diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 1070a92f6..7ebd785bd 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -18,6 +18,11 @@ import ( var _ crypto.RingClient = (*ringClient)(nil) +// ringClient is an implementation of the RingClient interface that uses the +// client.ApplicationQueryClient to get applications delegation information and +// needed to construct the ring for signing relay requests. +// It also uses the pubkeyclient.PubKeyClient to get the public keys for the +// addresses in the ring. type ringClient struct { // logger is the logger for the ring cache. logger polylog.Logger diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index 0262bc158..4cf53aca7 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -17,6 +17,11 @@ type AccountKeeperQueryClient struct { // NewAccountKeeperQueryClient returns a new AccountQueryClient that is backed // by an AccountKeeper instance. +// It is used by the PubKeyClient to get the public key that corresponds to the +// provided address. +// This implementation is a thin wrapper around the AccountKeeper and does +// not rely on the QueryClient contrariwise to the off-chain implementation. +// It should be injected into the PubKeyClient when initialized from within the a keeper. func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQueryClient { return &AccountKeeperQueryClient{keeper: accountKeeper} } diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go index b0164c416..4a1d61252 100644 --- a/x/proof/types/application_query_client.go +++ b/x/proof/types/application_query_client.go @@ -15,10 +15,16 @@ type AppKeeperQueryClient struct { // NewAppKeeperQueryClient returns a new ApplicationQueryClient that is backed // by an ApplicationKeeper instance. +// It is used by the RingClient to get the applications that are delegated to +// by a given application. +// This implementation is a thin wrapper around the ApplicationKeeper and does +// not rely on the QueryClient contrariwise to the off-chain implementation. +// It should be injected into the RingClient when initialized from within the a keeper. func NewAppKeeperQueryClient(appKeeper ApplicationKeeper) client.ApplicationQueryClient { return &AppKeeperQueryClient{keeper: appKeeper} } +// GetApplication returns the application corresponding to the given address. func (appQueryClient *AppKeeperQueryClient) GetApplication( ctx context.Context, appAddr string, From 3f277e7d21bd709a959342acd1605d111a6162de Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Fri, 8 Mar 2024 14:59:56 +0100 Subject: [PATCH 08/23] chore: Add godoc comments --- x/proof/types/account_query_client.go | 1 + x/proof/types/application_query_client.go | 1 + 2 files changed, 2 insertions(+) diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index 4cf53aca7..cce747858 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -26,6 +26,7 @@ func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQuer return &AccountKeeperQueryClient{keeper: accountKeeper} } +// GetAccount returns the account associated with the provided address. func (accountQueryClient *AccountKeeperQueryClient) GetAccount( ctx context.Context, addr string, diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go index 4a1d61252..2ecd57f7e 100644 --- a/x/proof/types/application_query_client.go +++ b/x/proof/types/application_query_client.go @@ -33,6 +33,7 @@ func (appQueryClient *AppKeeperQueryClient) GetApplication( return app, nil } +// GetAllApplications returns all the applications in the application store. func (appQueryClient *AppKeeperQueryClient) GetAllApplications(ctx context.Context) ([]apptypes.Application, error) { return appQueryClient.keeper.GetAllApplications(ctx), nil } From 2d5af98a2656db084db7b792ff9d20f63b2bf8e4 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Mon, 11 Mar 2024 20:18:00 +0100 Subject: [PATCH 09/23] fix: test error msg assertion --- pkg/relayer/proxy/proxy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/relayer/proxy/proxy_test.go b/pkg/relayer/proxy/proxy_test.go index 5ddba5d5f..ceccb2dd7 100644 --- a/pkg/relayer/proxy/proxy_test.go +++ b/pkg/relayer/proxy/proxy_test.go @@ -419,7 +419,7 @@ func TestRelayerProxy_Relays(t *testing.T) { inputScenario: sendRequestWithMissingSessionHeaderApplicationAddress, expectedErrCode: -32000, - expectedErrMsg: "missing application address from relay request", + expectedErrMsg: "invalid session header: invalid application address", }, { desc: "Non staked application address", From 4bf0b9fcb03ee5f19e83728d9a15b60083729260 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Wed, 13 Mar 2024 20:49:40 +0100 Subject: [PATCH 10/23] chore: Address review change requests --- pkg/client/interface.go | 3 +- pkg/client/query/accquerier.go | 3 +- pkg/crypto/interface.go | 2 +- pkg/crypto/rings/cache.go | 10 +-- pkg/crypto/rings/client.go | 29 +++------ pkg/crypto/rings/ring.go | 4 +- pkg/relayer/proxy/errors.go | 6 +- pkg/relayer/proxy/relay_verifier.go | 12 ++-- pkg/sdk/deps_builder.go | 4 +- pkg/sdk/sdk.go | 12 ++-- pkg/sdk/send_relay.go | 14 +++-- x/proof/keeper/keeper.go | 5 +- x/proof/keeper/msg_server_submit_proof.go | 77 ++++++++++++++--------- x/proof/module/module.go | 7 +-- x/proof/module/simulation.go | 8 +-- x/proof/simulation/create_claim.go | 1 - x/proof/simulation/submit_proof.go | 1 - x/proof/types/account_query_client.go | 7 +-- x/proof/types/application_query_client.go | 16 +++-- x/proof/types/errors.go | 4 +- x/proof/types/expected_keepers.go | 6 -- x/service/types/relay.go | 50 +++++++-------- 22 files changed, 139 insertions(+), 142 deletions(-) diff --git a/pkg/client/interface.go b/pkg/client/interface.go index da18a8790..6393252cf 100644 --- a/pkg/client/interface.go +++ b/pkg/client/interface.go @@ -20,7 +20,6 @@ import ( cosmosclient "github.com/cosmos/cosmos-sdk/client" cosmoskeyring "github.com/cosmos/cosmos-sdk/crypto/keyring" cosmostypes "github.com/cosmos/cosmos-sdk/types" - accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/pokt-network/smt" "github.com/pokt-network/poktroll/pkg/either" @@ -231,7 +230,7 @@ type SupplierClientOption func(SupplierClient) // on-chain account information type AccountQueryClient interface { // GetAccount queries the chain for the details of the account provided - GetAccount(ctx context.Context, address string) (accounttypes.AccountI, error) + GetAccount(ctx context.Context, address string) (cosmostypes.AccountI, error) } // ApplicationQueryClient defines an interface that enables the querying of the diff --git a/pkg/client/query/accquerier.go b/pkg/client/query/accquerier.go index c61ec9760..5d0d5eaab 100644 --- a/pkg/client/query/accquerier.go +++ b/pkg/client/query/accquerier.go @@ -4,6 +4,7 @@ import ( "context" "cosmossdk.io/depinject" + "github.com/cosmos/cosmos-sdk/types" accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" grpc "github.com/cosmos/gogoproto/grpc" @@ -44,7 +45,7 @@ func NewAccountQuerier(deps depinject.Config) (client.AccountQueryClient, error) func (aq *accQuerier) GetAccount( ctx context.Context, address string, -) (accounttypes.AccountI, error) { +) (types.AccountI, error) { req := &accounttypes.QueryAccountRequest{Address: address} res, err := aq.accountQuerier.Account(ctx, req) if err != nil { diff --git a/pkg/crypto/interface.go b/pkg/crypto/interface.go index 36302d277..da4e8f74d 100644 --- a/pkg/crypto/interface.go +++ b/pkg/crypto/interface.go @@ -30,7 +30,7 @@ type RingCache interface { } // RingClient is used to construct rings by querying the application module for -// the addresses of the gateways the application is delegated to, and converting +// the addresses of the gateways the application delegated to, and converting // them into their corresponding public key points on the secp256k1 curve. type RingClient interface { // GetRingForAddress returns the ring for the given application address if diff --git a/pkg/crypto/rings/cache.go b/pkg/crypto/rings/cache.go index f4ff001e4..b06db1826 100644 --- a/pkg/crypto/rings/cache.go +++ b/pkg/crypto/rings/cache.go @@ -69,8 +69,8 @@ func NewRingCache(deps depinject.Config) (_ crypto.RingCache, err error) { // Start starts the ring cache by subscribing to on-chain redelegation events. func (rc *ringCache) Start(ctx context.Context) { rc.logger.Info().Msg("starting ring ringsByAddr") - // Listen for redelegation events and invalidate the cache if contains a ring - // corresponding to the redelegation event's address . + // Listen for redelegation events and invalidate the cache if it contains an + // address corresponding to the redelegation event's. go func() { select { case <-ctx.Done(): @@ -82,7 +82,7 @@ func (rc *ringCache) Start(ctx context.Context) { } // goInvalidateCache listens for redelegation events and invalidates the -// cache if ring corresponding to the app address in the redelegation event +// cache if the ring corresponding to the app address in the redelegation event // exists in the cache. // This function is intended to be run in a goroutine. func (rc *ringCache) goInvalidateCache(ctx context.Context) { @@ -154,10 +154,10 @@ func (rc *ringCache) GetRingForAddress( // Add the address points to the cache. rc.ringsByAddr[appAddress] = ring } else { - // If the ring is in the cache, create it from the points. + // Use the ring if it is present in the cache. rc.logger.Debug(). Str("app_address", appAddress). - Msg("ring ringsByAddr hit; creating from points") + Msg("ring ringsByAddr hit; using cached ring") } if err != nil { return nil, err diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 7ebd785bd..717e0e237 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -19,10 +19,8 @@ import ( var _ crypto.RingClient = (*ringClient)(nil) // ringClient is an implementation of the RingClient interface that uses the -// client.ApplicationQueryClient to get applications delegation information and +// client.ApplicationQueryClient to get application's delegation information // needed to construct the ring for signing relay requests. -// It also uses the pubkeyclient.PubKeyClient to get the public keys for the -// addresses in the ring. type ringClient struct { // logger is the logger for the ring cache. logger polylog.Logger @@ -31,7 +29,7 @@ type ringClient struct { // used to get the addresses of the gateways an application is delegated to. applicationQuerier client.ApplicationQueryClient - // pubKeyClient + // pubKeyClient is used to get the public keys given an address. pubKeyClient crypto.PubKeyClient } @@ -104,27 +102,20 @@ func (rc *ringClient) VerifyRelayRequestSignature( }). Msg("verifying relay request signature") - // Extract the relay request's ring signature + // Extract the relay request's ring signature. if relayRequest.GetMeta().GetSignature() == nil { return ErrRingClientInvalidRelayRequest.Wrap("missing signature from relay request") } signature := relayRequest.GetMeta().GetSignature() ringSig := new(ring.RingSig) - if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { return ErrRingClientInvalidRelayRequestSignature.Wrapf( "error deserializing ring signature: %s", err, ) } - if sessionHeader.GetApplicationAddress() == "" { - return ErrRingClientInvalidRelayRequest.Wrap( - "missing application address from relay request", - ) - } - - // Get the ring for the application address of the relay request + // Get the ring for the application address of the relay request. appAddress := sessionHeader.GetApplicationAddress() appRing, err := rc.GetRingForAddress(ctx, appAddress) if err != nil { @@ -133,30 +124,28 @@ func (rc *ringClient) VerifyRelayRequestSignature( ) } - // Verify the ring signature against the ring + // Verify the ring signature against the app ring. if !ringSig.Ring().Equals(appRing) { return ErrRingClientInvalidRelayRequestSignature.Wrapf( "ring signature does not match ring for application address %s", appAddress, ) } - // Get and hash the signable bytes of the relay request + // Get and hash the signable bytes of the relay request. requestSignableBz, err := relayRequest.GetSignableBytesHash() if err != nil { return ErrRingClientInvalidRelayRequest.Wrapf("error getting signable bytes: %v", err) } - // Verify the relay request's signature + // Verify the relay request's signature. if valid := ringSig.Verify(requestSignableBz); !valid { return ErrRingClientInvalidRelayRequestSignature.Wrapf("invalid ring signature") } return nil } -// getDelegatedPubKeysForAddress returns the ring used to sign a message for -// the given application address, by querying the application module for it's -// delegated pubkeys and converting them to points on the secp256k1 curve in -// order to create the ring. +// getDelegatedPubKeysForAddress returns the public keys used to sign a +// relay request for the given application address. func (rc *ringClient) getDelegatedPubKeysForAddress( ctx context.Context, appAddress string, diff --git a/pkg/crypto/rings/ring.go b/pkg/crypto/rings/ring.go index 35d023101..5b8d815a0 100644 --- a/pkg/crypto/rings/ring.go +++ b/pkg/crypto/rings/ring.go @@ -8,13 +8,13 @@ import ( "github.com/noot/ring-go" ) -// newRingFromPoints creates a new ring from points on the secp256k1 curve +// newRingFromPoints creates a new ring from points (i.e. public keys) on the secp256k1 curve func newRingFromPoints(points []ringtypes.Point) (*ring.Ring, error) { return ring.NewFixedKeyRingFromPublicKeys(ring_secp256k1.NewCurve(), points) } // pointsFromPublicKeys returns the secp256k1 points for the given public keys. -// It returns an errof if any of the keys is not a secp256k1 key. +// It returns an error if any of the keys is not on the secp256k1 curve. func pointsFromPublicKeys( publicKeys ...cryptotypes.PubKey, ) (points []ringtypes.Point, err error) { diff --git a/pkg/relayer/proxy/errors.go b/pkg/relayer/proxy/errors.go index 9650ee577..3bf45da08 100644 --- a/pkg/relayer/proxy/errors.go +++ b/pkg/relayer/proxy/errors.go @@ -4,10 +4,10 @@ import sdkerrors "cosmossdk.io/errors" var ( codespace = "relayer_proxy" - ErrRelayerProxyUnsupportedRPCType = sdkerrors.Register(codespace, 1, "unsupported relayer proxy rpc type") + ErrRelayerProxyUnsupportedRPCType = sdkerrors.Register(codespace, 1, "unsupported rpc type") ErrRelayerProxyInvalidSession = sdkerrors.Register(codespace, 2, "invalid session in relayer request") - ErrRelayerProxyInvalidSupplier = sdkerrors.Register(codespace, 3, "invalid relayer proxy supplier") - ErrRelayerProxyUndefinedSigningKeyName = sdkerrors.Register(codespace, 4, "undefined relayer proxy signing key name") + ErrRelayerProxyInvalidSupplier = sdkerrors.Register(codespace, 3, "supplier does not belong to session") + ErrRelayerProxyUndefinedSigningKeyName = sdkerrors.Register(codespace, 4, "supplier signing key name is undefined") ErrRelayerProxyUndefinedProxiedServicesEndpoints = sdkerrors.Register(codespace, 5, "undefined proxied services endpoints for relayer proxy") ErrRelayerProxyInvalidRelayRequest = sdkerrors.Register(codespace, 6, "invalid relay request") ErrRelayerProxyInvalidRelayResponse = sdkerrors.Register(codespace, 7, "invalid relay response") diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index e347c9ee5..2883fd1b5 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -18,17 +18,19 @@ func (rp *relayerProxy) VerifyRelayRequest( return err } + sessionHeader := relayRequest.GetMeta().GetSessionHeader() + // Application address is used to verify the relayRequest signature, it is // guaranteed to be present in the relayRequest since the signature has already // been verified. - appAddress := relayRequest.GetMeta().GetSessionHeader().GetApplicationAddress() + appAddress := sessionHeader.GetApplicationAddress() // Query for the current session to check if relayRequest sessionId matches the current session. rp.logger.Debug(). Fields(map[string]any{ - "session_id": relayRequest.GetMeta().GetSessionHeader().GetSessionId(), - "application_address": relayRequest.GetMeta().GetSessionHeader().GetApplicationAddress(), - "service_id": relayRequest.GetMeta().GetSessionHeader().GetService().GetId(), + "session_id": sessionHeader.GetSessionId(), + "application_address": sessionHeader.GetApplicationAddress(), + "service_id": sessionHeader.GetService().GetId(), }). Msg("verifying relay request session") @@ -55,7 +57,7 @@ func (rp *relayerProxy) VerifyRelayRequest( // we can reduce the session validity check to checking if the retrieved session's sessionId // matches the relayRequest sessionId. // TODO_INVESTIGATE: Revisit the assumptions above at some point in the future, but good enough for now. - if session.SessionId != relayRequest.GetMeta().GetSessionHeader().GetSessionId() { + if session.SessionId != sessionHeader.GetSessionId() { return ErrRelayerProxyInvalidSession.Wrapf( "session mismatch, expecting: %+v, got: %+v", session.Header, diff --git a/pkg/sdk/deps_builder.go b/pkg/sdk/deps_builder.go index f1359c5c3..2de34d6b2 100644 --- a/pkg/sdk/deps_builder.go +++ b/pkg/sdk/deps_builder.go @@ -55,13 +55,13 @@ func (sdk *poktrollSDK) buildDeps( } deps = depinject.Configs(deps, depinject.Supply(grpcClient)) - // Create and supply the account querier to the pubKey Client + // Create the account querier and add it to the pubKey client required dependencies. accountQuerier, err := query.NewAccountQuerier(deps) if err != nil { return nil, err } - // Create and supply the pubKey Client + // Create the pubKey client and add it to the required dependencies pubKeyClientDeps := depinject.Supply(accountQuerier) pubKeyClient, err := pubkeyclient.NewPubKeyClient(pubKeyClientDeps) if err != nil { diff --git a/pkg/sdk/sdk.go b/pkg/sdk/sdk.go index aa3474a57..8b7c3a47e 100644 --- a/pkg/sdk/sdk.go +++ b/pkg/sdk/sdk.go @@ -61,12 +61,13 @@ type poktrollSDK struct { // It is used to get the current block height to query for the current session. blockClient client.BlockClient - // pubKeyClient the client used to get the public key given an account address. - // TODO_TECHDEBT: Add a size limit to the cache. + // pubKeyClient the client used to get the public key for a given account address. + // TODO_TECHDEBT: Add a size limit to the cache. Also consider clearing it every + // N sessions. pubKeyClient crypto.PubKeyClient // supplierPubKeyCache is a cache of the suppliers pubKeys that has been queried. - // TODO_TECHDEBT: Add a size limit to the cache. + // TODO_TECHDEBT: Add a size limit to the cache and consider an LRU cache. supplierPubKeyCache map[string]cryptotypes.PubKey } @@ -114,10 +115,9 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK return sdk, nil } -// getPubKeyFromAddress returns the public key of the given address. +// getPubKeyFromAddress returns the public key of a supplier given its address. // It uses the accountQuerier to get the account if it is not already in the cache. -// If the account does not have a public key, it returns an error. -func (sdk *poktrollSDK) getPubKeyFromAddress( +func (sdk *poktrollSDK) getSupplierPubKeyFromAddress( ctx context.Context, address string, ) (cryptotypes.PubKey, error) { diff --git a/pkg/sdk/send_relay.go b/pkg/sdk/send_relay.go index 16a338344..035f71c0b 100644 --- a/pkg/sdk/send_relay.go +++ b/pkg/sdk/send_relay.go @@ -105,17 +105,21 @@ func (sdk *poktrollSDK) SendRelay( return nil, ErrSDKHandleRelay.Wrapf("%s", err) } + // relayResponse.ValidateBasic also validates Meta and SessionHeader, + // we can safely use the session header. + sessionHeader := relayResponse.GetMeta().GetSessionHeader() + // Get the supplier's public key. - supplierPubKey, err := sdk.getPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) + supplierPubKey, err := sdk.getSupplierPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) if err != nil { return nil, ErrSDKHandleRelay.Wrapf("error getting supplier public key: %s", err) } sdk.logger.Debug(). Str("supplier", supplierEndpoint.SupplierAddress). - Str("application", relayResponse.GetMeta().GetSessionHeader().GetApplicationAddress()). - Str("service", relayResponse.GetMeta().GetSessionHeader().GetService().GetId()). - Int64("end_height", relayResponse.GetMeta().GetSessionHeader().GetSessionEndBlockHeight()). + Str("application", sessionHeader.GetApplicationAddress()). + Str("service", sessionHeader.GetService().GetId()). + Int64("end_height", sessionHeader.GetSessionEndBlockHeight()). Msg("About to verify relay response signature.") // Verify the response signature. We use the supplier address that we got from @@ -124,7 +128,7 @@ func (sdk *poktrollSDK) SendRelay( // as in some relayer early failures, it may not be signed by the supplier. // TODO_IMPROVE: Add more logging & telemetry so we can get visibility and signal into // failed responses. - if err := relayResponse.VerifySignature(supplierPubKey); err != nil { + if err := relayResponse.VerifySupplierSignature(supplierPubKey); err != nil { return nil, ErrSDKVerifyResponseSignature.Wrapf("%s", err) } diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 3db7d25b4..4bb3319bb 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -31,8 +31,9 @@ type ( sessionKeeper types.SessionKeeper applicationKeeper types.ApplicationKeeper accountKeeper types.AccountKeeper - ringClient crypto.RingClient - pubKeyClient crypto.PubKeyClient + + ringClient crypto.RingClient + pubKeyClient crypto.PubKeyClient } ) diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index d2f4dd897..8980900b9 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -17,7 +17,9 @@ import ( ) const ( - // TODO_TECHDEBT: relayMinDifficultyBits should be a governance-based parameter + // relayMinDifficultyBits is the minimum difficulty that a relay must have to be + // volume / reward applicable. + // TODO_BLOCKER: relayMinDifficultyBits should be a governance-based parameter relayMinDifficultyBits = 0 // sumSize is the size of the sum of the relay request and response @@ -28,9 +30,18 @@ const ( sumSize = 8 ) +// SMT specification used for the proof verification. +var spec *smt.TrieSpec + +func init() { + // Use a no prehash spec that returns a nil value hasher for the proof + // verification to avoid hashing the value twice. + spec = smt.NoPrehashSpec(sha256.New(), true) +} + func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) (*types.MsgSubmitProofResponse, error) { // TODO_BLOCKER: Prevent Proof upserts after the tokenomics module has processes the respective session. - logger := k.Logger().With("method", "SubmitProof") + logger := k.Logger().With("TECHDEBTmethod", "SubmitProof") logger.Debug("submitting proof") /* @@ -69,6 +80,11 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return nil, status.Error(codes.InvalidArgument, err.Error()) } + supplierPubKey, err := k.pubKeyClient.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) + if err != nil { + return nil, status.Error(codes.FailedPrecondition, err.Error()) + } + if _, err := k.queryAndValidateSessionHeader( ctx, msg.GetSessionHeader(), @@ -115,13 +131,8 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return nil, status.Error(codes.FailedPrecondition, err.Error()) } - supplierPubKey, err := k.pubKeyClient.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) - if err != nil { - return nil, status.Error(codes.FailedPrecondition, err.Error()) - } - // Verify the relay response's signature. - if err := relay.GetRes().VerifySignature(supplierPubKey); err != nil { + if err := relay.GetRes().VerifySupplierSignature(supplierPubKey); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } @@ -174,8 +185,9 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return &types.MsgSubmitProofResponse{}, nil } -// queryAndValidateClaimForProof ensures that a claim corresponding to the given proof's -// session exists & has a matching supplier address and session header. +// queryAndValidateClaimForProof ensures that a claim corresponding to the given +// proof's session exists & has a matching supplier address and session header, +// it then returns the corresponding claim if the validation is successful. func (k msgServer) queryAndValidateClaimForProof( ctx context.Context, msg *types.MsgSubmitProof, @@ -235,10 +247,7 @@ func (k msgServer) queryAndValidateClaimForProof( } // compareSessionHeaders compares a session header against an expected session header. -func compareSessionHeaders( - expectedSessionHeader *sessiontypes.SessionHeader, - sessionHeader *sessiontypes.SessionHeader, -) error { +func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.SessionHeader) error { if sessionHeader.GetApplicationAddress() != expectedSessionHeader.GetApplicationAddress() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders application addresses mismatch expect: %q, got: %q", @@ -255,6 +264,14 @@ func compareSessionHeaders( ) } + if sessionHeader.GetService().GetName() != expectedSessionHeader.GetService().GetName() { + return types.ErrProofInvalidRelay.Wrapf( + "sessionHeaders service names mismatch expect: %q, got: %q", + expectedSessionHeader.GetService().GetName(), + sessionHeader.GetService().GetName(), + ) + } + if sessionHeader.GetSessionStartBlockHeight() != expectedSessionHeader.GetSessionStartBlockHeight() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders session start heights mismatch expect: %d, got: %d", @@ -287,8 +304,6 @@ func verifyClosestProof( proof *smt.SparseMerkleClosestProof, expectedRootHash []byte, ) error { - spec := smt.NoPrehashSpec(sha256.New(), true) - valid, err := smt.VerifyClosestProof(proof, expectedRootHash, spec) if err != nil { return err @@ -308,9 +323,9 @@ func verifyClosestProof( func validateMiningDifficulty(relayBz []byte) error { hasher := sha256.New() hasher.Write(relayBz) - realyHash := hasher.Sum(nil) + relayHash := hasher.Sum(nil) - difficultyBits, err := protocol.CountDifficultyBits(realyHash) + difficultyBits, err := protocol.CountDifficultyBits(relayHash) if err != nil { return types.ErrProofInvalidRelay.Wrapf( "error counting difficulty bits: %s", @@ -318,6 +333,8 @@ func validateMiningDifficulty(relayBz []byte) error { ) } + // TODO: Devise a test that tries to attack the network and ensure that there + // is sufficient telemetry. if difficultyBits < relayMinDifficultyBits { return types.ErrProofInvalidRelay.Wrapf( "relay difficulty %d is less than the required difficulty %d", @@ -337,20 +354,18 @@ func (k msgServer) validateClosestPath( ) error { // The RelayMiner has to wait until the createClaimWindowStartHeight and the // submitProofWindowStartHeight are open to respectively create the claim and - // submit the proof. - // These windows are calculated as SessionEndBlockHeight + GracePeriodBlockCount. - // see: relayerSessionsManager.waitForEarliest{CreateClaim,SubmitProof}Height(). - // The RelayMiner hast to wait this long to ensure that late relays are accepted - // and included in the SessionNumber=N tree. - // (i.e. relays initiated by Applications/Gateways in SessionNumber=N but - // arriving to the RelayMiner in SessionNumber=N + 1) - // Otherwise, the RelayMiner would not be able to include the late relays in - // the SessionNumber N claim and the proof. - // Since smt.ProveClosest is in terms of submitProofWindowStartHeight, this - // block's hash needs to be used for validation too. + // submit the proof respectively. + // These windows are calculated as (SessionEndBlockHeight + GracePeriodBlockCount). + // For reference, see relayerSessionsManager.waitForEarliest{CreateClaim,SubmitProof}Height(). + // The RelayMiner has to wait this long to ensure that late relays (i.e. + // submitted during SessionNumber=(N+1) but created during SessionNumber=N) are + // still included as part of SessionNumber=N. + // Since smt.ProveClosest is defined in terms of submitProofWindowStartHeight, + // this block's hash needs to be used for validation too. // TODO_TECHDEBT(#409): Reference the session rollover documentation here. - blockHeight := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() - blockHash := k.sessionKeeper.GetBlockHash(ctx, blockHeight) + sessionEndWithGracePeriodBlockHeight := sessionHeader.GetSessionEndBlockHeight() + + sessionkeeper.GetSessionGracePeriodBlockCount() + blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndWithGracePeriodBlockHeight) if !bytes.Equal(proof.Path, blockHash) { return types.ErrProofInvalidProof.Wrapf( diff --git a/x/proof/module/module.go b/x/proof/module/module.go index c18584733..07ba82954 100644 --- a/x/proof/module/module.go +++ b/x/proof/module/module.go @@ -97,20 +97,17 @@ type AppModule struct { keeper keeper.Keeper accountKeeper types.AccountKeeper - bankKeeper types.BankKeeper } func NewAppModule( cdc codec.Codec, keeper keeper.Keeper, accountKeeper types.AccountKeeper, - bankKeeper types.BankKeeper, ) AppModule { return AppModule{ AppModuleBasic: NewAppModuleBasic(cdc), keeper: keeper, accountKeeper: accountKeeper, - bankKeeper: bankKeeper, } } @@ -177,10 +174,9 @@ type ModuleInputs struct { Config *modulev1.Module Logger log.Logger - AccountKeeper types.AccountKeeper - BankKeeper types.BankKeeper SessionKeeper types.SessionKeeper ApplicationKeeper types.ApplicationKeeper + AccountKeeper types.AccountKeeper } type ModuleOutputs struct { @@ -209,7 +205,6 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { in.Cdc, k, in.AccountKeeper, - in.BankKeeper, ) return ModuleOutputs{ProofKeeper: k, Module: m} diff --git a/x/proof/module/simulation.go b/x/proof/module/simulation.go index 0458e0998..0f10c54b1 100644 --- a/x/proof/module/simulation.go +++ b/x/proof/module/simulation.go @@ -67,7 +67,7 @@ func (am AppModule) WeightedOperations(simState module.SimulationState) []simtyp ) operations = append(operations, simulation.NewWeightedOperation( weightMsgCreateClaim, - proofsimulation.SimulateMsgCreateClaim(am.accountKeeper, am.bankKeeper, am.keeper), + proofsimulation.SimulateMsgCreateClaim(am.accountKeeper, am.keeper), )) var weightMsgSubmitProof int @@ -78,7 +78,7 @@ func (am AppModule) WeightedOperations(simState module.SimulationState) []simtyp ) operations = append(operations, simulation.NewWeightedOperation( weightMsgSubmitProof, - proofsimulation.SimulateMsgSubmitProof(am.accountKeeper, am.bankKeeper, am.keeper), + proofsimulation.SimulateMsgSubmitProof(am.accountKeeper, am.keeper), )) // this line is used by starport scaffolding # simapp/module/operation @@ -93,7 +93,7 @@ func (am AppModule) ProposalMsgs(simState module.SimulationState) []simtypes.Wei opWeightMsgCreateClaim, defaultWeightMsgCreateClaim, func(r *rand.Rand, ctx sdk.Context, accs []simtypes.Account) sdk.Msg { - proofsimulation.SimulateMsgCreateClaim(am.accountKeeper, am.bankKeeper, am.keeper) + proofsimulation.SimulateMsgCreateClaim(am.accountKeeper, am.keeper) return nil }, ), @@ -101,7 +101,7 @@ func (am AppModule) ProposalMsgs(simState module.SimulationState) []simtypes.Wei opWeightMsgSubmitProof, defaultWeightMsgSubmitProof, func(r *rand.Rand, ctx sdk.Context, accs []simtypes.Account) sdk.Msg { - proofsimulation.SimulateMsgSubmitProof(am.accountKeeper, am.bankKeeper, am.keeper) + proofsimulation.SimulateMsgSubmitProof(am.accountKeeper, am.keeper) return nil }, ), diff --git a/x/proof/simulation/create_claim.go b/x/proof/simulation/create_claim.go index 4abeddc02..08b6b7ddb 100644 --- a/x/proof/simulation/create_claim.go +++ b/x/proof/simulation/create_claim.go @@ -13,7 +13,6 @@ import ( func SimulateMsgCreateClaim( ak types.AccountKeeper, - bk types.BankKeeper, k keeper.Keeper, ) simtypes.Operation { return func(r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, diff --git a/x/proof/simulation/submit_proof.go b/x/proof/simulation/submit_proof.go index 4b0fa83c6..0b6bfeacb 100644 --- a/x/proof/simulation/submit_proof.go +++ b/x/proof/simulation/submit_proof.go @@ -13,7 +13,6 @@ import ( func SimulateMsgSubmitProof( ak types.AccountKeeper, - bk types.BankKeeper, k keeper.Keeper, ) simtypes.Operation { return func(r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index cce747858..115eefd7c 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -4,13 +4,14 @@ import ( "context" "github.com/cosmos/cosmos-sdk/types" - accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/pokt-network/poktroll/pkg/client" ) var _ client.AccountQueryClient = (*AccountKeeperQueryClient)(nil) +// AccountKeeperQueryClient is a thin wrapper around the AccountKeeper and does +// not rely on the QueryClient contrariwise to the off-chain implementation. type AccountKeeperQueryClient struct { keeper AccountKeeper } @@ -19,8 +20,6 @@ type AccountKeeperQueryClient struct { // by an AccountKeeper instance. // It is used by the PubKeyClient to get the public key that corresponds to the // provided address. -// This implementation is a thin wrapper around the AccountKeeper and does -// not rely on the QueryClient contrariwise to the off-chain implementation. // It should be injected into the PubKeyClient when initialized from within the a keeper. func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQueryClient { return &AccountKeeperQueryClient{keeper: accountKeeper} @@ -30,7 +29,7 @@ func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQuer func (accountQueryClient *AccountKeeperQueryClient) GetAccount( ctx context.Context, addr string, -) (accounttypes.AccountI, error) { +) (types.AccountI, error) { addrBz, err := types.AccAddressFromBech32(addr) if err != nil { return nil, err diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go index 2ecd57f7e..c652d2f1c 100644 --- a/x/proof/types/application_query_client.go +++ b/x/proof/types/application_query_client.go @@ -9,16 +9,16 @@ import ( var _ client.ApplicationQueryClient = (*AppKeeperQueryClient)(nil) +// AppKeeperQueryClient is a thin wrapper around the ApplicationKeeper and does +// not rely on the QueryClient contrariwise to the off-chain implementation. type AppKeeperQueryClient struct { keeper ApplicationKeeper } // NewAppKeeperQueryClient returns a new ApplicationQueryClient that is backed // by an ApplicationKeeper instance. -// It is used by the RingClient to get the applications that are delegated to -// by a given application. -// This implementation is a thin wrapper around the ApplicationKeeper and does -// not rely on the QueryClient contrariwise to the off-chain implementation. +// It is used by the RingClient to get the gateway address that an application +// has delegated its signing power to. // It should be injected into the RingClient when initialized from within the a keeper. func NewAppKeeperQueryClient(appKeeper ApplicationKeeper) client.ApplicationQueryClient { return &AppKeeperQueryClient{keeper: appKeeper} @@ -29,8 +29,12 @@ func (appQueryClient *AppKeeperQueryClient) GetApplication( ctx context.Context, appAddr string, ) (apptypes.Application, error) { - app, _ := appQueryClient.keeper.GetApplication(ctx, appAddr) - return app, nil + foundApp, appFound := appQueryClient.keeper.GetApplication(ctx, appAddr) + if !appFound { + return apptypes.Application{}, ErrProofApplicationNotFound + } + + return foundApp, nil } // GetAllApplications returns all the applications in the application store. diff --git a/x/proof/types/errors.go b/x/proof/types/errors.go index bc8e45db7..acaefcd31 100644 --- a/x/proof/types/errors.go +++ b/x/proof/types/errors.go @@ -22,7 +22,5 @@ var ( ErrProofInvalidRelayRequest = sdkerrors.Register(ModuleName, 1113, "invalid relay request") ErrProofInvalidRelayResponse = sdkerrors.Register(ModuleName, 1114, "invalid relay response") ErrProofNotSecp256k1Curve = sdkerrors.Register(ModuleName, 1115, "not secp256k1 curve") - //ErrProofUnauthorized = sdkerrors.Register(ModuleName, 1116, "unauthorized supplier signer") - //ErrProofInvalidServiceConfig = sdkerrors.Register(ModuleName, 1117, "invalid service config") - //ErrProofInvalidClosestMerkleProof = sdkerrors.Register(ModuleName, 1118, "invalid closest merkle proof") + ErrProofApplicationNotFound = sdkerrors.Register(ModuleName, 1116, "application not found") ) diff --git a/x/proof/types/expected_keepers.go b/x/proof/types/expected_keepers.go index cb6758d32..410b89141 100644 --- a/x/proof/types/expected_keepers.go +++ b/x/proof/types/expected_keepers.go @@ -21,12 +21,6 @@ type AccountKeeper interface { GetAccount(context.Context, sdk.AccAddress) sdk.AccountI } -// BankKeeper defines the expected interface for the Bank module. -type BankKeeper interface { - SpendableCoins(context.Context, sdk.AccAddress) sdk.Coins - // Methods imported from bank should be defined here -} - // ApplicationKeeper defines the expected application keeper to retrieve applications type ApplicationKeeper interface { GetApplication(ctx context.Context, address string) (app apptypes.Application, found bool) diff --git a/x/service/types/relay.go b/x/service/types/relay.go index e0ef6c2c6..9bd509cb2 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -6,24 +6,20 @@ import ( cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" ) -// getSignableBytes returns the bytes resulting from marshaling the relay request -// A value receiver is used to avoid overwriting any pre-existing signature -func (req RelayRequest) getSignableBytes() ([]byte, error) { - // set signature to nil - req.Meta.Signature = nil - - return req.Marshal() -} - // GetSignableBytesHash returns the hash of the signable bytes of the relay request // Hashing the marshaled request message guarantees that the signable bytes are // always of a constant and expected length. func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { // Save the signature and restore it after getting the signable bytes - // since getSignableBytes sets the signature to nil but does not preserve - // its original value. + // Since req.Meta is a pointer, this approach is not concurrent safe, + // if two goroutines are calling this method at the same time, the last one + // could get the nil signature resulting form the first go routine and restore + // nil after getting the signable bytes. + // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue + // by having req.Meta as a non-pointer type in the corresponding proto file. signature := req.Meta.Signature - requestBz, err := req.getSignableBytes() + req.Meta.Signature = nil + requestBz, err := req.Marshal() // Set the signature back to its original value req.Meta.Signature = signature @@ -37,6 +33,8 @@ func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { return sha256.Sum256(requestBz), nil } +// ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader +// and Signature fields. func (req *RelayRequest) ValidateBasic() error { if req.GetMeta() == nil { return ErrServiceInvalidRelayRequest.Wrap("missing meta") @@ -47,30 +45,26 @@ func (req *RelayRequest) ValidateBasic() error { } if len(req.GetMeta().GetSignature()) == 0 { - return ErrServiceInvalidRelayRequest.Wrap("missing signature") + return ErrServiceInvalidRelayRequest.Wrap("missing application signature") } return nil } -// getSignableBytes returns the bytes resulting from marshaling the relay response -// A value receiver is used to avoid overwriting any pre-existing signature -func (res RelayResponse) getSignableBytes() ([]byte, error) { - // set signature to nil - res.Meta.SupplierSignature = nil - - return res.Marshal() -} - // GetSignableBytesHash returns the hash of the signable bytes of the relay response // Hashing the marshaled response message guarantees that the signable bytes are // always of a constant and expected length. func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { // Save the signature and restore it after getting the signable bytes - // since getSignableBytes sets the signature to nil but does not preserve - // its original value. + // Since res.Meta is a pointer, this approach is not concurrent safe, + // if two goroutines are calling this method at the same time, the last one + // could get the nil signature resulting form the first go routine and restore + // nil after getting the signable bytes. + // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue + // by having res.Meta as a non-pointer type in the corresponding proto file. signature := res.Meta.SupplierSignature - responseBz, err := res.getSignableBytes() + res.Meta.SupplierSignature = nil + responseBz, err := res.Marshal() // Set the signature back to its original value res.Meta.SupplierSignature = signature @@ -84,6 +78,8 @@ func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { return sha256.Sum256(responseBz), nil } +// ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader +// and SupplierSignature fields. func (res *RelayResponse) ValidateBasic() error { // TODO_FUTURE: if a client gets a response with an invalid/incomplete // SessionHeader, consider sending an on-chain challenge, lowering their @@ -104,7 +100,9 @@ func (res *RelayResponse) ValidateBasic() error { return nil } -func (res *RelayResponse) VerifySignature(supplierPubKey cryptotypes.PubKey) error { +// VerifySupplierSignature ensures the signature provided by the supplier is +// valid according to their relay response. +func (res *RelayResponse) VerifySupplierSignature(supplierPubKey cryptotypes.PubKey) error { // Get the signable bytes hash of the response. signableBz, err := res.GetSignableBytesHash() if err != nil { From 75db9071f964b2d4dd1576d56d2f6455609b2230 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Wed, 13 Mar 2024 21:22:49 +0100 Subject: [PATCH 11/23] chore: Add missing change requests --- pkg/crypto/rings/client.go | 2 +- pkg/relayer/proxy/relay_verifier.go | 3 +++ pkg/sdk/deps_builder.go | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 717e0e237..f6d3e67ed 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -29,7 +29,7 @@ type ringClient struct { // used to get the addresses of the gateways an application is delegated to. applicationQuerier client.ApplicationQueryClient - // pubKeyClient is used to get the public keys given an address. + // pubKeyClient is used to get the public keys for a given an account address. pubKeyClient crypto.PubKeyClient } diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index 2883fd1b5..a749c7f5d 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -18,6 +18,9 @@ func (rp *relayerProxy) VerifyRelayRequest( return err } + // ringCache.VerifyRelayRequestSignature has already verified the relayRequest + // meta and session header fields with ValidateBasic, so we can safely assume + // that the session header is valid. sessionHeader := relayRequest.GetMeta().GetSessionHeader() // Application address is used to verify the relayRequest signature, it is diff --git a/pkg/sdk/deps_builder.go b/pkg/sdk/deps_builder.go index 2de34d6b2..fcac762d2 100644 --- a/pkg/sdk/deps_builder.go +++ b/pkg/sdk/deps_builder.go @@ -55,7 +55,7 @@ func (sdk *poktrollSDK) buildDeps( } deps = depinject.Configs(deps, depinject.Supply(grpcClient)) - // Create the account querier and add it to the pubKey client required dependencies. + // Create the account querier and add it to the required dependencies. accountQuerier, err := query.NewAccountQuerier(deps) if err != nil { return nil, err From 3357fea37ba963ea26c8aaf26cbf187e53c45756 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Thu, 14 Mar 2024 03:29:48 +0100 Subject: [PATCH 12/23] chore: Remove pubkey client --- pkg/appgateserver/cmd/cmd.go | 1 - pkg/client/interface.go | 4 +++ pkg/client/query/accquerier.go | 36 ++++++++++++++++++--- pkg/client/query/errors.go | 1 + pkg/crypto/rings/client.go | 14 +++----- pkg/deps/config/suppliers.go | 19 ----------- pkg/sdk/deps_builder.go | 10 +----- pkg/sdk/sdk.go | 35 +++----------------- pkg/sdk/send_relay.go | 4 +-- x/proof/keeper/keeper.go | 39 +++++++++++------------ x/proof/keeper/msg_server_submit_proof.go | 2 +- x/proof/types/account_query_client.go | 36 +++++++++++++++++++-- x/proof/types/errors.go | 1 + x/service/types/relay.go | 4 ++- 14 files changed, 107 insertions(+), 99 deletions(-) diff --git a/pkg/appgateserver/cmd/cmd.go b/pkg/appgateserver/cmd/cmd.go index 43213d721..f8a439716 100644 --- a/pkg/appgateserver/cmd/cmd.go +++ b/pkg/appgateserver/cmd/cmd.go @@ -191,7 +191,6 @@ func setupAppGateServerDependencies( config.NewSupplyAccountQuerierFn(), // leaf config.NewSupplyApplicationQuerierFn(), // leaf config.NewSupplySessionQuerierFn(), // leaf - config.NewSupplyPubKeyClientFn(), config.NewSupplyRingCacheFn(), config.NewSupplyPOKTRollSDKFn(appGateConfig.SigningKey), diff --git a/pkg/client/interface.go b/pkg/client/interface.go index 6393252cf..52b50e052 100644 --- a/pkg/client/interface.go +++ b/pkg/client/interface.go @@ -19,6 +19,7 @@ import ( comettypes "github.com/cometbft/cometbft/rpc/core/types" cosmosclient "github.com/cosmos/cosmos-sdk/client" cosmoskeyring "github.com/cosmos/cosmos-sdk/crypto/keyring" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" cosmostypes "github.com/cosmos/cosmos-sdk/types" "github.com/pokt-network/smt" @@ -231,6 +232,9 @@ type SupplierClientOption func(SupplierClient) type AccountQueryClient interface { // GetAccount queries the chain for the details of the account provided GetAccount(ctx context.Context, address string) (cosmostypes.AccountI, error) + + // GetPubKeyFromAddress returns the public key of the given address. + GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) } // ApplicationQueryClient defines an interface that enables the querying of the diff --git a/pkg/client/query/accquerier.go b/pkg/client/query/accquerier.go index 5d0d5eaab..f61e96999 100644 --- a/pkg/client/query/accquerier.go +++ b/pkg/client/query/accquerier.go @@ -4,6 +4,7 @@ import ( "context" "cosmossdk.io/depinject" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/types" accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" grpc "github.com/cosmos/gogoproto/grpc" @@ -19,6 +20,10 @@ var _ client.AccountQueryClient = (*accQuerier)(nil) type accQuerier struct { clientConn grpc.ClientConn accountQuerier accounttypes.QueryClient + + // accountCache is a cache of accounts that have already been queried. + // TODO_TECHDEBT: Add a size limit to the cache and consider an LRU cache. + accountCache map[string]types.AccountI } // NewAccountQuerier returns a new instance of a client.AccountQueryClient by @@ -27,7 +32,7 @@ type accQuerier struct { // Required dependencies: // - clientCtx func NewAccountQuerier(deps depinject.Config) (client.AccountQueryClient, error) { - aq := &accQuerier{} + aq := &accQuerier{accountCache: make(map[string]types.AccountI)} if err := depinject.Inject( deps, @@ -46,14 +51,37 @@ func (aq *accQuerier) GetAccount( ctx context.Context, address string, ) (types.AccountI, error) { + if foundAccount, accountFound := aq.accountCache[address]; accountFound { + return foundAccount, nil + } + req := &accounttypes.QueryAccountRequest{Address: address} res, err := aq.accountQuerier.Account(ctx, req) if err != nil { return nil, ErrQueryAccountNotFound.Wrapf("address: %s [%v]", address, err) } - var acc accounttypes.AccountI - if err = queryCodec.UnpackAny(res.Account, &acc); err != nil { + var fetchedAccount types.AccountI + if err = queryCodec.UnpackAny(res.Account, &fetchedAccount); err != nil { return nil, ErrQueryUnableToDeserializeAccount.Wrapf("address: %s [%v]", address, err) } - return acc, nil + + aq.accountCache[address] = fetchedAccount + return fetchedAccount, nil +} + +// GetPubKeyFromAddress returns the public key of the given address. +// It uses the accountQuerier to get the account and then returns its public key. +func (aq *accQuerier) GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) { + acc, err := aq.GetAccount(ctx, address) + if err != nil { + return nil, err + } + + // If the account's public key is nil, then return an error. + pubKey := acc.GetPubKey() + if pubKey == nil { + return nil, ErrQueryPubKeyNotFound + } + + return pubKey, nil } diff --git a/pkg/client/query/errors.go b/pkg/client/query/errors.go index 7d28cff84..352e56ce3 100644 --- a/pkg/client/query/errors.go +++ b/pkg/client/query/errors.go @@ -7,4 +7,5 @@ var ( ErrQueryAccountNotFound = sdkerrors.Register(codespace, 1, "account not found") ErrQueryUnableToDeserializeAccount = sdkerrors.Register(codespace, 2, "unable to deserialize account") ErrQueryRetrieveSession = sdkerrors.Register(codespace, 3, "error while trying to retrieve a session") + ErrQueryPubKeyNotFound = sdkerrors.Register(codespace, 4, "account pub key not found") ) diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index f6d3e67ed..6cbf371d3 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -11,7 +11,6 @@ import ( "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" - pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/polylog" "github.com/pokt-network/poktroll/x/service/types" ) @@ -29,8 +28,8 @@ type ringClient struct { // used to get the addresses of the gateways an application is delegated to. applicationQuerier client.ApplicationQueryClient - // pubKeyClient is used to get the public keys for a given an account address. - pubKeyClient crypto.PubKeyClient + // accountQuerier is used to fetch accounts for a given an account address. + accountQuerier client.AccountQueryClient } // NewRingClient returns a new ring client constructed from the given dependencies. @@ -42,14 +41,11 @@ type ringClient struct { // - client.AccountQueryClient func NewRingClient(deps depinject.Config) (_ crypto.RingClient, err error) { rc := new(ringClient) - rc.pubKeyClient, err = pubkeyclient.NewPubKeyClient(deps) - if err != nil { - return nil, err - } if err := depinject.Inject( deps, &rc.logger, + &rc.accountQuerier, &rc.applicationQuerier, ); err != nil { return nil, err @@ -91,7 +87,7 @@ func (rc *ringClient) VerifyRelayRequestSignature( sessionHeader := relayRequest.GetMeta().GetSessionHeader() if err := sessionHeader.ValidateBasic(); err != nil { - return ErrRingClientInvalidRelayRequest.Wrapf("invalid session header: %q", err) + return ErrRingClientInvalidRelayRequest.Wrapf("invalid session header: %v", err) } rc.logger.Debug(). @@ -188,7 +184,7 @@ func (rc *ringClient) addressesToPubKeys( ) ([]cryptotypes.PubKey, error) { pubKeys := make([]cryptotypes.PubKey, len(addresses)) for i, addr := range addresses { - acc, err := rc.pubKeyClient.GetPubKeyFromAddress(ctx, addr) + acc, err := rc.accountQuerier.GetPubKeyFromAddress(ctx, addr) if err != nil { return nil, err } diff --git a/pkg/deps/config/suppliers.go b/pkg/deps/config/suppliers.go index 17c412840..8f53a0ca6 100644 --- a/pkg/deps/config/suppliers.go +++ b/pkg/deps/config/suppliers.go @@ -17,7 +17,6 @@ import ( "github.com/pokt-network/poktroll/pkg/client/query" querytypes "github.com/pokt-network/poktroll/pkg/client/query/types" txtypes "github.com/pokt-network/poktroll/pkg/client/tx/types" - pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" "github.com/pokt-network/poktroll/pkg/sdk" @@ -377,21 +376,3 @@ func NewSupplyPOKTRollSDKFn(signingKeyName string) SupplierFn { return depinject.Configs(deps, depinject.Supply(poktrollSDK)), nil } } - -// NewSupplyPubKeyClientFn supplies a depinject config with a PubKeyClient. -func NewSupplyPubKeyClientFn() SupplierFn { - return func( - _ context.Context, - deps depinject.Config, - _ *cobra.Command, - ) (depinject.Config, error) { - // Create the pubKey client. - pubKeyClient, err := pubkeyclient.NewPubKeyClient(deps) - if err != nil { - return nil, err - } - - // Supply the pubKey client to the provided deps - return depinject.Configs(deps, depinject.Supply(pubKeyClient)), nil - } -} diff --git a/pkg/sdk/deps_builder.go b/pkg/sdk/deps_builder.go index fcac762d2..64d18dc64 100644 --- a/pkg/sdk/deps_builder.go +++ b/pkg/sdk/deps_builder.go @@ -12,7 +12,6 @@ import ( "github.com/pokt-network/poktroll/pkg/client/delegation" eventsquery "github.com/pokt-network/poktroll/pkg/client/events" "github.com/pokt-network/poktroll/pkg/client/query" - pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" ) @@ -60,14 +59,7 @@ func (sdk *poktrollSDK) buildDeps( if err != nil { return nil, err } - - // Create the pubKey client and add it to the required dependencies - pubKeyClientDeps := depinject.Supply(accountQuerier) - pubKeyClient, err := pubkeyclient.NewPubKeyClient(pubKeyClientDeps) - if err != nil { - return nil, err - } - deps = depinject.Configs(deps, depinject.Supply(pubKeyClient)) + deps = depinject.Configs(deps, depinject.Supply(accountQuerier)) // Create and supply the application querier applicationQuerier, err := query.NewApplicationQuerier(deps) diff --git a/pkg/sdk/sdk.go b/pkg/sdk/sdk.go index 8b7c3a47e..bd944e5bd 100644 --- a/pkg/sdk/sdk.go +++ b/pkg/sdk/sdk.go @@ -57,18 +57,13 @@ type poktrollSDK struct { // It is used to query a specific application or all applications applicationQuerier client.ApplicationQueryClient + // accountQuerier is the querier for the account module. + // It is used to query a specific account given an address. + accountQuerier client.AccountQueryClient + // blockClient is the client for the block module. // It is used to get the current block height to query for the current session. blockClient client.BlockClient - - // pubKeyClient the client used to get the public key for a given account address. - // TODO_TECHDEBT: Add a size limit to the cache. Also consider clearing it every - // N sessions. - pubKeyClient crypto.PubKeyClient - - // supplierPubKeyCache is a cache of the suppliers pubKeys that has been queried. - // TODO_TECHDEBT: Add a size limit to the cache and consider an LRU cache. - supplierPubKeyCache map[string]cryptotypes.PubKey } // NewPOKTRollSDK creates a new POKTRollSDK instance with the given configuration. @@ -76,7 +71,6 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK sdk := &poktrollSDK{ config: config, serviceSessionSuppliers: make(map[string]map[string]*SessionSuppliers), - supplierPubKeyCache: make(map[string]cryptotypes.PubKey), } var err error @@ -94,7 +88,7 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK &sdk.logger, &sdk.ringCache, &sdk.sessionQuerier, - &sdk.pubKeyClient, + &sdk.accountQuerier, &sdk.applicationQuerier, &sdk.blockClient, ); err != nil { @@ -114,22 +108,3 @@ func NewPOKTRollSDK(ctx context.Context, config *POKTRollSDKConfig) (POKTRollSDK return sdk, nil } - -// getPubKeyFromAddress returns the public key of a supplier given its address. -// It uses the accountQuerier to get the account if it is not already in the cache. -func (sdk *poktrollSDK) getSupplierPubKeyFromAddress( - ctx context.Context, - address string, -) (cryptotypes.PubKey, error) { - if pubKey, ok := sdk.supplierPubKeyCache[address]; ok { - return pubKey, nil - } - - pubKey, err := sdk.pubKeyClient.GetPubKeyFromAddress(ctx, address) - if err != nil { - return nil, err - } - - sdk.supplierPubKeyCache[address] = pubKey - return pubKey, nil -} diff --git a/pkg/sdk/send_relay.go b/pkg/sdk/send_relay.go index 035f71c0b..47a475396 100644 --- a/pkg/sdk/send_relay.go +++ b/pkg/sdk/send_relay.go @@ -110,9 +110,9 @@ func (sdk *poktrollSDK) SendRelay( sessionHeader := relayResponse.GetMeta().GetSessionHeader() // Get the supplier's public key. - supplierPubKey, err := sdk.getSupplierPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) + supplierPubKey, err := sdk.accountQuerier.GetPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) if err != nil { - return nil, ErrSDKHandleRelay.Wrapf("error getting supplier public key: %s", err) + return nil, ErrSDKHandleRelay.Wrapf("error getting supplier public key: %v", err) } sdk.logger.Debug(). diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 4bb3319bb..df5b3391e 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -10,8 +10,8 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" - pubkeyclient "github.com/pokt-network/poktroll/pkg/crypto/pubkey_client" "github.com/pokt-network/poktroll/pkg/crypto/rings" "github.com/pokt-network/poktroll/pkg/polylog" _ "github.com/pokt-network/poktroll/pkg/polylog/polyzero" @@ -30,10 +30,9 @@ type ( sessionKeeper types.SessionKeeper applicationKeeper types.ApplicationKeeper - accountKeeper types.AccountKeeper - ringClient crypto.RingClient - pubKeyClient crypto.PubKeyClient + ringClient crypto.RingClient + accountQuerier client.AccountQueryClient } ) @@ -51,23 +50,21 @@ func NewKeeper( panic(fmt.Sprintf("invalid authority address: %s", authority)) } + // TODO_TECHDEBT: Use cosmos-sdk based polylog implementation once available. Also remove polyzero import. + polylogger := polylog.Ctx(context.Background()) applicationQuerier := types.NewAppKeeperQueryClient(applicationKeeper) accountQuerier := types.NewAccountKeeperQueryClient(accountKeeper) - // TODO_TECHDEBT: Use cosmos-sdk based polylog implementation once available. Also remove polyzero import. - polylogger := polylog.Ctx(context.TODO()) - - ringClientDeps := depinject.Supply( - polylogger, - applicationQuerier, - accountQuerier, - ) - - ringClient, err := rings.NewRingClient(ringClientDeps) - if err != nil { - panic(err) - } - pubKeyClient, err := pubkeyclient.NewPubKeyClient(depinject.Supply(accountQuerier)) + // RingKeeperClient holds the logic of verifying RelayRequests ring signatures + // for both on-chain and off-chain actors. + // As it takes care of getting the ring signature details (i.e. application + // and gateways pub keys) it uses an Application and Account querier interfaces + // that are compatible with the environment it's being used in. + // In this on-chain context, the Proof keeper supplies AppKeeperQueryClient and + // AccountKeeperQueryClient that are thin wrappers around the Application and + // Account keepers respectively, and satisfy the RingClient needs. + ringKeeperClientDeps := depinject.Supply(polylogger, applicationQuerier, accountQuerier) + ringKeeperClient, err := rings.NewRingClient(ringKeeperClientDeps) if err != nil { panic(err) } @@ -80,9 +77,9 @@ func NewKeeper( sessionKeeper: sessionKeeper, applicationKeeper: applicationKeeper, - accountKeeper: accountKeeper, - ringClient: ringClient, - pubKeyClient: pubKeyClient, + + ringClient: ringKeeperClient, + accountQuerier: accountQuerier, } } diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 8980900b9..586297594 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -80,7 +80,7 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( return nil, status.Error(codes.InvalidArgument, err.Error()) } - supplierPubKey, err := k.pubKeyClient.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) + supplierPubKey, err := k.accountQuerier.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) if err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index 115eefd7c..48c43a064 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -3,6 +3,7 @@ package types import ( "context" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/types" "github.com/pokt-network/poktroll/pkg/client" @@ -29,11 +30,42 @@ func NewAccountKeeperQueryClient(accountKeeper AccountKeeper) client.AccountQuer func (accountQueryClient *AccountKeeperQueryClient) GetAccount( ctx context.Context, addr string, -) (types.AccountI, error) { +) (account types.AccountI, err error) { addrBz, err := types.AccAddressFromBech32(addr) if err != nil { return nil, err } - return accountQueryClient.keeper.GetAccount(ctx, addrBz), nil + // keeper.GetAccount panics if the account is not found. Recover from the panic + // and return an error instead if that's the case. + defer func() { + if r := recover(); r != nil { + err = ErrProofPubKeyNotFound + account = nil + } + }() + + account = accountQueryClient.keeper.GetAccount(ctx, addrBz) + + return account, err +} + +// GetPubKeyFromAddress returns the public key of the given address. +// It uses the accountQuerier to get the account and then returns its public key. +func (accountQueryClient *AccountKeeperQueryClient) GetPubKeyFromAddress( + ctx context.Context, + address string, +) (cryptotypes.PubKey, error) { + acc, err := accountQueryClient.GetAccount(ctx, address) + if err != nil { + return nil, err + } + + // If the account's public key is nil, then return an error. + pubKey := acc.GetPubKey() + if pubKey == nil { + return nil, ErrProofPubKeyNotFound + } + + return pubKey, nil } diff --git a/x/proof/types/errors.go b/x/proof/types/errors.go index acaefcd31..414bf467a 100644 --- a/x/proof/types/errors.go +++ b/x/proof/types/errors.go @@ -23,4 +23,5 @@ var ( ErrProofInvalidRelayResponse = sdkerrors.Register(ModuleName, 1114, "invalid relay response") ErrProofNotSecp256k1Curve = sdkerrors.Register(ModuleName, 1115, "not secp256k1 curve") ErrProofApplicationNotFound = sdkerrors.Register(ModuleName, 1116, "application not found") + ErrProofPubKeyNotFound = sdkerrors.Register(ModuleName, 1117, "public key not found") ) diff --git a/x/service/types/relay.go b/x/service/types/relay.go index 9bd509cb2..abf94a5db 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -33,6 +33,7 @@ func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { return sha256.Sum256(requestBz), nil } +// TODO_TEST: Add tests for RelayRequest validation // ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader // and Signature fields. func (req *RelayRequest) ValidateBasic() error { @@ -78,6 +79,7 @@ func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { return sha256.Sum256(responseBz), nil } +// TODO_TEST: Add tests for RelayResponse validation // ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader // and SupplierSignature fields. func (res *RelayResponse) ValidateBasic() error { @@ -90,7 +92,7 @@ func (res *RelayResponse) ValidateBasic() error { } if err := res.GetMeta().GetSessionHeader().ValidateBasic(); err != nil { - return ErrServiceInvalidRelayResponse.Wrapf("invalid session header: %s", err) + return ErrServiceInvalidRelayResponse.Wrapf("invalid session header: %v", err) } if len(res.GetMeta().GetSupplierSignature()) == 0 { From f90ca760839255a9db35fc2b3fb13b80c64689d9 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Thu, 14 Mar 2024 04:45:02 +0100 Subject: [PATCH 13/23] chore: Fix unit tests and ring client removal consideration --- .../testclient/testqueryclients/accquerier.go | 23 +++++++++++++++---- x/proof/keeper/keeper.go | 5 ++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/testutil/testclient/testqueryclients/accquerier.go b/testutil/testclient/testqueryclients/accquerier.go index 6924dc963..cd645be68 100644 --- a/testutil/testclient/testqueryclients/accquerier.go +++ b/testutil/testclient/testqueryclients/accquerier.go @@ -6,9 +6,11 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/types" accounttypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/golang/mock/gomock" + "github.com/pokt-network/poktroll/pkg/client/query" "github.com/pokt-network/poktroll/testutil/mockclient" ) @@ -32,12 +34,12 @@ func NewTestAccountQueryClient( ) *mockclient.MockAccountQueryClient { ctrl := gomock.NewController(t) - accoutQuerier := mockclient.NewMockAccountQueryClient(ctrl) - accoutQuerier.EXPECT().GetAccount(gomock.Any(), gomock.Any()). + accountQuerier := mockclient.NewMockAccountQueryClient(ctrl) + accountQuerier.EXPECT().GetAccount(gomock.Any(), gomock.Any()). DoAndReturn(func( _ context.Context, address string, - ) (account accounttypes.AccountI, err error) { + ) (account types.AccountI, err error) { anyPk := (*codectypes.Any)(nil) if pk, ok := addressAccountMap[address]; ok { anyPk, err = codectypes.NewAnyWithValue(pk) @@ -52,7 +54,20 @@ func NewTestAccountQueryClient( }). AnyTimes() - return accoutQuerier + accountQuerier.EXPECT().GetPubKeyFromAddress(gomock.Any(), gomock.Any()). + DoAndReturn(func( + _ context.Context, + address string, + ) (pk cryptotypes.PubKey, err error) { + pk, ok := addressAccountMap[address] + if !ok { + return nil, query.ErrQueryAccountNotFound + } + return pk, nil + }). + AnyTimes() + + return accountQuerier } // addAddressToAccountMap adds the given address to the addressAccountMap diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index df5b3391e..4d5babb7e 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -63,6 +63,11 @@ func NewKeeper( // In this on-chain context, the Proof keeper supplies AppKeeperQueryClient and // AccountKeeperQueryClient that are thin wrappers around the Application and // Account keepers respectively, and satisfy the RingClient needs. + // TODO_CONSIDERATION: Make ring signature verification a stateless function + // and get rid of the RingClient and its dependencies by moving application + // ring retrieval to the application keeper and making it retrievable using + // the application query client for off-chain actors. + // Signature verification code will still be shared across off/on chain environments ringKeeperClientDeps := depinject.Supply(polylogger, applicationQuerier, accountQuerier) ringKeeperClient, err := rings.NewRingClient(ringKeeperClientDeps) if err != nil { From 46c61dddcbb1f7cb19d2e5673713924085f500c0 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Mon, 18 Mar 2024 17:21:22 -0700 Subject: [PATCH 14/23] Offline review of 406 --- pkg/crypto/interface.go | 10 +- pkg/crypto/pubkey_client/client.go | 3 +- pkg/crypto/rings/cache.go | 54 ++++----- pkg/crypto/rings/client.go | 71 ++++++----- pkg/crypto/rings/errors.go | 4 +- pkg/crypto/rings/ring.go | 2 +- pkg/relayer/proxy/errors.go | 4 +- pkg/relayer/proxy/relay_verifier.go | 37 +++--- pkg/sdk/sdk.go | 2 +- pkg/sdk/send_relay.go | 26 ++-- x/proof/keeper/keeper.go | 24 ++-- x/proof/keeper/msg_server_submit_proof.go | 139 ++++++++++++++-------- x/proof/types/account_query_client.go | 10 +- x/proof/types/application_query_client.go | 5 +- x/service/types/relay.go | 34 +++--- 15 files changed, 243 insertions(+), 182 deletions(-) diff --git a/pkg/crypto/interface.go b/pkg/crypto/interface.go index da4e8f74d..a2e35931d 100644 --- a/pkg/crypto/interface.go +++ b/pkg/crypto/interface.go @@ -5,7 +5,7 @@ import ( "context" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - "github.com/noot/ring-go" + ring "github.com/noot/ring-go" "github.com/pokt-network/poktroll/x/service/types" ) @@ -37,8 +37,8 @@ type RingClient interface { // it exists. GetRingForAddress(ctx context.Context, appAddress string) (*ring.Ring, error) - // VerifyRelayRequestSignature verifies the relay request signature against the - // ring for the application address in the relay request. + // VerifyRelayRequestSignature verifies the relay request signature against + // the ring for the application address in the relay request. VerifyRelayRequestSignature(ctx context.Context, relayRequest *types.RelayRequest) error } @@ -46,7 +46,7 @@ type RingClient interface { // On-chain and off-chain implementations should take care of retrieving the // address' account and returning its public key. type PubKeyClient interface { - // GetPubKeyFromAddress returns the public key of the given account address if - // it exists. + // GetPubKeyFromAddress returns the public key of the given account address + // if it exists. GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) } diff --git a/pkg/crypto/pubkey_client/client.go b/pkg/crypto/pubkey_client/client.go index 0b753026a..042bed580 100644 --- a/pkg/crypto/pubkey_client/client.go +++ b/pkg/crypto/pubkey_client/client.go @@ -41,7 +41,8 @@ func NewPubKeyClient(deps depinject.Config) (crypto.PubKeyClient, error) { } // GetPubKeyFromAddress returns the public key of the given address. -// It uses the accountQuerier to get the account and then returns its public key. +// It retrieves the corresponding account by querying for it and returns +// the associated public key. func (pc *pubKeyClient) GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) { acc, err := pc.accountQuerier.GetAccount(ctx, address) if err != nil { diff --git a/pkg/crypto/rings/cache.go b/pkg/crypto/rings/cache.go index b06db1826..2e7a62af4 100644 --- a/pkg/crypto/rings/cache.go +++ b/pkg/crypto/rings/cache.go @@ -5,7 +5,7 @@ import ( "sync" "cosmossdk.io/depinject" - "github.com/noot/ring-go" + ring "github.com/noot/ring-go" "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" @@ -20,17 +20,16 @@ type ringCache struct { // logger is the logger for the ring cache. logger polylog.Logger - // ringsByAddr maintains a map of application addresses to the ring composed of - // the public keys of the application and the gateways the application is delegated to. + // ringsByAddr maintains a map from app addresses to the ring composed of + // the public keys of both the application and its delegated gateways. ringsByAddr map[string]*ring.Ring ringsByAddrMu *sync.RWMutex // delegationClient is used to listen for on-chain delegation events and - // invalidate ringsByAddr entries for rings that have been updated on chain. + // invalidate entries in ringsByAddr if an associated updated has been made. delegationClient client.DelegationClient - // ringClient is used to retrieve the rings that are cached and verify relay - // request signatures against the rings. + // ringClient is used to retrieve cached rings and verify relay requests. ringClient crypto.RingClient } @@ -69,15 +68,15 @@ func NewRingCache(deps depinject.Config) (_ crypto.RingCache, err error) { // Start starts the ring cache by subscribing to on-chain redelegation events. func (rc *ringCache) Start(ctx context.Context) { rc.logger.Info().Msg("starting ring ringsByAddr") - // Listen for redelegation events and invalidate the cache if it contains an - // address corresponding to the redelegation event's. + // Stop the ringCache when the context is cancelled. go func() { select { case <-ctx.Done(): - // Stop the ring cache if the context is cancelled. rc.Stop() } }() + // Listen for redelegation events and invalidate the cache if it contains an + // address corresponding to the redelegation event's. go rc.goInvalidateCache(ctx) } @@ -107,7 +106,8 @@ func (rc *ringCache) goInvalidateCache(ctx context.Context) { }) } -// Stop stops the ring cache by unsubscribing from on-chain redelegation events. +// Stop stops the ring cache by unsubscribing from on-chain redelegation events +// and clears any existing entries. func (rc *ringCache) Stop() { // Clear the cache. rc.ringsByAddrMu.Lock() @@ -122,11 +122,12 @@ func (rc *ringCache) Stop() { func (rc *ringCache) GetCachedAddresses() []string { rc.ringsByAddrMu.RLock() defer rc.ringsByAddrMu.RUnlock() - keys := make([]string, 0, len(rc.ringsByAddr)) - for k := range rc.ringsByAddr { - keys = append(keys, k) + + appAddresses := make([]string, 0, len(rc.ringsByAddr)) + for appAddr := range rc.ringsByAddr { + appAddresses = append(appAddresses, appAddr) } - return keys + return appAddresses } // GetRingForAddress returns the ring for the address provided. If it does not @@ -137,33 +138,32 @@ func (rc *ringCache) GetRingForAddress( ctx context.Context, appAddress string, ) (ring *ring.Ring, err error) { - // Lock the ringsByAddr map. rc.ringsByAddrMu.Lock() defer rc.ringsByAddrMu.Unlock() // Check if the ring is in the cache. ring, ok := rc.ringsByAddr[appAddress] - if !ok { - // If the ring is not in the cache, get it from the ring client. - rc.logger.Debug(). - Str("app_address", appAddress). - Msg("ring ringsByAddr miss; fetching from application module") - ring, err = rc.ringClient.GetRingForAddress(ctx, appAddress) - - // Add the address points to the cache. - rc.ringsByAddr[appAddress] = ring - } else { - // Use the ring if it is present in the cache. + // Use the existing ring if it's cached. + if ok { rc.logger.Debug(). Str("app_address", appAddress). Msg("ring ringsByAddr hit; using cached ring") + + return ring, nil } + + // If the ring is not in the cache, get it from the ring client. + rc.logger.Debug(). + Str("app_address", appAddress). + Msg("ring ringsByAddr miss; fetching from application module") + + ring, err = rc.ringClient.GetRingForAddress(ctx, appAddress) if err != nil { return nil, err } + rc.ringsByAddr[appAddress] = ring - // Return the ring. return ring, nil } diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 6cbf371d3..4ec6d4bc7 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -7,7 +7,7 @@ import ( "cosmossdk.io/depinject" ring_secp256k1 "github.com/athanorlabs/go-dleq/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - "github.com/noot/ring-go" + ring "github.com/noot/ring-go" "github.com/pokt-network/poktroll/pkg/client" "github.com/pokt-network/poktroll/pkg/crypto" @@ -21,11 +21,10 @@ var _ crypto.RingClient = (*ringClient)(nil) // client.ApplicationQueryClient to get application's delegation information // needed to construct the ring for signing relay requests. type ringClient struct { - // logger is the logger for the ring cache. logger polylog.Logger // applicationQuerier is the querier for the application module, and is - // used to get the addresses of the gateways an application is delegated to. + // used to get the gateways an application is delegated to. applicationQuerier client.ApplicationQueryClient // accountQuerier is used to fetch accounts for a given an account address. @@ -54,9 +53,10 @@ func NewRingClient(deps depinject.Config) (_ crypto.RingClient, err error) { return rc, nil } -// GetRingForAddress returns the ring for the address provided. The ring is created by -// querying for the application address and delegated gateways' account public keys and -// converting them to their secp256k1 curve points. +// GetRingForAddress returns the ring for the address provided. +// The ring is created by querying for the application's and its delegated +// gateways public keys. These keys are converted to secp256k1 curve points +// before forming the ring. func (rc *ringClient) GetRingForAddress( ctx context.Context, appAddress string, @@ -65,27 +65,30 @@ func (rc *ringClient) GetRingForAddress( if err != nil { return nil, err } + // Get the points on the secp256k1 curve for the public keys in the ring. points, err := pointsFromPublicKeys(pubKeys...) if err != nil { return nil, err } - // Return the ring the constructed from the public key points on the secp256k1 curve. + // Return the ring the constructed from the points retrieved above. return newRingFromPoints(points) } -// VerifyRelayRequestSignature verifies the relay request signature against the -// ring for the application address in the relay request. +// VerifyRelayRequestSignature verifies the signature of the relay request +// provided against the corresponding ring for the application address in +// the same request. func (rc *ringClient) VerifyRelayRequestSignature( ctx context.Context, relayRequest *types.RelayRequest, ) error { - if relayRequest.GetMeta() == nil { + relayRequestMeta := relayRequest.GetMeta() + if relayRequestMeta == nil { return ErrRingClientInvalidRelayRequest.Wrap("missing meta from relay request") } - sessionHeader := relayRequest.GetMeta().GetSessionHeader() + sessionHeader := relayRequestMeta.GetSessionHeader() if err := sessionHeader.ValidateBasic(); err != nil { return ErrRingClientInvalidRelayRequest.Wrapf("invalid session header: %v", err) } @@ -99,13 +102,14 @@ func (rc *ringClient) VerifyRelayRequestSignature( Msg("verifying relay request signature") // Extract the relay request's ring signature. - if relayRequest.GetMeta().GetSignature() == nil { + signature := relayRequestMeta.GetSignature() + if signature == nil { return ErrRingClientInvalidRelayRequest.Wrap("missing signature from relay request") } - signature := relayRequest.GetMeta().GetSignature() - ringSig := new(ring.RingSig) - if err := ringSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { + // Convert the request signature to a ring signature. + relayRequestRingSig := new(ring.RingSig) + if err := relayRequestRingSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { return ErrRingClientInvalidRelayRequestSignature.Wrapf( "error deserializing ring signature: %s", err, ) @@ -113,35 +117,36 @@ func (rc *ringClient) VerifyRelayRequestSignature( // Get the ring for the application address of the relay request. appAddress := sessionHeader.GetApplicationAddress() - appRing, err := rc.GetRingForAddress(ctx, appAddress) + expectedAppRingSig, err := rc.GetRingForAddress(ctx, appAddress) if err != nil { return ErrRingClientInvalidRelayRequest.Wrapf( "error getting ring for application address %s: %v", appAddress, err, ) } - // Verify the ring signature against the app ring. - if !ringSig.Ring().Equals(appRing) { + // Compare the expected ring signature against the one provided in the relay request. + if !relayRequestRingSig.Ring().Equals(expectedAppRingSig) { return ErrRingClientInvalidRelayRequestSignature.Wrapf( - "ring signature does not match ring for application address %s", appAddress, + "ring signature in the relay request does not match the expected one for the app %s", appAddress, ) } // Get and hash the signable bytes of the relay request. requestSignableBz, err := relayRequest.GetSignableBytesHash() if err != nil { - return ErrRingClientInvalidRelayRequest.Wrapf("error getting signable bytes: %v", err) + return ErrRingClientInvalidRelayRequest.Wrapf("error getting relay request signable bytes: %v", err) } // Verify the relay request's signature. - if valid := ringSig.Verify(requestSignableBz); !valid { - return ErrRingClientInvalidRelayRequestSignature.Wrapf("invalid ring signature") + if valid := relayRequestRingSig.Verify(requestSignableBz); !valid { + return ErrRingClientInvalidRelayRequestSignature.Wrapf("invalid relay request signature or bytes") } + return nil } -// getDelegatedPubKeysForAddress returns the public keys used to sign a -// relay request for the given application address. +// getDelegatedPubKeysForAddress returns the gateway public keys an application +// delegated the ability to sign relay requests on its behalf. func (rc *ringClient) getDelegatedPubKeysForAddress( ctx context.Context, appAddress string, @@ -156,12 +161,12 @@ func (rc *ringClient) getDelegatedPubKeysForAddress( ringAddresses := make([]string, 0) ringAddresses = append(ringAddresses, appAddress) // app address is index 0 if len(app.DelegateeGatewayAddresses) == 0 { - // add app address twice to make the ring size of mininmum 2 - // TODO_HACK: We are adding the appAddress twice because a ring - // signature requires AT LEAST two pubKeys. When the Application has - // not delegated to any gateways, we add the application's own address - // twice. This is a HACK and should be investigated as to what is the - // best approach to take in this situation. + // add app address twice to make the ring size of minimum 2 + // TODO_IMPROVE: The appAddress is added twice because a ring signature + // requires AT LEAST two pubKeys. If the Application has not delegated + // to any gateways, the app's own address needs to be used twice to + // create a ring. This is not a huge issue but an improvement should + // be investigated in the future. ringAddresses = append(ringAddresses, appAddress) } else { // add the delegatee gateway addresses @@ -169,15 +174,15 @@ func (rc *ringClient) getDelegatedPubKeysForAddress( } rc.logger.Debug(). - // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` instead of formatting here. + // TODO_TECHDEBT: implement and use `polylog.Event#Strs([]string)` Str("addresses", fmt.Sprintf("%v", ringAddresses)). Msg("converting addresses to points") return rc.addressesToPubKeys(ctx, ringAddresses) } -// addressesToPubKeys uses the public key client to query the account module for -// the public key corresponding to each address given. +// addressesToPubKeys queries for and returns the public keys for the addresses +// provided. func (rc *ringClient) addressesToPubKeys( ctx context.Context, addresses []string, diff --git a/pkg/crypto/rings/errors.go b/pkg/crypto/rings/errors.go index 588afeee7..2c51776bd 100644 --- a/pkg/crypto/rings/errors.go +++ b/pkg/crypto/rings/errors.go @@ -1,6 +1,8 @@ package rings -import sdkerrors "cosmossdk.io/errors" +import ( + sdkerrors "cosmossdk.io/errors" +) var ( codespace = "rings" diff --git a/pkg/crypto/rings/ring.go b/pkg/crypto/rings/ring.go index 5b8d815a0..13fd4269b 100644 --- a/pkg/crypto/rings/ring.go +++ b/pkg/crypto/rings/ring.go @@ -5,7 +5,7 @@ import ( ringtypes "github.com/athanorlabs/go-dleq/types" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - "github.com/noot/ring-go" + ring "github.com/noot/ring-go" ) // newRingFromPoints creates a new ring from points (i.e. public keys) on the secp256k1 curve diff --git a/pkg/relayer/proxy/errors.go b/pkg/relayer/proxy/errors.go index 3bf45da08..74e259502 100644 --- a/pkg/relayer/proxy/errors.go +++ b/pkg/relayer/proxy/errors.go @@ -1,6 +1,8 @@ package proxy -import sdkerrors "cosmossdk.io/errors" +import ( + sdkerrors "cosmossdk.io/errors" +) var ( codespace = "relayer_proxy" diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index f2cf02669..fcde49294 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -8,57 +8,61 @@ import ( sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) -// VerifyRelayRequest is a shared method used by RelayServers to check the relay request signature and session validity. +// VerifyRelayRequest is a shared method used by RelayServers to check the relay +// request signature and session validity. func (rp *relayerProxy) VerifyRelayRequest( ctx context.Context, relayRequest *types.RelayRequest, - service *sharedtypes.Service, + supplierService *sharedtypes.Service, ) error { + // Verify the relayRequest metadata, signature, session header and other + // basic validation. if err := rp.ringCache.VerifyRelayRequestSignature(ctx, relayRequest); err != nil { return err } - // ringCache.VerifyRelayRequestSignature has already verified the relayRequest - // meta and session header fields with ValidateBasic, so we can safely assume - // that the session header is valid. + // Extract the session header for usage below. + // ringCache.VerifyRelayRequestSignature already verified the header's validaity. sessionHeader := relayRequest.GetMeta().GetSessionHeader() - // Application address is used to verify the relayRequest signature, it is - // guaranteed to be present in the relayRequest since the signature has already - // been verified. + // Application address is used to verify the relayRequest signature. + // It is guaranteed to be present in the relayRequest since the signature + // has already been verified. appAddress := sessionHeader.GetApplicationAddress() - // Query for the current session to check if relayRequest sessionId matches the current session. rp.logger.Debug(). Fields(map[string]any{ "session_id": sessionHeader.GetSessionId(), - "application_address": sessionHeader.GetApplicationAddress(), + "application_address": appAddress, "service_id": sessionHeader.GetService().GetId(), }). Msg("verifying relay request session") + // Get the block height at which the relayRequest should be processed. sessionBlockHeight, err := rp.getTargetSessionBlockHeight(ctx, relayRequest) if err != nil { return err } + // Query for the current session to check if relayRequest sessionId matches the current session. session, err := rp.sessionQuerier.GetSession( ctx, appAddress, - service.Id, + supplierService.Id, sessionBlockHeight, ) if err != nil { return err } + // Session validity can be checked via a basic ID comparison due to the reasons below. + // // 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) - // we can reduce the session validity check to checking if the retrieved session's sessionId - // matches the relayRequest sessionId. - // TODO_INVESTIGATE: Revisit the assumptions above at some point in the future, but good enough for now. + // + // TODO_BLOCKER: Revisit the assumptions above but good enough for now. if session.SessionId != sessionHeader.GetSessionId() { return ErrRelayerProxyInvalidSession.Wrapf( "session mismatch, expecting: %+v, got: %+v", @@ -89,11 +93,13 @@ func (rp *relayerProxy) getTargetSessionBlockHeight( currentBlockHeight := rp.blockClient.LastNBlocks(ctx, 1)[0].Height() sessionEndblockHeight := relayRequest.Meta.SessionHeader.GetSessionEndBlockHeight() - // Check if the `RelayRequest`'s session has expired. + // Check if the RelayRequest's session has expired. if sessionEndblockHeight < currentBlockHeight { // Do not process the `RelayRequest` if the session has expired and the current // block height is outside the session's grace period. if sessiontypes.IsWithinGracePeriod(sessionEndblockHeight, currentBlockHeight) { + // The RelayRequest's session has expired but is still within the + // grace period so process it as if the session is still active. return sessionEndblockHeight, nil } @@ -104,5 +110,6 @@ func (rp *relayerProxy) getTargetSessionBlockHeight( ) } + // The RelayRequest's session is active so return the current block height. return currentBlockHeight, nil } diff --git a/pkg/sdk/sdk.go b/pkg/sdk/sdk.go index bd944e5bd..5cc4d1458 100644 --- a/pkg/sdk/sdk.go +++ b/pkg/sdk/sdk.go @@ -58,7 +58,7 @@ type poktrollSDK struct { applicationQuerier client.ApplicationQueryClient // accountQuerier is the querier for the account module. - // It is used to query a specific account given an address. + // It retrieves on-chain accounts provided the address. accountQuerier client.AccountQueryClient // blockClient is the client for the block module. diff --git a/pkg/sdk/send_relay.go b/pkg/sdk/send_relay.go index 47a475396..ccb9e8c97 100644 --- a/pkg/sdk/send_relay.go +++ b/pkg/sdk/send_relay.go @@ -19,19 +19,19 @@ func init() { // SendRelay sends a relay request to the given supplier's endpoint. // It signs the request, relays it to the supplier and verifies the response signature. -// It takes an http.Request as an argument and uses its method and headers to create -// the relay request. +// The relay request is created by adding method headers to the provided http.Request. func (sdk *poktrollSDK) SendRelay( ctx context.Context, supplierEndpoint *SingleSupplierEndpoint, request *http.Request, ) (response *types.RelayResponse, err error) { + // Retrieve the request's payload. payloadBz, err := io.ReadAll(request.Body) if err != nil { return nil, ErrSDKHandleRelay.Wrapf("reading request body: %s", err) } - // Create the relay request. + // Prepare the relay request. relayRequest := &types.RelayRequest{ Meta: &types.RelayRequestMetadata{ SessionHeader: supplierEndpoint.Header, @@ -48,12 +48,13 @@ func (sdk *poktrollSDK) SendRelay( } signer := signer.NewRingSigner(appRing, sdk.signingKey) - // Hash and sign the request's signable bytes. + // Hash request's signable bytes. signableBz, err := relayRequest.GetSignableBytesHash() if err != nil { return nil, ErrSDKHandleRelay.Wrapf("error getting signable bytes: %s", err) } + // Sign the relay request. requestSig, err := signer.Sign(signableBz) if err != nil { return nil, ErrSDKHandleRelay.Wrapf("error signing relay: %s", err) @@ -66,6 +67,7 @@ func (sdk *poktrollSDK) SendRelay( return nil, ErrSDKHandleRelay.Wrapf("error marshaling relay request: %s", err) } relayRequestReader := io.NopCloser(bytes.NewReader(relayRequestBz)) + // TODO_IN_THIS_PR(@red-0ne): Why do we need the following 4 lines? var relayReq types.RelayRequest if err := relayReq.Unmarshal(relayRequestBz); err != nil { return nil, ErrSDKHandleRelay.Wrapf("error unmarshaling relay request: %s", err) @@ -84,6 +86,7 @@ func (sdk *poktrollSDK) SendRelay( sdk.logger.Debug(). Str("supplier_url", supplierEndpoint.Url.String()). Msg("sending relay request") + relayHTTPResponse, err := http.DefaultClient.Do(relayHTTPRequest) if err != nil { return nil, ErrSDKHandleRelay.Wrapf("error sending relay request: %s", err) @@ -95,17 +98,16 @@ func (sdk *poktrollSDK) SendRelay( return nil, ErrSDKHandleRelay.Wrapf("error reading relay response body: %s", err) } - // Unmarshal the response bytes into a RelayResponse. + // Unmarshal the response bytes into a RelayResponse and validate it. relayResponse := &types.RelayResponse{} if err := relayResponse.Unmarshal(relayResponseBz); err != nil { return nil, ErrSDKHandleRelay.Wrapf("error unmarshaling relay response: %s", err) } - if err := relayResponse.ValidateBasic(); err != nil { return nil, ErrSDKHandleRelay.Wrapf("%s", err) } - // relayResponse.ValidateBasic also validates Meta and SessionHeader, + // relayResponse.ValidateBasic validates Meta and SessionHeader, so // we can safely use the session header. sessionHeader := relayResponse.GetMeta().GetSessionHeader() @@ -122,12 +124,10 @@ func (sdk *poktrollSDK) SendRelay( Int64("end_height", sessionHeader.GetSessionEndBlockHeight()). Msg("About to verify relay response signature.") - // Verify the response signature. We use the supplier address that we got from - // the getRelayerUrl function since this is the address we are expecting to sign the response. - // TODO_TECHDEBT: if the RelayResponse is an internal error response, we should not verify the signature - // as in some relayer early failures, it may not be signed by the supplier. - // TODO_IMPROVE: Add more logging & telemetry so we can get visibility and signal into - // failed responses. + // Verify the relay response's supplier signature. + // TODO_TECHDEBT: if the RelayResponse has an internal error response, we + // SHOULD NOT verify the signature, and return an error early. + // TODO_IMPROVE: Increase logging & telemetry get visibility into failed responses. if err := relayResponse.VerifySupplierSignature(supplierPubKey); err != nil { return nil, ErrSDKVerifyResponseSignature.Wrapf("%s", err) } diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 4d5babb7e..585478bfd 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -57,17 +57,19 @@ func NewKeeper( // RingKeeperClient holds the logic of verifying RelayRequests ring signatures // for both on-chain and off-chain actors. - // As it takes care of getting the ring signature details (i.e. application - // and gateways pub keys) it uses an Application and Account querier interfaces - // that are compatible with the environment it's being used in. - // In this on-chain context, the Proof keeper supplies AppKeeperQueryClient and - // AccountKeeperQueryClient that are thin wrappers around the Application and - // Account keepers respectively, and satisfy the RingClient needs. - // TODO_CONSIDERATION: Make ring signature verification a stateless function - // and get rid of the RingClient and its dependencies by moving application - // ring retrieval to the application keeper and making it retrievable using - // the application query client for off-chain actors. - // Signature verification code will still be shared across off/on chain environments + // + // ApplicationQueries & accountQuerier are compatible with the environment + // they're used in and may or may not make an actual network request. + // + // When used in an on-chain context, the ProofKeeper supplies AppKeeperQueryClient + // and AccountKeeperQueryClient that are thin wrappers around the Application and + // Account keepers respectively to satisfy the RingClient needs. + // + // TODO_IMPROVE_CONSIDERATION: Make ring signature verification a stateless + // function and get rid of the RingClient and its dependencies by moving + // application ring retrieval to the application keeper, and making it + // retrievable using the application query client for off-chain actors. Signature + // verification code will still be shared across off/on chain environments. ringKeeperClientDeps := depinject.Supply(polylogger, applicationQuerier, accountQuerier) ringKeeperClient, err := rings.NewRingClient(ringKeeperClientDeps) if err != nil { diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 586297594..6835f11e6 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -17,16 +17,16 @@ import ( ) const ( - // relayMinDifficultyBits is the minimum difficulty that a relay must have to be - // volume / reward applicable. + // relayMinDifficultyBits is the minimum difficulty that a relay must have + // to be reward (i.e. volume) applicable. // TODO_BLOCKER: relayMinDifficultyBits should be a governance-based parameter relayMinDifficultyBits = 0 - // sumSize is the size of the sum of the relay request and response - // in bytes. This is used to extract the relay request and response - // from the closest merkle proof. - // TODO_TECHDEBT: Have a method on the smst to extract the value hash or export - // sumSize to be used instead of current local value + // sumSize is the size of the sum of the relay request and response in bytes. + // This is needed to extract the relay request and response // from the closest + // merkle proof. + // TODO_TECHDEBT(@h5law): Add a method on the smst to extract the value hash + // or export sumSize to be used instead of current local value sumSize = 8 ) @@ -34,18 +34,23 @@ const ( var spec *smt.TrieSpec func init() { - // Use a no prehash spec that returns a nil value hasher for the proof - // verification to avoid hashing the value twice. + // Use a spec that does not prehash values in the smst. This returns a nil + // value hasher for the proof verification in order to to avoid hashing the + // value twice. spec = smt.NoPrehashSpec(sha256.New(), true) } +// SubmitProof is the server handler to submit and store a proof on-chain. +// A proof that's stored on-chain is what leads to rewards (i.e. inflation) +// downstream, making the series of checks a critical part of the protocol. +// TODO_BLOCKER: Prevent proof upserts after the tokenomics module has processes the respective session. func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) (*types.MsgSubmitProofResponse, error) { - // TODO_BLOCKER: Prevent Proof upserts after the tokenomics module has processes the respective session. - logger := k.Logger().With("TECHDEBTmethod", "SubmitProof") - logger.Debug("submitting proof") + logger := k.Logger().With("method", "SubmitProof") + logger.Debug("about to start submitting proof") /* - TODO_INCOMPLETE: Handling the message + TODO_DOCUMENT(@bryanchriswhite): Document these steps in proof + verification, link to the doc for reference and delete the comments. ## Actions (error if anything fails) 1. Retrieve a fully hydrated `session` from on-chain store using `msg` metadata @@ -72,23 +77,23 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( 3. verify(claim.Root, proof.ClosestProof); verify the closest proof is correct */ - // Ensure that all validation and verification checks are successful on the - // MsgSubmitProof message before constructing the proof and inserting it into - // the store. - + // Basic validation of the SubmitProof message. if err := msg.ValidateBasic(); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } - supplierPubKey, err := k.accountQuerier.GetPubKeyFromAddress(ctx, msg.GetSupplierAddress()) + // Retrieve the supplier's public key. + supplierAddr := msg.GetSupplierAddress() + supplierPubKey, err := k.accountQuerier.GetPubKeyFromAddress(ctx, supplierAddr) if err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + // Validate the session header. if _, err := k.queryAndValidateSessionHeader( ctx, msg.GetSessionHeader(), - msg.GetSupplierAddress(), + supplierAddr, ); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } @@ -113,57 +118,81 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( ) } - if err := relay.GetReq().ValidateBasic(); err != nil { + logger = logger. + With( + "session_id", msg.GetSessionHeader().GetSessionId(), + "session_end_height", msg.GetSessionHeader().GetSessionEndBlockHeight(), + "supplier", supplierAddr, + ) + + // Basic validation of the relay request. + relayReq := relay.GetReq() + if err := relayReq.ValidateBasic(); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully validated relay request") - if err := relay.GetRes().ValidateBasic(); err != nil { + // Basic validation of the relay response. + relayRes := relay.GetRes() + if err := relayRes.ValidateBasic(); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully validated relay response") // Verify that the relay request session header matches the proof session header. - if err := compareSessionHeaders(msg.GetSessionHeader(), relay.GetReq().Meta.GetSessionHeader()); err != nil { + if err := compareSessionHeaders(msg.GetSessionHeader(), relayReq.Meta.GetSessionHeader()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully compared relay request session header") // Verify that the relay response session header matches the proof session header. - if err := compareSessionHeaders(msg.GetSessionHeader(), relay.GetRes().Meta.GetSessionHeader()); err != nil { + if err := compareSessionHeaders(msg.GetSessionHeader(), relayRes.Meta.GetSessionHeader()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully compared relay response session header") - // Verify the relay response's signature. - if err := relay.GetRes().VerifySupplierSignature(supplierPubKey); err != nil { + // Verify the relay request's signature. + if err := k.ringClient.VerifyRelayRequestSignature(ctx, relayReq); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully verified relay request signature") - // Verify the relay request's signature. - if err := k.ringClient.VerifyRelayRequestSignature(ctx, relay.GetReq()); err != nil { + // Verify the relay response's signature. + if err := relayRes.VerifySupplierSignature(supplierPubKey); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully verified relay response signature") - // Validate that proof's path matches the earliest proof submission block hash. - if err := k.validateClosestPath(ctx, sparseMerkleClosestProof, msg.GetSessionHeader()); err != nil { + // Verify the relay difficulty is above the minimum required to earn rewards. + if err := validateMiningDifficulty(relayBz); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully validated relay mining difficulty") - // Verify the relay's difficulty. - if err := validateMiningDifficulty(relayBz); err != nil { + // Validate that path the proof is submitted for matches the expected one + // based on the pseudo-random on-chain data associated with the header. + if err := k.validateClosestPath(ctx, sparseMerkleClosestProof, msg.GetSessionHeader()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully validated proof path") + // Retrieve the corresponding claim for the proof submitted so it can be + // used in the proof validation below. claim, err := k.queryAndValidateClaimForProof(ctx, msg) if err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully retrieved and validated claim") // Verify the proof's closest merkle proof. if err := verifyClosestProof(sparseMerkleClosestProof, claim.GetRootHash()); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } + logger.Debug("successfully verified closest merkle proof") // Construct and insert proof after all validation. proof := types.Proof{ - SupplierAddress: msg.GetSupplierAddress(), + SupplierAddress: supplierAddr, SessionHeader: msg.GetSessionHeader(), ClosestMerkleProof: msg.GetProof(), } @@ -174,13 +203,7 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( // TODO_UPNEXT(@Olshansk, #359): Call `tokenomics.SettleSessionAccounting()` here - logger. - With( - "session_id", proof.GetSessionHeader().GetSessionId(), - "session_end_height", proof.GetSessionHeader().GetSessionEndBlockHeight(), - "supplier", proof.GetSupplierAddress(), - ). - Debug("created proof") + logger.Debug("successfully submitted proof") return &types.MsgSubmitProofResponse{}, nil } @@ -247,7 +270,10 @@ func (k msgServer) queryAndValidateClaimForProof( } // compareSessionHeaders compares a session header against an expected session header. +// This is necessary to validate the proof's session header against both the relay +// request and response's session headers. func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.SessionHeader) error { + // Compare the Application address. if sessionHeader.GetApplicationAddress() != expectedSessionHeader.GetApplicationAddress() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders application addresses mismatch expect: %q, got: %q", @@ -256,6 +282,7 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se ) } + // Compare the Service IDs. if sessionHeader.GetService().GetId() != expectedSessionHeader.GetService().GetId() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders service IDs mismatch expect: %q, got: %q", @@ -264,6 +291,7 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se ) } + // Compare the Service names. if sessionHeader.GetService().GetName() != expectedSessionHeader.GetService().GetName() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders service names mismatch expect: %q, got: %q", @@ -272,6 +300,7 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se ) } + // Compare the Session start block heights. if sessionHeader.GetSessionStartBlockHeight() != expectedSessionHeader.GetSessionStartBlockHeight() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders session start heights mismatch expect: %d, got: %d", @@ -280,6 +309,7 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se ) } + // Compare the Session end block heights. if sessionHeader.GetSessionEndBlockHeight() != expectedSessionHeader.GetSessionEndBlockHeight() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders session end heights mismatch expect: %d, got: %d", @@ -288,6 +318,7 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se ) } + // Compare the Session IDs. if sessionHeader.GetSessionId() != expectedSessionHeader.GetSessionId() { return types.ErrProofInvalidRelay.Wrapf( "sessionHeaders session IDs mismatch expect: %q, got: %q", @@ -299,12 +330,13 @@ func compareSessionHeaders(expectedSessionHeader, sessionHeader *sessiontypes.Se return nil } -// verifyClosestProof verifies the closest merkle proof against the expected root hash. +// verifyClosestProof verifies the the correctness of the ClosestMerkleProof +// against the root hash committed to when creating the claim. func verifyClosestProof( proof *smt.SparseMerkleClosestProof, - expectedRootHash []byte, + claimRootHash []byte, ) error { - valid, err := smt.VerifyClosestProof(proof, expectedRootHash, spec) + valid, err := smt.VerifyClosestProof(proof, claimRootHash, spec) if err != nil { return err } @@ -316,10 +348,10 @@ func verifyClosestProof( return nil } -// validateMiningDifficulty ensures that the relay's mining difficulty meets the required -// difficulty. -// TODO_TECHDEBT: Factor out the relay mining difficulty validation into a shared function -// that can be used by both the proof and the miner packages. +// validateMiningDifficulty ensures that the relay's mining difficulty meets the +// required minimum threshold. +// TODO_TECHDEBT: Factor out the relay mining difficulty validation into a shared +// function that can be used by both the proof and the miner packages. func validateMiningDifficulty(relayBz []byte) error { hasher := sha256.New() hasher.Write(relayBz) @@ -347,26 +379,35 @@ func validateMiningDifficulty(relayBz []byte) error { } // validateClosestPath ensures that the proof's path matches the expected path. +// Since the proof path needs to be pseudo-randomly selected AFTER the session +// ends, the seed for this is the block hash at the height when the proof window +// opens. func (k msgServer) validateClosestPath( ctx context.Context, proof *smt.SparseMerkleClosestProof, sessionHeader *sessiontypes.SessionHeader, ) error { // The RelayMiner has to wait until the createClaimWindowStartHeight and the - // submitProofWindowStartHeight are open to respectively create the claim and + // submitProofWindowStartHeight windows are open to create the claim and // submit the proof respectively. // These windows are calculated as (SessionEndBlockHeight + GracePeriodBlockCount). + // // For reference, see relayerSessionsManager.waitForEarliest{CreateClaim,SubmitProof}Height(). + // // The RelayMiner has to wait this long to ensure that late relays (i.e. // submitted during SessionNumber=(N+1) but created during SessionNumber=N) are // still included as part of SessionNumber=N. + // // Since smt.ProveClosest is defined in terms of submitProofWindowStartHeight, // this block's hash needs to be used for validation too. + // // TODO_TECHDEBT(#409): Reference the session rollover documentation here. - sessionEndWithGracePeriodBlockHeight := sessionHeader.GetSessionEndBlockHeight() + + sessionEndBlockHeightWithGracePeriod := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() - blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndWithGracePeriodBlockHeight) + blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndBlockHeightWithGracePeriod) + // TODO_IN_THIS_PR: Finish off the conversation related to this check: https://github.com/pokt-network/poktroll/pull/406/files#r1520790083 + // and #PUC afterwards. if !bytes.Equal(proof.Path, blockHash) { return types.ErrProofInvalidProof.Wrapf( "proof path %x does not match block hash %x", diff --git a/x/proof/types/account_query_client.go b/x/proof/types/account_query_client.go index 48c43a064..9d89b44d7 100644 --- a/x/proof/types/account_query_client.go +++ b/x/proof/types/account_query_client.go @@ -11,8 +11,9 @@ import ( var _ client.AccountQueryClient = (*AccountKeeperQueryClient)(nil) -// AccountKeeperQueryClient is a thin wrapper around the AccountKeeper and does -// not rely on the QueryClient contrariwise to the off-chain implementation. +// AccountKeeperQueryClient is a thin wrapper around the AccountKeeper. +// It does not rely on the QueryClient, and therefore does not make any +// network requests as in the off-chain implementation. type AccountKeeperQueryClient struct { keeper AccountKeeper } @@ -36,8 +37,8 @@ func (accountQueryClient *AccountKeeperQueryClient) GetAccount( return nil, err } - // keeper.GetAccount panics if the account is not found. Recover from the panic - // and return an error instead if that's the case. + // keeper.GetAccount panics if the account is not found. + // Capture the panic and return an error if one occurs. defer func() { if r := recover(); r != nil { err = ErrProofPubKeyNotFound @@ -45,6 +46,7 @@ func (accountQueryClient *AccountKeeperQueryClient) GetAccount( } }() + // Retrieve an account from the account keeper. account = accountQueryClient.keeper.GetAccount(ctx, addrBz) return account, err diff --git a/x/proof/types/application_query_client.go b/x/proof/types/application_query_client.go index c652d2f1c..1cd887314 100644 --- a/x/proof/types/application_query_client.go +++ b/x/proof/types/application_query_client.go @@ -9,8 +9,9 @@ import ( var _ client.ApplicationQueryClient = (*AppKeeperQueryClient)(nil) -// AppKeeperQueryClient is a thin wrapper around the ApplicationKeeper and does -// not rely on the QueryClient contrariwise to the off-chain implementation. +// AppKeeperQueryClient is a thin wrapper around the AccountKeeper. +// It does not rely on the QueryClient, and therefore does not make any +// network requests as in the off-chain implementation. type AppKeeperQueryClient struct { keeper ApplicationKeeper } diff --git a/x/service/types/relay.go b/x/service/types/relay.go index abf94a5db..85aba5aad 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -10,32 +10,31 @@ import ( // Hashing the marshaled request message guarantees that the signable bytes are // always of a constant and expected length. func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { - // Save the signature and restore it after getting the signable bytes - // Since req.Meta is a pointer, this approach is not concurrent safe, - // if two goroutines are calling this method at the same time, the last one + // NB: Since req.Meta is a pointer, this approach is not concurrent safe. + // Save the signature and restore it after getting the signable bytes. + // If two goroutines are calling this method at the same time, the last one // could get the nil signature resulting form the first go routine and restore // nil after getting the signable bytes. // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue // by having req.Meta as a non-pointer type in the corresponding proto file. signature := req.Meta.Signature req.Meta.Signature = nil - requestBz, err := req.Marshal() - // Set the signature back to its original value + requestBz, err := req.Marshal() + // Set the signature back to its original value before checking the error req.Meta.Signature = signature - if err != nil { return [32]byte{}, err } - // return the marshaled request hash to guarantee that the signable bytes are - // always of a constant and expected length + // return the marshaled request hash to guarantee that the signable bytes + // are always of a constant and expected length return sha256.Sum256(requestBz), nil } -// TODO_TEST: Add tests for RelayRequest validation // ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader // and Signature fields. +// TODO_TEST: Add tests for RelayRequest validation func (req *RelayRequest) ValidateBasic() error { if req.GetMeta() == nil { return ErrServiceInvalidRelayRequest.Wrap("missing meta") @@ -56,32 +55,31 @@ func (req *RelayRequest) ValidateBasic() error { // Hashing the marshaled response message guarantees that the signable bytes are // always of a constant and expected length. func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { - // Save the signature and restore it after getting the signable bytes - // Since res.Meta is a pointer, this approach is not concurrent safe, - // if two goroutines are calling this method at the same time, the last one + // NB: Since req.Meta is a pointer, this approach is not concurrent safe. + // Save the signature and restore it after getting the signable bytes. + // If two goroutines are calling this method at the same time, the last one // could get the nil signature resulting form the first go routine and restore // nil after getting the signable bytes. // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue - // by having res.Meta as a non-pointer type in the corresponding proto file. + // by having req.Meta as a non-pointer type in the corresponding proto file. signature := res.Meta.SupplierSignature res.Meta.SupplierSignature = nil - responseBz, err := res.Marshal() + responseBz, err := res.Marshal() // Set the signature back to its original value res.Meta.SupplierSignature = signature - if err != nil { return [32]byte{}, err } - // return the marshaled response hash to guarantee that the signable bytes are - // always of a constant and expected length + // return the marshaled response hash to guarantee that the signable bytes + // are always of a constant and expected length return sha256.Sum256(responseBz), nil } -// TODO_TEST: Add tests for RelayResponse validation // ValidateBasic performs basic validation of the RelayResponse Meta, SessionHeader // and SupplierSignature fields. +// TODO_TEST: Add tests for RelayResponse validation func (res *RelayResponse) ValidateBasic() error { // TODO_FUTURE: if a client gets a response with an invalid/incomplete // SessionHeader, consider sending an on-chain challenge, lowering their From 14638bdeed8163b776ff748d560a44fc8f422e3f Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Tue, 19 Mar 2024 17:37:34 +0100 Subject: [PATCH 15/23] fix: Make relay req/res meta a non-pointer --- api/poktroll/service/relay.pulsar.go | 95 ++++++++++++----------- pkg/client/query/accquerier.go | 2 +- pkg/crypto/rings/client.go | 7 +- pkg/relayer/proxy/proxy_test.go | 27 +------ pkg/relayer/proxy/relay_builders.go | 2 +- pkg/relayer/proxy/relay_verifier.go | 2 +- pkg/relayer/proxy/synchronous.go | 9 --- pkg/relayer/session/session.go | 2 +- pkg/sdk/send_relay.go | 11 +-- proto/poktroll/service/relay.proto | 6 +- testutil/testproxy/relayerproxy.go | 2 +- testutil/testrelayer/relays.go | 2 +- x/proof/keeper/msg_server_submit_proof.go | 2 +- x/service/types/relay.go | 48 ++++-------- 14 files changed, 83 insertions(+), 134 deletions(-) diff --git a/api/poktroll/service/relay.pulsar.go b/api/poktroll/service/relay.pulsar.go index 58e673ebe..320fb76a4 100644 --- a/api/poktroll/service/relay.pulsar.go +++ b/api/poktroll/service/relay.pulsar.go @@ -4,6 +4,7 @@ package service import ( fmt "fmt" runtime "github.com/cosmos/cosmos-proto/runtime" + _ "github.com/cosmos/gogoproto/gogoproto" session "github.com/pokt-network/poktroll/api/poktroll/session" protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoiface "google.golang.org/protobuf/runtime/protoiface" @@ -2781,56 +2782,58 @@ var file_poktroll_service_relay_proto_rawDesc = []byte{ 0x0a, 0x1c, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2f, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2f, 0x72, 0x65, 0x6c, 0x61, 0x79, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x10, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, - 0x1a, 0x1e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2f, 0x73, 0x65, 0x73, 0x73, 0x69, - 0x6f, 0x6e, 0x2f, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, - 0x22, 0x6c, 0x0a, 0x05, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x12, 0x30, 0x0a, 0x03, 0x72, 0x65, 0x71, - 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1e, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, - 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2e, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, - 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x52, 0x03, 0x72, 0x65, 0x71, 0x12, 0x31, 0x0a, 0x03, 0x72, - 0x65, 0x73, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, - 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2e, 0x52, 0x65, 0x6c, 0x61, - 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x52, 0x03, 0x72, 0x65, 0x73, 0x22, 0x7c, - 0x0a, 0x14, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x4d, 0x65, + 0x1a, 0x14, 0x67, 0x6f, 0x67, 0x6f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, 0x67, 0x6f, + 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x1a, 0x1e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, + 0x2f, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x2f, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, + 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0x6c, 0x0a, 0x05, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x12, + 0x30, 0x0a, 0x03, 0x72, 0x65, 0x71, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1e, 0x2e, 0x70, + 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2e, + 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x52, 0x03, 0x72, 0x65, + 0x71, 0x12, 0x31, 0x0a, 0x03, 0x72, 0x65, 0x73, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, + 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, + 0x65, 0x2e, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x52, + 0x03, 0x72, 0x65, 0x73, 0x22, 0x7c, 0x0a, 0x14, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, + 0x75, 0x65, 0x73, 0x74, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x12, 0x46, 0x0a, 0x0e, + 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x5f, 0x68, 0x65, 0x61, 0x64, 0x65, 0x72, 0x18, 0x01, + 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, + 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x2e, 0x53, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, + 0x65, 0x61, 0x64, 0x65, 0x72, 0x52, 0x0d, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, + 0x61, 0x64, 0x65, 0x72, 0x12, 0x1c, 0x0a, 0x09, 0x73, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, 0x72, + 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x09, 0x73, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, + 0x72, 0x65, 0x22, 0x6a, 0x0a, 0x0c, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, + 0x73, 0x74, 0x12, 0x40, 0x0a, 0x04, 0x6d, 0x65, 0x74, 0x61, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, + 0x32, 0x26, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, + 0x69, 0x63, 0x65, 0x2e, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, + 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x42, 0x04, 0xc8, 0xde, 0x1f, 0x00, 0x52, 0x04, + 0x6d, 0x65, 0x74, 0x61, 0x12, 0x18, 0x0a, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x18, + 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x22, 0x6c, + 0x0a, 0x0d, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, + 0x41, 0x0a, 0x04, 0x6d, 0x65, 0x74, 0x61, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x27, 0x2e, + 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, + 0x2e, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x4d, 0x65, + 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x42, 0x04, 0xc8, 0xde, 0x1f, 0x00, 0x52, 0x04, 0x6d, 0x65, + 0x74, 0x61, 0x12, 0x18, 0x0a, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x18, 0x02, 0x20, + 0x01, 0x28, 0x0c, 0x52, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x22, 0x8e, 0x01, 0x0a, + 0x15, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x12, 0x46, 0x0a, 0x0e, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x5f, 0x68, 0x65, 0x61, 0x64, 0x65, 0x72, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x2e, 0x53, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, 0x61, 0x64, 0x65, 0x72, 0x52, - 0x0d, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, 0x61, 0x64, 0x65, 0x72, 0x12, 0x1c, - 0x0a, 0x09, 0x73, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, 0x72, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, - 0x0c, 0x52, 0x09, 0x73, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, 0x72, 0x65, 0x22, 0x64, 0x0a, 0x0c, - 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x3a, 0x0a, 0x04, - 0x6d, 0x65, 0x74, 0x61, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x26, 0x2e, 0x70, 0x6f, 0x6b, - 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2e, 0x52, 0x65, - 0x6c, 0x61, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, - 0x74, 0x61, 0x52, 0x04, 0x6d, 0x65, 0x74, 0x61, 0x12, 0x18, 0x0a, 0x07, 0x70, 0x61, 0x79, 0x6c, - 0x6f, 0x61, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, - 0x61, 0x64, 0x22, 0x66, 0x0a, 0x0d, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, - 0x6e, 0x73, 0x65, 0x12, 0x3b, 0x0a, 0x04, 0x6d, 0x65, 0x74, 0x61, 0x18, 0x01, 0x20, 0x01, 0x28, - 0x0b, 0x32, 0x27, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, - 0x76, 0x69, 0x63, 0x65, 0x2e, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, - 0x73, 0x65, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x52, 0x04, 0x6d, 0x65, 0x74, 0x61, - 0x12, 0x18, 0x0a, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, - 0x0c, 0x52, 0x07, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, 0x22, 0x8e, 0x01, 0x0a, 0x15, 0x52, - 0x65, 0x6c, 0x61, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x4d, 0x65, 0x74, 0x61, - 0x64, 0x61, 0x74, 0x61, 0x12, 0x46, 0x0a, 0x0e, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x5f, - 0x68, 0x65, 0x61, 0x64, 0x65, 0x72, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x70, - 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x2e, - 0x53, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, 0x61, 0x64, 0x65, 0x72, 0x52, 0x0d, 0x73, - 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, 0x61, 0x64, 0x65, 0x72, 0x12, 0x2d, 0x0a, 0x12, - 0x73, 0x75, 0x70, 0x70, 0x6c, 0x69, 0x65, 0x72, 0x5f, 0x73, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, - 0x72, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x11, 0x73, 0x75, 0x70, 0x70, 0x6c, 0x69, - 0x65, 0x72, 0x53, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, 0x72, 0x65, 0x42, 0xa6, 0x01, 0x0a, 0x14, - 0x63, 0x6f, 0x6d, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, 0x65, 0x72, - 0x76, 0x69, 0x63, 0x65, 0x42, 0x0a, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x50, 0x72, 0x6f, 0x74, 0x6f, - 0x50, 0x01, 0x5a, 0x21, 0x63, 0x6f, 0x73, 0x6d, 0x6f, 0x73, 0x73, 0x64, 0x6b, 0x2e, 0x69, 0x6f, - 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2f, 0x73, 0x65, - 0x72, 0x76, 0x69, 0x63, 0x65, 0xa2, 0x02, 0x03, 0x50, 0x53, 0x58, 0xaa, 0x02, 0x10, 0x50, 0x6f, - 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0xca, 0x02, - 0x10, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x5c, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, - 0x65, 0xe2, 0x02, 0x1c, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x5c, 0x53, 0x65, 0x72, - 0x76, 0x69, 0x63, 0x65, 0x5c, 0x47, 0x50, 0x42, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, - 0xea, 0x02, 0x11, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x3a, 0x3a, 0x53, 0x65, 0x72, - 0x76, 0x69, 0x63, 0x65, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x0d, 0x73, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x48, 0x65, 0x61, 0x64, 0x65, 0x72, 0x12, 0x2d, + 0x0a, 0x12, 0x73, 0x75, 0x70, 0x70, 0x6c, 0x69, 0x65, 0x72, 0x5f, 0x73, 0x69, 0x67, 0x6e, 0x61, + 0x74, 0x75, 0x72, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x11, 0x73, 0x75, 0x70, 0x70, + 0x6c, 0x69, 0x65, 0x72, 0x53, 0x69, 0x67, 0x6e, 0x61, 0x74, 0x75, 0x72, 0x65, 0x42, 0xa6, 0x01, + 0x0a, 0x14, 0x63, 0x6f, 0x6d, 0x2e, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x73, + 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x42, 0x0a, 0x52, 0x65, 0x6c, 0x61, 0x79, 0x50, 0x72, 0x6f, + 0x74, 0x6f, 0x50, 0x01, 0x5a, 0x21, 0x63, 0x6f, 0x73, 0x6d, 0x6f, 0x73, 0x73, 0x64, 0x6b, 0x2e, + 0x69, 0x6f, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x70, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2f, + 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0xa2, 0x02, 0x03, 0x50, 0x53, 0x58, 0xaa, 0x02, 0x10, + 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x2e, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, + 0xca, 0x02, 0x10, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x5c, 0x53, 0x65, 0x72, 0x76, + 0x69, 0x63, 0x65, 0xe2, 0x02, 0x1c, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x5c, 0x53, + 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x5c, 0x47, 0x50, 0x42, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, + 0x74, 0x61, 0xea, 0x02, 0x11, 0x50, 0x6f, 0x6b, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x3a, 0x3a, 0x53, + 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/pkg/client/query/accquerier.go b/pkg/client/query/accquerier.go index f61e96999..f02b68369 100644 --- a/pkg/client/query/accquerier.go +++ b/pkg/client/query/accquerier.go @@ -51,7 +51,7 @@ func (aq *accQuerier) GetAccount( ctx context.Context, address string, ) (types.AccountI, error) { - if foundAccount, accountFound := aq.accountCache[address]; accountFound { + if foundAccount, isAccountFound := aq.accountCache[address]; isAccountFound { return foundAccount, nil } diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index 4ec6d4bc7..a64b74a76 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -84,9 +84,6 @@ func (rc *ringClient) VerifyRelayRequestSignature( relayRequest *types.RelayRequest, ) error { relayRequestMeta := relayRequest.GetMeta() - if relayRequestMeta == nil { - return ErrRingClientInvalidRelayRequest.Wrap("missing meta from relay request") - } sessionHeader := relayRequestMeta.GetSessionHeader() if err := sessionHeader.ValidateBasic(); err != nil { @@ -117,7 +114,7 @@ func (rc *ringClient) VerifyRelayRequestSignature( // Get the ring for the application address of the relay request. appAddress := sessionHeader.GetApplicationAddress() - expectedAppRingSig, err := rc.GetRingForAddress(ctx, appAddress) + expectedAppRing, err := rc.GetRingForAddress(ctx, appAddress) if err != nil { return ErrRingClientInvalidRelayRequest.Wrapf( "error getting ring for application address %s: %v", appAddress, err, @@ -125,7 +122,7 @@ func (rc *ringClient) VerifyRelayRequestSignature( } // Compare the expected ring signature against the one provided in the relay request. - if !relayRequestRingSig.Ring().Equals(expectedAppRingSig) { + if !relayRequestRingSig.Ring().Equals(expectedAppRing) { return ErrRingClientInvalidRelayRequestSignature.Wrapf( "ring signature in the relay request does not match the expected one for the app %s", appAddress, ) diff --git a/pkg/relayer/proxy/proxy_test.go b/pkg/relayer/proxy/proxy_test.go index ceccb2dd7..b223f726e 100644 --- a/pkg/relayer/proxy/proxy_test.go +++ b/pkg/relayer/proxy/proxy_test.go @@ -17,7 +17,6 @@ import ( "github.com/pokt-network/poktroll/pkg/relayer/config" "github.com/pokt-network/poktroll/pkg/relayer/proxy" "github.com/pokt-network/poktroll/testutil/testproxy" - servicetypes "github.com/pokt-network/poktroll/x/service/types" sessionkeeper "github.com/pokt-network/poktroll/x/session/keeper" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -385,15 +384,6 @@ func TestRelayerProxy_Relays(t *testing.T) { expectedErrCode: 0, expectedErrMsg: "cannot unmarshal request payload", }, - { - desc: "Missing session meta from relay request", - - relayerProxyBehavior: defaultRelayerProxyBehavior, - inputScenario: sendRequestWithMissingMeta, - - expectedErrCode: -32000, - expectedErrMsg: "missing meta from relay request", - }, { desc: "Missing signature from relay request", @@ -437,7 +427,7 @@ func TestRelayerProxy_Relays(t *testing.T) { inputScenario: sendRequestWithRingSignatureMismatch, expectedErrCode: -32000, - expectedErrMsg: "ring signature does not match ring for application address", + expectedErrMsg: "ring signature in the relay request does not match the expected one for the app", }, { desc: "Session mismatch", @@ -471,7 +461,7 @@ func TestRelayerProxy_Relays(t *testing.T) { inputScenario: sendRequestWithSignatureForDifferentPayload, expectedErrCode: -32000, - expectedErrMsg: "invalid ring signature", + expectedErrMsg: "invalid relay request signature or bytes", }, { desc: "Successful relay", @@ -572,19 +562,6 @@ func sendRequestWithUnparsableBody( return testproxy.GetRelayResponseError(t, res) } -func sendRequestWithMissingMeta( - t *testing.T, - test *testproxy.TestBehavior, -) (errorCode int32, errorMessage string) { - - req := &servicetypes.RelayRequest{ - // RelayRequest is missing Metadata - Payload: testproxy.PrepareJsonRPCRequestPayload(), - } - - return testproxy.MarshalAndSend(test, proxiedServices, defaultProxyServer, defaultService, req) -} - func sendRequestWithMissingSignature( t *testing.T, test *testproxy.TestBehavior, diff --git a/pkg/relayer/proxy/relay_builders.go b/pkg/relayer/proxy/relay_builders.go index c6e9c9a77..7abba5c2e 100644 --- a/pkg/relayer/proxy/relay_builders.go +++ b/pkg/relayer/proxy/relay_builders.go @@ -34,7 +34,7 @@ func (sync *synchronousRPCServer) newRelayResponse( sessionHeader *sessiontypes.SessionHeader, ) (*types.RelayResponse, error) { relayResponse := &types.RelayResponse{ - Meta: &types.RelayResponseMetadata{SessionHeader: sessionHeader}, + Meta: types.RelayResponseMetadata{SessionHeader: sessionHeader}, } responseBz, err := io.ReadAll(response.Body) diff --git a/pkg/relayer/proxy/relay_verifier.go b/pkg/relayer/proxy/relay_verifier.go index fcde49294..98cdd65cd 100644 --- a/pkg/relayer/proxy/relay_verifier.go +++ b/pkg/relayer/proxy/relay_verifier.go @@ -23,7 +23,7 @@ func (rp *relayerProxy) VerifyRelayRequest( // Extract the session header for usage below. // ringCache.VerifyRelayRequestSignature already verified the header's validaity. - sessionHeader := relayRequest.GetMeta().GetSessionHeader() + sessionHeader := relayRequest.GetMeta().SessionHeader // Application address is used to verify the relayRequest signature. // It is guaranteed to be present in the relayRequest since the signature diff --git a/pkg/relayer/proxy/synchronous.go b/pkg/relayer/proxy/synchronous.go index 174e896f2..ad0870a52 100644 --- a/pkg/relayer/proxy/synchronous.go +++ b/pkg/relayer/proxy/synchronous.go @@ -175,15 +175,6 @@ func (sync *synchronousRPCServer) ServeHTTP(writer http.ResponseWriter, request return } - if relayRequest.Meta == nil { - err = ErrRelayerProxyInvalidRelayRequest.Wrapf( - "missing meta from relay request: %v", relayRequest, - ) - sync.replyWithError(ctx, relayRequest.Payload, writer, sync.proxyConfig.ProxyName, supplierService.Id, err) - sync.logger.Warn().Err(err).Msg("relay request metadata is nil which could be a result of failed unmashaling") - return - } - // Relay the request to the proxied service and build the response that will be sent back to the client. relay, err := sync.serveHTTP(ctx, serviceUrl, supplierService, request, relayRequest) if err != nil { diff --git a/pkg/relayer/session/session.go b/pkg/relayer/session/session.go index dc46e644e..d356ee44b 100644 --- a/pkg/relayer/session/session.go +++ b/pkg/relayer/session/session.go @@ -256,7 +256,7 @@ func (rs *relayerSessionsManager) mapAddMinedRelayToSessionTree( // ensure the session tree exists for this relay // TODO_CONSIDERATION: if we get the session header from the response, there // is no possibility that we forgot to hydrate it (i.e. blindly trust the client). - sessionHeader := relay.GetReq().GetMeta().GetSessionHeader() + sessionHeader := relay.GetReq().GetMeta().SessionHeader smst, err := rs.ensureSessionTree(sessionHeader) if err != nil { // TODO_IMPROVE: log additional info? diff --git a/pkg/sdk/send_relay.go b/pkg/sdk/send_relay.go index ccb9e8c97..447418048 100644 --- a/pkg/sdk/send_relay.go +++ b/pkg/sdk/send_relay.go @@ -33,7 +33,7 @@ func (sdk *poktrollSDK) SendRelay( // Prepare the relay request. relayRequest := &types.RelayRequest{ - Meta: &types.RelayRequestMetadata{ + Meta: types.RelayRequestMetadata{ SessionHeader: supplierEndpoint.Header, Signature: nil, // signature added below }, @@ -67,11 +67,6 @@ func (sdk *poktrollSDK) SendRelay( return nil, ErrSDKHandleRelay.Wrapf("error marshaling relay request: %s", err) } relayRequestReader := io.NopCloser(bytes.NewReader(relayRequestBz)) - // TODO_IN_THIS_PR(@red-0ne): Why do we need the following 4 lines? - var relayReq types.RelayRequest - if err := relayReq.Unmarshal(relayRequestBz); err != nil { - return nil, ErrSDKHandleRelay.Wrapf("error unmarshaling relay request: %s", err) - } // Create the HTTP request to send the request to the relayer. // All the RPC protocols to be supported (JSONRPC, Rest, Websockets, gRPC, etc) @@ -109,7 +104,7 @@ func (sdk *poktrollSDK) SendRelay( // relayResponse.ValidateBasic validates Meta and SessionHeader, so // we can safely use the session header. - sessionHeader := relayResponse.GetMeta().GetSessionHeader() + sessionHeader := relayResponse.GetMeta().SessionHeader // Get the supplier's public key. supplierPubKey, err := sdk.accountQuerier.GetPubKeyFromAddress(ctx, supplierEndpoint.SupplierAddress) @@ -126,7 +121,7 @@ func (sdk *poktrollSDK) SendRelay( // Verify the relay response's supplier signature. // TODO_TECHDEBT: if the RelayResponse has an internal error response, we - // SHOULD NOT verify the signature, and return an error early. + // SHOULD NOT verify the signature, and return an error early. // TODO_IMPROVE: Increase logging & telemetry get visibility into failed responses. if err := relayResponse.VerifySupplierSignature(supplierPubKey); err != nil { return nil, ErrSDKVerifyResponseSignature.Wrapf("%s", err) diff --git a/proto/poktroll/service/relay.proto b/proto/poktroll/service/relay.proto index 9b94a4d91..8dc0bdab9 100644 --- a/proto/poktroll/service/relay.proto +++ b/proto/poktroll/service/relay.proto @@ -3,6 +3,8 @@ package poktroll.service; option go_package = "github.com/pokt-network/poktroll/x/service/types"; +import "gogoproto/gogo.proto"; + import "poktroll/session/session.proto"; // Relay contains both the RelayRequest (signed by the Application) and the RelayResponse (signed by the Supplier). @@ -24,7 +26,7 @@ message RelayRequestMetadata { // RelayRequest holds the request details for a relay. message RelayRequest { - RelayRequestMetadata meta = 1; + RelayRequestMetadata meta = 1 [(gogoproto.nullable) = false]; // payload is the serialized payload for the request. // The payload is passed directly to the service and as such can be any // format that the service supports: JSON-RPC, REST, gRPC, etc. @@ -33,7 +35,7 @@ message RelayRequest { // RelayResponse contains the response details for a RelayRequest. message RelayResponse { - RelayResponseMetadata meta = 1; + RelayResponseMetadata meta = 1 [(gogoproto.nullable) = false]; // payload is the serialized payload for the response. // The payload is passed directly from the service and as such can be any // format the the service responds with: JSON-RPC, REST, gRPC, etc. diff --git a/testutil/testproxy/relayerproxy.go b/testutil/testproxy/relayerproxy.go index ba1101abf..64d21ee38 100644 --- a/testutil/testproxy/relayerproxy.go +++ b/testutil/testproxy/relayerproxy.go @@ -393,7 +393,7 @@ func GenerateRelayRequest( sessionId, _ := sessionkeeper.GetSessionId(appAddress, serviceId, blockHashBz, blockHeight) return &servicetypes.RelayRequest{ - Meta: &servicetypes.RelayRequestMetadata{ + Meta: servicetypes.RelayRequestMetadata{ SessionHeader: &sessiontypes.SessionHeader{ ApplicationAddress: appAddress, SessionId: string(sessionId[:]), diff --git a/testutil/testrelayer/relays.go b/testutil/testrelayer/relays.go index 53ddbadc2..db415b468 100644 --- a/testutil/testrelayer/relays.go +++ b/testutil/testrelayer/relays.go @@ -20,7 +20,7 @@ func NewMinedRelay( ) *relayer.MinedRelay { relay := servicetypes.Relay{ Req: &servicetypes.RelayRequest{ - Meta: &servicetypes.RelayRequestMetadata{ + Meta: servicetypes.RelayRequestMetadata{ SessionHeader: &sessiontypes.SessionHeader{ SessionStartBlockHeight: sessionStartHeight, SessionEndBlockHeight: sessionEndHeight, diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 6835f11e6..9831c5205 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -23,7 +23,7 @@ const ( relayMinDifficultyBits = 0 // sumSize is the size of the sum of the relay request and response in bytes. - // This is needed to extract the relay request and response // from the closest + // This is needed to extract the relay request and response from the closest // merkle proof. // TODO_TECHDEBT(@h5law): Add a method on the smst to extract the value hash // or export sumSize to be used instead of current local value diff --git a/x/service/types/relay.go b/x/service/types/relay.go index 85aba5aad..cbc45161c 100644 --- a/x/service/types/relay.go +++ b/x/service/types/relay.go @@ -9,20 +9,11 @@ import ( // GetSignableBytesHash returns the hash of the signable bytes of the relay request // Hashing the marshaled request message guarantees that the signable bytes are // always of a constant and expected length. -func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { - // NB: Since req.Meta is a pointer, this approach is not concurrent safe. - // Save the signature and restore it after getting the signable bytes. - // If two goroutines are calling this method at the same time, the last one - // could get the nil signature resulting form the first go routine and restore - // nil after getting the signable bytes. - // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue - // by having req.Meta as a non-pointer type in the corresponding proto file. - signature := req.Meta.Signature +func (req RelayRequest) GetSignableBytesHash() ([32]byte, error) { + // req and req.Meta are not pointers, so we can set the signature to nil + // in order to generate the signable bytes hash without the need restore it. req.Meta.Signature = nil - requestBz, err := req.Marshal() - // Set the signature back to its original value before checking the error - req.Meta.Signature = signature if err != nil { return [32]byte{}, err } @@ -36,15 +27,16 @@ func (req *RelayRequest) GetSignableBytesHash() ([32]byte, error) { // and Signature fields. // TODO_TEST: Add tests for RelayRequest validation func (req *RelayRequest) ValidateBasic() error { - if req.GetMeta() == nil { - return ErrServiceInvalidRelayRequest.Wrap("missing meta") + meta := req.GetMeta() + if meta.GetSessionHeader() == nil { + return ErrServiceInvalidRelayRequest.Wrap("missing session header") } - if err := req.GetMeta().GetSessionHeader().ValidateBasic(); err != nil { + if err := meta.GetSessionHeader().ValidateBasic(); err != nil { return ErrServiceInvalidRelayRequest.Wrapf("invalid session header: %s", err) } - if len(req.GetMeta().GetSignature()) == 0 { + if len(meta.GetSignature()) == 0 { return ErrServiceInvalidRelayRequest.Wrap("missing application signature") } @@ -54,20 +46,11 @@ func (req *RelayRequest) ValidateBasic() error { // GetSignableBytesHash returns the hash of the signable bytes of the relay response // Hashing the marshaled response message guarantees that the signable bytes are // always of a constant and expected length. -func (res *RelayResponse) GetSignableBytesHash() ([32]byte, error) { - // NB: Since req.Meta is a pointer, this approach is not concurrent safe. - // Save the signature and restore it after getting the signable bytes. - // If two goroutines are calling this method at the same time, the last one - // could get the nil signature resulting form the first go routine and restore - // nil after getting the signable bytes. - // TODO_TECHDEBT: Consider using a deep copy of the response to avoid this issue - // by having req.Meta as a non-pointer type in the corresponding proto file. - signature := res.Meta.SupplierSignature +func (res RelayResponse) GetSignableBytesHash() ([32]byte, error) { + // res and res.Meta are not pointers, so we can set the signature to nil + // in order to generate the signable bytes hash without the need restore it. res.Meta.SupplierSignature = nil - responseBz, err := res.Marshal() - // Set the signature back to its original value - res.Meta.SupplierSignature = signature if err != nil { return [32]byte{}, err } @@ -85,15 +68,16 @@ func (res *RelayResponse) ValidateBasic() error { // SessionHeader, consider sending an on-chain challenge, lowering their // QoS, or other future work. - if res.GetMeta() == nil { + meta := res.GetMeta() + if meta.GetSessionHeader() == nil { return ErrServiceInvalidRelayResponse.Wrap("missing meta") } - if err := res.GetMeta().GetSessionHeader().ValidateBasic(); err != nil { + if err := meta.GetSessionHeader().ValidateBasic(); err != nil { return ErrServiceInvalidRelayResponse.Wrapf("invalid session header: %v", err) } - if len(res.GetMeta().GetSupplierSignature()) == 0 { + if len(meta.GetSupplierSignature()) == 0 { return ErrServiceInvalidRelayResponse.Wrap("missing supplier signature") } @@ -109,7 +93,7 @@ func (res *RelayResponse) VerifySupplierSignature(supplierPubKey cryptotypes.Pub return err } - if ok := supplierPubKey.VerifySignature(signableBz[:], res.GetMeta().GetSupplierSignature()); !ok { + if ok := supplierPubKey.VerifySignature(signableBz[:], res.GetMeta().SupplierSignature); !ok { return ErrServiceInvalidRelayResponse.Wrap("invalid signature") } From ab43ea70de2f30e2c63eb822767117a0a1a881b3 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Wed, 20 Mar 2024 05:37:50 +0100 Subject: [PATCH 16/23] fix: Restore bank expected keeper --- x/proof/types/expected_keepers.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x/proof/types/expected_keepers.go b/x/proof/types/expected_keepers.go index 410b89141..ef1555452 100644 --- a/x/proof/types/expected_keepers.go +++ b/x/proof/types/expected_keepers.go @@ -21,6 +21,13 @@ type AccountKeeper interface { GetAccount(context.Context, sdk.AccAddress) sdk.AccountI } +// BankKeeper defines the expected interface for the Bank module. +type BankKeeper interface { + SpendableCoins(context.Context, sdk.AccAddress) sdk.Coins + DelegateCoinsFromAccountToModule(ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error + UndelegateCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error +} + // ApplicationKeeper defines the expected application keeper to retrieve applications type ApplicationKeeper interface { GetApplication(ctx context.Context, address string) (app apptypes.Application, found bool) From 298e7adb3d835088fb82d19ecd93fbf20810ba99 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Thu, 21 Mar 2024 17:53:03 +0100 Subject: [PATCH 17/23] chore: Update SMT dependency --- go.mod | 1 + go.sum | 4 +- pkg/crypto/pubkey_client/client.go | 59 ----------------------- pkg/crypto/pubkey_client/errors.go | 8 --- pkg/crypto/rings/cache.go | 6 +-- pkg/crypto/rings/client.go | 2 +- x/proof/keeper/keeper.go | 2 +- x/proof/keeper/msg_server_submit_proof.go | 15 +++--- 8 files changed, 14 insertions(+), 83 deletions(-) delete mode 100644 pkg/crypto/pubkey_client/client.go delete mode 100644 pkg/crypto/pubkey_client/errors.go diff --git a/go.mod b/go.mod index b844f0cb9..61f9b82a4 100644 --- a/go.mod +++ b/go.mod @@ -277,3 +277,4 @@ require ( ) // replace github.com/cosmos/cosmos-sdk => github.com/rollkit/cosmos-sdk v0.50.1-rollkit-v0.11.19-no-fraud-proofs +replace github.com/pokt-network/smt => github.com/pokt-network/smt v0.9.3-0.20240321060129-e3dbbbd9f97d diff --git a/go.sum b/go.sum index f6f3a1e70..fa93e23d4 100644 --- a/go.sum +++ b/go.sum @@ -978,8 +978,8 @@ github.com/pkg/profile v1.7.0/go.mod h1:8Uer0jas47ZQMJ7VD+OHknK4YDY07LPUC6dEvqDj github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/pokt-network/smt v0.9.2 h1:h/GnFm1F6mNBbF1hopr+9+y7nr173SU55NX7NxTVU0Y= -github.com/pokt-network/smt v0.9.2/go.mod h1:S4Ho4OPkK2v2vUCHNtA49XDjqUC/OFYpBbynRVYmxvA= +github.com/pokt-network/smt v0.9.3-0.20240321060129-e3dbbbd9f97d h1:6x7LiRWV+mHugWbJlGaYSWESEV+by8hGIbXb3/bWXOg= +github.com/pokt-network/smt v0.9.3-0.20240321060129-e3dbbbd9f97d/go.mod h1:S4Ho4OPkK2v2vUCHNtA49XDjqUC/OFYpBbynRVYmxvA= github.com/pokt-network/smt/kvstore/badger v0.0.0-20240109205447-868237978c0b h1:TjfgV3vgW0zW47Br/OgUXD4M8iyR74EYanbFfN4ed8o= github.com/pokt-network/smt/kvstore/badger v0.0.0-20240109205447-868237978c0b/go.mod h1:GbzcG5ebj8twKmBL1VzdPM4NS44okwYXBfQaVXT+6yU= github.com/posener/complete v1.1.1/go.mod h1:em0nMJCgc9GFtwrmVmEMR/ZL6WyhyjMBndrE9hABlRI= diff --git a/pkg/crypto/pubkey_client/client.go b/pkg/crypto/pubkey_client/client.go deleted file mode 100644 index 042bed580..000000000 --- a/pkg/crypto/pubkey_client/client.go +++ /dev/null @@ -1,59 +0,0 @@ -package pubkeyclient - -import ( - "context" - - "cosmossdk.io/depinject" - cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - - "github.com/pokt-network/poktroll/pkg/client" - "github.com/pokt-network/poktroll/pkg/crypto" -) - -var _ crypto.PubKeyClient = (*pubKeyClient)(nil) - -// pubKeyClient is an implementation of the PubKeyClient that uses an account -// querier to get the public key of an address. -type pubKeyClient struct { - // accountQuerier is the querier for the account module, it is used to get - // the public key corresponding to an address. - accountQuerier client.AccountQueryClient -} - -// NewPubKeyClient creates a new PubKeyClient with the given dependencies. -// The querier is injected using depinject and has to be specific to the -// environment in which the pubKeyClient is initialized as on-chain and off-chain -// environments may have different queriers. -// -// Required dependencies: -// - client.AccountQueryClient -func NewPubKeyClient(deps depinject.Config) (crypto.PubKeyClient, error) { - pc := new(pubKeyClient) - - if err := depinject.Inject( - deps, - &pc.accountQuerier, - ); err != nil { - return nil, err - } - - return pc, nil -} - -// GetPubKeyFromAddress returns the public key of the given address. -// It retrieves the corresponding account by querying for it and returns -// the associated public key. -func (pc *pubKeyClient) GetPubKeyFromAddress(ctx context.Context, address string) (cryptotypes.PubKey, error) { - acc, err := pc.accountQuerier.GetAccount(ctx, address) - if err != nil { - return nil, err - } - - // If the account's public key is nil, then return an error. - pubKey := acc.GetPubKey() - if pubKey == nil { - return nil, ErrPubKeyClientEmptyPubKey - } - - return pubKey, nil -} diff --git a/pkg/crypto/pubkey_client/errors.go b/pkg/crypto/pubkey_client/errors.go deleted file mode 100644 index d59d7f0a5..000000000 --- a/pkg/crypto/pubkey_client/errors.go +++ /dev/null @@ -1,8 +0,0 @@ -package pubkeyclient - -import sdkerrors "cosmossdk.io/errors" - -var ( - codespace = "pubkeyclient" - ErrPubKeyClientEmptyPubKey = sdkerrors.Register(codespace, 1, "empty public key") -) diff --git a/pkg/crypto/rings/cache.go b/pkg/crypto/rings/cache.go index 2e7a62af4..4817832d4 100644 --- a/pkg/crypto/rings/cache.go +++ b/pkg/crypto/rings/cache.go @@ -67,7 +67,7 @@ func NewRingCache(deps depinject.Config) (_ crypto.RingCache, err error) { // Start starts the ring cache by subscribing to on-chain redelegation events. func (rc *ringCache) Start(ctx context.Context) { - rc.logger.Info().Msg("starting ring ringsByAddr") + rc.logger.Info().Msg("starting ring cache") // Stop the ringCache when the context is cancelled. go func() { select { @@ -148,7 +148,7 @@ func (rc *ringCache) GetRingForAddress( if ok { rc.logger.Debug(). Str("app_address", appAddress). - Msg("ring ringsByAddr hit; using cached ring") + Msg("ring cache hit; using cached ring") return ring, nil } @@ -156,7 +156,7 @@ func (rc *ringCache) GetRingForAddress( // If the ring is not in the cache, get it from the ring client. rc.logger.Debug(). Str("app_address", appAddress). - Msg("ring ringsByAddr miss; fetching from application module") + Msg("ring cache miss; fetching from application module") ring, err = rc.ringClient.GetRingForAddress(ctx, appAddress) if err != nil { diff --git a/pkg/crypto/rings/client.go b/pkg/crypto/rings/client.go index a64b74a76..fd1106327 100644 --- a/pkg/crypto/rings/client.go +++ b/pkg/crypto/rings/client.go @@ -104,7 +104,7 @@ func (rc *ringClient) VerifyRelayRequestSignature( return ErrRingClientInvalidRelayRequest.Wrap("missing signature from relay request") } - // Convert the request signature to a ring signature. + // Deserialize the request signature bytes back into a ring signature. relayRequestRingSig := new(ring.RingSig) if err := relayRequestRingSig.Deserialize(ring_secp256k1.NewCurve(), signature); err != nil { return ErrRingClientInvalidRelayRequestSignature.Wrapf( diff --git a/x/proof/keeper/keeper.go b/x/proof/keeper/keeper.go index 585478bfd..4a6c69268 100644 --- a/x/proof/keeper/keeper.go +++ b/x/proof/keeper/keeper.go @@ -58,7 +58,7 @@ func NewKeeper( // RingKeeperClient holds the logic of verifying RelayRequests ring signatures // for both on-chain and off-chain actors. // - // ApplicationQueries & accountQuerier are compatible with the environment + // ApplicationQueriers & AccountQuerier are compatible with the environment // they're used in and may or may not make an actual network request. // // When used in an on-chain context, the ProofKeeper supplies AppKeeperQueryClient diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 9831c5205..7808368aa 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -21,13 +21,6 @@ const ( // to be reward (i.e. volume) applicable. // TODO_BLOCKER: relayMinDifficultyBits should be a governance-based parameter relayMinDifficultyBits = 0 - - // sumSize is the size of the sum of the relay request and response in bytes. - // This is needed to extract the relay request and response from the closest - // merkle proof. - // TODO_TECHDEBT(@h5law): Add a method on the smst to extract the value hash - // or export sumSize to be used instead of current local value - sumSize = 8 ) // SMT specification used for the proof verification. @@ -108,8 +101,7 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( } // Get the relay request and response from the proof.GetClosestMerkleProof. - closestValueHash := sparseMerkleClosestProof.ClosestValueHash - relayBz := closestValueHash[:len(closestValueHash)-sumSize] + relayBz := sparseMerkleClosestProof.GetValueHash(spec) relay := &servicetypes.Relay{} if err := k.cdc.Unmarshal(relayBz, relay); err != nil { return nil, types.ErrProofInvalidRelay.Wrapf( @@ -152,6 +144,7 @@ func (k msgServer) SubmitProof(ctx context.Context, msg *types.MsgSubmitProof) ( logger.Debug("successfully compared relay response session header") // Verify the relay request's signature. + // TODO_TECHDEBT(@h5law): Fetch the correct ring for the session this relay is from. if err := k.ringClient.VerifyRelayRequestSignature(ctx, relayReq); err != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } @@ -402,12 +395,16 @@ func (k msgServer) validateClosestPath( // this block's hash needs to be used for validation too. // // TODO_TECHDEBT(#409): Reference the session rollover documentation here. + // TODO_BLOCKER: Update `blockHeight` to be the value of when the `ProofWindow` + // opens once the variable is added. sessionEndBlockHeightWithGracePeriod := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndBlockHeightWithGracePeriod) // TODO_IN_THIS_PR: Finish off the conversation related to this check: https://github.com/pokt-network/poktroll/pull/406/files#r1520790083 // and #PUC afterwards. + // TODO_BLOCKER: The seed of the path should be `ConcatAndHash(blockHash, '.', sessionId)` + // to prevent all proofs needing to use the same path. if !bytes.Equal(proof.Path, blockHash) { return types.ErrProofInvalidProof.Wrapf( "proof path %x does not match block hash %x", From 19ae70fe2aa8b8620380b8a414d9c7624bd1f1ae Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 12:03:15 -0700 Subject: [PATCH 18/23] A couple more nits --- pkg/client/query/accquerier.go | 5 ++++- x/proof/keeper/msg_server_submit_proof.go | 9 ++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/client/query/accquerier.go b/pkg/client/query/accquerier.go index f02b68369..7e84bdbde 100644 --- a/pkg/client/query/accquerier.go +++ b/pkg/client/query/accquerier.go @@ -55,17 +55,20 @@ func (aq *accQuerier) GetAccount( return foundAccount, nil } + // Query the blockchain for the account record req := &accounttypes.QueryAccountRequest{Address: address} res, err := aq.accountQuerier.Account(ctx, req) if err != nil { return nil, ErrQueryAccountNotFound.Wrapf("address: %s [%v]", address, err) } + + // Unpack and cache the account object var fetchedAccount types.AccountI if err = queryCodec.UnpackAny(res.Account, &fetchedAccount); err != nil { return nil, ErrQueryUnableToDeserializeAccount.Wrapf("address: %s [%v]", address, err) } - aq.accountCache[address] = fetchedAccount + return fetchedAccount, nil } diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index 7808368aa..b8f1ba3a7 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -400,11 +400,10 @@ func (k msgServer) validateClosestPath( sessionEndBlockHeightWithGracePeriod := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndBlockHeightWithGracePeriod) - - // TODO_IN_THIS_PR: Finish off the conversation related to this check: https://github.com/pokt-network/poktroll/pull/406/files#r1520790083 - // and #PUC afterwards. - // TODO_BLOCKER: The seed of the path should be `ConcatAndHash(blockHash, '.', sessionId)` - // to prevent all proofs needing to use the same path. +. + // TODO_BLOCKER(@Olshansk, @red-0ne, @h5law): The seed of the path should be + // `ConcatAndHash(blockHash, '.', sessionId)` to prevent all proofs needing to use the same path. + // See the conversation in the following thread for more details: https://github.com/pokt-network/poktroll/pull/406#discussion_r1520790083 if !bytes.Equal(proof.Path, blockHash) { return types.ErrProofInvalidProof.Wrapf( "proof path %x does not match block hash %x", From ac91121a181fbbce7fd142f2f7171e60440abc17 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 12:06:49 -0700 Subject: [PATCH 19/23] Removed accidental period --- x/proof/keeper/msg_server_submit_proof.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/proof/keeper/msg_server_submit_proof.go b/x/proof/keeper/msg_server_submit_proof.go index b8f1ba3a7..6ce05d024 100644 --- a/x/proof/keeper/msg_server_submit_proof.go +++ b/x/proof/keeper/msg_server_submit_proof.go @@ -400,7 +400,7 @@ func (k msgServer) validateClosestPath( sessionEndBlockHeightWithGracePeriod := sessionHeader.GetSessionEndBlockHeight() + sessionkeeper.GetSessionGracePeriodBlockCount() blockHash := k.sessionKeeper.GetBlockHash(ctx, sessionEndBlockHeightWithGracePeriod) -. + // TODO_BLOCKER(@Olshansk, @red-0ne, @h5law): The seed of the path should be // `ConcatAndHash(blockHash, '.', sessionId)` to prevent all proofs needing to use the same path. // See the conversation in the following thread for more details: https://github.com/pokt-network/poktroll/pull/406#discussion_r1520790083 From 25295d36d8471ac47410e45806311c1429fc4fc0 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 13:57:58 -0700 Subject: [PATCH 20/23] Fixing unit tests --- pkg/client/supplier/client_test.go | 5 ++++- pkg/client/tx/client_test.go | 7 ++++++- pkg/relayer/session/proof.go | 1 + pkg/relayer/session/session_test.go | 16 ++++++++++------ testutil/testclient/testblock/client.go | 20 +++++++++++++------- 5 files changed, 34 insertions(+), 15 deletions(-) diff --git a/pkg/client/supplier/client_test.go b/pkg/client/supplier/client_test.go index 03fc27567..a72542ce4 100644 --- a/pkg/client/supplier/client_test.go +++ b/pkg/client/supplier/client_test.go @@ -172,8 +172,11 @@ func TestSupplierClient_SubmitProof(t *testing.T) { kvStore, err := badger.NewKVStore("") require.NoError(t, err) + // Generating an ephemeral tree & spec just so we can submit + // a proof of the right size. tree := smt.NewSparseMerkleSumTrie(kvStore, sha256.New()) - proof, err := tree.ProveClosest([]byte{1}) + emptyPath := make([]byte, tree.PathHasherSize()) + proof, err := tree.ProveClosest(emptyPath) require.NoError(t, err) go func() { diff --git a/pkg/client/tx/client_test.go b/pkg/client/tx/client_test.go index 770848a09..ed6d6b208 100644 --- a/pkg/client/tx/client_test.go +++ b/pkg/client/tx/client_test.go @@ -2,6 +2,7 @@ package tx_test import ( "context" + "crypto/sha256" "sync" "testing" "time" @@ -15,6 +16,7 @@ import ( cosmoskeyring "github.com/cosmos/cosmos-sdk/crypto/keyring" "github.com/cosmos/cosmos-sdk/types" "github.com/golang/mock/gomock" + "github.com/pokt-network/smt" "github.com/stretchr/testify/require" "github.com/pokt-network/poktroll/pkg/client" @@ -407,8 +409,11 @@ func TestTxClient_SignAndBroadcast_Timeout(t *testing.T) { err, errCh := eitherErr.SyncOrAsyncError() require.NoError(t, err) + spec := smt.NoPrehashSpec(sha256.New(), true) + emptyBlockHash := make([]byte, spec.PathHasherSize()) + for i := 0; i < tx.DefaultCommitTimeoutHeightOffset; i++ { - blocksPublishCh <- testblock.NewAnyTimesBlock(t, []byte{}, int64(i+1)) + blocksPublishCh <- testblock.NewAnyTimesBlock(t, emptyBlockHash, int64(i+1)) } // Assert that we receive the expected error type & message. diff --git a/pkg/relayer/session/proof.go b/pkg/relayer/session/proof.go index 081d10dca..ec4fe9f48 100644 --- a/pkg/relayer/session/proof.go +++ b/pkg/relayer/session/proof.go @@ -2,6 +2,7 @@ package session import ( "context" + "fmt" "github.com/pokt-network/poktroll/pkg/either" "github.com/pokt-network/poktroll/pkg/observable" diff --git a/pkg/relayer/session/session_test.go b/pkg/relayer/session/session_test.go index 64d3dcef1..7387702b1 100644 --- a/pkg/relayer/session/session_test.go +++ b/pkg/relayer/session/session_test.go @@ -2,10 +2,12 @@ package session_test import ( "context" + "crypto/sha256" "testing" "time" "cosmossdk.io/depinject" + "github.com/pokt-network/smt" "github.com/stretchr/testify/require" "github.com/pokt-network/poktroll/pkg/client" @@ -25,14 +27,16 @@ func TestRelayerSessionsManager_Start(t *testing.T) { sessionStartHeight = 1 sessionEndHeight = 2 ) + var ( - zeroByteSlice = []byte{0} - _, ctx = testpolylog.NewLoggerWithCtx(context.Background(), polyzero.DebugLevel) + _, ctx = testpolylog.NewLoggerWithCtx(context.Background(), polyzero.DebugLevel) + spec = smt.NoPrehashSpec(sha256.New(), true) + emptyBlockHash = make([]byte, spec.PathHasherSize()) ) // Set up dependencies. blocksObs, blockPublishCh := channel.NewReplayObservable[client.Block](ctx, 1) - blockClient := testblock.NewAnyTimesCommittedBlocksSequenceBlockClient(t, blocksObs) + blockClient := testblock.NewAnyTimesCommittedBlocksSequenceBlockClient(t, emptyBlockHash, blocksObs) supplierClient := testsupplier.NewOneTimeClaimProofSupplierClient(ctx, t) deps := depinject.Supply(blockClient, supplierClient) @@ -60,7 +64,7 @@ func TestRelayerSessionsManager_Start(t *testing.T) { time.Sleep(10 * time.Millisecond) // Publish a block to the blockPublishCh to simulate non-actionable blocks. - noopBlock := testblock.NewAnyTimesBlock(t, zeroByteSlice, sessionStartHeight) + noopBlock := testblock.NewAnyTimesBlock(t, emptyBlockHash, sessionStartHeight) blockPublishCh <- noopBlock // Calculate the session grace period end block height to emit that block height @@ -70,7 +74,7 @@ func TestRelayerSessionsManager_Start(t *testing.T) { // Publish a block to the blockPublishCh to trigger claim creation for the session. // TODO_TECHDEBT: assumes claiming at sessionGracePeriodEndBlockHeight is valid. // This will likely change in future work. - triggerClaimBlock := testblock.NewAnyTimesBlock(t, zeroByteSlice, sessionGracePeriodEndBlockHeight) + triggerClaimBlock := testblock.NewAnyTimesBlock(t, emptyBlockHash, sessionGracePeriodEndBlockHeight) blockPublishCh <- triggerClaimBlock // TODO_IMPROVE: ensure correctness of persisted session trees here. @@ -78,7 +82,7 @@ func TestRelayerSessionsManager_Start(t *testing.T) { // Publish a block to the blockPublishCh to trigger proof submission for the session. // TODO_TECHDEBT: assumes proving at sessionGracePeriodEndBlockHeight + 1 is valid. // This will likely change in future work. - triggerProofBlock := testblock.NewAnyTimesBlock(t, zeroByteSlice, sessionGracePeriodEndBlockHeight+1) + triggerProofBlock := testblock.NewAnyTimesBlock(t, emptyBlockHash, sessionGracePeriodEndBlockHeight+1) blockPublishCh <- triggerProofBlock // Wait a tick to allow the relayer sessions manager to process asynchronously. diff --git a/testutil/testclient/testblock/client.go b/testutil/testclient/testblock/client.go index 5948cb9db..2a8eb708a 100644 --- a/testutil/testclient/testblock/client.go +++ b/testutil/testclient/testblock/client.go @@ -2,6 +2,7 @@ package testblock import ( "context" + "fmt" "testing" "cosmossdk.io/depinject" @@ -36,12 +37,13 @@ func NewLocalnetClient(ctx context.Context, t *testing.T) client.BlockClient { // and when that call is made, it returns the given EventsObservable[Block]. func NewAnyTimesCommittedBlocksSequenceBlockClient( t *testing.T, + blockHash []byte, blocksObs observable.Observable[client.Block], ) *mockclient.MockBlockClient { t.Helper() // Create a mock for the block client which expects the LastNBlocks method to be called any number of times. - blockClientMock := NewAnyTimeLastNBlocksBlockClient(t, nil, 0) + blockClientMock := NewAnyTimeLastNBlocksBlockClient(t, blockHash, 0) // Set up the mock expectation for the CommittedBlocksSequence method. When // the method is called, it returns a new replay observable that publishes @@ -91,14 +93,14 @@ func NewOneTimeCommittedBlocksSequenceBlockClient( // method is called, it returns a mock Block with the provided hash and height. func NewAnyTimeLastNBlocksBlockClient( t *testing.T, - hash []byte, - height int64, + blockHash []byte, + blockHeight int64, ) *mockclient.MockBlockClient { t.Helper() ctrl := gomock.NewController(t) // Create a mock block that returns the provided hash and height. - blockMock := NewAnyTimesBlock(t, hash, height) + blockMock := NewAnyTimesBlock(t, blockHash, blockHeight) // Create a mock block client that expects calls to LastNBlocks method and // returns the mock block. blockClientMock := mockclient.NewMockBlockClient(ctrl) @@ -110,14 +112,18 @@ func NewAnyTimeLastNBlocksBlockClient( // NewAnyTimesBlock creates a mock Block that expects calls to Height and Hash // methods any number of times. When the methods are called, they return the // provided height and hash respectively. -func NewAnyTimesBlock(t *testing.T, hash []byte, height int64) *mockclient.MockBlock { +func NewAnyTimesBlock( + t *testing.T, + blockHash []byte, + blockHeight int64, +) *mockclient.MockBlock { t.Helper() ctrl := gomock.NewController(t) // Create a mock block that returns the provided hash and height AnyTimes. blockMock := mockclient.NewMockBlock(ctrl) - blockMock.EXPECT().Height().Return(height).AnyTimes() - blockMock.EXPECT().Hash().Return(hash).AnyTimes() + blockMock.EXPECT().Height().Return(blockHeight).AnyTimes() + blockMock.EXPECT().Hash().Return(blockHash).AnyTimes() return blockMock } From e09e71943dc9af816a44761ac18e7090d5c1ccb7 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 14:02:50 -0700 Subject: [PATCH 21/23] Fix impors after debugging --- pkg/relayer/session/proof.go | 1 - testutil/testclient/testblock/client.go | 1 - 2 files changed, 2 deletions(-) diff --git a/pkg/relayer/session/proof.go b/pkg/relayer/session/proof.go index ec4fe9f48..081d10dca 100644 --- a/pkg/relayer/session/proof.go +++ b/pkg/relayer/session/proof.go @@ -2,7 +2,6 @@ package session import ( "context" - "fmt" "github.com/pokt-network/poktroll/pkg/either" "github.com/pokt-network/poktroll/pkg/observable" diff --git a/testutil/testclient/testblock/client.go b/testutil/testclient/testblock/client.go index 2a8eb708a..0724de904 100644 --- a/testutil/testclient/testblock/client.go +++ b/testutil/testclient/testblock/client.go @@ -2,7 +2,6 @@ package testblock import ( "context" - "fmt" "testing" "cosmossdk.io/depinject" From 6df5967654681f7deb25dcf47e4bd3ece6db21c3 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 14:08:08 -0700 Subject: [PATCH 22/23] Empty commit From b103308d4ceff8eaab923a1b697ff20f98346609 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Thu, 21 Mar 2024 15:26:08 -0700 Subject: [PATCH 23/23] Minor e2e test fix --- Makefile | 10 ++++++++-- e2e/tests/tokenomics.feature | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 8b704b4de..a467c6b63 100644 --- a/Makefile +++ b/Makefile @@ -616,8 +616,14 @@ acc_initialize_pubkeys: ## Make sure the account keeper has public keys for all .PHONY: acc_initialize_pubkeys_warn_message acc_initialize_pubkeys_warn_message: ## Print a warning message about the need to run `make acc_initialize_pubkeys` - @printf "!!!!!!!!! YOU MUST RUN THE FOLLOWING COMMAND ONCE FOR E2E TESTS TO WORK AFTER THE NETWORK HAS STARTED !!!!!!!!!\n"\ - "\t\tmake acc_initialize_pubkeys\n" + @echo "+----------------------------------------------------------------------------------+" + @echo "| |" + @echo "| IMPORTANT: Please run the following command once to initialize E2E tests |" + @echo "| after the network has started: |" + @echo "| make acc_initialize_pubkeys |" + @echo "| |" + @echo "+----------------------------------------------------------------------------------+" + ############## ### Claims ### diff --git a/e2e/tests/tokenomics.feature b/e2e/tests/tokenomics.feature index 5362550b0..09e6cd83b 100644 --- a/e2e/tests/tokenomics.feature +++ b/e2e/tests/tokenomics.feature @@ -6,7 +6,7 @@ Feature: Tokenomics Namespaces And an account exists for "supplier1" And an account exists for "app1" When the supplier "supplier1" has serviced a session with "20" relays for service "svc1" for application "app1" - And the user should wait for "5" seconds + # And the user should wait for "5" seconds # TODO_UPNEXT(@Olshansk, #359): Expand on the two expectations below after integrating the tokenomics module # into the supplier module. # Then the account balance of "supplier1" should be "1000" uPOKT "more" than before