From 5cbf1ed4fc12bef0e0b5b883e188d387cdd86669 Mon Sep 17 00:00:00 2001
From: Redouane Lakrache <r3d0ne@gmail.com>
Date: Wed, 7 Feb 2024 04:09:59 +0100
Subject: [PATCH 1/5] fix: session mismatch due to off-by-one block height

---
 e2e/tests/session.feature             | 2 +-
 x/session/keeper/keeper.go            | 9 ++++-----
 x/session/keeper/query_get_session.go | 6 ++----
 x/session/keeper/session_hydrator.go  | 6 +++++-
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/e2e/tests/session.feature b/e2e/tests/session.feature
index 0a1f9381c..03d475583 100644
--- a/e2e/tests/session.feature
+++ b/e2e/tests/session.feature
@@ -5,7 +5,7 @@ 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
+    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...
diff --git a/x/session/keeper/keeper.go b/x/session/keeper/keeper.go
index 3bae5c617..2673891f3 100644
--- a/x/session/keeper/keeper.go
+++ b/x/session/keeper/keeper.go
@@ -60,12 +60,11 @@ 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,
+	// ctx.BlockHeader().Height is the height of the last committed block.
+	height := ctx.BlockHeader().Height
+	// If height is 1, ctx.BlockHeader().LastBlockId.Hash will be nil,
 	// fall back to an empty byte slice.
-	if height == 0 {
+	if height == 1 {
 		hash = []byte{}
 	}
 
diff --git a/x/session/keeper/query_get_session.go b/x/session/keeper/query_get_session.go
index cce44fc2c..b35f4bcd1 100644
--- a/x/session/keeper/query_get_session.go
+++ b/x/session/keeper/query_get_session.go
@@ -21,16 +21,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
+	// GetSession should be deterministic and always return the same session for
+	// the same 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()
-	}
 
 	sessionHydrator := NewSessionHydrator(req.ApplicationAddress, req.Service.Id, blockHeight)
 	session, err := k.HydrateSession(ctx, sessionHydrator)
diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go
index 5d876a0e2..19e7c3737 100644
--- a/x/session/keeper/session_hydrator.go
+++ b/x/session/keeper/session_hydrator.go
@@ -96,7 +96,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

From 3e23df9c8869c5dda9b1519eed0e27d89840f41a Mon Sep 17 00:00:00 2001
From: Redouane Lakrache <r3d0ne@gmail.com>
Date: Wed, 7 Feb 2024 16:53:59 +0100
Subject: [PATCH 2/5] chore: Explain empty hash when block height is 1

---
 x/session/keeper/keeper.go | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/x/session/keeper/keeper.go b/x/session/keeper/keeper.go
index 2673891f3..a6976f4da 100644
--- a/x/session/keeper/keeper.go
+++ b/x/session/keeper/keeper.go
@@ -62,8 +62,9 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 	hash := ctx.BlockHeader().LastBlockId.Hash
 	// ctx.BlockHeader().Height is the height of the last committed block.
 	height := ctx.BlockHeader().Height
-	// If height is 1, ctx.BlockHeader().LastBlockId.Hash will be nil,
-	// fall back to an empty byte slice.
+	// When height is 1, ctx.BlockHeader().LastBlockId.Hash will not be populated,
+	// and leads to a panic when trying to save the hash in the store.
+	// Explicitly setting hash to an empty byte slice when height is 1 prevents the panic.
 	if height == 1 {
 		hash = []byte{}
 	}

From 61000c96c957bdc32ac6cb6ef85a81646100b2fb Mon Sep 17 00:00:00 2001
From: Redouane Lakrache <r3d0ne@gmail.com>
Date: Fri, 9 Feb 2024 16:19:00 +0100
Subject: [PATCH 3/5] chore: Add height 1 genesis comments

---
 x/session/keeper/keeper.go                 | 6 +++---
 x/session/keeper/query_get_session.go      | 6 ++++--
 x/session/keeper/query_get_session_test.go | 2 ++
 x/session/keeper/session_hydrator.go       | 4 ++++
 x/session/keeper/session_hydrator_test.go  | 4 ++++
 5 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/x/session/keeper/keeper.go b/x/session/keeper/keeper.go
index a6976f4da..1cb58d030 100644
--- a/x/session/keeper/keeper.go
+++ b/x/session/keeper/keeper.go
@@ -62,9 +62,9 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 	hash := ctx.BlockHeader().LastBlockId.Hash
 	// ctx.BlockHeader().Height is the height of the last committed block.
 	height := ctx.BlockHeader().Height
-	// When height is 1, ctx.BlockHeader().LastBlockId.Hash will not be populated,
-	// and leads to a panic when trying to save the hash in the store.
-	// Explicitly setting hash to an empty byte slice when height is 1 prevents the panic.
+	// Block height 1 is the genesis block, there is no state root since there is
+	// no block preceding it.
+	// Fallback to an empty byte slice in this case.
 	if height == 1 {
 		hash = []byte{}
 	}
diff --git a/x/session/keeper/query_get_session.go b/x/session/keeper/query_get_session.go
index b35f4bcd1..eac1345a1 100644
--- a/x/session/keeper/query_get_session.go
+++ b/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,8 +23,6 @@ func (k Keeper) GetSession(goCtx context.Context, req *types.QueryGetSessionRequ
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	// GetSession should be deterministic and always return the same session for
-	// the same 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,
@@ -30,6 +30,8 @@ func (k Keeper) GetSession(goCtx context.Context, req *types.QueryGetSessionRequ
 	// is being proposed.
 	blockHeight := req.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)
 	if err != nil {
diff --git a/x/session/keeper/query_get_session_test.go b/x/session/keeper/query_get_session_test.go
index 8ae251e45..992acdad6 100644
--- a/x/session/keeper/query_get_session_test.go
+++ b/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: 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
 
diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go
index 19e7c3737..9225fda3c 100644
--- a/x/session/keeper/session_hydrator.go
+++ b/x/session/keeper/session_hydrator.go
@@ -247,11 +247,15 @@ func sha3Hash(bz []byte) []byte {
 }
 
 // GetSessionStartBlockHeight returns the block height at which the session starts
+// TODO_TECHDEBT: This function assumes that the genesis block has a height of 0,
+// rewrite this function to handle the genesis state having a height of 1.
 func GetSessionStartBlockHeight(blockHeight int64) int64 {
 	return blockHeight - (blockHeight % NumBlocksPerSession)
 }
 
 // GetSessionEndBlockHeight returns the block height at which the session ends
+// TODO_TECHDEBT: This function assumes that the genesis block has a height of 0,
+// rewrite this function to handle the genesis state having a height of 1.
 func GetSessionEndBlockHeight(blockHeight int64) int64 {
 	return GetSessionStartBlockHeight(blockHeight) + NumBlocksPerSession - 1
 }
diff --git a/x/session/keeper/session_hydrator_test.go b/x/session/keeper/session_hydrator_test.go
index 9a6e07781..948beb874 100644
--- a/x/session/keeper/session_hydrator_test.go
+++ b/x/session/keeper/session_hydrator_test.go
@@ -61,6 +61,8 @@ func TestSession_HydrateSession_Metadata(t *testing.T) {
 
 	// TODO_TECHDEBT: Extend these tests once `NumBlocksPerSession` is configurable.
 	// Currently assumes NumBlocksPerSession=4
+	// TODO_TECHDEBT: These test assume that the genesis block has a height of 0,
+	// rewrite them in terms of height = 1 genesis.
 	tests := []test{
 		{
 			desc:        "blockHeight = 0",
@@ -153,6 +155,8 @@ func TestSession_HydrateSession_SessionId(t *testing.T) {
 
 	// TODO_TECHDEBT: Extend these tests once `NumBlocksPerSession` is configurable.
 	// Currently assumes NumBlocksPerSession=4
+	// TODO_TECHDEBT: These test assume that the genesis block has a height of 0,
+	// rewrite them in terms of height = 1 genesis.
 	tests := []test{
 		{
 			desc: "(app1, svc1): sessionId at first session block != sessionId at next session block",

From bf143dc2369850c1bad592fcd8af0dcdc83229ac Mon Sep 17 00:00:00 2001
From: Daniel Olshansky <olshansky.daniel@gmail.com>
Date: Fri, 9 Feb 2024 13:23:53 -0800
Subject: [PATCH 4/5] Minor comment improvements

---
 e2e/tests/session.feature                  | 10 ++++++----
 go.mod                                     |  4 ++--
 x/session/keeper/keeper.go                 |  6 +++---
 x/session/keeper/query_get_session_test.go |  2 +-
 x/session/keeper/session_hydrator.go       |  8 ++++----
 x/session/keeper/session_hydrator_test.go  |  7 +++----
 6 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/e2e/tests/session.feature b/e2e/tests/session.feature
index 03d475583..f0626011d 100644
--- a/e2e/tests/session.feature
+++ b/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"
+    # 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
diff --git a/go.mod b/go.mod
index 3bba35590..d8360ef37 100644
--- a/go.mod
+++ b/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
diff --git a/x/session/keeper/keeper.go b/x/session/keeper/keeper.go
index 1cb58d030..52c5e42d3 100644
--- a/x/session/keeper/keeper.go
+++ b/x/session/keeper/keeper.go
@@ -62,9 +62,9 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 	hash := ctx.BlockHeader().LastBlockId.Hash
 	// ctx.BlockHeader().Height is the height of the last committed block.
 	height := ctx.BlockHeader().Height
-	// Block height 1 is the genesis block, there is no state root since there is
-	// no block preceding it.
-	// Fallback to an empty byte slice in this case.
+	// 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{}
 	}
diff --git a/x/session/keeper/query_get_session_test.go b/x/session/keeper/query_get_session_test.go
index 992acdad6..9cd574c51 100644
--- a/x/session/keeper/query_get_session_test.go
+++ b/x/session/keeper/query_get_session_test.go
@@ -26,7 +26,7 @@ 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: These test assume that the genesis block has a height of 0,
+	// 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
diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go
index 9225fda3c..6f081fb3c 100644
--- a/x/session/keeper/session_hydrator.go
+++ b/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
@@ -247,15 +251,11 @@ func sha3Hash(bz []byte) []byte {
 }
 
 // GetSessionStartBlockHeight returns the block height at which the session starts
-// TODO_TECHDEBT: This function assumes that the genesis block has a height of 0,
-// rewrite this function to handle the genesis state having a height of 1.
 func GetSessionStartBlockHeight(blockHeight int64) int64 {
 	return blockHeight - (blockHeight % NumBlocksPerSession)
 }
 
 // GetSessionEndBlockHeight returns the block height at which the session ends
-// TODO_TECHDEBT: This function assumes that the genesis block has a height of 0,
-// rewrite this function to handle the genesis state having a height of 1.
 func GetSessionEndBlockHeight(blockHeight int64) int64 {
 	return GetSessionStartBlockHeight(blockHeight) + NumBlocksPerSession - 1
 }
diff --git a/x/session/keeper/session_hydrator_test.go b/x/session/keeper/session_hydrator_test.go
index 948beb874..e914ca0ea 100644
--- a/x/session/keeper/session_hydrator_test.go
+++ b/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
@@ -61,8 +64,6 @@ func TestSession_HydrateSession_Metadata(t *testing.T) {
 
 	// TODO_TECHDEBT: Extend these tests once `NumBlocksPerSession` is configurable.
 	// Currently assumes NumBlocksPerSession=4
-	// TODO_TECHDEBT: These test assume that the genesis block has a height of 0,
-	// rewrite them in terms of height = 1 genesis.
 	tests := []test{
 		{
 			desc:        "blockHeight = 0",
@@ -155,8 +156,6 @@ func TestSession_HydrateSession_SessionId(t *testing.T) {
 
 	// TODO_TECHDEBT: Extend these tests once `NumBlocksPerSession` is configurable.
 	// Currently assumes NumBlocksPerSession=4
-	// TODO_TECHDEBT: These test assume that the genesis block has a height of 0,
-	// rewrite them in terms of height = 1 genesis.
 	tests := []test{
 		{
 			desc: "(app1, svc1): sessionId at first session block != sessionId at next session block",

From f3f5eedfd8ff0f3ff198895b65e2b5ca4f694308 Mon Sep 17 00:00:00 2001
From: Redouane Lakrache <r3d0ne@gmail.com>
Date: Fri, 9 Feb 2024 23:26:02 +0100
Subject: [PATCH 5/5] Empty commit