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

Custom proof specs in config #1574

Merged
merged 23 commits into from
Nov 25, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d15f6a2
Impl serde for ics23 proto types
hu55a1n1 Nov 17, 2021
bd9ff26
Add proof specs to client state
hu55a1n1 Nov 17, 2021
6750982
Get proof specs from chain config
hu55a1n1 Nov 17, 2021
ee32e66
Fix test compilation
hu55a1n1 Nov 18, 2021
f0fd102
Define ProofSpecs domain type using ibc_proto::ics23 types
hu55a1n1 Nov 18, 2021
67c8991
Minor refactoring
hu55a1n1 Nov 18, 2021
623e766
Add attrs for ics23 protos in proto-compiler
hu55a1n1 Nov 18, 2021
d55f57c
Fix clippy errors
hu55a1n1 Nov 18, 2021
f98cf85
Refactor Tendermint to use domain type
hu55a1n1 Nov 19, 2021
2c109ed
Parse config proofspecs as JSON serialized string
hu55a1n1 Nov 19, 2021
3873c11
Add Default impl for ProofSpecs
hu55a1n1 Nov 19, 2021
c7aaa03
Use serde(with) instead of newtype
hu55a1n1 Nov 19, 2021
e4b8ea3
Get rid of unwraps
hu55a1n1 Nov 19, 2021
4ab7308
Fix failing test
hu55a1n1 Nov 19, 2021
0822feb
Document proof-specs opt in config
hu55a1n1 Nov 19, 2021
1803828
Minor refactoring to match style
hu55a1n1 Nov 19, 2021
9347949
Create .changelog entry
hu55a1n1 Nov 19, 2021
0b9a709
Serialilze ProofSpecs as a pretty formatted JSON string
hu55a1n1 Nov 24, 2021
a72ab39
Set serde dep version to '1'
hu55a1n1 Nov 24, 2021
12a46a4
Manually implement conversions b/w ProofSpec protos
hu55a1n1 Nov 24, 2021
a229708
Disallow empty proof-specs
hu55a1n1 Nov 24, 2021
450992f
Merge branch 'master' into hu55a1n1/1561-custom-proof-specs
hu55a1n1 Nov 24, 2021
9ab79a1
Move custom serde serializer to its own module
romac Nov 24, 2021
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 @@
- Allow custom proof-specs in chain config
([#1561](https://github.com/informalsystems/ibc-rs/issues/1561))
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 54 additions & 0 deletions config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,60 @@ memo_prefix = ''
# ['transfer', 'channel-0'],
# ]

# Specify custom ICS23 proof-specs for the chain (serialized as JSON).
# Default: [`ProofSpecs::cosmos()`](modules/src/core/ics23_commitment/specs.rs)
proof-specs = '''
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more operator-friendly manner to specify this? I tried to give an alternative suggestion in #1630 .

[
{
"leaf_spec": {
"hash": 1,
"prehash_key": 0,
"prehash_value": 1,
"length": 1,
"prefix": [
0
]
},
"inner_spec": {
"child_order": [
0,
1
],
"child_size": 33,
"min_prefix_length": 4,
"max_prefix_length": 12,
"empty_child": [],
"hash": 1
},
"max_depth": 0,
"min_depth": 0
},
{
"leaf_spec": {
"hash": 1,
"prehash_key": 0,
"prehash_value": 1,
"length": 1,
"prefix": [
0
]
},
"inner_spec": {
"child_order": [
0,
1
],
"child_size": 32,
"min_prefix_length": 1,
"max_prefix_length": 1,
"empty_child": [],
"hash": 1
},
"max_depth": 0,
"min_depth": 0
}
]
'''

[[chains]]
id = 'ibc-1'
Expand Down
12 changes: 10 additions & 2 deletions modules/src/clients/ics07_tendermint/client_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub struct ClientState {
pub max_clock_drift: Duration,
pub frozen_height: Height,
pub latest_height: Height,
// pub proof_specs: ::core::vec::Vec<super::super::super::super::ics23::ProofSpec>,
pub proof_specs: ProofSpecs,
pub upgrade_path: Vec<String>,
pub allow_update: AllowUpdate,
}
Expand All @@ -52,6 +52,7 @@ impl ClientState {
max_clock_drift: Duration,
latest_height: Height,
frozen_height: Height,
proof_specs: ProofSpecs,
upgrade_path: Vec<String>,
allow_update: AllowUpdate,
) -> Result<ClientState, Error> {
Expand Down Expand Up @@ -99,6 +100,7 @@ impl ClientState {
max_clock_drift,
frozen_height,
latest_height,
proof_specs,
hu55a1n1 marked this conversation as resolved.
Show resolved Hide resolved
upgrade_path,
allow_update,
})
Expand Down Expand Up @@ -228,6 +230,7 @@ impl TryFrom<RawClientState> for ClientState {
after_expiry: raw.allow_update_after_expiry,
after_misbehaviour: raw.allow_update_after_misbehaviour,
},
proof_specs: raw.proof_specs.into(),
})
}
}
Expand All @@ -242,7 +245,7 @@ impl From<ClientState> for RawClientState {
max_clock_drift: Some(value.max_clock_drift.into()),
frozen_height: Some(value.frozen_height.into()),
latest_height: Some(value.latest_height.into()),
proof_specs: ProofSpecs::cosmos().into(),
proof_specs: value.proof_specs.into(),
allow_update_after_expiry: value.allow_update.after_expiry,
allow_update_after_misbehaviour: value.allow_update.after_misbehaviour,
upgrade_path: value.upgrade_path,
Expand All @@ -261,6 +264,7 @@ mod tests {

use crate::clients::ics07_tendermint::client_state::{AllowUpdate, ClientState};
use crate::core::ics02_client::trust_threshold::TrustThreshold;
use crate::core::ics23_commitment::specs::ProofSpecs;
use crate::core::ics24_host::identifier::ChainId;
use crate::test::test_serialization_roundtrip;
use crate::timestamp::ZERO_DURATION;
Expand Down Expand Up @@ -293,6 +297,7 @@ mod tests {
max_clock_drift: Duration,
latest_height: Height,
frozen_height: Height,
proof_specs: ProofSpecs,
upgrade_path: Vec<String>,
allow_update: AllowUpdate,
}
Expand All @@ -306,6 +311,7 @@ mod tests {
max_clock_drift: Duration::new(3, 0),
latest_height: Height::new(0, 10),
frozen_height: Height::default(),
proof_specs: ProofSpecs::default(),
upgrade_path: vec!["".to_string()],
allow_update: AllowUpdate {
after_expiry: false,
Expand Down Expand Up @@ -373,6 +379,7 @@ mod tests {
p.max_clock_drift,
p.latest_height,
p.frozen_height,
p.proof_specs,
p.upgrade_path,
p.allow_update,
);
Expand Down Expand Up @@ -414,6 +421,7 @@ pub mod test_util {
u64::from(tm_header.height),
),
Height::zero(),
Default::default(),
vec!["".to_string()],
AllowUpdate {
after_expiry: false,
Expand Down
2 changes: 2 additions & 0 deletions modules/src/core/ics02_client/handler/create_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ mod tests {
use crate::core::ics02_client::msgs::create_client::MsgCreateAnyClient;
use crate::core::ics02_client::msgs::ClientMsg;
use crate::core::ics02_client::trust_threshold::TrustThreshold;
use crate::core::ics23_commitment::specs::ProofSpecs;
use crate::core::ics24_host::identifier::ClientId;
use crate::events::IbcEvent;
use crate::handler::HandlerOutput;
Expand Down Expand Up @@ -230,6 +231,7 @@ mod tests {
max_clock_drift: Duration::from_millis(3000),
latest_height: Height::new(0, u64::from(tm_header.height)),
frozen_height: Height::zero(),
proof_specs: ProofSpecs::default(),
allow_update: AllowUpdate {
after_expiry: false,
after_misbehaviour: false,
Expand Down
5 changes: 3 additions & 2 deletions modules/src/core/ics23_commitment/commitment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,12 @@ impl Serialize for CommitmentPrefix {
pub mod test_util {
use crate::prelude::*;
use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof;
use ibc_proto::ics23::CommitmentProof;

/// Returns a dummy `RawMerkleProof`, for testing only!
pub fn get_dummy_merkle_proof() -> RawMerkleProof {
let parsed = ibc_proto::ics23::CommitmentProof { proof: None };
let mproofs: Vec<ibc_proto::ics23::CommitmentProof> = vec![parsed];
let parsed = CommitmentProof { proof: None };
let mproofs: Vec<CommitmentProof> = vec![parsed];
RawMerkleProof { proofs: mproofs }
}
}
48 changes: 38 additions & 10 deletions modules/src/core/ics23_commitment/specs.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::prelude::*;
use ibc_proto::ics23::ProofSpec as ProtoProofSpec;
use ics23::ProofSpec;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just FYI, with regard to #853 - this file (i.e. modules/src/core/ics23_commitment/specs.rs) is the only place where we use the ics23 crate. We only use it to get the definitions for the proof-specs for cosmos and to provide convert::From impls for our domain type (i.e. ProofSpecs). For all other cases, we use the ibc_proto::ics23 defs because we need to derive serde and other traits for these types.

use serde::{Deserialize, Serialize};

/// An array of proof specifications.
///
Expand All @@ -9,19 +10,23 @@ use ics23::ProofSpec;
/// Additionally, this type also aids in the conversion from `ProofSpec` types from crate `ics23`
/// into proof specifications as represented in the `ibc_proto` type; see the
/// `From` trait(s) below.
pub struct ProofSpecs {
specs: Vec<ProofSpec>,
}
#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
pub struct ProofSpecs(Vec<ProtoProofSpec>);

impl ProofSpecs {
/// Returns the specification for Cosmos-SDK proofs
pub fn cosmos() -> Self {
Self {
specs: vec![
ics23::iavl_spec(), // Format of proofs-iavl (iavl merkle proofs)
ics23::tendermint_spec(), // Format of proofs-tendermint (crypto/ merkle SimpleProof)
],
}
vec![
ics23::iavl_spec(), // Format of proofs-iavl (iavl merkle proofs)
ics23::tendermint_spec(), // Format of proofs-tendermint (crypto/ merkle SimpleProof)
]
.into()
}
}

impl Default for ProofSpecs {
fn default() -> Self {
Self::cosmos()
}
}

Expand All @@ -31,7 +36,7 @@ impl ProofSpecs {
impl From<ProofSpecs> for Vec<ProtoProofSpec> {
fn from(domain_specs: ProofSpecs) -> Self {
let mut raw_specs = Vec::new();
for ds in domain_specs.specs.iter() {
for ds in domain_specs.0.iter() {
// Both `ProofSpec` types implement trait `prost::Message`. Convert by encoding, then
// decoding into the destination type.
// Safety note: the source and target data structures are identical, hence the
Expand All @@ -44,3 +49,26 @@ impl From<ProofSpecs> for Vec<ProtoProofSpec> {
raw_specs
}
}

impl From<Vec<ProofSpec>> for ProofSpecs {
fn from(proto_specs: Vec<ProofSpec>) -> Self {
let mut specs = Vec::new();
for ds in proto_specs {
// Both `ProofSpec` types implement trait `prost::Message`. Convert by encoding, then
romac marked this conversation as resolved.
Show resolved Hide resolved
// decoding into the destination type.
// Safety note: the source and target data structures are identical, hence the
// encode/decode conversion here should be infallible.
let mut encoded = Vec::new();
prost::Message::encode(&ds, &mut encoded).unwrap();
let decoded: ProtoProofSpec = prost::Message::decode(&*encoded).unwrap();
specs.push(decoded);
}
Self(specs)
}
}

impl From<Vec<ProtoProofSpec>> for ProofSpecs {
fn from(specs: Vec<ProtoProofSpec>) -> Self {
Self(specs)
}
}
2 changes: 1 addition & 1 deletion proto-compiler/src/cmd/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,4 @@ fn checkout_tag(repo: &Repository, tag_name: &str) -> Result<(), git2::Error> {
}

Ok(())
}
}
8 changes: 6 additions & 2 deletions proto-compiler/src/cmd/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,18 @@ impl CompileCmd {

// List available paths for dependencies
let includes: Vec<PathBuf> = proto_includes_paths.iter().map(PathBuf::from).collect();
let attrs_serde_eq = "#[derive(::serde::Serialize, ::serde::Deserialize, Eq)]";

let compilation = tonic_build::configure()
.build_client(true)
.build_server(false)
.format(true)
.out_dir(out_dir)
.extern_path(".tendermint", "::tendermint_proto")
.type_attribute(".ics23.LeafOp", attrs_serde_eq)
.type_attribute(".ics23.InnerOp", attrs_serde_eq)
.type_attribute(".ics23.ProofSpec", attrs_serde_eq)
.type_attribute(".ics23.InnerSpec", attrs_serde_eq)
.compile(&protos, &includes);

match compilation {
Expand Down Expand Up @@ -139,7 +144,6 @@ impl CompileCmd {
format!("{}/proto/cosmos/upgrade", sdk_dir.display()),
];


let mut proto_includes_paths = vec![
format!("{}/../proto", root),
format!("{}/proto", sdk_dir.display()),
Expand All @@ -148,7 +152,7 @@ impl CompileCmd {

if let Some(ibc_dir) = ibc_dep {
// Use the IBC proto files from the SDK
proto_includes_paths.push(format!("{}/proto", ibc_dir.display()),);
proto_includes_paths.push(format!("{}/proto", ibc_dir.display()));
}

// List available proto files
Expand Down
1 change: 1 addition & 0 deletions proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ prost-types = "0.9"
bytes = "1.1"
tonic = "0.6"
getrandom = { version = "0.2", features = ["js"] }
serde = "1.0.130"
romac marked this conversation as resolved.
Show resolved Hide resolved

[dependencies.tendermint-proto]
version = "=0.23.0"
8 changes: 4 additions & 4 deletions proto/src/prost/ics23.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub mod commitment_proof {
///
///Then combine the bytes, and hash it
///output = hash(prefix || length(hkey) || hkey || length(hvalue) || hvalue)
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(::serde::Serialize, ::serde::Deserialize, Eq, Clone, PartialEq, ::prost::Message)]
pub struct LeafOp {
#[prost(enumeration = "HashOp", tag = "1")]
pub hash: i32,
Expand Down Expand Up @@ -110,7 +110,7 @@ pub struct LeafOp {
///Any special data, like prepending child with the length, or prepending the entire operation with
///some value to differentiate from leaf nodes, should be included in prefix and suffix.
///If either of prefix or suffix is empty, we just treat it as an empty string
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(::serde::Serialize, ::serde::Deserialize, Eq, Clone, PartialEq, ::prost::Message)]
pub struct InnerOp {
#[prost(enumeration = "HashOp", tag = "1")]
pub hash: i32,
Expand All @@ -130,7 +130,7 @@ pub struct InnerOp {
///generate a given hash (by interpretting the preimage differently).
///We need this for proper security, requires client knows a priori what
///tree format server uses. But not in code, rather a configuration object.
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(::serde::Serialize, ::serde::Deserialize, Eq, Clone, PartialEq, ::prost::Message)]
pub struct ProofSpec {
/// any field in the ExistenceProof must be the same as in this spec.
/// except Prefix, which is just the first bytes of prefix (spec can be longer)
Expand All @@ -154,7 +154,7 @@ pub struct ProofSpec {
///isLeftMost(spec: InnerSpec, op: InnerOp)
///isRightMost(spec: InnerSpec, op: InnerOp)
///isLeftNeighbor(spec: InnerSpec, left: InnerOp, right: InnerOp)
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(::serde::Serialize, ::serde::Deserialize, Eq, Clone, PartialEq, ::prost::Message)]
pub struct InnerSpec {
/// Child order is the ordering of the children node, must count from 0
/// iavl tree is [0, 1] (left then right)
Expand Down
1 change: 1 addition & 0 deletions relayer/src/chain/cosmos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1927,6 +1927,7 @@ impl ChainEndpoint for CosmosSdkChain {
max_clock_drift,
height,
ICSHeight::zero(),
self.config.proof_specs.clone(),
vec!["upgrade".to_string(), "upgradedIBCState".to_string()],
AllowUpdate {
after_expiry: true,
Expand Down
4 changes: 3 additions & 1 deletion relayer/src/chain/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use ibc::core::ics03_connection::connection::{ConnectionEnd, IdentifiedConnectio
use ibc::core::ics04_channel::channel::{ChannelEnd, IdentifiedChannelEnd};
use ibc::core::ics04_channel::context::ChannelReader;
use ibc::core::ics04_channel::packet::{PacketMsgType, Sequence};
use ibc::core::ics23_commitment::commitment::CommitmentPrefix;
use ibc::core::ics23_commitment::{commitment::CommitmentPrefix, specs::ProofSpecs};
use ibc::core::ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId};
use ibc::downcast;
use ibc::events::IbcEvent;
Expand Down Expand Up @@ -356,6 +356,7 @@ impl ChainEndpoint for MockChain {
self.config.clock_drift + dst_config.clock_drift + dst_config.max_block_time,
height,
Height::zero(),
ProofSpecs::default(),
vec!["upgrade/upgradedClient".to_string()],
AllowUpdate {
after_expiry: false,
Expand Down Expand Up @@ -468,6 +469,7 @@ pub mod test_utils {
packet_filter: PacketFilter::default(),
address_type: AddressType::default(),
memo_prefix: Default::default(),
proof_specs: Default::default(),
}
}
}
Loading