Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish Timeboost BlockMetadata to Sequencer Coordinator's redis #2735

Open
wants to merge 2 commits into
base: add-timeboosted-broadcastfeedmessage
Choose a base branch
from

Conversation

ganeshvanahalli
Copy link
Contributor

With this PR, the Timeboost information of transactions of a block represented as BlockMetadata (introduced in #2695) will be published to the Sequencer Coordinator redis cluster. Enabling sequencers that are part of this cluster to retrieve BlockMetadata corresponding to a block (when they attempt to sync up new messages from redis).

Resolves NIT-2839

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 14, 2024
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty solid, just some comments about error handling.

Comment on lines +584 to +588
blockMetadataStr, err := c.Client.Get(ctx, redisutil.BlockMetadataKeyFor(pos)).Result()
if err != nil {
log.Debug("SeqCoordinator couldn't read blockMetadata from redis", "err", err, "pos", pos)
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should distinguish missing data, which I think is legitimate since we might not have metadata, from another error from the redis client like missing data.
Something like:

   if err != nil {
        if errors.Is(err, redis.Nil) {
            // Legitimate case of no metadata
            return nil, nil
        }
        // Real error reading from Redis
        return nil, fmt.Errorf("failed reading block metadata: %w", err)
    }

Then in update() you can add error handling matching the existing error flow for messages:

    metadata, err := c.blockMetadataAt(ctx, msgToRead)
    if err != nil {
        log.Warn("coordinator failed reading block metadata", "pos", msgToRead, "err", err)
        msgReadErr = err // Use existing error flow
        break
    }
    blockMetadataArr = append(blockMetadataArr, metadata)
    msgToRead++

This will ultimately cause a retry


	if (wantsLockoutErr != nil) || (msgReadErr != nil) {
		return c.retryAfterRedisError()
	}

Comment on lines +690 to +692
} else if len(blockMetadataArr) > 0 {
log.Warn("Size of blockMetadata array doesn't match the size of messages array", "lockMetadataArrSize", len(blockMetadataArr), "messagesSize", len(messages))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an error return here. Even if there's no metadata, blockMetadataArr should be an array of nils the same size as messagesWithBlockInfo.

Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to click request changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants