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

Only commit to saving payload once the response has been found #532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

austonst
Copy link
Contributor

📝 Summary

In #512 handleGetPayload was changed to save execution payloads even if the publication was unsuccessful. However, the function commits to saving the payload before it has performed its check to see if the payload in question is available. This PR swaps the order of these operations, so that it does not try to save a payload that cannot be found.

⛱ Motivation and Context

Currently, handleGetPayload commits to saving the requested execution payload (via defer func()) immediately after the header signature is verified, in order to ensure that data is stored long-term. Next, the function checks if the payload response is available in the datastore. If it is not available (the relay had never seen the payload, and the proposer is querying all relays anyway), that situation will be caught here and the function will exit relatively gracefully with a descriptive warning.

After that, the deferred code is run, regardless of whether the response was found. If the corresponding bid was never received, the code will fail its first check, producing an error that appears to be serious at first glance:

level=error msg="failed to get bidTrace for delivered payload from redis"

This PR changes the order of operations: the function only attempts to save the execution payload if the payload is actually available to be saved. The main benefit is in removal of confusing false-positive errors, though it additionally removes one redis read from the case where the payload is unavailable. This does not affect the desired behavior in #512: the payload will be saved regardless of whether publication was successful.


✅ I have run these commands

  • [✅] make lint
  • [✅] make test-race
  • [✅] go mod tidy
  • [✅] I have seen and agree to CONTRIBUTING.md

@JustinDrake
Copy link

cc @michaelneuder

@michaelneuder
Copy link
Collaborator

nice catch! LGTM

michaelneuder
michaelneuder previously approved these changes Oct 3, 2023
@austonst
Copy link
Contributor Author

austonst commented Feb 7, 2024

Same deal, just the code in the moved block has changed slightly, so needed to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants