chore: Address review change requests

Makefile

@@ -289,14 +289,14 @@ localnet_regenesis: check_yq acc_initialize_pubkeys_warn_message ## Regenerate t
  cp -r ${HOME}/.poktroll/keyring-test $(POKTROLLD_HOME) ;\
  cp -r ${HOME}/.poktroll/config $(POKTROLLD_HOME)/ ;\
 
-.PHONY: send_relay
-send_relay:
+.PHONY: send_relay_self_signing_app
+send_relay_self_signing_app: # Send a relay through the AppGateServer as an application that's self-signing
  curl -X POST -H "Content-Type: application/json" \
  --data '{"jsonrpc":"2.0","method":"eth_blockNumber","params":[],"id":1}' \
  http://localhost:42069/anvil
 
-.PHONY: send_relay_to_gateway
-send_relay_to_gateway:
+.PHONY: send_relay_delegating_app
+send_relay_delegating_app: # Send a relay through the gateway as an application that's delegating to this gateway
  @appAddr=$$(poktrolld keys show app1 -a) && \
  curl -X POST -H "Content-Type: application/json" \
  --data '{"jsonrpc":"2.0","method":"eth_blockNumber","params":[],"id":1}' \

pkg/crypto/rings/client.go

@@ -169,13 +169,25 @@ func (rc *ringClient) getRingPubKeysForAddress(
  return nil, err
  }
 
+ // Reconstruct the delegatee gateway addresses at the given block height and
+ // add them to the ring addresses.
+ delegateeGatewayAddresses := GetRingAddressesAtBlock(&app, blockHeight)
+
  // Create a slice of addresses for the ring.
  ringAddresses := make([]string, 0)
  ringAddresses = append(ringAddresses, appAddress) // app address is index 0
 
- // Reconstruct the delegatee gateway addresses at the given block height and
- // add them to the ring addresses.
- delegateeGatewayAddresses := GetRingAddressesAtBlock(&app, blockHeight)
+ // If there is no delegateeGatewayAddresses, add app address a second time 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.
+ if len(delegateeGatewayAddresses) == 0 {
+ delegateeGatewayAddresses = append(delegateeGatewayAddresses, app.Address)
+ }
+
  ringAddresses = append(ringAddresses, delegateeGatewayAddresses...)
 
  // Sort the ring addresses to ensure the ring is consistent between signing and
@@ -207,10 +219,12 @@ func (rc *ringClient) addressesToPubKeys(
  return pubKeys, nil
 }
 
-// GetRingAddressesAtBlock returns the active gateway delegations for the given
-// application and target block height while accounting for the pending undelegations.
-// The ring addresses slice is reconstructed by adding back the past delegated gateways
-// that have been undelegated after the target session end height.
+// GetRingAddressesAtBlock returns the active gateway addresses that need to be
+// used to construct the ring to validate the app should pay for. It takes into
+// account both active delegations and pending undelegations that should still
+// be part of the ring.
+// The ring addresses slice is reconstructed by adding back the past delegated
+// gateways that have been undelegated after the target session end height.
 func GetRingAddressesAtBlock(app *apptypes.Application, blockHeight int64) []string {
  // Get the target session end height at which we want to get the active delegations.
  targetSessionEndHeight := uint64(sessionkeeper.GetSessionEndBlockHeight(blockHeight))
@@ -243,15 +257,5 @@ func GetRingAddressesAtBlock(app *apptypes.Application, blockHeight int64) []str
 
  }
 
- // 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.
- if len(activeDelegationsAtHeight) == 0 {
- activeDelegationsAtHeight = append(activeDelegationsAtHeight, app.Address)
- }
-
  return activeDelegationsAtHeight
 }

x/application/keeper/application_test.go

@@ -22,7 +22,7 @@ func createNApplications(keeper keeper.Keeper, ctx context.Context, n int) []typ
  apps := make([]types.Application, n)
  for i := range apps {
  apps[i].Address = strconv.Itoa(i)
- // Setting pending undelegations since nullify.Fill does not seem to do it.
+ // Setting pending undelegations since nullify. Fill does not seem to do it.
  apps[i].PendingUndelegations = make(map[uint64]types.UndelegatingGatewayList)
 
  keeper.SetApplication(ctx, apps[i])

x/application/keeper/msg_server_undelegate_from_gateway.go

@@ -78,9 +78,9 @@ func (k msgServer) UndelegateFromGateway(ctx context.Context, msg *types.MsgUnde
 func (k Keeper) recordPendingUndelegation(
  app *types.Application,
  gatewayAddress string,
- currentBlock int64,
+ currentBlockHeight int64,
 ) {
- sessionEndHeight := uint64(sessionkeeper.GetSessionEndBlockHeight(currentBlock))
+ sessionEndHeight := uint64(sessionkeeper.GetSessionEndBlockHeight(currentBlockHeight))
 
  // Create the session pending undelegations list entry for the given session
  // end height if it doesn't already exist.

x/application/keeper/msg_server_undelegate_from_gateway_test.go

@@ -95,9 +95,6 @@ func TestMsgServer_UndelegateFromGateway_SuccessfullyUndelegate(t *testing.T) {
  _, err = srv.UndelegateFromGateway(ctx, undelegateMsg)
  require.NoError(t, err)
 
- sdkCtx.WithBlockHeight(4)
- k.EndBlockerPruneAppToGatewayPendingUndelegation(sdkCtx)
-
  events = sdkCtx.EventManager().Events()
  require.Equal(t, int(maxDelegatedGateways)+1, len(events))
  require.Equal(t, "poktroll.application.EventRedelegation", events[7].Type)
@@ -264,9 +261,6 @@ func TestMsgServer_UndelegateFromGateway_SuccessfullyUndelegateFromUnstakedGatew
  _, err = srv.UndelegateFromGateway(ctx, undelegateMsg)
  require.NoError(t, err)
 
- sdkCtx.WithBlockHeight(4)
- k.EndBlockerPruneAppToGatewayPendingUndelegation(sdkCtx)
-
  events = sdkCtx.EventManager().Events()
  require.Equal(t, 2, len(events))
  require.Equal(t, "poktroll.application.EventRedelegation", events[1].Type)
@@ -287,10 +281,8 @@ func TestMsgServer_UndelegateFromGateway_DelegationIsActiveUntilNextSession(t *t
  srv := keeper.NewMsgServerImpl(k)
 
  undelegationHeight := int64(1)
- sdkCtx, app, gatewayAddrs := delegateThenUndelegate(ctx, t, srv, k, undelegationHeight)
-
- gatewayAddrToUndelegate := gatewayAddrs[0]
- delegatedGatewayAddr := gatewayAddrs[1]
+ sdkCtx, app, delegatedToAddr, pendingUndelegateToAddr :=
+ createAppStakeDelegateAndUndelegate(ctx, t, srv, k, undelegationHeight)
 
  // Increment the block height without moving to the next session, then run the
  // pruning undelegations logic.
@@ -303,24 +295,24 @@ func TestMsgServer_UndelegateFromGateway_DelegationIsActiveUntilNextSession(t *t
  require.NotNil(t, app)
 
  // Verify that the gateway was removed from the application's delegatee gateway addresses.
- require.NotContains(t, app.DelegateeGatewayAddresses, gatewayAddrToUndelegate)
+ require.NotContains(t, app.DelegateeGatewayAddresses, pendingUndelegateToAddr)
 
  // Verify that the gateway is added to the pending undelegation list with the
  // right sessionEndHeight as the map key.
  sessionEndHeight := uint64(sessionkeeper.GetSessionEndBlockHeight(undelegationHeight))
  require.Contains(t,
  app.PendingUndelegations[sessionEndHeight].GatewayAddresses,
- gatewayAddrToUndelegate,
+ pendingUndelegateToAddr,
  )
 
  // Verify that the other gateways are still delegated to the application.
- require.Contains(t, app.DelegateeGatewayAddresses, delegatedGatewayAddr)
+ require.Contains(t, app.DelegateeGatewayAddresses, delegatedToAddr)
 
  // Verify that the reconstructed delegatee gateway list include the undelegated gateway.
  gatewayAddresses := rings.GetRingAddressesAtBlock(&app, sdkCtx.BlockHeight())
- require.Contains(t, gatewayAddresses, gatewayAddrToUndelegate)
+ require.Contains(t, gatewayAddresses, pendingUndelegateToAddr)
 
- // Increment the block height to the next session and  run the pruning
+ // Increment the block height to the next session and run the pruning
  // undelegations logic again.
  nextSessionStartHeight := int64(sessionEndHeight + 1)
  sdkCtx = sdkCtx.WithBlockHeight(nextSessionStartHeight)
@@ -332,24 +324,40 @@ func TestMsgServer_UndelegateFromGateway_DelegationIsActiveUntilNextSession(t *t
  require.NotNil(t, app)
 
  // Verify that when queried for the next session the reconstructed delegatee
- // gateway list does include the undelegated gateway.
+ // gateway list does not include the undelegated gateway.
  nextSessionGatewayAddresses := rings.GetRingAddressesAtBlock(&app, nextSessionStartHeight)
- require.NotContains(t, nextSessionGatewayAddresses, gatewayAddrToUndelegate)
+ require.NotContains(t, nextSessionGatewayAddresses, pendingUndelegateToAddr)
+
+ // Increment the block height past the tested session's grace period and run
+ // the pruning undelegations logic again.
+ sessionGracePeriodBlockCount := uint64(sessionkeeper.GetSessionGracePeriodBlockCount())
+ afterSessionGracePeriodHeight := int64(sessionEndHeight + sessionGracePeriodBlockCount + 1)
+ sdkCtx = sdkCtx.WithBlockHeight(afterSessionGracePeriodHeight)
+ k.EndBlockerPruneAppToGatewayPendingUndelegation(sdkCtx)
+
+ // Verify that when queried for a block height past the tested session's grace period,
+ // the reconstructed delegatee gateway list does not include the undelegated gateway.
+ pastGracePeriodGatewayAddresses := rings.GetRingAddressesAtBlock(&app, afterSessionGracePeriodHeight)
+ require.NotContains(t, pastGracePeriodGatewayAddresses, pendingUndelegateToAddr)
+
+ // Verify that when queried for a block height corresponding to the past session
+ // that has its grace period elapsed, the reconstructed delegatee gateway list
+ // does include the undelegated gateway.
+ pastGracePeriodGatewayAddresses = rings.GetRingAddressesAtBlock(&app, int64(sessionEndHeight))
+ require.Contains(t, pastGracePeriodGatewayAddresses, pendingUndelegateToAddr)
 }
 
 func TestMsgServer_UndelegateFromGateway_DelegationIsPrunedAfterRetentionPeriod(t *testing.T) {
  k, ctx := keepertest.ApplicationKeeper(t)
  srv := keeper.NewMsgServerImpl(k)
 
  undelegationHeight := int64(1)
- sdkCtx, app, gatewayAddrs := delegateThenUndelegate(ctx, t, srv, k, undelegationHeight)
-
- gatewayAddrToUndelegate := gatewayAddrs[0]
- delegatedGatewayAddr := gatewayAddrs[1]
+ sdkCtx, app, delegatedToAddr, pendingUndelegateToAddr :=
+ createAppStakeDelegateAndUndelegate(ctx, t, srv, k, undelegationHeight)
 
  // Increment the block height past the undelegation retention period then run
  // the pruning undelegations logic.
- pruningBlockHeight := getPruningBlockHeight(undelegationHeight)
+ pruningBlockHeight := getUndelegationPruningBlockHeight(undelegationHeight)
  sdkCtx = sdkCtx.WithBlockHeight(pruningBlockHeight)
  k.EndBlockerPruneAppToGatewayPendingUndelegation(sdkCtx)
 
@@ -366,18 +374,17 @@ func TestMsgServer_UndelegateFromGateway_DelegationIsPrunedAfterRetentionPeriod(
  // Verify that the reconstructed delegatee gateway list can no longer include
  // the undelegated gateway since it has been pruned.
  gatewayAddressesAfterPruning := rings.GetRingAddressesAtBlock(&app, undelegationHeight)
- require.NotContains(t, gatewayAddressesAfterPruning, gatewayAddrToUndelegate)
- require.Contains(t, gatewayAddressesAfterPruning, delegatedGatewayAddr)
+ require.NotContains(t, gatewayAddressesAfterPruning, pendingUndelegateToAddr)
+ require.Contains(t, gatewayAddressesAfterPruning, delegatedToAddr)
 }
 
 func TestMsgServer_UndelegateFromGateway_RedelegationAfterUndelegationAtTheSameSessionNumber(t *testing.T) {
  k, ctx := keepertest.ApplicationKeeper(t)
  srv := keeper.NewMsgServerImpl(k)
 
  undelegationHeight := int64(1)
- sdkCtx, app, gatewayAddrs := delegateThenUndelegate(ctx, t, srv, k, undelegationHeight)
-
- gatewayAddrToRedelegate := gatewayAddrs[0]
+ sdkCtx, app, _, gatewayAddrToRedelegate :=
+ createAppStakeDelegateAndUndelegate(ctx, t, srv, k, undelegationHeight)
 
  // Increment the block height without moving to the next session.
  sdkCtx = sdkCtx.WithBlockHeight(undelegationHeight + 1)
@@ -412,7 +419,7 @@ func TestMsgServer_UndelegateFromGateway_RedelegationAfterUndelegationAtTheSameS
 
  // Increment the block height past the undelegation retention period then run
  // the pruning undelegations logic.
- pruningBlockHeight := getPruningBlockHeight(undelegationHeight)
+ pruningBlockHeight := getUndelegationPruningBlockHeight(undelegationHeight)
  sdkCtx = sdkCtx.WithBlockHeight(pruningBlockHeight)
  k.EndBlockerPruneAppToGatewayPendingUndelegation(sdkCtx)
 
@@ -433,16 +440,24 @@ func TestMsgServer_UndelegateFromGateway_RedelegationAfterUndelegationAtTheSameS
  require.Contains(t, gatewayAddressesAfterPruning, gatewayAddrToRedelegate)
 }
 
-// delegateThenUndelegate factors delegating an application to two gateways,
-// then undelegates the application from one of the gateways to be used in
-// multiple tests.
-func delegateThenUndelegate(
+// createAppStakeDelegateAndUndelegate is a helper function that is used in the tests
+// that exercise the pruning undelegations and ring addresses reconstruction logic.
+// * It creates an account address and stakes it as an application.
+// * Creates two gateway addresses and mocks them being staked.
+// * Delegates the application to the gateways.
+// * Undelegates the application from one of the gateways.
+func createAppStakeDelegateAndUndelegate(
  ctx context.Context,
  t *testing.T,
  srv types.MsgServer,
  k keeper.Keeper,
  undelegationHeight int64,
-) (sdkCtx sdk.Context, app types.Application, gatewayAddrs []string) {
+) (
+ sdkCtx sdk.Context,
+ app types.Application,
+ delegatedToAddr,
+ pendingUndelegateToAddr string,
+) {
  // Generate an application address and stake the application.
  appAddr := sample.AccAddress()
  stakeMsg := &types.MsgStakeApplication{
@@ -459,29 +474,33 @@ func delegateThenUndelegate(
 
  // Generate gateway addresses, mock the gateways being staked then delegate the
  // application to the gateways.
- for i := 0; i < 2; i++ {
- gatewayAddr := sample.AccAddress()
- gatewayAddrs = append(gatewayAddrs, gatewayAddr)
+ delegatedToAddr = sample.AccAddress()
+ keepertest.AddGatewayToStakedGatewayMap(t, delegatedToAddr)
 
- keepertest.AddGatewayToStakedGatewayMap(t, gatewayAddr)
-
- delegateMsg := &types.MsgDelegateToGateway{
- AppAddress: appAddr,
- GatewayAddress: gatewayAddr,
- }
- _, err = srv.DelegateToGateway(ctx, delegateMsg)
- require.NoError(t, err)
+ delegateMsg := &types.MsgDelegateToGateway{
+ AppAddress: appAddr,
+ GatewayAddress: delegatedToAddr,
  }
+ _, err = srv.DelegateToGateway(ctx, delegateMsg)
+ require.NoError(t, err)
 
- gatewayAddrToUndelegate := gatewayAddrs[0]
+ pendingUndelegateToAddr = sample.AccAddress()
+ keepertest.AddGatewayToStakedGatewayMap(t, pendingUndelegateToAddr)
+
+ delegateMsg = &types.MsgDelegateToGateway{
+ AppAddress: appAddr,
+ GatewayAddress: pendingUndelegateToAddr,
+ }
+ _, err = srv.DelegateToGateway(ctx, delegateMsg)
+ require.NoError(t, err)
 
  // Create a context with a block height of 2.
  sdkCtx = sdk.UnwrapSDKContext(ctx).WithBlockHeight(undelegationHeight)
 
- // Undelegate from the first gateway
+ // Undelegate from the first gateway.
  undelegateMsg := &types.MsgUndelegateFromGateway{
  AppAddress: appAddr,
- GatewayAddress: gatewayAddrToUndelegate,
+ GatewayAddress: pendingUndelegateToAddr,
  }
  _, err = srv.UndelegateFromGateway(sdkCtx, undelegateMsg)
  require.NoError(t, err)
@@ -492,14 +511,13 @@ func delegateThenUndelegate(
  require.True(t, isAppFound)
  require.NotNil(t, foundApp)
 
- return sdkCtx, foundApp, gatewayAddrs
+ return sdkCtx, foundApp, delegatedToAddr, pendingUndelegateToAddr
 }
 
-func getPruningBlockHeight(currentHeight int64) (pruningHeihgt int64) {
- nextSessionStartHeight := int64(sessionkeeper.GetSessionEndBlockHeight(currentHeight) + 1)
-
- numBlocksUndelegationRetention := sessionkeeper.GetSessionGracePeriodBlockCount() +
- (sessionkeeper.NumBlocksPerSession * keeper.NumSessionsAppToGatewayUndelegationRetention)
+// getUndelegationPruningBlockHeight returns the block height at which undelegations
+// should be pruned for a given undlegation block height.
+func getUndelegationPruningBlockHeight(blockHeight int64) (pruningHeihgt int64) {
+ nextSessionStartHeight := sessionkeeper.GetSessionEndBlockHeight(blockHeight) + 1
 
- return nextSessionStartHeight + numBlocksUndelegationRetention
+ return nextSessionStartHeight + keeper.GetNumBlocksUndelegationRetention()
 }

x/application/keeper/prune_undelegations.go

@@ -20,13 +20,10 @@ func (k Keeper) EndBlockerPruneAppToGatewayPendingUndelegation(ctx sdk.Context)
  currentHeight := ctx.BlockHeight()
 
  // Calculate the block height at which undelegations should be pruned
- numBlocksUndelegationRetention := sessionkeeper.GetSessionGracePeriodBlockCount() +
- (sessionkeeper.NumBlocksPerSession * NumSessionsAppToGatewayUndelegationRetention)
-
+ numBlocksUndelegationRetention := GetNumBlocksUndelegationRetention()
  if currentHeight <= numBlocksUndelegationRetention {
  return nil
  }
-
  pruningBlockHeight := uint64(currentHeight - numBlocksUndelegationRetention)
 
  // Iterate over all applications and prune undelegations that are older than
@@ -44,3 +41,10 @@ func (k Keeper) EndBlockerPruneAppToGatewayPendingUndelegation(ctx sdk.Context)
 
  return nil
 }
+
+// GetNumBlocksUndelegationRetention returns the number of blocks undelegations
+// should be kept before being pruned.
+func GetNumBlocksUndelegationRetention() int64 {
+ return sessionkeeper.GetSessionGracePeriodBlockCount() +
+ (sessionkeeper.NumBlocksPerSession * NumSessionsAppToGatewayUndelegationRetention)
+}