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

chore: local -> external builder mapping #47

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

namn-grg
Copy link
Contributor

@namn-grg namn-grg commented Jan 14, 2025

Overview

This PR adds payload ID mapping functionality to track and map between local and external payload IDs when interacting with the builder node. This ensures proper correlation between payloads across different nodes.

Key changes:

  • Added local_to_external_payload_ids cache to PayloadTraceContext to store mappings
  • Added methods to store and retrieve payload ID mappings
  • Updated get_payload_v3 to use external payload ID when requesting from builder
  • Added payload ID mapping in fork_choice_updated_v3 response handling
  • Added logging improvements to track both local and external payload IDs

The changes help maintain proper tracing and correlation between local execution engine payloads and external builder payloads, improving debuggability and reliability of the system.

fixes #46

@namn-grg namn-grg changed the title chore: mapping builder chore: local -> external builder mapping Jan 14, 2025
@namn-grg
Copy link
Contributor Author

@avalonche can you pls check if the logic makes sense?
Its currently failing failing test

'server::tests::test_server' panicked at src/server.rs:760:9:
assertion `left == right` failed
  left: 0
 right: 1

@dmarzzz
Copy link
Member

dmarzzz commented Jan 15, 2025

thanks for the PR! will review soon!

Copy link
Collaborator

@avalonche avalonche left a comment

Choose a reason for hiding this comment

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

LGTM, it'll be great if you can add some tests asserting when

  1. payload ids are the same
  2. payload ids are different
    and check that the correct payload id is used.
    Also there seems to be lint issues

@namn-grg
Copy link
Contributor Author

Thanks for the review @avalonche . Addressed it!

@avalonche
Copy link
Collaborator

I've fixed some failing tests here

@avalonche avalonche merged commit b10510e into flashbots:main Jan 31, 2025
2 checks passed
@namn-grg
Copy link
Contributor Author

namn-grg commented Feb 1, 2025

Thanks! I see it was about running the tests sequentially. Personally not a fan of having all of them under test_server just looks dirty. Can be considered to use serial_test for it.

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.

Map payload_id of local builder to payload_ids of external builders
3 participants