Ensure listening coefficients never drops to 0 by fernandofcampos · Pull Request #800 · allora-network/allora-chain

fernandofcampos · 2025-04-11T17:26:17Z

Purpose of Changes and their Description

Link(s) to Ticket(s) or Issue(s) resolved by this PR

Are these changes tested and documented?

If tested, please describe how. If not, why tests are not needed.
If documented, please describe where. If not, describe why docs are not needed.
Added to Unreleased section of CHANGELOG.md?

Still Left Todo

Fill this out if this is a Draft PR so others can help.

xmariachi

Some comments (a full review on the most complicated parts is pending)

x/emissions/types/errors.go

math/dec.go

amimart

Thanks, I go some first remarks here, didn't check the whole PR yet

x/emissions/module/rewards/rewards_internal.go

x/emissions/keeper/actor_utils/losses.go

guilherme-brandao · 2025-05-06T14:56:08Z

x/emissions/module/rewards/rewards_internal.go

+const epsilon = "1e-18"
+
+// EpsilonDec represents a small positive value (1e-18), used to prevent division by zero or issues with zero values.
+var EpsilonDec = alloraMath.MustNewDecFromString(epsilon)


We already have this value in emissions params, it's called EpsilonSafeDiv, please reuse it.

The problem with using that is that it implies reading from the keeper when we need it. I will see if I can optimize it.

guilherme-brandao · 2025-05-06T15:11:36Z

x/emissions/module/rewards/rewards_test.go

+	topicId1 := s.setUpTopic(blockHeight2, workerIndexes, reputerIndexes, stake, alphaRegret)
+
+	// Add additional stake to each reputer for second topic
+	for _, index := range reputerIndexes {
+		s.MintTokensToAddress(s.addrs[index], stake)
+		_, err := s.msgServer.AddStake(s.ctx, &types.AddStakeRequest{
+			Sender:  s.addrsStr[index],
+			Amount:  stake,
+			TopicId: topicId1,
+		})
+		require.NoError(err)
+	}
+
+	// Fund the second topic with the same significant initial stake
+	s.MintTokensToAddress(s.addrs[reputerIndexes[0]], initialStake)
+	fundTopicMessage = types.FundTopicRequest{
+		Sender:  s.addrsStr[reputerIndexes[0]],
+		TopicId: topicId1,
+		Amount:  initialStake,
+	}
+	_, err = s.msgServer.FundTopic(s.ctx, &fundTopicMessage)
+	require.NoError(err)


It seems like you're staking and topic funding both inside and outside the setUpTopic function. Is that intended? If not, please review other cases

I didn't realize setUpTopic was already doing that. I cleaned up the tests.

guilherme-brandao · 2025-05-06T15:13:17Z

x/emissions/module/rewards/scores_test.go

+		// Use a more lenient epsilon value (1% instead of 0.001%)
+		testutil.InEpsilon2(s.T(), reputerScore.Score, expectedScores[i].String())


Are these changes compromising reputer scoring accuracy? If yes, has it already been double-checked with the research team to see if it's ok?

Yes, because of the normalization (check at x/emissions/module/rewards/scores.go). I will double check with research.

fstecker

As we discussed today, it would be great if you could wait with this PR until we have some potential problems in the design figured out.

fstecker · 2025-05-07T18:24:59Z

x/emissions/module/rewards/rewards_internal.go

+	}
+
+	// If all stakes are zero, use epsilon values instead to avoid division by zero
+	if allZeroOrEpsilon {


Isn't the whole allZeroOrEpsilon thing redundant? If the only thing that happens in there is also inside the if allZero?

fstecker · 2025-05-07T18:41:14Z

x/emissions/module/rewards/rewards_internal.go

 			max, err := alloraMath.Max(
 				coefficientsPlusLearningRateTimesGradient,
-				alloraMath.ZeroDec(),
+				EpsilonDec,


This wouldn't allow any of the listening coefficients to go below epsilon? I think that should still be allowed, we only need to check that they don't all become zero (or equivalently, their sum isn't zero). I think we already do this elsewhere, so this shouldn't be necessary?

fstecker · 2025-05-07T18:44:54Z

x/emissions/module/rewards/scores.go

+
+		// Only normalize coefficients when there are multiple reputers
+		// When there's only one reputer, normalization is unnecessary and would force the value to 1.0
+		if len(reputerListeningCoefficients) > 1 {


I don't think that's necessary. If there's only one reputer, its listening coefficient doesn't matter anyway (the losses will be whatever this reputer says, what else should it be).

fstecker · 2025-05-07T18:55:52Z

x/emissions/module/rewards/rewards_internal.go

+				return nil, nil, errors.Wrapf(err, "GetAllReputersOutput, error normalizing coefficient %d", j)
+			}
+		}
+


I'm having second thoughts about the normalization, and now think it might be better to normalize so that the max listening coefficient is 1, instead of normalizing so the sum is 1. That would make the listenedStakeFraction as high as possible and the if listenedStakeFraction.Lt(params.MinStakeFraction) branch less likely to be taken. Which might improve convergence. Maybe. I'll think about it some more.

fernandofcampos force-pushed the ENGN-3543 branch 2 times, most recently from 447c72b to eaf0d4d Compare April 16, 2025 14:23

fernandofcampos marked this pull request as ready for review April 16, 2025 14:24

fernandofcampos force-pushed the ENGN-3543 branch 3 times, most recently from 5591d37 to 80384a6 Compare April 16, 2025 14:40

xmariachi reviewed Apr 24, 2025

View reviewed changes

x/emissions/types/errors.go Outdated Show resolved Hide resolved

math/dec.go Outdated Show resolved Hide resolved

xmariachi requested review from amimart, fstecker and guilherme-brandao April 24, 2025 15:16

amimart reviewed Apr 25, 2025

View reviewed changes

fernandofcampos force-pushed the ENGN-3543 branch 3 times, most recently from f50ce77 to 47e408e Compare May 5, 2025 14:13

xmariachi reviewed May 5, 2025

View reviewed changes

x/emissions/keeper/actor_utils/losses.go Outdated Show resolved Hide resolved

guilherme-brandao suggested changes May 6, 2025

View reviewed changes

Ensure listening coefficients never drops to 0

54b5d8c

fernandofcampos force-pushed the ENGN-3543 branch 3 times, most recently from fe49dfc to 6e2c27d Compare May 7, 2025 14:28

fstecker requested changes May 7, 2025

View reviewed changes

fernandofcampos force-pushed the ENGN-3543 branch from 6e2c27d to 3586668 Compare May 9, 2025 14:06

Code review findings

bc093dd

fernandofcampos force-pushed the ENGN-3543 branch from 3586668 to bc093dd Compare May 9, 2025 14:07

		// Use a more lenient epsilon value (1% instead of 0.001%)
		testutil.InEpsilon2(s.T(), reputerScore.Score, expectedScores[i].String())

Conversation

fernandofcampos commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of Changes and their Description

Link(s) to Ticket(s) or Issue(s) resolved by this PR

Are these changes tested and documented?

Still Left Todo

Uh oh!

xmariachi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amimart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fstecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fernandofcampos commented Apr 11, 2025 •

edited

Loading