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

Change poet client to cache the full response of the v1/info #6226

Closed
fasmat opened this issue Aug 7, 2024 · 3 comments
Closed

Change poet client to cache the full response of the v1/info #6226

fasmat opened this issue Aug 7, 2024 · 3 comments

Comments

@fasmat
Copy link
Member

fasmat commented Aug 7, 2024

Description

From a discussion on PR #6116: #6116 (comment)

The whole info response should be cached instead of just the types.CertifierInfo field of the response.

Affected code

The poet client already caches part of the response returned from the v1/info endpoint via these two methods:

func (c *poetService) Certify(ctx context.Context, id types.NodeID) (*certifier.PoetCert, error) {
if c.certifier == nil {
return nil, errors.New("certifier not configured")
}
info, err := c.getCertifierInfo(ctx)
if err != nil {
return nil, err
}
return c.certifier.Certificate(ctx, id, info.Url, info.Pubkey)
}
func (c *poetService) getCertifierInfo(ctx context.Context) (*types.CertifierInfo, error) {
info, err := c.certifierInfoCache.get(func() (*types.CertifierInfo, error) {
certifierInfo, err := c.client.CertifierInfo(ctx)
if err != nil {
return nil, fmt.Errorf("getting certifier info: %w", err)
}
return certifierInfo, nil
})
if err != nil {
return nil, err
}
return info, nil
}

This should be changed so that the whole response is cached:

func (c *poetService) getInfo(ctx context.Context) (*types.PoetInfo, error) {
	info, err := c.infoCache.get(func() (*types.PoetInfo, error) {
		info, err := c.client.Info(ctx)
		if err != nil {
			return nil, fmt.Errorf("getting info: %w", err)
		}
		return info, nil
	})
	if err != nil {
		return nil, err
	}
	return info, nil
}

simplifying verifyPhaseShiftConfiguration to the following:

func (c *poetService) verifyPhaseShiftConfiguration(ctx context.Context) error {
	info, err := c.getInfo(ctx)
	if err != nil {
		return err
	}

	if info.PhaseShift != c.expectedPhaseShift {
		return ErrIncompatiblePhaseShift
	}

	return nil
}
@acud
Copy link
Contributor

acud commented Aug 20, 2024

A note for consideration is etags. They're meant to allow for resource expiration at-the-server and allow intermediate layers to do caching and invalidation more easily. Just caching for some time might backfire under certain edge cases (please do consider my ignorance over the topic because I'm not sure they exist here).

@fasmat
Copy link
Member Author

fasmat commented Aug 21, 2024

Sure, but this is just to keep track of a change that we excluded from a previous PR to get it merged quicker 🙂. PoET doesn't send Etags, the primary reason we are caching at all is to reduce the number of requests from the node to the servers. The node sends the same request for each identity to the same PoET (usually) around the same time, this just ensures that in that case we re-use the response we already got for the first identity for all others.

@poszu poszu moved this from 🔖 Next to 🏗 Doing in Dev team kanban Aug 21, 2024
@poszu
Copy link
Contributor

poszu commented Sep 3, 2024

Implemented in #6274

@poszu poszu closed this as completed Sep 3, 2024
@github-project-automation github-project-automation bot moved this from 🏗 Doing to ✅ Done in Dev team kanban Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants