Delete messages from coordinator after they become final #2471
Conversation
cla-bot
bot
added
the
s
ganeshvanahalli
requested review from
Tristan-Wilson,
magicxyyz and
diegoximenes
| @@ -492,6 +511,56 @@ func (c *SeqCoordinator) updateWithLockout(ctx context.Context, nextChosen strin | |||
| return c.noRedisError() | |||
| } | |||
|
|
|||
| func (c *SeqCoordinator) deleteFinalizedMsgsFromRedis(ctx context.Context, finalized arbutil.MessageIndex) error { | |||
| deleteMsgsAndUpdateFinalizedMsgCount := func(keys []string) error { | |||
There was a problem hiding this comment.
do you gain something by having this function defined that way and not as a normal functin?
There was a problem hiding this comment.
like we use a bunch of SeqCoordinator methods such as msgCountToSignedBytes, getRemoteMsgCountImpl, c.Client etc.. so thought having a method on SeqCoordinator is a better option over a standalone function
arbnode/seq_coordinator.go
Outdated
| @@ -473,6 +483,15 @@ func (c *SeqCoordinator) updateWithLockout(ctx context.Context, nextChosen strin | |||
| return c.noRedisError() | |||
| } | |||
| // Was, and still is, the active sequencer | |||
| // Before proceeding, first try deleting finalized messages from redis and setting the finalizedMsgCount key | |||
| finalized, err := c.sync.GetFinalizedMsgCount(ctx) | |||
There was a problem hiding this comment.
this could fail if parent chain doesn't support finalized..
For now, I think a good solution would be to have a boolean (default: true) option that enables deleting finalized messages
arbnode/seq_coordinator.go
Outdated
| if err != nil { | ||
| log.Warn("Error getting finalizedMessageCount from syncMonitor: %w", err) | ||
| } else if finalized == 0 { | ||
| log.Warn("SyncMonitor returned zero finalizedMessageCount") |
There was a problem hiding this comment.
Is there a specific reason you are checking for zero?
A little after initialization it is a valid return value and people will ask us if they get warnings.
There was a problem hiding this comment.
I was checking for 0 because sequencer coordinator could be enabled without inboxreader/l1reader and l1reader is required to fetch finalizedMsgCount. In cases when inboxreader/l1reader is nil, sync monitor is made to return 0, nil this meant a reasonable return value to check if we should skip deleting of finalized messages
This PR enables the chosen sequencer to delete finalized messages from redis. The sequencer is allowed to delete finalized messages if it is chosen for at least
SeqCoordinatorConfig.UpdateIntervalamount of time.Note: Sequencer coordinator tests will see loud warnings as parent chain reader is not enabled in these tests and that is required to get current finalized message count
Resolves NIT-2605