certusone
/
wormhole
mirror of https://github.com/certusone/wormhole.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Chirantan Ekbote 2023-01-16 17:28:50 +09:00 committed by Evan Gray
parent 289d37771d
commit 2ee213254f
4 changed files with 74 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
cosmwasm/contracts/wormchain-accounting/schema/wormchain-accounting.json
View File

@ -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
}
}
}
}
},

11
cosmwasm/contracts/wormchain-accounting/src/contract.rs
View File

@ -109,11 +109,6 @@ fn submit_observations(
let quorum = deps
.querier
.query::<u32>(&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<Observation> =
@ -159,7 +154,7 @@ fn handle_observation(
mut deps: DepsMut<WormholeQuery>,
o: Observation,
guardian_set_index: u32,
quorum: usize,
quorum: u32,
sig: Signature,
) -> anyhow::Result<(ObservationStatus, Option<Event>)> {
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")?;

86
cosmwasm/contracts/wormchain-accounting/src/state.rs
View File

@ -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<Data>,
}
#[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<Signature>,
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");
}
}
}

13
cosmwasm/contracts/wormchain-accounting/tests/submit_observations.rs
View File

@ -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::<Vec<SubmitObservationResponse>>(&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));