[Session] fix: session mismatch due to off-by-one block height (#368)

Co-authored-by: Daniel Olshansky <[email protected]>

e2e/tests/session.feature

@@ -5,14 +5,16 @@ Feature: Session Namespace
   Scenario: Supplier completes claim/proof lifecycle for a valid session
     Given the user has the pocketd binary installed
     When the supplier "supplier1" has serviced a session with "5" relays for service "svc1" for application "app1"
-    And the user should wait for "5" seconds
+    # TODO_TECHDEBT: Once the session grace period is configurable, set it to 0 at the beginning of this test.
+    # 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
     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...
+    # TODO_IMPROVE: And an event should be emitted...
     And after the supplier submits a proof for the session for service "svc1" for application "app1"
     Then the proof submitted by supplier "supplier1" for service "svc1" for application "app1" should be persisted on-chain
-#  TODO_IMPROVE: ...
-#    And an event should be emitted...
+    # TODO_IMPROVE: And an event should be emitted...
 
 # TODO_BLOCKER(@red-0ne): Make sure to implement and validate this test
 # One way to exercise this behavior is to close the `RelayMiner` port to prevent

go.mod

@@ -24,7 +24,6 @@ require (
 	github.com/athanorlabs/go-dleq v0.1.0
 	github.com/cometbft/cometbft v0.37.2
 	github.com/cometbft/cometbft-db v0.8.0
-	github.com/cosmos/cosmos-proto v1.0.0-beta.2
 	github.com/cosmos/cosmos-sdk v0.47.5
 	github.com/cosmos/gogoproto v1.4.11
 	github.com/cosmos/ibc-go/v7 v7.3.1
@@ -51,7 +50,6 @@ require (
 	golang.org/x/crypto v0.15.0
 	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
 	golang.org/x/sync v0.5.0
-	google.golang.org/genproto/googleapis/api v0.0.0-20230913181813-007df8e322eb
 	google.golang.org/grpc v1.59.0
 	gopkg.in/yaml.v2 v2.4.0
 )
@@ -98,6 +96,7 @@ require (
 	github.com/containerd/cgroups v1.1.0 // indirect
 	github.com/coreos/go-systemd/v22 v22.5.0 // indirect
 	github.com/cosmos/btcutil v1.0.5 // indirect
+	github.com/cosmos/cosmos-proto v1.0.0-beta.2 // indirect
 	github.com/cosmos/go-bip39 v1.0.0 // indirect
 	github.com/cosmos/gogogateway v1.2.0 // indirect
 	github.com/cosmos/iavl v0.20.0 // indirect
@@ -294,6 +293,7 @@ require (
 	google.golang.org/api v0.143.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20230913181813-007df8e322eb // indirect
+	google.golang.org/genproto/googleapis/api v0.0.0-20230913181813-007df8e322eb // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13 // indirect
 	google.golang.org/protobuf v1.31.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect

x/session/keeper/keeper.go

@@ -60,12 +60,12 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 func (k Keeper) BeginBlocker(ctx sdk.Context) {
 	// ctx.BlockHeader().LastBlockId.Hash is the hash of the last block committed
 	hash := ctx.BlockHeader().LastBlockId.Hash
-	// ctx.BlockHeader().Height is the height of the current block being processed,
-	// decrementing it by 1 gives us the height of the last block committed.
-	height := ctx.BlockHeader().Height - 1
-	// If height is 0, ctx.BlockHeader().LastBlockId.Hash will be nil,
-	// fall back to an empty byte slice.
-	if height == 0 {
+	// ctx.BlockHeader().Height is the height of the last committed block.
+	height := ctx.BlockHeader().Height
+	// Block height 1 is the first committed block which uses `genesis.json` as its parent.
+	// See the explanation here for more details: https://github.com/pokt-network/poktroll/issues/377#issuecomment-1936607294
+	// Fallback to an empty byte slice during the genesis block.
+	if height == 1 {
 		hash = []byte{}
 	}
 

x/session/keeper/query_get_session.go

@@ -10,6 +10,8 @@ import (
 	"github.com/pokt-network/poktroll/x/session/types"
 )
 
+// GetSession should be deterministic and always return the same session for
+// the same block height.
 func (k Keeper) GetSession(goCtx context.Context, req *types.QueryGetSessionRequest) (*types.QueryGetSessionResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "invalid request")
@@ -21,16 +23,14 @@ func (k Keeper) GetSession(goCtx context.Context, req *types.QueryGetSessionRequ
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	// If block height is not specified, use the current (context's latest) block height
 	// Note that `GetSession` is called via the `Query` service rather than the `Msg` server.
 	// The former is stateful but does not lead to state transitions, while the latter one
 	// does. The request height depends on how much the node has synched and only acts as a read,
 	// while the `Msg` server handles the code flow of the validator/sequencer when a new block
 	// is being proposed.
 	blockHeight := req.BlockHeight
-	if blockHeight == 0 {
-		blockHeight = ctx.BlockHeight()
-	}
+
+	k.Logger(ctx).Info("Getting session for height: %d", blockHeight)
 
 	sessionHydrator := NewSessionHydrator(req.ApplicationAddress, req.Service.Id, blockHeight)
 	session, err := k.HydrateSession(ctx, sessionHydrator)

x/session/keeper/query_get_session_test.go

@@ -26,6 +26,8 @@ func TestSession_GetSession_Success(t *testing.T) {
 	ctx = ctx.WithBlockHeight(100) // provide a sufficiently large block height to avoid errors
 	wctx := sdk.WrapSDKContext(ctx)
 
+	// TODO_TECHDEBT(#377): These test assume that the genesis block has a height of 0,
+	// rewrite them in terms of height = 1 genesis.
 	type test struct {
 		name string
 

x/session/keeper/session_hydrator.go

@@ -16,6 +16,10 @@ import (
 	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 )
 
+// TODO_TECHDEBT(#377): The business logic in this file assume that genesis has
+// a block height of 0. Revisit it and adjust, where/if necessary, accounting for the
+// fact that it's 1.
+
 var SHA3HashLen = crypto.SHA3_256.Size()
 
 // TODO_BLOCKER(#21): Make these configurable governance param
@@ -96,7 +100,11 @@ func (k Keeper) hydrateSessionMetadata(ctx sdk.Context, sh *sessionHydrator) err
 	// TODO_TECHDEBT: Add a test if `blockHeight` is ahead of the current chain or what this node is aware of
 
 	if sh.blockHeight > ctx.BlockHeight() {
-		return sdkerrors.Wrapf(types.ErrSessionHydration, "block height %d is ahead of the current block height %d", sh.blockHeight, ctx.BlockHeight())
+		return sdkerrors.Wrapf(
+			types.ErrSessionHydration,
+			"block height %d is ahead of the last committed block height %d",
+			sh.blockHeight, ctx.BlockHeight(),
+		)
 	}
 
 	sh.session.NumBlocksPerSession = NumBlocksPerSession

x/session/keeper/session_hydrator_test.go

@@ -11,6 +11,9 @@ import (
 	"github.com/pokt-network/poktroll/x/session/types"
 )
 
+// TODO_TECHDEBT(#377): All the tests in this file assume genesis has a block
+// height of 0. Rewrite them in terms of height = 1 genesis.
+
 func TestSession_HydrateSession_Success_BaseCase(t *testing.T) {
 	sessionKeeper, ctx := keepertest.SessionKeeper(t)
 	ctx = ctx.WithBlockHeight(100) // provide a sufficiently large block height to avoid errors