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

Rebase ABCI domain types onto main #1203

Merged
merged 29 commits into from
Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
3021889
split A.4.1, proto tooling changes
hdevalence Sep 20, 2022
38a5de3
split A.2, changelog abci
hdevalence Sep 20, 2022
b7f3125
split A.1, tendermint and abci changes
hdevalence Sep 20, 2022
8bc6a97
split A.3, abci-in-rpc
hdevalence Sep 20, 2022
3096775
clean up imports in abci crate
hdevalence Sep 20, 2022
e403925
Return to 0.34 ABCI encoding
hdevalence Aug 17, 2022
5de9425
Cherry-pick chrono changes
mzabaluev Dec 7, 2021
b9b6596
fixup! split A.3, abci-in-rpc
hdevalence Sep 20, 2022
c02739a
split A.4.1.1, check_event_attrs
hdevalence Sep 20, 2022
2e429e7
split A.4.1.2, some slice change?
hdevalence Sep 20, 2022
064fb82
split A.4.1.3, b.data.get(0).is_none()
hdevalence Sep 20, 2022
39f7210
split A.4.1.4, remove test attrs
hdevalence Sep 20, 2022
60dafb4
split A.4.1.5, someting optiony
hdevalence Sep 20, 2022
61e6bf5
clippy fix
hdevalence Sep 20, 2022
08a6a1e
Remove unnessarily generated files
mzabaluev Sep 22, 2022
ce30fe6
Fix deliver_tx.events KV tests
mzabaluev Sep 22, 2022
35b75fc
Remove Option wart in net_info::Response
mzabaluev Sep 22, 2022
9667148
rpc: Revert Event member events to a KV map
mzabaluev Sep 23, 2022
8f45e28
Merge branch 'main' into mikhail/abci-domain-types
mzabaluev Sep 23, 2022
3aba2d2
Revert tests to checking decoded KVs
mzabaluev Sep 23, 2022
6c38131
rpc: Restore base64 serialization of event KV tags
mzabaluev Sep 23, 2022
0ca7299
Post-merge fixes for rpc tests
mzabaluev Sep 23, 2022
3a92d77
rpc: Use the re-exported serializers mod
mzabaluev Sep 23, 2022
8d31c4e
Remove duplicate changelog entries
mzabaluev Sep 23, 2022
f9ae6a4
Fix kvstore-test build
mzabaluev Sep 23, 2022
308d060
Changelog entry for switching to Bytes
mzabaluev Sep 26, 2022
35e3fe5
Remove a commented out line
mzabaluev Sep 28, 2022
2a89abe
abci: Restore Client::set_option as deprecated
mzabaluev Sep 28, 2022
b21d86b
Update abci/src/client.rs
thanethomson Sep 28, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[tendermint-proto]` Use `Bytes` for byte array fields of ABCI protobuf types.
([#1203](https://github.com/informalsystems/tendermint-rs/pull/1203))
Comment on lines +1 to +2
Copy link
Contributor Author

@mzabaluev mzabaluev Sep 26, 2022

Choose a reason for hiding this comment

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

@thanethomson I've added a notice about this change, but as stated already, I'm not sure we need to go ahead with it for 0.26. This is certainly unrelated to domain types, it's applied piecemeal to a single proto module not as a consistent change throughout tendermint-proto, and has the overall smell of a premature optimization.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is related to the domain types, because the ABCI domain types use Bytes internally, so unwinding this part of the ABCI domain types will break existing users of the ABCI domain types. This is not meaningfully a breaking change for anyone else, because there weren't users of the ABCI proto types other than the stub ABCI demo application. It's also not a breaking change relative to master, because this code has been merged on that branch (albeit unreleased) for about a year.

If this change is unwound out, you'll have to edit all of the ABCI domain types, and then all the users will have to edit all their uses of those domain types. I don't think it would be good to put this on the critical path for doing a semver release of the tendermint crates that contains ABCI domain types.

If the decision, later, is that the domain types should not use the Bytes type, it would be much better to do that as a later change that can be modeled with semver. (For the record, I don't think this is a good decision, but in any case I think it should be separate from getting this code backported as-is).

Copy link
Contributor Author

@mzabaluev mzabaluev Sep 26, 2022

Choose a reason for hiding this comment

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

Thank you @hdevalence for the detailed exposition.

As a late-to-the-scene comment, if I were into micro-optimizing domain type conversions to protobuf, I'd start with the Protobuf trait forcing a clone of the entire structure, because the domain-to-raw conversion is implemented via From<Self>, but the encode* methods take &self. But this is a subject for a larger rework.

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[tendermint]` Added domain types for ABCI
([#862](https://github.com/informalsystems/tendermint-rs/issues/862))
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[tendermint-abci]` Deprecate `Client::set_option`.
([#1203](https://github.com/informalsystems/tendermint-rs/pull/1203))
12 changes: 3 additions & 9 deletions abci/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ pub mod kvstore;
use tendermint_proto::abci::{
request::Value, response, Request, RequestApplySnapshotChunk, RequestBeginBlock,
RequestCheckTx, RequestDeliverTx, RequestEcho, RequestEndBlock, RequestInfo, RequestInitChain,
RequestLoadSnapshotChunk, RequestOfferSnapshot, RequestQuery, RequestSetOption, Response,
RequestLoadSnapshotChunk, RequestOfferSnapshot, RequestQuery, Response,
ResponseApplySnapshotChunk, ResponseBeginBlock, ResponseCheckTx, ResponseCommit,
ResponseDeliverTx, ResponseEcho, ResponseEndBlock, ResponseFlush, ResponseInfo,
ResponseInitChain, ResponseListSnapshots, ResponseLoadSnapshotChunk, ResponseOfferSnapshot,
ResponseQuery, ResponseSetOption,
ResponseQuery,
};

/// An ABCI application.
Expand Down Expand Up @@ -76,12 +76,6 @@ pub trait Application: Send + Clone + 'static {
Default::default()
}

/// Allows the Tendermint node to request that the application set an
/// option to a particular value.
fn set_option(&self, _request: RequestSetOption) -> ResponseSetOption {
Default::default()
}

/// Used during state sync to discover available snapshots on peers.
fn list_snapshots(&self) -> ResponseListSnapshots {
Default::default()
Expand Down Expand Up @@ -123,7 +117,6 @@ impl<A: Application> RequestDispatcher for A {
Value::Echo(req) => response::Value::Echo(self.echo(req)),
Value::Flush(_) => response::Value::Flush(self.flush()),
Value::Info(req) => response::Value::Info(self.info(req)),
Value::SetOption(req) => response::Value::SetOption(self.set_option(req)),
Value::InitChain(req) => response::Value::InitChain(self.init_chain(req)),
Value::Query(req) => response::Value::Query(self.query(req)),
Value::BeginBlock(req) => response::Value::BeginBlock(self.begin_block(req)),
Expand All @@ -141,6 +134,7 @@ impl<A: Application> RequestDispatcher for A {
Value::ApplySnapshotChunk(req) => {
response::Value::ApplySnapshotChunk(self.apply_snapshot_chunk(req))
},
Value::SetOption(_) => response::Value::SetOption(Default::default()),
}),
}
}
Expand Down
43 changes: 20 additions & 23 deletions abci/src/application/kvstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ use tendermint_proto::abci::{
};
use tracing::{debug, info};

use crate::{
codec::{encode_varint, MAX_VARINT_LENGTH},
Application, Error,
};
use crate::{codec::MAX_VARINT_LENGTH, Application, Error};

/// In-memory, hashmap-backed key/value store ABCI application.
///
Expand Down Expand Up @@ -50,15 +47,15 @@ use crate::{
/// // Deliver a transaction and then commit the transaction
/// client
/// .deliver_tx(RequestDeliverTx {
/// tx: "test-key=test-value".as_bytes().to_owned(),
/// tx: "test-key=test-value".into(),
/// })
/// .unwrap();
/// client.commit().unwrap();
///
/// // We should be able to query for the data we just delivered above
/// let res = client
/// .query(RequestQuery {
/// data: "test-key".as_bytes().to_owned(),
/// data: "test-key".into(),
/// path: "".to_string(),
/// height: 0,
/// prove: false,
Expand Down Expand Up @@ -129,25 +126,25 @@ impl Application for KeyValueStoreApp {
version: "0.1.0".to_string(),
app_version: 1,
last_block_height,
last_block_app_hash,
last_block_app_hash: last_block_app_hash.into(),
}
}

fn query(&self, request: RequestQuery) -> ResponseQuery {
let key = match String::from_utf8(request.data.clone()) {
let key = match std::str::from_utf8(&request.data) {
Ok(s) => s,
Err(e) => panic!("Failed to intepret key as UTF-8: {}", e),
};
debug!("Attempting to get key: {}", key);
match self.get(key.clone()) {
match self.get(key) {
Ok((height, value_opt)) => match value_opt {
Some(value) => ResponseQuery {
code: 0,
log: "exists".to_string(),
info: "".to_string(),
index: 0,
key: request.data,
value: value.into_bytes(),
value: value.into_bytes().into(),
proof_ops: None,
height,
codespace: "".to_string(),
Expand All @@ -158,7 +155,7 @@ impl Application for KeyValueStoreApp {
info: "".to_string(),
index: 0,
key: request.data,
value: vec![],
value: Default::default(),
proof_ops: None,
height,
codespace: "".to_string(),
Expand All @@ -171,7 +168,7 @@ impl Application for KeyValueStoreApp {
fn check_tx(&self, _request: RequestCheckTx) -> ResponseCheckTx {
ResponseCheckTx {
code: 0,
data: vec![],
data: Default::default(),
log: "".to_string(),
info: "".to_string(),
gas_wanted: 1,
Expand All @@ -183,17 +180,17 @@ impl Application for KeyValueStoreApp {
}

fn deliver_tx(&self, request: RequestDeliverTx) -> ResponseDeliverTx {
let tx = String::from_utf8(request.tx).unwrap();
let tx = std::str::from_utf8(&request.tx).unwrap();
let tx_parts = tx.split('=').collect::<Vec<&str>>();
let (key, value) = if tx_parts.len() == 2 {
(tx_parts[0], tx_parts[1])
} else {
(tx.as_ref(), tx.as_ref())
(tx, tx)
};
let _ = self.set(key, value).unwrap();
ResponseDeliverTx {
code: 0,
data: vec![],
data: Default::default(),
log: "".to_string(),
info: "".to_string(),
gas_wanted: 0,
Expand All @@ -202,18 +199,18 @@ impl Application for KeyValueStoreApp {
r#type: "app".to_string(),
attributes: vec![
EventAttribute {
key: "key".as_bytes().to_owned(),
value: key.as_bytes().to_owned(),
key: "key".to_string().into_bytes().into(),
value: key.to_string().into_bytes().into(),
index: true,
},
EventAttribute {
key: "index_key".as_bytes().to_owned(),
value: "index is working".as_bytes().to_owned(),
key: "index_key".to_string().into_bytes().into(),
value: "index is working".to_string().into_bytes().into(),
index: true,
},
EventAttribute {
key: "noindex_key".as_bytes().to_owned(),
value: "index is working".as_bytes().to_owned(),
key: "noindex_key".to_string().into_bytes().into(),
value: "index is working".to_string().into_bytes().into(),
index: false,
},
],
Expand All @@ -228,7 +225,7 @@ impl Application for KeyValueStoreApp {
let (height, app_hash) = channel_recv(&result_rx).unwrap();
info!("Committed height {}", height);
ResponseCommit {
data: app_hash,
data: app_hash.into(),
retain_height: height - 1,
}
}
Expand Down Expand Up @@ -285,7 +282,7 @@ impl KeyValueStoreDriver {
// As in the Go-based key/value store, simply encode the number of
// items as the "app hash"
let mut app_hash = BytesMut::with_capacity(MAX_VARINT_LENGTH);
encode_varint(self.store.len() as u64, &mut app_hash);
prost::encoding::encode_varint(self.store.len() as u64, &mut app_hash);
self.app_hash = app_hash.to_vec();
self.height += 1;
channel_send(&result_tx, (self.height, self.app_hash.clone()))
Expand Down
4 changes: 4 additions & 0 deletions abci/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ impl Client {
}

/// Request that the application set an option to a particular value.
///
/// This request lacks specification and should not be used.
/// It will be removed in Tendermint Core v0.37.
#[deprecated(note = "The set_option ABCI method will be removed in Tendermint Core v0.37")]
pub fn set_option(&mut self, req: RequestSetOption) -> Result<ResponseSetOption, Error> {
perform!(self, SetOption, req)
}
Expand Down
6 changes: 3 additions & 3 deletions abci/tests/kvstore_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@ mod kvstore_app_integration {

client
.deliver_tx(RequestDeliverTx {
tx: "test-key=test-value".as_bytes().to_owned(),
tx: "test-key=test-value".into(),
})
.unwrap();
client.commit().unwrap();

let res = client
.query(RequestQuery {
data: "test-key".as_bytes().to_owned(),
data: "test-key".into(),
path: "".to_string(),
height: 0,
prove: false,
})
.unwrap();
assert_eq!(res.value, "test-value".as_bytes().to_owned());
assert_eq!(res.value, "test-value".as_bytes());
}
}
2 changes: 1 addition & 1 deletion config/src/prelude.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Re-export according to alloc::prelude::v1 because it is not yet stabilized
// https://doc.rust-lang.org/src/alloc/prelude/v1.rs.html
pub use alloc::borrow::ToOwned;
pub use alloc::{
borrow::ToOwned,
boxed::Box,
format,
string::{String, ToString},
Expand Down
2 changes: 1 addition & 1 deletion light-client-verifier/src/prelude.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Re-export according to alloc::prelude::v1 because it is not yet stabilized
// https://doc.rust-lang.org/src/alloc/prelude/v1.rs.html
pub use alloc::borrow::ToOwned;
pub use alloc::{
borrow::ToOwned,
boxed::Box,
format,
string::{String, ToString},
Expand Down
2 changes: 1 addition & 1 deletion light-client/src/evidence.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! Fork evidence data structures and interfaces.

use contracts::contract_trait;
use tendermint::abci::transaction::Hash;
pub use tendermint::evidence::Evidence;
use tendermint_rpc::abci::transaction::Hash;

use crate::{components::io::IoError, verifier::types::PeerId};

Expand Down
2 changes: 1 addition & 1 deletion light-client/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ use std::{collections::HashMap, time::Duration};
use contracts::contract_trait;
use serde::{Deserialize, Serialize};
use tendermint::{
abci::transaction::Hash,
block::Height as HeightStr,
evidence::{Duration as DurationStr, Evidence},
};
use tendermint_rpc as rpc;
use tendermint_rpc::abci::transaction::Hash;

use crate::{
components::{
Expand Down
2 changes: 1 addition & 1 deletion proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ all-features = true
[dependencies]
prost = { version = "0.11", default-features = false }
prost-types = { version = "0.11", default-features = false }
bytes = { version = "1.0", default-features = false }
bytes = { version = "1.0", default-features = false, features = ["serde"]}
serde = { version = "1.0", default-features = false, features = ["derive"] }
serde_bytes = { version = "0.11", default-features = false, features = ["alloc"] }
subtle-encoding = { version = "0.5", default-features = false, features = ["hex", "base64", "alloc"] }
Expand Down
2 changes: 1 addition & 1 deletion proto/src/prelude.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Re-export according to alloc::prelude::v1 because it is not yet stabilized
// https://doc.rust-lang.org/src/alloc/prelude/v1.rs.html
pub use alloc::borrow::ToOwned;
pub use alloc::{
borrow::ToOwned,
boxed::Box,
format,
string::{String, ToString},
Expand Down
Loading