fix: use sequential sub_dag_index instead of nonce for LeaderSchedule#520
Conversation
This commit fixes issue Telcoin-Association#472 where the LeaderSchedule was using the Certificate's nonce (epoch << 32 | round) instead of a sequential subdag index for reputation score calculations. Changes: - Add `sub_dag_index` field to CommittedSubDag for sequential tracking - Replace `leader.nonce()` with `state.next_sub_dag_index()` in Bullshark - Remove the `/2` division hack that was compensating for round-based indices - Persist sub_dag_index across epoch boundaries by recovering it from the global latest subdag before filtering by current epoch - Add test `sub_dag_index_persists_across_epochs` to verify the fix The sequential index ensures that: - ReputationScores are recalculated at consistent intervals (every 60 commits) - final_of_schedule is triggered correctly regardless of epoch changes - LeaderSwapTable updates happen predictably
sstanfield
left a comment
sstanfield
left a comment
There was a problem hiding this comment.
I want to go over this one with Grant. That ticket was actually something we were not in total agreement on but had shelved (sorry, should have had a note in the issue). I removed the original sub dag index and just used the nonce since it was one less data field in the consensus "chain" and I think the nonce is good enough. Anyway now that you have implemented this that may resolve it... I also may be wrong about the nonce being good enough but it does grow per round it just will have large gaps per epoch.
| /// Sequential index of this committed subdag (monotonically increasing). | ||
| /// This is a counter that increments by 1 for each commit, regardless of | ||
| /// which rounds are committed or skipped. | ||
| #[serde(default)] // Backward compatibility: 0 if absent |
There was a problem hiding this comment.
This is fine but we have other changes coming that will be data incompatible so you probably don't need this. The next reset of devnet will likely not have compatible data. There are some small changes and some big ones, I am implementing static/pack file for the consensus data right now that cause the DB access to change at it's core.
There was a problem hiding this comment.
No worries if this doesn't end up being the right approach - just close it if needed.
2 participants
This commit fixes issue #472 where the LeaderSchedule was using the Certificate's nonce (epoch << 32 | round) instead of a sequential subdag index for reputation score calculations.
Changes:
sub_dag_indexfield to CommittedSubDag for sequential trackingleader.nonce()withstate.next_sub_dag_index()in Bullshark/2division hack that was compensating for round-based indicessub_dag_index_persists_across_epochsto verify the fixThe sequential index ensures that: