Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TODO] refactor: query expiring claims w/ index #671

Merged
merged 11 commits into from
Jul 15, 2024
1 change: 1 addition & 0 deletions testutil/integration/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ func NewCompleteIntegrationApp(t *testing.T) *App {
applicationKeeper,
proofKeeper,
sharedKeeper,
sessionKeeper,
)
tokenomicsModule := tokenomics.NewAppModule(
cdc,
Expand Down
5 changes: 5 additions & 0 deletions testutil/keeper/tokenomics.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ func TokenomicsKeeperWithActorAddrs(t testing.TB) (
mockSharedKeeper := mocks.NewMockSharedKeeper(ctrl)
mockSharedKeeper.EXPECT().GetProofWindowCloseHeight(gomock.Any(), gomock.Any()).AnyTimes()

// Mock the session keeper
mockSessionKeeper := mocks.NewMockSessionKeeper(ctrl)

k := tokenomicskeeper.NewKeeper(
cdc,
runtime.NewKVStoreService(storeKey),
Expand All @@ -165,6 +168,7 @@ func TokenomicsKeeperWithActorAddrs(t testing.TB) (
mockApplicationKeeper,
mockProofKeeper,
mockSharedKeeper,
mockSessionKeeper,
)

sdkCtx := sdk.NewContext(stateStore, cmtproto.Header{}, false, log.NewNopLogger())
Expand Down Expand Up @@ -331,6 +335,7 @@ func NewTokenomicsModuleKeepers(
appKeeper,
proofKeeper,
sharedKeeper,
sessionKeeper,
)

require.NoError(t, tokenomicsKeeper.SetParams(ctx, tokenomicstypes.DefaultParams()))
Expand Down
37 changes: 2 additions & 35 deletions x/shared/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@ import (
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
)

const (
minimumClaimWindowSizeBlocks = 1
minimumProofWindowSizeBlocks = 1
)

// TODO_DOCUMENT(@bryanchriswhite): Move this into the documentation: https://github.com/pokt-network/poktroll/pull/571#discussion_r1630923625

// GetSessionStartHeight returns the block height at which the session containing
Expand Down Expand Up @@ -123,26 +118,12 @@ func GetEarliestSupplierClaimCommitHeight(
// window open block hash and the supplier address.
randomNumber := poktrand.SeededInt63(claimWindowOpenBlockHash, []byte(supplierAddr))

distributionWindowSizeBlocks := GetClaimWindowSizeBlocks(sharedParams)
distributionWindowSizeBlocks := sharedParams.GetClaimWindowCloseOffsetBlocks()
randCreateClaimHeightOffset := randomNumber % int64(distributionWindowSizeBlocks)

return claimWindowOpenHeight + randCreateClaimHeightOffset
}

// GetClaimWindowSizeBlocks returns the number of blocks between the opening and closing
// of the claim window, given the passed sharedParams.
func GetClaimWindowSizeBlocks(sharedParams *sharedtypes.Params) uint64 {
windowSizeBlocks := sharedParams.ClaimWindowCloseOffsetBlocks -
sharedParams.ClaimWindowOpenOffsetBlocks -
minimumClaimWindowSizeBlocks

if windowSizeBlocks < 1 {
return 1
}

return windowSizeBlocks
}

// GetEarliestSupplierProofCommitHeight returns the earliest block height at which a proof
// for the session that includes queryHeight can be committed for a given supplier
// and the passed sharedParams.
Expand All @@ -158,22 +139,8 @@ func GetEarliestSupplierProofCommitHeight(
// window open block hash and the supplier address.
randomNumber := poktrand.SeededInt63(proofWindowOpenBlockHash, []byte(supplierAddr))

distributionWindowSizeBlocks := GetProofWindowSizeBlocks(sharedParams)
distributionWindowSizeBlocks := sharedParams.GetProofWindowCloseOffsetBlocks()
randCreateProofHeightOffset := randomNumber % int64(distributionWindowSizeBlocks)

return proofWindowOpenHeight + randCreateProofHeightOffset
}

// GetProofWindowSizeBlocks returns the number of blocks between the opening and closing
// of the proof window, given the passed sharedParams.
func GetProofWindowSizeBlocks(sharedParams *sharedtypes.Params) uint64 {
windowSizeBlocks := sharedParams.ProofWindowCloseOffsetBlocks -
sharedParams.ProofWindowOpenOffsetBlocks -
minimumProofWindowSizeBlocks

if windowSizeBlocks < 1 {
return 1
}

return windowSizeBlocks
}
4 changes: 2 additions & 2 deletions x/shared/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ func TestClaimProofWindows(t *testing.T) {
require.GreaterOrEqual(t, earliestProofCommitHeight, claimWindowCloseHeight)
require.Greater(t, proofWindowCloseHeight, earliestProofCommitHeight)

claimWindowSizeBlocks := GetClaimWindowSizeBlocks(&test.sharedParams)
claimWindowSizeBlocks := test.sharedParams.GetClaimWindowCloseOffsetBlocks()
require.Greater(t, claimWindowSizeBlocks, uint64(0))

proofWindowSizeBlocks := GetProofWindowSizeBlocks(&test.sharedParams)
proofWindowSizeBlocks := test.sharedParams.GetProofWindowCloseOffsetBlocks()
require.Greater(t, proofWindowSizeBlocks, uint64(0))

wg.Done()
Expand Down
11 changes: 11 additions & 0 deletions x/tokenomics/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/pokt-network/poktroll/pkg/client"
prooftypes "github.com/pokt-network/poktroll/x/proof/types"
"github.com/pokt-network/poktroll/x/tokenomics/types"
)

Expand All @@ -26,6 +28,9 @@ type Keeper struct {
applicationKeeper types.ApplicationKeeper
proofKeeper types.ProofKeeper
sharedKeeper types.SharedKeeper
sessionKeeper types.SessionKeeper

sharedQuerier client.SharedQueryClient
}

func NewKeeper(
Expand All @@ -39,11 +44,14 @@ func NewKeeper(
applicationKeeper types.ApplicationKeeper,
proofKeeper types.ProofKeeper,
sharedKeeper types.SharedKeeper,
sessionKeeper types.SessionKeeper,
) Keeper {
if _, err := sdk.AccAddressFromBech32(authority); err != nil {
panic(fmt.Sprintf("invalid authority address: %s", authority))
}

sharedQuerier := prooftypes.NewSharedKeeperQueryClient(sharedKeeper, sessionKeeper)

return Keeper{
cdc: cdc,
storeService: storeService,
Expand All @@ -55,6 +63,9 @@ func NewKeeper(
applicationKeeper: applicationKeeper,
proofKeeper: proofKeeper,
sharedKeeper: sharedKeeper,
sessionKeeper: sessionKeeper,

sharedQuerier: sharedQuerier,
}
}

Expand Down
55 changes: 40 additions & 15 deletions x/tokenomics/keeper/settle_pending_claims.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"

poktrand "github.com/pokt-network/poktroll/pkg/crypto/rand"
"github.com/pokt-network/poktroll/telemetry"
Expand All @@ -27,10 +28,10 @@ func (k Keeper) SettlePendingClaims(ctx sdk.Context) (
) {
logger := k.Logger().With("method", "SettlePendingClaims")

// TODO_BLOCKER(@Olshansk): Optimize this by indexing expiringClaims appropriately
// and only retrieving the expiringClaims that need to be settled rather than all
// of them and iterating through them one by one.
expiringClaims := k.getExpiringClaims(ctx)
expiringClaims, err := k.getExpiringClaims(ctx)
if err != nil {
return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure relaysPerServiceMap and computeUnitsPerServiceMap are initialized before use.

The variables relaysPerServiceMap and computeUnitsPerServiceMap should be initialized before being used in the return statement to avoid potential nil map issues.

	expiringClaims, err := k.getExpiringClaims(ctx)
	if err != nil {
+		relaysPerServiceMap = make(map[string]uint64)
+		computeUnitsPerServiceMap = make(map[string]uint64)
		return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err
	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expiringClaims, err := k.getExpiringClaims(ctx)
if err != nil {
return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err
}
expiringClaims, err := k.getExpiringClaims(ctx)
if err != nil {
relaysPerServiceMap = make(map[string]uint64)
computeUnitsPerServiceMap = make(map[string]uint64)
return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err
}


blockHeight := ctx.BlockHeight()

Expand Down Expand Up @@ -160,25 +161,49 @@ func (k Keeper) SettlePendingClaims(ctx sdk.Context) (
// This is the height at which the proof window closes.
// If the proof window closes and a proof IS NOT required -> settle the claim.
// If the proof window closes and a proof IS required -> only settle it if a proof is available.
func (k Keeper) getExpiringClaims(ctx sdk.Context) (expiringClaims []prooftypes.Claim) {
func (k Keeper) getExpiringClaims(ctx sdk.Context) (expiringClaims []prooftypes.Claim, err error) {
blockHeight := ctx.BlockHeight()

// TODO_TECHDEBT: Optimize this by indexing claims appropriately
// and only retrieving the claims that need to be settled rather than all
// of them and iterating through them one by one.
claims := k.proofKeeper.GetAllClaims(ctx)
// NB: This error can be safely ignored as on-chain SharedQueryClient implementation cannot return an error.
sharedParams, _ := k.sharedQuerier.GetParams(ctx)
claimWindowSizeBlocks := sharedParams.GetClaimWindowCloseOffsetBlocks()
proofWindowSizeBlocks := sharedParams.GetProofWindowCloseOffsetBlocks()

previousSessionEndHeight := blockHeight -
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
int64(sharedParams.GetGracePeriodEndOffsetBlocks()+
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
claimWindowSizeBlocks+
proofWindowSizeBlocks+1)

allClaims := k.proofKeeper.GetAllClaims(ctx)
_ = allClaims

for {
claimsRes, err := k.proofKeeper.AllClaims(ctx, &prooftypes.QueryAllClaimsRequest{
Pagination: &query.PageRequest{
Offset: uint64(len(expiringClaims)),
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
},
Filter: &prooftypes.QueryAllClaimsRequest_SessionEndHeight{
SessionEndHeight: uint64(previousSessionEndHeight),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to just use SessionNumber for filtering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but at the moment we don't have any such index; we have:

 oneof filter {
    string supplier_address= 2;
    string session_id = 3;
    uint64 session_end_height = 4;
  }

Even if we had a session_number index, we would still have to do something more sophistocated than simply currentHeight / num_blocks_per_session. Do you think it's worth adding a TODO_CONSIDERATION or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe.

My concern is about the potential case where the session we want to settle is n-2 or older (relative to the current session).

If claim and proof window offsets do not allow that, then we should be fine, Otherwise we should consider other means to retrieve the session we want to settle.

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method will be called for each block, progressively "draining" all expiring claims as the current height climbs. Since this is part of consensus, all sessions considered here MUST be from the same, most recently expiring session number.

},
})
if err != nil {
return nil, err
}

// Loop over all claims we need to check for expiration
for _, claim := range claims {
claimSessionStartHeight := claim.GetSessionHeader().GetSessionStartBlockHeight()
expirationHeight := k.sharedKeeper.GetProofWindowCloseHeight(ctx, claimSessionStartHeight)
if blockHeight >= expirationHeight {
for _, claim := range claimsRes.GetClaims() {
expiringClaims = append(expiringClaims, claim)
}

// Continue if there are more claims to fetch.
if claimsRes.Pagination.GetNextKey() != nil {
continue
}

break
}

// Return the actually expiring claims
return expiringClaims
return expiringClaims, nil
}

// proofRequirementForClaim checks if a proof is required for a claim.
Expand Down
2 changes: 2 additions & 0 deletions x/tokenomics/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ type ModuleInputs struct {
ApplicationKeeper types.ApplicationKeeper
ProofKeeper types.ProofKeeper
SharedKeeper types.SharedKeeper
SessionKeeper types.SessionKeeper
}

type ModuleOutputs struct {
Expand All @@ -207,6 +208,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
in.ApplicationKeeper,
in.ProofKeeper,
in.SharedKeeper,
in.SessionKeeper,
)
m := NewAppModule(
in.Cdc,
Expand Down
11 changes: 10 additions & 1 deletion x/tokenomics/types/expected_keepers.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:generate mockgen -destination ../../../testutil/tokenomics/mocks/expected_keepers_mock.go -package mocks . AccountKeeper,BankKeeper,ApplicationKeeper,ProofKeeper,SharedKeeper
//go:generate mockgen -destination ../../../testutil/tokenomics/mocks/expected_keepers_mock.go -package mocks . AccountKeeper,BankKeeper,ApplicationKeeper,ProofKeeper,SharedKeeper,SessionKeeper

package types

Expand All @@ -9,6 +9,7 @@ import (

apptypes "github.com/pokt-network/poktroll/x/application/types"
prooftypes "github.com/pokt-network/poktroll/x/proof/types"
sessiontypes "github.com/pokt-network/poktroll/x/session/types"
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
)

Expand Down Expand Up @@ -39,6 +40,8 @@ type ProofKeeper interface {
GetProof(ctx context.Context, sessionId, supplierAddr string) (proof prooftypes.Proof, isProofFound bool)
RemoveProof(ctx context.Context, sessionId, supplierAddr string)

AllClaims(ctx context.Context, req *prooftypes.QueryAllClaimsRequest) (*prooftypes.QueryAllClaimsResponse, error)

// Only used for testing & simulation
UpsertClaim(ctx context.Context, claim prooftypes.Claim)
UpsertProof(ctx context.Context, claim prooftypes.Proof)
Expand All @@ -53,3 +56,9 @@ type SharedKeeper interface {

GetProofWindowCloseHeight(ctx context.Context, queryHeight int64) int64
}

type SessionKeeper interface {
GetSession(context.Context, *sessiontypes.QueryGetSessionRequest) (*sessiontypes.QueryGetSessionResponse, error)
GetBlockHash(ctx context.Context, height int64) []byte
StoreBlockHash(ctx context.Context)
}
Loading