-
Notifications
You must be signed in to change notification settings - Fork 792
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
Implement data columns by network boilerplate #6224
Changes from 1 commit
5c59e7f
32b27a2
3690a46
d045a6a
3a67dcd
f3bd42c
815599c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -423,7 +423,6 @@ fn context_bytes<E: EthSpec>( | |
| RPCResponse::BlobsByRoot(_) | ||
| RPCResponse::DataColumnsByRoot(_) | ||
| RPCResponse::DataColumnsByRange(_) => { | ||
// TODO(electra) | ||
return fork_context.to_context_bytes(ForkName::Deneb); | ||
jimmygchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
RPCResponse::LightClientBootstrap(lc_bootstrap) => { | ||
|
@@ -625,14 +624,18 @@ fn handle_rpc_response<E: EthSpec>( | |
)), | ||
}, | ||
SupportedProtocol::DataColumnsByRootV1 => match fork_name { | ||
// TODO(das): update fork name | ||
Some(ForkName::Deneb) => Ok(Some(RPCResponse::DataColumnsByRoot(Arc::new( | ||
DataColumnSidecar::from_ssz_bytes(decoded_buffer)?, | ||
)))), | ||
Some(_) => Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
"Invalid fork name for data columns by root".to_string(), | ||
)), | ||
Some(fork_name) => { | ||
if fork_name.deneb_enabled() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! we can perhaps add a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PeerDAS is currently supported for both deneb and electra. This check does not advertise the topic on deneb, simply allows it to decode it. Advertise logic is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah right, thanks |
||
Ok(Some(RPCResponse::DataColumnsByRoot(Arc::new( | ||
DataColumnSidecar::from_ssz_bytes(decoded_buffer)?, | ||
)))) | ||
} else { | ||
Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
"Invalid fork name for data columns by root".to_string(), | ||
)) | ||
} | ||
} | ||
None => Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
format!( | ||
|
@@ -642,14 +645,18 @@ fn handle_rpc_response<E: EthSpec>( | |
)), | ||
}, | ||
SupportedProtocol::DataColumnsByRangeV1 => match fork_name { | ||
// TODO(das): update fork name | ||
Some(ForkName::Deneb) => Ok(Some(RPCResponse::DataColumnsByRange(Arc::new( | ||
DataColumnSidecar::from_ssz_bytes(decoded_buffer)?, | ||
)))), | ||
Some(_) => Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
"Invalid fork name for data columns by range".to_string(), | ||
)), | ||
Some(fork_name) => { | ||
if fork_name.deneb_enabled() { | ||
dapplion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Ok(Some(RPCResponse::DataColumnsByRange(Arc::new( | ||
DataColumnSidecar::from_ssz_bytes(decoded_buffer)?, | ||
)))) | ||
} else { | ||
Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
"Invalid fork name for data columns by range".to_string(), | ||
)) | ||
} | ||
} | ||
None => Err(RPCError::ErrorResponse( | ||
RPCResponseErrorCode::InvalidRequest, | ||
format!( | ||
|
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.
Looks like the intent before was to prioritize all by roots requests ahead of all by range requests?
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 have no evidence this is useful, so let's just extend the deneb priorization