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

x/slashing/keeper: SlashRedelegation should hoist non-changing ValidatorAddress and DelegatorAddress parsing out of hot loop #18032

Closed
1 task done
odeke-em opened this issue Oct 10, 2023 · 0 comments · Fixed by #18035
Labels
S:orijtech Squad: OrijTech T: Performance Performance improvements Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@odeke-em
Copy link
Collaborator

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

I was auditing and testing out code in x/slashing/keeper over the weekend and I noticed this code in a loop

continue
}
valDstAddr, err := k.validatorAddressCodec.StringToBytes(redelegation.ValidatorDstAddress)
if err != nil {
panic(err)
}
delegatorAddress, err := k.authKeeper.AddressCodec().StringToBytes(redelegation.DelegatorAddress)
if err != nil {
panic(err)
}

and it doesn't change much so no need to keep it in that loop while redelegating

This fixes it by hoisting that code out

diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go
index 5a0886141f..e61cbb5181 100644
--- a/x/staking/keeper/slash.go
+++ b/x/staking/keeper/slash.go
math.LegacyNewDecFromInt(tokensToBurn).QuoRoundUp(math.LegacyNewDecFromInt(validator.Tokens))
@@ -287,6 +299,16 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida
 	totalSlashAmount = math.ZeroInt()
 	bondedBurnedAmount, notBondedBurnedAmount := math.ZeroInt(), math.ZeroInt()
 
+	valDstAddr, err := k.validatorAddressCodec.StringToBytes(redelegation.ValidatorDstAddress)
+	if err != nil {
+		return math.ZeroInt(), fmt.Errorf("SlashRedelegation: could not parse validator destination address: %w", err)
+	}
+
+	delegatorAddress, err := k.authKeeper.AddressCodec().StringToBytes(redelegation.DelegatorAddress)
+	if err != nil {
+		return math.ZeroInt(), fmt.Errorf("SlashRedelegation: could not parse delegator address: %w", err)
+	}
+
 	// perform slashing on all entries within the redelegation
 	for _, entry := range redelegation.Entries {
 		// If redelegation started before this height, stake didn't contribute to infraction
@@ -310,16 +332,7 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida
 			continue
 		}
 
-		valDstAddr, err := k.validatorAddressCodec.StringToBytes(redelegation.ValidatorDstAddress)
-		if err != nil {
-			panic(err)
-		}
-
-		delegatorAddress, err := k.authKeeper.AddressCodec().StringToBytes(redelegation.DelegatorAddress)
-		if err != nil {
-			panic(err)
-		}
-
+		// Delegations can be dynamic hence need to be looked up on every redelegation entry loop.
 		delegation, err := k.Delegations.Get(ctx, collections.Join(sdk.AccAddress(delegatorAddress), sdk.ValAddress(valDstAddr)))
 		if err != nil {
 			// If deleted, delegation has zero shares, and we can't unbond any more

/cc @elias-orijtech

Cosmos SDK Version

main

How to reproduce?

Auditing the code

@odeke-em odeke-em added T: Performance Performance improvements Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. S:orijtech Squad: OrijTech labels Oct 10, 2023
odeke-em added a commit that referenced this issue Oct 10, 2023
…edelegation loop

This change hoists out, the parsing of non-changing validator address and
delegator address which before were being re-parsed in every loop of
the redelegation entries, parsing them once and reusing the already
parsed addresses.

Fixes #18032
odeke-em added a commit that referenced this issue Oct 10, 2023
…edelegation loop

This change hoists out, the parsing of non-changing validator address and
delegator address which before were being re-parsed in every loop of
the redelegation entries, parsing them once and reusing the already
parsed addresses.

Fixes #18032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:orijtech Squad: OrijTech T: Performance Performance improvements Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
1 participant