upate tendermint to 0.40.0 #4963
Conversation
| tonic = { git = "https://github.com/penumbra-zone/tonic.git", tag = "v0.10.3-penumbra" } | ||
| tonic-reflection = { git = "https://github.com/penumbra-zone/tonic.git", tag = "v0.10.3-penumbra" } | ||
| tonic-web = { git = "https://github.com/penumbra-zone/tonic.git", tag = "v0.10.3-penumbra" } | ||
| tendermint = { git = "https://github.com/informalsystems/tendermint-rs/", rev = "refs/pull/1480/head" } |
There was a problem hiding this comment.
Merged, I shall point them to their main repo. We should create an issue to followup with their patch release.
| @@ -344,7 +344,7 @@ impl Opt { | |||
| .register_encoded_file_descriptor_set( | |||
| penumbra_proto::FILE_DESCRIPTOR_SET, | |||
| ) | |||
| .build() | |||
| .build_v1() | |||
There was a problem hiding this comment.
build() was deprecated
| use tower::ServiceExt; | ||
|
|
||
| fn proxy( | ||
| channel: Channel, | ||
| req: http::Request<Body>, | ||
| req: http::Request<BoxBody>, |
There was a problem hiding this comment.
Body is now a trait - BoxBody seems to have taken its place entirely.
| ) -> Pin<Box<dyn Future<Output = Result<http::Response<BoxBody>, Infallible>> + Send + 'static>> { | ||
| tracing::debug!(headers = ?req.headers(), "proxying request"); | ||
| // Convert request types | ||
| let req = req.map(|b| { |
There was a problem hiding this comment.
This conversion seems no longer necessary because BoxBody is an aliaes for UnsyncBoxBody<Bytes, Status>
|
|
||
| let rsp = channel.oneshot(req); | ||
|
|
||
| async move { | ||
| // Once we get the response, we need to convert any transport errors into | ||
| // an Ok(HTTP response reporting an internal error), so we can have Error = Infallible | ||
| let rsp = match rsp.await { | ||
| Ok(rsp) => rsp.map(|b| { |
There was a problem hiding this comment.
Similar to above, this conversion is no longer necessary due to the use of BoxBody
| }), | ||
| Err(e) => tonic::Status::internal(format!("grpc proxy error: {e}")).to_http(), | ||
| Ok(rsp) => rsp, | ||
| Err(e) => tonic::Status::internal(format!("grpc proxy error: {e}")).into_http(), |
There was a problem hiding this comment.
to_http is now into_http
There was a problem hiding this comment.
All changes in this file are primarily due to Body now being a traid, with tonic::body::BoxBody seemingly having taken over its place.
| @@ -28,7 +28,7 @@ anyhow = { workspace = true } | |||
| ark-ff = { workspace = true, default-features = true } | |||
| async-stream = { workspace = true } | |||
| async-trait = { workspace = true } | |||
| axum = "0.6" | |||
| axum = { workspace = true, features = ["http2"] } | |||
There was a problem hiding this comment.
I am not completely sure about axum, axum::serve, and when/how to activate http2. I have left a lengthy comment in penumbra-core::rpc to that effect.
crates/bin/pd/src/main.rs
Outdated
| let grpc_server = penumbra_app::rpc::router(&storage, tm_proxy, enable_expensive_rpc)?; | ||
|
|
||
| let grpc_routes = penumbra_app::rpc::routes(&storage, tm_proxy, enable_expensive_rpc)?; | ||
| // let grpc_server = penumbra_app::rpc::router(&storage, tm_proxy, enable_expensive_rpc)?; |
There was a problem hiding this comment.
Remove commented out code
| // typically uses HTTP/2, which requires HTTPS. Accepting HTTP/2 | ||
| // allows local applications such as web browsers to talk to pd. | ||
| .accept_http1(true) | ||
|
|
There was a problem hiding this comment.
This still needs to be addressed.
The http2-bit might just magically work at all callsites due to our use of axum::serve. But a new trace layer might need be established, for example using tower-http.
| @@ -125,8 +143,7 @@ pub fn router( | |||
| )))) | |||
| .add_service(we(tonic_reflection::server::Builder::configure() | |||
| .register_encoded_file_descriptor_set(penumbra_proto::FILE_DESCRIPTOR_SET) | |||
| .build() | |||
| .build_v1() | |||
erwanor
added
the
consensus-breaking
| @@ -25,7 +25,10 @@ fn trace_events(events: &[Event]) { | |||
| let span = tracing::debug_span!("event", kind = ?event.kind); | |||
| span.in_scope(|| { | |||
| for attr in &event.attributes { | |||
| tracing::debug!(k = ?attr.key, v=?attr.value); | |||
| tracing::debug!( | |||
There was a problem hiding this comment.
These seemed to be reasonable. Tendermint no longer guarantees that keys and values are utf8: https://docs.rs/tendermint/0.40.0/tendermint/abci/enum.EventAttribute.html
| // plain bytes and need not be utf8. This messes with the | ||
| // regex and is likely not desirable to have in downstream | ||
| // consumers of the indexed events. | ||
| match attr.key_str() { |
There was a problem hiding this comment.
Related to tendermint-rs no longer guaranteeing that keys are utf8: https://docs.rs/tendermint/0.40.0/tendermint/abci/enum.EventAttribute.html
It seemed to make sense that, in the case of keys not being utf8, we do not want to index them. But should this case just be dropped quietly?
| @@ -259,6 +259,7 @@ impl Info { | |||
| channel_id: chan_id, | |||
| port_id: PortId::transfer(), | |||
| channel_end: channel, | |||
| upgrade_sequence: 0, | |||
There was a problem hiding this comment.
New field; I did not know what to do with it, so opted for the default value (as per protobuf convention).
| @@ -308,6 +309,7 @@ impl Info { | |||
| channel_id: chan_id, | |||
| port_id: PortId::transfer(), | |||
| channel_end: channel, | |||
| upgrade_sequence: 0, | |||
There was a problem hiding this comment.
New field; I did not know what to do with it, so opted for the default value (as per protobuf convention).
| @@ -70,6 +70,7 @@ impl MsgHandler for MsgChannelCloseConfirm { | |||
| remote: expected_counterparty, | |||
| connection_hops: expected_connection_hops, | |||
| version: channel.version.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -58,6 +58,7 @@ impl MsgHandler for MsgChannelOpenAck { | |||
| remote: expected_counterparty, | |||
| connection_hops: expected_connection_hops, | |||
| version: self.version_on_b.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -65,6 +65,7 @@ impl MsgHandler for MsgChannelOpenConfirm { | |||
| remote: expected_counterparty, | |||
| connection_hops: expected_connection_hops, | |||
| version: channel.version.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -57,6 +57,7 @@ impl MsgHandler for MsgChannelOpenInit { | |||
| remote: Counterparty::new(self.port_id_on_b.clone(), None), | |||
| connection_hops: self.connection_hops_on_a.clone(), | |||
| version: self.version_proposal.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -49,6 +49,7 @@ impl MsgHandler for MsgChannelOpenTry { | |||
| .clone() | |||
| .ok_or_else(|| anyhow::anyhow!("no counterparty connection id provided"))?], | |||
| version: self.version_supported_on_a.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -81,6 +82,7 @@ impl MsgHandler for MsgChannelOpenTry { | |||
| remote: Counterparty::new(self.port_id_on_a.clone(), Some(self.chan_id_on_a.clone())), | |||
| connection_hops: self.connection_hops_on_b.clone(), | |||
| version: self.version_supported_on_a.clone(), | |||
| ..ChannelEnd::default() | |||
There was a problem hiding this comment.
ChannelEnd now has a default impl - so I used it for all new fields. Could set them explicitly?
| @@ -130,6 +132,7 @@ impl<HI: HostInterface + Send + Sync + 'static> ConsensusQuery for IbcQuery<HI> | |||
| channel_id: chan_id, | |||
| port_id: PortId::transfer(), | |||
| channel_end: channel, | |||
| upgrade_sequence: 0, | |||
There was a problem hiding this comment.
New field; I did not know what to do with it, so opted for the default value (as per protobuf convention).
| @@ -187,6 +190,7 @@ impl<HI: HostInterface + Send + Sync + 'static> ConsensusQuery for IbcQuery<HI> | |||
| channel_id: chan_id, | |||
| port_id: PortId::transfer(), | |||
| channel_end: channel, | |||
| upgrade_sequence: 0, | |||
There was a problem hiding this comment.
New field; I did not know what to do with it, so opted for the default value (as per protobuf convention).
| @@ -856,4 +860,28 @@ impl<HI: HostInterface + Send + Sync + 'static> ConsensusQuery for IbcQuery<HI> | |||
| }), | |||
| })) | |||
| } | |||
|
|
|||
| #[tracing::instrument(skip(self), err, level = "debug")] | |||
There was a problem hiding this comment.
There are other examples in the penumbra codebase for services that are not yet implemented - so I just opted to go with the same flow here.
| @@ -207,4 +209,18 @@ impl BankQuery for Server { | |||
| ) -> std::result::Result<tonic::Response<QuerySendEnabledResponse>, tonic::Status> { | |||
| Err(tonic::Status::unimplemented("not implemented")) | |||
| } | |||
|
|
|||
| async fn denom_metadata_by_query_string( | |||
There was a problem hiding this comment.
As above, these are new and I chose to do the same thing as other penumbra handlers that have these unimplemented.
| @@ -21,7 +21,9 @@ tokio-util = {workspace = true, features = ["full"]} | |||
| tonic = {workspace = true} | |||
| prost = {workspace = true} | |||
| tokio-stream = {workspace = true} | |||
| axum = {workspace = true, features = ["headers", "query"]} | |||
| axum = {workspace = true } | |||
| # TODO: consider using the Query extractor provided by axum-extra | |||
There was a problem hiding this comment.
The query extractor is now provided by both axum and axum-extra, although they have minute differences. Just flagging this here.
| axum = {workspace = true, features = ["headers", "query"]} | ||
| axum = {workspace = true } | ||
| # TODO: consider using the Query extractor provided by axum-extra | ||
| axum-extra = {workspace = true, features = ["typed-header"] } |
There was a problem hiding this comment.
A lot of extractors have been moved out of axum into axum-extra.
| @@ -79,8 +80,8 @@ async fn main() -> anyhow::Result<()> { | |||
| .unwrap() | |||
| } | |||
| (None, None) => { | |||
| axum::Server::bind(&address) | |||
| .serve(app.into_make_service()) | |||
| let listener = tokio::net::TcpListener::bind(&address).await.unwrap(); | |||
There was a problem hiding this comment.
This could be seen also in other parts of the penumbra stack: axum has removed its Server type, instead replacing it with a axum::serve utiilty function.
| @@ -262,15 +262,14 @@ fn render_dot(mut tree: watch::Receiver<Tree>) -> MethodRouter { | |||
| if !query.graph { | |||
| return Ok::<_, (StatusCode, String)>(( | |||
| TypedHeader(ContentType::json()), | |||
| StreamBody::new( | |||
| stream::iter(vec![json!({ | |||
| Body::from_stream(stream::once(future::ok::<_, Infallible>( | |||
There was a problem hiding this comment.
Axum replaced StreamBody in favor of Body::from_stream
| @@ -334,7 +333,7 @@ fn render_dot(mut tree: watch::Receiver<Tree>) -> MethodRouter { | |||
| // Manually construct a streaming response to avoid allocating a copy of the large | |||
| // rendered bytes: the graph is already rendered as a JSON-escaped string, so we include | |||
| // it literally in this output | |||
| StreamBody::new( | |||
| Body::from_stream( | |||
There was a problem hiding this comment.
As above, StreamBody was replaced in favor of Body::from_stream
| @@ -8,7 +8,7 @@ anyhow = "1" | |||
|
|
|||
| [features] | |||
| rpc = ["dep:tonic", "ibc-proto/client"] | |||
| box-grpc = ["dep:http-body", "dep:tonic", "dep:tower"] | |||
| box-grpc = ["dep:http-body", "dep:http-body-util", "dep:tonic", "dep:tower"] | |||
There was a problem hiding this comment.
Adaptors on the http::Body trait have been moved to http-body-util
| http-body = { version = "0.4.5" } | ||
| http = { version = "1.2.0" } | ||
| http-body = { version = "1.0.1" } | ||
| http-body-util = { version = "0.1.2" } |
There was a problem hiding this comment.
Adaptors on the http::Body trait have been moved to http-body-util
| @@ -20,7 +17,7 @@ pub type RspBody = UnsyncBoxBody<Bytes, BoxError>; | |||
| pub async fn connect(ep: Endpoint) -> anyhow::Result<BoxGrpcService> { | |||
| let conn = ep.connect().await?; | |||
| let svc = ServiceBuilder::new() | |||
| .map_response(|rsp: grpc::Response<transport::Body>| rsp.map(box_rsp_body)) | |||
| .map_response(|rsp: grpc::Response<tonic::body::BoxBody>| rsp.map(box_rsp_body)) | |||
There was a problem hiding this comment.
As elsewhere: tonic::body::BoxBody took the place of transport::Body
| .with_context(|| format!("could not parse JSON for attribute {:?}", attr))?; | ||
| attributes.insert(attr.key.clone(), value); | ||
| attributes.insert(String::from_utf8_lossy(attr.key_bytes()).into(), value); |
There was a problem hiding this comment.
This seemed to be the least invasive way to deal with tendermints new flexibility around utf8. In the worst case there will be some weird symbols in the attributes. But that's probably fine?
| @@ -78,6 +72,25 @@ impl From<tendermint::abci::EventAttribute> for penumbra_pb::Tag { | |||
| } | |||
| } | |||
|
|
|||
| // impl From<tendermint::abci::event::v0_37::EventAttribute> for penumbra_pb::Tag { | |||
There was a problem hiding this comment.
I need to remove this: this turned out to not be used.
| @@ -89,6 +102,7 @@ impl From<tendermint_rpc::endpoint::broadcast::tx_async::Response> | |||
| data, | |||
| log, | |||
| hash, | |||
| .. | |||
There was a problem hiding this comment.
New unused fields
| @@ -111,6 +125,7 @@ impl From<tendermint_rpc::endpoint::broadcast::tx_sync::Response> | |||
| data, | |||
| log, | |||
| hash, | |||
| .. | |||
There was a problem hiding this comment.
New unused fields
| @@ -288,7 +288,7 @@ where | |||
| "made block" | |||
| ); | |||
| // pass the current value of last_commit with this header | |||
| let block = Block::new(header.clone(), data, evidence, last_commit)?; | |||
| let block = Block::new(header.clone(), data, evidence, last_commit); | |||
There was a problem hiding this comment.
This has become infallible
| @@ -12,7 +12,7 @@ publish = false | |||
| [dependencies] | |||
| anyhow = {workspace = true} | |||
| futures = {workspace = true} | |||
| rustls = "0.21" | |||
| rustls = "0.23.20" | |||
There was a problem hiding this comment.
The bump to rustls was induced to due rustls-acme needing http 1.0.0
| @@ -45,7 +45,6 @@ pub fn axum_acceptor( | |||
|
|
|||
| // Define our server configuration, using the ACME certificate resolver. | |||
| let mut rustls_config = ServerConfig::builder() | |||
| .with_safe_defaults() | |||
There was a problem hiding this comment.
Apparently the ServerConfig is now safe by default. I need to dig it up, this was mentioned either in the hyper issue tracker or in their changelog.
| @@ -70,7 +70,7 @@ fn main() -> anyhow::Result<()> { | |||
| .emit_rerun_if_changed(false) | |||
| .server_mod_attribute(".", rpc_doc_attr) | |||
| .client_mod_attribute(".", rpc_doc_attr) | |||
| .compile_with_config( | |||
There was a problem hiding this comment.
Deprecated, replaced by compile_protos_with_config
| @@ -84,7 +84,7 @@ fn main() -> anyhow::Result<()> { | |||
| // We need to feature-gate the RPCs. | |||
| .server_mod_attribute(".", rpc_doc_attr) | |||
| .client_mod_attribute(".", rpc_doc_attr) | |||
| .compile_with_config( | |||
There was a problem hiding this comment.
Deprecated, replaced by compile_protos_with_config
| @@ -206,11 +207,13 @@ impl Opt { | |||
| }; | |||
| let service = | |||
| CoordinatorService::new(knower, storage.clone(), queue.clone(), marker); | |||
| let grpc_server = Server::builder().add_service( | |||
|
|
|||
| let routes = Routes::new( | |||
There was a problem hiding this comment.
The trusty tonic Server can no longer be turned into an (axum) router - instead, tonic routes need to be constructed.
| @@ -30,7 +30,7 @@ async fn serve_summoning_jpg() -> impl IntoResponse { | |||
| .status(StatusCode::OK) | |||
| .header("Content-Type", "image/jpeg") | |||
| .header("Cache-Control", "public, max-age=3600") // Cache for 1 hour | |||
| .body(axum::body::Full::from(jpg)) | |||
| .body(axum::body::Body::from(jpg)) | |||
There was a problem hiding this comment.
axum::body::Full was removed, and one is to use Body instead (according to their changelog)
| @@ -40,7 +40,7 @@ async fn serve_css() -> impl IntoResponse { | |||
| .status(StatusCode::OK) | |||
| .header("Content-Type", "text/css") | |||
| .header("Cache-Control", "public, max-age=3600") // Cache for 1 hour | |||
| .body(axum::body::Full::from(css)) | |||
| .body(axum::body::Body::from(css)) | |||
There was a problem hiding this comment.
axum::body::Full was removed, and one is to use Body instead (according to their changelog)
| @@ -62,7 +62,7 @@ async fn serve_woff2(filename: &str) -> impl IntoResponse { | |||
| .status(StatusCode::OK) | |||
| .header("Content-Type", "font/woff2") | |||
| .header("Cache-Control", "public, max-age=3600") // Cache for 1 hour | |||
| .body(axum::body::Full::from(data)) | |||
| .body(axum::body::Body::from(data)) | |||
There was a problem hiding this comment.
axum::body::Full was removed, and one is to use Body instead (according to their changelog)
| let grpc_routes = penumbra_app::rpc::routes(&storage, tm_proxy, enable_expensive_rpc)? | ||
| .into_axum_router() | ||
| .layer( | ||
| ServiceBuilder::new().layer(TraceLayer::new_for_grpc().make_span_with( |
There was a problem hiding this comment.
This addresses the fact that we can no longer make use of tonic::transport::Server and it's trace_fn setter (since we can no longer turn the "battries included server" back into an axum router).
Describe your changes
This patch started out as an attempt to penumbra to use crates.io cnidarium, but quickly snowballed from there to include several connected changs (primarily due to a hard coupling with prost and/or tonic):
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: