@TxCorpi0x resolved 6 discussions.
Reviewable status: 2 of 18 files reviewed, 13 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and ysv).

@ysv partially reviewed 3 files and made 8 comments.
Reviewable status: 3 of 18 files reviewed, 19 unresolved discussions (waiting on masihyeganeh, metalarm10, and miladz68).

x/pse/keeper/delegation.go line 80 at r2 (raw file):

errors.Is(err, collections.ErrNotFound) could be nested into if err != nil but I think this doesn't influence logic.

Don't have strong preference

x/pse/keeper/distribute.go line 71 at r2 (raw file):

yes, in current implementation tend to agree with Mehdi.
on batch initialization step we can check for excluded addresses.

However AFAIK we will also process excluded addresses similar way in the next change. So as you prefer @metalarm10

Also I was thinking about reading all excluded addresses once and just checking presence in map.
TODO could be added as improvement. For current PR no caching of excluded is needed

a discussion (no related file):
Partially reviewed.
Posting batch 1 comments

x/pse/keeper/delegation.go line 97 at r9 (raw file):

		return err
	}
	return k.TotalScore.Set(ctx, distributionID, currentTotal.Add(score))

I can see that we call addToScore in loop as result we read TotalScore and Set it in keeper each time.

Does it make sense to store it as variable inside this structure and set only once.
I see benefits in both approaches. Just not sure how big is the overhead.

Lets discuss

x/pse/keeper/delegation.go line 97 at r9 (raw file):

		return err
	}
	return k.TotalScore.Set(ctx, distributionID, currentTotal.Add(score))

As side effect of this we set TotalScore for current distribution from hooks. Not sure if this is correct.

lets also discuss

x/pse/keeper/distribute.go line 30 at r9 (raw file):

//
// Returns true when all ongoingID entries have been processed.
func (k Keeper) ConsumeOngoingDelegationTimeEntry(

nit: should it be plural ?
ConsumeOngoingDelegationTimeEntries

x/pse/keeper/distribute.go line 208 at r9 (raw file):

	// Distribute rewards to each delegator in the batch proportional to their score.
	for _, item := range batch {
		userAmount := totalPSEAmount.Mul(item.score).Quo(totalScore)

could you please double check all places where we have division and make sure that we have no division by 0. Like we face on testnet

Here it seems to be handled but lets check other places

x/pse/keeper/distribute.go line 16 at r5 (raw file):

// defaultBatchSize is the number of entries processed per EndBlock during multi-block distribution.
const defaultBatchSize = 100 // TODO: make configurable

shall we move such small leftovers to a separate task ?

@miladz68 reviewed 16 files and all commit messages, made 6 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on masihyeganeh, metalarm10, and ysv).

x/pse/keeper/distribute.go line 156 at r9 (raw file):

	// No amount to distribute: clean up.
	if !totalPSEAmount.IsPositive() {

this check seems redundanat due to the check in previous line.

x/pse/keeper/genesis.go line 65 at r9 (raw file):

		}
	}
	for distID, total := range totalScores {

this is non-deterministic behaviour. Just export the total score as part of the genesis.

x/pse/keeper/hooks.go line 44 at r9 (raw file):

// TODO: handle empty distribution schedule — currently returns 0 when no schedule exists.
func (k Keeper) getNextDistributionID(ctx context.Context) (uint64, error) {
	ongoing, found, err := k.getOngoingDistribution(ctx)

I am not 100% sure about this, so please double check, but I guess only calling PeekNextAllocationSchedule should return the correct ID.

x/pse/migrations/v2/migrate.go line 18 at r9 (raw file):

// - DelegationTimeEntries key: Pair[AccAddress, ValAddress] -> Triple[uint64, AccAddress, ValAddress].
// - AccountScoreSnapshot key: AccAddress -> Pair[uint64, AccAddress].
func MigrateStore(

do you have tests for this migration ?

x/pse/keeper/distribution_test.go line 104 at r6 (raw file):

	ctx = ctx.WithBlockTime(time.Unix(int64(time1)+10, 0))
	ctx = ctx.WithBlockHeight(100)
	for range 10 {

why are we calling it 10 times ?

x/pse/keeper/distribute_test.go line 36 at r5 (raw file):

				func(r *runEnv) {
					assertDistributionAction(r, map[*sdk.AccAddress]sdkmath.Int{
						&r.delegators[0]: sdkmath.NewInt(1_100_366), // + 1000 * 1.1 / 2

we are dividing it by 3 since there 1 million delegation exsisting from empty simapp. I think you should update other places as well.

@miladz68 resolved 4 discussions.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on masihyeganeh, metalarm10, and ysv).

@ysv reviewed 15 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and TxCorpi0x).

x/pse/keeper/delegation.go line 97 at r9 (raw file):

Also hook might interfer with distribution logic

x/pse/keeper/migrations.go line 23 at r2 (raw file):

you mean this code should live in app/upgrade, right ?

Tend to agree

x/pse/migrations/v2/migrate.go line 18 at r9 (raw file):

I think we definitely need this because it is very sensitive part.
Also would be nice to have upgrade integration tests for this.

I propose we do this in a separate PR. This one is already very big.

a discussion (no related file):
I have a feeling that with this more complex distribution flow we should be have additional protection agains overspend.
Maybe on distribution start we should move portion allocated for specific ID to a designated account pse_community -> pse_community_{distribution_id}.

So in case of overspend we fail and know about it.

Out of scope for this PR but something to consider.

x/pse/keeper/distribute.go line 219 at r9 (raw file):

		if err := sdkCtx.EventManager().EmitTypedEvent(&types.EventCommunityDistributed{
			DelegatorAddress: item.delAddr.String(),

should we also add DistributionID to EventCommunityDistributed ?
Or this will be broken change ?

x/pse/keeper/distribute.go line 283 at r9 (raw file):

// addToDistributedAmount atomically adds to the cumulative distributed amount.
func (k Keeper) addToDistributedAmount(ctx context.Context, distributionID uint64, amount sdkmath.Int) error {

lets discuss overhead this causes

but implementation out of scope for this PR

@metalarm10 made 15 comments and resolved 1 discussion.
Reviewable status: 7 of 24 files reviewed, 16 unresolved discussions (waiting on masihyeganeh, miladz68, TxCorpi0x, and ysv).

a discussion (no related file):

Done.

a discussion (no related file):

Done.

x/pse/keeper/delegation.go line 97 at r9 (raw file):

1. About the overhead: these multiple KV writes are batched in-memory and only the final state is written to disk in block commit phase. Since operation executed in EndBlocker, the gas isn't charged to users, it is just computational overhead.
2. TotalScore sync: addToScore function used both in hooks and multi-block processing logic. But they never overlap because hooks triggered in DeliverTx, batch processing is triggered in EndBlock.

A local variable for score accumulation makes sense but I believe it splits current addToScore function logic into two code paths, because ongoing pse loop would use local variable, but hooks still need direct KV writes since they run in separate execution context.

So it will likely add more complexity but happy to refactor if you prefer.

x/pse/keeper/delegation.go line 97 at r9 (raw file):

Done (Answered below)

x/pse/keeper/distribute.go line 71 at r2 (raw file):

KV read optimization for excluded addresses fixed in: 6d3b8f6 .
Like you mentioned @ysv , let's still process them inside the batches since follow-up PR will need this logic for internal score accounting for excluded addresses.

x/pse/keeper/distribute.go line 16 at r5 (raw file):

Yes, I agree. I think let's collect all TODO's after merging all of the main PRs, and then create a separate task for fixing all of the leftovers as the final step.

x/pse/keeper/distribute.go line 156 at r9 (raw file):

Done.

x/pse/keeper/distribute.go line 208 at r9 (raw file):

Sure, I have double checked all current divisions in PSE:

1. distribution.go:178- Amount.Quo(numRecipients) : SAFE (guarded by a numRecipients.IsZero() check above).
2. distribute.go:210- totalPSEAmount.Mul(score).Quo(totalScore): SAFE (the invariant check errors out if totalScore is non-positive).
3. distribute.go:338: SAFE (we already hotfixed this)

I think all divisions are properly handled at the moment.

x/pse/keeper/distribute.go line 219 at r9 (raw file):

I double checked and it should be backwards compatible. Feature added: "7aef3a2"

x/pse/keeper/distribute.go line 283 at r9 (raw file):

Done.

x/pse/keeper/distribute_test.go line 36 at r5 (raw file):

Done.

x/pse/keeper/distribution_test.go line 104 at r6 (raw file):

It was the maximum allowed endblocker calls to trigger first pse multi-block distribution.
I've added explict comments and a more meaningful assertion for this part in this commit: 46ae112

x/pse/keeper/genesis.go line 65 at r9 (raw file):

Done.

x/pse/keeper/hooks.go line 44 at r9 (raw file):

I believe PeekNextAllocationSchedule isn't enough on its own: during an ongoing distribution (ID=N), its schedule entry still exists until cleanup (which happens at the end of pse distribution), so it would return id=N when queried, but hooks must write to N+1 (next ID).
The if found check handles this with ongoing.ID + 1. When idle, PeekNextAllocationSchedule returns the next target ID.
I think let's keep this.

x/pse/migrations/v2/migrate.go line 18 at r9 (raw file):

Yes, a dedicated PR for finalized migration logic (with tests) will be sent after this core part is merged.
I will add dedicated test into upgrade integration tests as well.

@metalarm10 made 1 comment.
Reviewable status: 7 of 24 files reviewed, 16 unresolved discussions (waiting on masihyeganeh, miladz68, TxCorpi0x, and ysv).

x/pse/keeper/migrations.go line 23 at r2 (raw file):

Done.

@miladz68 partially reviewed 17 files and all commit messages, made 1 comment, and resolved 6 discussions.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on masihyeganeh, metalarm10, TxCorpi0x, and ysv).

x/pse/keeper/params.go line 197 at r11 (raw file):

// LoadExcludedAddressMap loads all excluded addresses into an in-memory map for efficient lookups.
// Returns false if params are not initialized.
func (k Keeper) LoadExcludedAddressMap(ctx context.Context) (map[string]bool, error) {

this does not look like an optimization to be honest, seems like this will add overhead. and I also think you misunderstood the original comment from Mehdi.
NOTE: this is a non-blocking comment, feel free to resolve if you guys agree to keep this.

@ysv partially reviewed 7 files, made 1 comment, and resolved 7 discussions.
Reviewable status: 23 of 24 files reviewed, 4 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and TxCorpi0x).

x/pse/keeper/distribute.go line 283 at r9 (raw file):

let use local variable for this one

@ysv reviewed 10 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and TxCorpi0x).

x/pse/keeper/params.go line 197 at r11 (raw file):

why won't this be optimization you think @miladz68 ?

x/pse/keeper/distribute.go line 84 at r13 (raw file):

			return false, err
		}
		isExcluded := excludedMap[addrStr]

nit: could be moved to if
if excludedMap[addrStr] {

@TxCorpi0x made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on masihyeganeh, metalarm10, and miladz68).