From 2ee213254fcbaf258d177926dda5e7737896b344 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Mon, 16 Jan 2023 17:28:50 +0900 Subject: [PATCH] cosmwasm: accounting: Don't store signature data on-chain When submitting observations to the accounting contract, clients sign the entire batch once. There's no point storing this signature in the on-chain data for each observation because it's already stored as part of the chain's transaction history and the signature would be different if an observation was submitted as part of a different batch (or the same batch in a different order) even if the observation itself didn't change. Also, nothing actually made use of this signature data. (Yes, technically it was returned by some queries but the usefulness of the signature by itself is questionable without the full batch of observations that were signed). All we really care about is the index of the guardian anyway so use a bitset to keep track of the indices of all the guardians that have signed an observation. We use a u128 for the bitset out of an abundance of caution in case the number of guardians increases in the future. Dealing with more than 128 guardians is left as a problem for future wormhole contributors if we ever get to that point. --- .../schema/wormchain-accounting.json | 96 ++----------------- .../wormchain-accounting/src/contract.rs | 11 +-- .../wormchain-accounting/src/state.rs | 86 +++++++++++------ .../tests/submit_observations.rs | 13 +-- 4 files changed, 74 insertions(+), 132 deletions(-) diff --git a/cosmwasm/contracts/wormchain-accounting/schema/wormchain-accounting.json b/cosmwasm/contracts/wormchain-accounting/schema/wormchain-accounting.json index 4be6f5c40..b3cc0aa5c 100644 --- a/cosmwasm/contracts/wormchain-accounting/schema/wormchain-accounting.json +++ b/cosmwasm/contracts/wormchain-accounting/schema/wormchain-accounting.json @@ -705,10 +705,9 @@ "$ref": "#/definitions/Observation" }, "signatures": { - "type": "array", - "items": { - "$ref": "#/definitions/Signature" - } + "type": "integer", + "format": "uint128", + "minimum": 0.0 } }, "additionalProperties": false @@ -806,31 +805,6 @@ }, "additionalProperties": false }, - "Signature": { - "description": "Signatures are typical ECDSA signatures prefixed with a Guardian position. These have the following byte layout: ```markdown 0 .. 64: Signature (ECDSA) 64 .. 65: Recovery ID (ECDSA) ```", - "type": "object", - "required": [ - "index", - "signature" - ], - "properties": { - "index": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "signature": { - "type": "array", - "items": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "maxItems": 65, - "minItems": 65 - } - } - }, "TokenAddress": { "type": "string" } @@ -997,10 +971,9 @@ "$ref": "#/definitions/Observation" }, "signatures": { - "type": "array", - "items": { - "$ref": "#/definitions/Signature" - } + "type": "integer", + "format": "uint128", + "minimum": 0.0 } }, "additionalProperties": false @@ -1079,31 +1052,6 @@ }, "additionalProperties": false }, - "Signature": { - "description": "Signatures are typical ECDSA signatures prefixed with a Guardian position. These have the following byte layout: ```markdown 0 .. 64: Signature (ECDSA) 64 .. 65: Recovery ID (ECDSA) ```", - "type": "object", - "required": [ - "index", - "signature" - ], - "properties": { - "index": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "signature": { - "type": "array", - "items": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "maxItems": 65, - "minItems": 65 - } - } - }, "TokenAddress": { "type": "string" }, @@ -1363,10 +1311,9 @@ "$ref": "#/definitions/Observation" }, "signatures": { - "type": "array", - "items": { - "$ref": "#/definitions/Signature" - } + "type": "integer", + "format": "uint128", + "minimum": 0.0 } }, "additionalProperties": false @@ -1420,31 +1367,6 @@ } }, "additionalProperties": false - }, - "Signature": { - "description": "Signatures are typical ECDSA signatures prefixed with a Guardian position. These have the following byte layout: ```markdown 0 .. 64: Signature (ECDSA) 64 .. 65: Recovery ID (ECDSA) ```", - "type": "object", - "required": [ - "index", - "signature" - ], - "properties": { - "index": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "signature": { - "type": "array", - "items": { - "type": "integer", - "format": "uint8", - "minimum": 0.0 - }, - "maxItems": 65, - "minItems": 65 - } - } } } }, diff --git a/cosmwasm/contracts/wormchain-accounting/src/contract.rs b/cosmwasm/contracts/wormchain-accounting/src/contract.rs index ad5a42783..e4302c265 100644 --- a/cosmwasm/contracts/wormchain-accounting/src/contract.rs +++ b/cosmwasm/contracts/wormchain-accounting/src/contract.rs @@ -109,11 +109,6 @@ fn submit_observations( let quorum = deps .querier .query::(&WormholeQuery::CalculateQuorum { guardian_set_index }.into()) - .and_then(|q| { - usize::try_from(q).map_err(|_| StdError::ConversionOverflow { - source: ConversionOverflowError::new("u32", "usize", q.to_string()), - }) - }) .context("failed to calculate quorum")?; let observations: Vec = @@ -159,7 +154,7 @@ fn handle_observation( mut deps: DepsMut, o: Observation, guardian_set_index: u32, - quorum: usize, + quorum: u32, sig: Signature, ) -> anyhow::Result<(ObservationStatus, Option)> { let digest_key = DIGESTS.key((o.emitter_chain, o.emitter_address.to_vec(), o.sequence)); @@ -194,9 +189,9 @@ fn handle_observation( } }; - data.add_signature(sig)?; + data.add_signature(sig.index); - if data.signatures().len() < quorum { + if data.num_signatures() < quorum { // Still need more signatures so just save the pending transfer data and exit. key.save(deps.storage, &pending) .context("failed to save pending transfers")?; diff --git a/cosmwasm/contracts/wormchain-accounting/src/state.rs b/cosmwasm/contracts/wormchain-accounting/src/state.rs index bb05d2d4a..f1b4563eb 100644 --- a/cosmwasm/contracts/wormchain-accounting/src/state.rs +++ b/cosmwasm/contracts/wormchain-accounting/src/state.rs @@ -2,9 +2,7 @@ use accounting::state::transfer; use cosmwasm_schema::cw_serde; use cosmwasm_std::Binary; use cw_storage_plus::Map; -use thiserror::Error; use tinyvec::TinyVec; -use wormhole::vaa::Signature; use crate::msg::Observation; @@ -18,18 +16,12 @@ pub struct PendingTransfer { pub data: Vec, } -#[derive(Error, Debug)] -#[error("cannot submit duplicate signatures for the same observation")] -pub struct DuplicateSignatureError; - #[cw_serde] #[derive(Default)] pub struct Data { observation: Observation, - guardian_set_index: u32, - - signatures: Vec, + signatures: u128, } impl Data { @@ -37,7 +29,7 @@ impl Data { Self { observation, guardian_set_index, - signatures: Vec::new(), + signatures: 0, } } @@ -49,29 +41,69 @@ impl Data { self.guardian_set_index } - pub fn signatures(&self) -> &[Signature] { - &self.signatures + pub fn signatures(&self) -> u128 { + self.signatures + } + + /// Returns the number of signatures for this `Data`. + pub fn num_signatures(&self) -> u32 { + self.signatures.count_ones() } /// Returns true if there is a signature associated with `index` in this `Data`. pub fn has_signature(&self, index: u8) -> bool { - self.signatures - .binary_search_by_key(&index, |s| s.index) - .is_ok() + assert!(index < 128); + self.signatures & (1u128 << index) != 0 } - /// Adds `sig` to the list of signatures for this transfer data. Returns true if `sig` - /// was successfully added or false if `sig` was already in the signature list. - pub fn add_signature(&mut self, sig: Signature) -> Result<(), DuplicateSignatureError> { - match self - .signatures - .binary_search_by_key(&sig.index, |s| s.index) - { - Ok(_) => Err(DuplicateSignatureError), - Err(idx) => { - self.signatures.insert(idx, sig); - Ok(()) - } + /// Adds `sig` to the list of signatures for this `Data`. + pub fn add_signature(&mut self, index: u8) { + assert!(index < 128); + self.signatures |= 1u128 << index; + } +} + +#[cfg(test)] +mod test { + use super::*; + + use std::panic::catch_unwind; + + #[test] + fn add_signatures() { + let mut data = Data::default(); + for i in 0..128 { + assert!(!data.has_signature(i)); + data.add_signature(i); + assert!(data.has_signature(i)); + assert_eq!(u32::from(i + 1), data.num_signatures()); + } + } + + #[test] + fn add_signatures_rev() { + let mut data = Data::default(); + for i in (0..128).rev() { + assert!(!data.has_signature(i)); + data.add_signature(i); + assert!(data.has_signature(i)); + assert_eq!(u32::from(128 - i), data.num_signatures()); + } + } + + #[test] + fn has_out_of_bounds_signature() { + for i in 128..=u8::MAX { + catch_unwind(|| Data::default().has_signature(i)) + .expect_err("successfully checked for out-of-bounds signature"); + } + } + + #[test] + fn add_out_of_bounds_signature() { + for i in 128..=u8::MAX { + catch_unwind(|| Data::default().add_signature(i)) + .expect_err("successfully added out-of-bounds signature"); } } } diff --git a/cosmwasm/contracts/wormchain-accounting/tests/submit_observations.rs b/cosmwasm/contracts/wormchain-accounting/tests/submit_observations.rs index 417dbad9a..99f26e09f 100644 --- a/cosmwasm/contracts/wormchain-accounting/tests/submit_observations.rs +++ b/cosmwasm/contracts/wormchain-accounting/tests/submit_observations.rs @@ -166,6 +166,8 @@ fn duplicates() { for (i, s) in signatures.into_iter().enumerate() { contract.submit_observations(obs.clone(), index, s).unwrap(); + // Submitting a duplicate signature is not an error for pending transfers. Submitting any + // signature for a committed transfer is not an error as long as the digests match. let resp = contract.submit_observations(obs.clone(), index, s).unwrap(); let status = from_binary::>(&resp.data.unwrap()) .unwrap() @@ -176,18 +178,9 @@ fn duplicates() { // Resubmitting the same signature without quorum will return an error. for o in &observations { let key = transfer::Key::new(o.emitter_chain, o.emitter_address.into(), o.sequence); - if let ObservationStatus::Error(ref err) = status[&key] { - assert!(err.contains("duplicate signatures")); - } else { - panic!( - "unexpected status for duplicate signature: {:?}", - status[&key] - ); - } + assert!(matches!(status[&key], ObservationStatus::Pending)); } } else { - // Submitting a signature for a committed transfer is not an error as long as the - // digests match. for o in &observations { let key = transfer::Key::new(o.emitter_chain, o.emitter_address.into(), o.sequence); assert!(matches!(status[&key], ObservationStatus::Committed));