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

a discussion (no related file):
Thanks for clear description of PR scope 👍

app/upgrade/v7/migrate_pse.go line 19 at r1 (raw file):

// distributionID is the initial distribution ID assigned to all existing entries.
// After this upgrade, a new governance proposal must set the schedule starting at ID 1.
const distributionID uint64 = 1

so If first distribution is executed before we make mainnet upgrade then 2nd one will have ID=1 ?

lets discuss how we approach this

app/upgrade/v7/migrate_pse.go line 34 at r1 (raw file):

	}

	if err := migrateAccountScoreSnapshot(ctx, storeService, distributionID); err != nil {

nit: shouldn't it be plural migrateAccountScoreSnapshot ?

app/upgrade/v7/migrate_pse.go line 46 at r1 (raw file):

// clearAllocationSchedule removes all entries from the AllocationSchedule.
// The schedule will be re-submitted via governance after the upgrade.

lets discuss

Code quote:

// The schedule will be re-submitted via governance after the upgrade.

app/upgrade/v7/migrate_pse_test.go line 159 at r1 (raw file):

	requireT.ErrorIs(err, collections.ErrNotFound)

	// Verify LastProcessedDistributionID = 0.

lets discuss

Code quote:

// Verify LastProcessedDistributionID = 0.

integration-tests/upgrade/pse_test.go line 79 at r1 (raw file):

	requireT.Empty(schedRes.ScheduledDistributions)

	// Validator's score should be preserved (and grown since time has passed).

what is the magnitude of growth here ?
Can we validate it 'inPercentage' or possible range is too big here ?

@metalarm10 made 4 comments and resolved 1 discussion.
Reviewable status: 25 of 28 files reviewed, 4 unresolved discussions (waiting on akhlopiachyi, masihyeganeh, miladz68, TxCorpi0x, and ysv).

app/upgrade/v7/migrate_pse.go line 19 at r1 (raw file):

Update: After your review, migration logic is improved: instead of fully pruning the old timestamp-keyed schedule and requiring a new governance proposal, migration now automatically re-keys remaining unprocessed schedule entries with sequential IDs starting from 1.

All existing scores and delegation time entries are intentionally migrated under ID=1, to be processed in the first scheduled multi-block PSE distribution.

Why ID=1 and not 0: The system uses LastProcessedDistributionID to track the last completed distribution. Its initial value is 0 (uint64 default), meaning "nothing processed yet." This value is updated to ongoing.ID after each distribution round completes.
If our first ID were 0, LastProcessedDistributionID would be 0 both before and after the first distribution, so there's no way to distinguish "not set" from "distribution 0 completed," since both return 0.
This ambiguity causes hooks and queries to point to the wrong distribution ID. I see potential issues with starting IDs at 0, but open to discussion if you have better suggestions.

app/upgrade/v7/migrate_pse.go line 46 at r1 (raw file):

Update: This part is updated in the migration logic. The migration now auto-assigns sequential IDs to existing (unprocessed) schedule entries, so no governance re-submission is needed post-upgrade.

app/upgrade/v7/migrate_pse_test.go line 159 at r1 (raw file):

Sure.

integration-tests/upgrade/pse_test.go line 79 at r1 (raw file):

The ~50s test duration (voting + upgrade) adds tokens × 50 to the score. Since the growth depends on elapsed test time, maybe adding a bound here can make the integration test flaky.
Logs from actual test run:
pse_test.go:129: PSE After: params preserved, LastProcessedDistributionID=0, schedule re-keyed (2 entries with IDs), score preserved (1260000000000000 -> 1760000000000000)

@ysv made 2 comments and resolved 2 discussions.
Reviewable status: 25 of 28 files reviewed, 2 unresolved discussions (waiting on akhlopiachyi, masihyeganeh, metalarm10, miladz68, and TxCorpi0x).

app/upgrade/v7/migrate_pse.go line 19 at r1 (raw file):

06.03-06.04 - PSE 1 -> ID: 1
06.04-06.05 - PSE 2 -> ID: 2

Agreed to query current mainnet config and set it inside upgrade directly. Only add ID field.

NOte: We want to preserve all schedule entries so we don't remove them.

integration-tests/upgrade/pse_test.go line 79 at r1 (raw file):

agreed to improve

Generally, I think the poin that i mentioned and also a good integration to cover the update schedule scenarios with multi-block is needed @ysv @metalarm10

@TxCorpi0x partially reviewed 30 files and made 2 comments.
Reviewable status: 28 of 30 files reviewed, 3 unresolved discussions (waiting on akhlopiachyi, masihyeganeh, metalarm10, and miladz68).

x/pse/keeper/hooks.go line 71 at r5 (raw file):

		return 0, err
	}
	return lastProcessed + 1, nil

I am curious about this scenario that may cause a discrepancy in the ID:
If a governance proposal for updating the schedule will be runing getNextDistributionID writes state change to LastProcessedDistributionID + 1 while in distribution.go:337 the schedule starts later, (e.g. lastProcessed=5, but new schedule starts at 10), so the Hok writes the new entries under ID 6, shceduler processed ID 10 first. and distribution.go:40 reads ID 10 (not 6) may produce empty community distribution.

Unit test added for disallowing schedule ID gaps on gov proposal handler. Detailed integration tests will be added in the next PR. (Thanks for pointing this out).

@metalarm10 made 4 comments.
Reviewable status: 27 of 31 files reviewed, 3 unresolved discussions (waiting on akhlopiachyi, masihyeganeh, miladz68, TxCorpi0x, and ysv).

app/upgrade/v7/migrate_pse.go line 19 at r1 (raw file):

Done.

integration-tests/upgrade/pse_test.go line 79 at r1 (raw file):

Done.

x/pse/keeper/hooks.go line 71 at r5 (raw file):

Done. Governance validation now enforces first schedule ID == LastProcessedDistributionID + 1 (no gaps allowed). This ensures hooks and scheduler always write/read the same distribution ID.

@ysv reviewed 12 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on akhlopiachyi, masihyeganeh, miladz68, and TxCorpi0x).

@TxCorpi0x reviewed 4 files and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on akhlopiachyi, masihyeganeh, and miladz68).