-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Bsc block import #14784
base: main
Are you sure you want to change the base?
Bsc block import #14784
Conversation
5de9fcf
to
d087b2d
Compare
c76fc1d
to
4f12ba6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some suggestions, but overall this is pretty good already
crates/net/network/src/import.rs
Outdated
/// Engine error | ||
#[error(transparent)] | ||
Engine(#[from] BeaconOnNewPayloadError), | ||
/// Invalid payload | ||
#[error("invalid payload: {0}")] | ||
InvalidPayload(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to open another pr that introduces a Other<Box<dyn Error>
variant for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go:
4c264f5
to
842751f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some additional suggestions, but over all this looks pretty good.
let's try to get the example working and then we can include this and make more incremental progress
pub struct BscBlockImport<EngineT: EngineTypes> { | ||
handle: ImportHandle<EngineT>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep this wrapper type for now but maybe we can also impl the trait directly on the handle
/// Future that processes a block import and returns its outcome | ||
type ImportFuture<T> = Pin<Box<dyn Future<Output = Outcome<T>> + Send + Sync>>; | ||
|
||
pub struct ImportHandle<T: EngineTypes> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add some docs that describe how this is wired
) -> Result<(), Box<dyn std::error::Error>> { | ||
self.to_import.send((block, peer_id))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use the same error type here and mention that this returns an error if the channel is closed
pending_imports: FuturesUnordered<ImportFuture<T>>, | ||
} | ||
|
||
impl<Provider: BlockNumReader + Clone + 'static, T: EngineTypes> ImportService<Provider, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put this in a where clause instead
// Send forkchoice update if the payload is valid | ||
if let Err(err) = | ||
Self::send_fork_choice_update(&engine, &provider, hash, number).await |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do this in two steps and avoid including to much in a single operation
new payload -> 1 future
on outcome -> next future
/// Determines the head block hash according to Parlia consensus rules: | ||
/// 1. Follow the highest block number | ||
/// 2. For same height blocks, pick the one with lower hash | ||
pub(crate) fn canonical_head( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can likely start with a simple struct ParliaConsesnsus struct and include all of the logic in there, this should make upcoming refactoring work easier
|
||
/// Sends a new payload to the engine and waits for the outcome. | ||
/// If it is valid, proceeds with Forkchoice Update (FCU). | ||
fn import(&self, block: BlockMsg<T>, peer_id: PeerId) -> ImportFuture<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this to something fn on_block
} | ||
} | ||
|
||
impl<Provider: BlockNumReader + Clone + 'static + Unpin, T: EngineTypes> Future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a where clause here as well
Block Import Process for BSC
This PR enables block import for BSC with the following steps:
NewBlockMessage
from the network.