Conversation
akundaz
left a comment
There was a problem hiding this comment.
How are you handling the race conditions p2p introduces? Like if two builder build different flashblocks or something like that.
Not really sure what's happening in the p2p crate, please make sure it's documented well so that others can contribute easily
crates/p2p/src/lib.rs
Outdated
| loop { | ||
| match reader.next().await { | ||
| Some(Ok(str)) => { | ||
| let payload: M = serde_json::from_str(&str) |
There was a problem hiding this comment.
Shouldn't deserialization happen in op-rbuilder code?
There was a problem hiding this comment.
i originally had it there, i was debating if the serialization format should be internal or external to the p2p layer. i'm going to refactor this to not be json anyways. could change the p2p layer back to just handle Bytes instead of Message if you prefer that
There was a problem hiding this comment.
sounds good, i'll look again after you refactor
There was a problem hiding this comment.
looks better now that it's over Message, just fix the error messages about flashblocks
out of scope for this PR, i am assuming the high availability setup will be used where op-conductor will manage which party is building at a specific time.
yes will add! PR is still draft, not ready for review yet tbh, will ping you again when it's fully ready. |
| &self.evm_config | ||
| } | ||
|
|
||
| pub(super) fn into_op_payload_builder_ctx( |
There was a problem hiding this comment.
why not for conversions between the types?
There was a problem hiding this comment.
i'm probably going to remove this function after cleanup
There was a problem hiding this comment.
Pull Request Overview
This PR implements a P2P layer for flashblocks using libp2p to enable broadcasting and receiving flashblock payloads between builder nodes. The implementation adds a new p2p crate with basic networking capabilities and integrates it into the flashblocks payload builder to broadcast newly built payloads.
- Implements P2P crate with libp2p node supporting stream protocols and broadcasting
- Updates flashblocks payload builder to broadcast payloads over P2P instead of just WebSocket
- Adds payload handler to manage incoming P2P flashblock messages and route them to the payload builder service
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/p2p/src/lib.rs |
Core P2P node implementation with libp2p networking, message handling, and stream management |
crates/p2p/src/outgoing.rs |
Outgoing stream handler for broadcasting messages to connected peers |
crates/p2p/src/behaviour.rs |
libp2p network behavior configuration with protocols like identify, ping, mDNS |
crates/op-rbuilder/src/builders/flashblocks/service.rs |
Integration of P2P node into flashblocks service with payload handler |
crates/op-rbuilder/src/builders/flashblocks/payload_handler.rs |
Handler for routing built payloads and incoming P2P messages |
crates/op-rbuilder/src/builders/flashblocks/payload.rs |
Updated payload builder to send payloads via channel instead of direct engine calls |
crates/op-rbuilder/src/builders/flashblocks/p2p.rs |
P2P message types and conversions for flashblock payloads |
crates/op-rbuilder/src/args/op.rs |
Added CLI arguments for P2P configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loop { | ||
| tokio::select! { | ||
| Some(payload) = built_rx.recv() => { | ||
| let _ = payload_events_handle.send(Events::BuiltPayload(payload.clone())); |
There was a problem hiding this comment.
Add some logging around this errors pls
| flashblocks_leeway_time: 100, | ||
| flashblocks_fixed: false, | ||
| ..Default::default() | ||
| flashblocks_fixed: false, ..Default::default() |
There was a problem hiding this comment.
Let's change it back? Is it rustfmt that formatted it that way?
There was a problem hiding this comment.
i think it wasn't formatted by rustfmt since it's in a macro, fixed it
| flashblocks: FlashblocksArgs { | ||
| flashblocks_number_contract_address: Some(FLASHBLOCKS_NUMBER_ADDRESS), | ||
| ..Default::default() | ||
| flashblocks_number_contract_address: Some(FLASHBLOCKS_NUMBER_ADDRESS), ..Default::default() |
| pub gas_used: u64, | ||
| pub output: T::Return, | ||
| pub state_changes: HashMap<Address, Account>, | ||
| pub state_changes: HashMap<Address, Account, DefaultHashBuilder>, |
There was a problem hiding this comment.
Wow, this looks strange, what's reason for this?
There was a problem hiding this comment.
see https://github.com/flashbots/op-rbuilder/actions/runs/18853582189/job/53795775017?pr=275 and #275 (comment)
i have no idea why this is happening, thought it was maybe a local issue but it's happening on CI also.
|
General Q: |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
do you mean so that only trusted peers are able to form connections? that could definitely be added as a flag, i plan to do a follow-up with more connection management features. i would say it's out of scope for this PR as it's already large, and is its own feature |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 Summary
future extensions:
💡 Motivation and Context
see https://hackmd.io/@nZ-twauPRISEa6G9zg3XRw/H1fekrwolx
✅ I have completed the following steps:
make lintmake test