Skip to content

Commit

Permalink
refactor: move serialisation out of ValidatorSigner (#12422)
Browse files Browse the repository at this point in the history
Currently we add a method to `ValidatorSigner` for every data type we
want to sign. I'm not sure what exactly is a benefit of doing that, but
such approach results in quite a few annoying issues:
* Overly verbose code: need to add 3 methods for every data type to be
signed.
* Not consistent with signature verification: we just call `verify`
method on public key passing signature and bytes, why should signing be
any different?
* It blows up `ValidatorSigner` public API making it awkward to work
with.

This PR repurposes `sign_chunk_contract_accesses` to be a generic
`sign_bytes` method and replaces usage for witness contract code
distribution related types. The rest will be addressed in a separate PR.
  • Loading branch information
pugachAG authored Nov 11, 2024
1 parent 780cb81 commit f963732
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 86 deletions.
4 changes: 2 additions & 2 deletions chain/network/src/network_protocol/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl VersionedAccountData {
MAX_ACCOUNT_DATA_SIZE_BYTES
);
}
let signature = signer.sign_account_key_payload(&payload);
let signature = signer.sign_bytes(&payload);
Ok(SignedAccountData {
account_data: self,
payload: AccountKeySignedPayload { payload, signature },
Expand Down Expand Up @@ -273,7 +273,7 @@ impl OwnedAccount {
"OwnedAccount.account_key doesn't match the signer's account_key"
);
let payload = proto::AccountKeyPayload::from(&self).write_to_bytes().unwrap();
let signature = signer.sign_account_key_payload(&payload);
let signature = signer.sign_bytes(&payload);
SignedOwnedAccount {
owned_account: self,
payload: AccountKeySignedPayload { payload, signature },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ impl ChunkContractAccessesV1 {
signer: &ValidatorSigner,
) -> Self {
let inner = ChunkContractAccessesInner::new(next_chunk, contracts, main_transition);
let signature = signer.sign_chunk_contract_accesses(&inner);
let signature = signer.sign_bytes(&borsh::to_vec(&inner).unwrap());
Self { inner, signature }
}

Expand Down Expand Up @@ -190,7 +190,7 @@ impl ContractCodeRequestV1 {
contracts,
main_transition,
);
let signature = signer.sign_contract_code_request(&inner);
let signature = signer.sign_bytes(&borsh::to_vec(&inner).unwrap());
Self { inner, signature }
}

Expand Down Expand Up @@ -466,7 +466,7 @@ impl PartialEncodedContractDeploysV1 {
signer: &ValidatorSigner,
) -> Self {
let inner = PartialEncodedContractDeploysInner::new(key, part);
let signature = signer.sign_partial_encoded_contract_deploys(&inner);
let signature = signer.sign_bytes(&borsh::to_vec(&inner).unwrap());
Self { inner, signature }
}

Expand Down
90 changes: 9 additions & 81 deletions core/primitives/src/validator_signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ use crate::sharding::ChunkHash;
use crate::stateless_validation::chunk_endorsement::{
ChunkEndorsementInner, ChunkEndorsementMetadata,
};
use crate::stateless_validation::contract_distribution::{
ChunkContractAccessesInner, ContractCodeRequestInner, PartialEncodedContractDeploysInner,
};
use crate::stateless_validation::partial_witness::PartialEncodedStateWitnessInner;
use crate::stateless_validation::state_witness::EncodedChunkStateWitness;
use crate::telemetry::TelemetryInfo;
Expand Down Expand Up @@ -149,49 +146,10 @@ impl ValidatorSigner {
}
}

/// Signs the inner contents of a ChunkContractAccesses message.
pub fn sign_chunk_contract_accesses(&self, inner: &ChunkContractAccessesInner) -> Signature {
match self {
ValidatorSigner::Empty(signer) => signer.sign_chunk_contract_accesses(inner),
ValidatorSigner::InMemory(signer) => signer.sign_chunk_contract_accesses(inner),
}
}

pub fn sign_partial_encoded_contract_deploys(
&self,
inner: &PartialEncodedContractDeploysInner,
) -> Signature {
pub fn sign_bytes(&self, data: &[u8]) -> Signature {
match self {
ValidatorSigner::Empty(signer) => signer.sign_partial_encoded_contract_deploys(inner),
ValidatorSigner::InMemory(signer) => {
signer.sign_partial_encoded_contract_deploys(inner)
}
}
}

/// Signs the inner contents of a ContractCodeRequest message.
pub fn sign_contract_code_request(&self, inner: &ContractCodeRequestInner) -> Signature {
match self {
ValidatorSigner::Empty(signer) => signer.sign_contract_code_request(inner),
ValidatorSigner::InMemory(signer) => signer.sign_contract_code_request(inner),
}
}

/// Signs a proto-serialized AccountKeyPayload (see
/// chain/network/src/network_protocol/network.proto).
/// Making it typesafe would require moving the definition of
/// AccountKeyPayload proto to this crate to avoid a dependency cycle,
/// so for now we are just signing an already-serialized byte sequence.
/// We are serializing a proto rather than borsh here (as an experiment,
/// to allow the network protocol to evolve faster than on-chain stuff),
/// but we can always revert that decision, because these signatures are
/// used only for networking purposes and are not persisted on chain.
/// Moving to proto serialization for stuff stored on chain would be way
/// harder.
pub fn sign_account_key_payload(&self, proto_bytes: &[u8]) -> Signature {
match self {
ValidatorSigner::Empty(signer) => signer.sign_account_key_payload(proto_bytes),
ValidatorSigner::InMemory(signer) => signer.sign_account_key_payload(proto_bytes),
ValidatorSigner::Empty(signer) => signer.noop_signature(),
ValidatorSigner::InMemory(signer) => signer.sign_bytes(data),
}
}

Expand Down Expand Up @@ -247,6 +205,10 @@ impl EmptyValidatorSigner {
PublicKey::empty(KeyType::ED25519)
}

fn noop_signature(&self) -> Signature {
Signature::default()
}

fn sign_telemetry(&self, _info: &TelemetryInfo) -> serde_json::Value {
serde_json::Value::default()
}
Expand Down Expand Up @@ -300,25 +262,6 @@ impl EmptyValidatorSigner {
) -> Signature {
Signature::default()
}

fn sign_account_key_payload(&self, _proto_bytes: &[u8]) -> Signature {
Signature::default()
}

fn sign_chunk_contract_accesses(&self, _inner: &ChunkContractAccessesInner) -> Signature {
Signature::default()
}

fn sign_partial_encoded_contract_deploys(
&self,
_inner: &PartialEncodedContractDeploysInner,
) -> Signature {
Signature::default()
}

fn sign_contract_code_request(&self, _inner: &ContractCodeRequestInner) -> Signature {
Signature::default()
}
}

/// Signer that keeps secret key in memory and signs locally.
Expand Down Expand Up @@ -420,23 +363,8 @@ impl InMemoryValidatorSigner {
self.signer.sign(hash.as_ref())
}

fn sign_account_key_payload(&self, proto_bytes: &[u8]) -> Signature {
self.signer.sign(proto_bytes)
}

fn sign_chunk_contract_accesses(&self, inner: &ChunkContractAccessesInner) -> Signature {
self.signer.sign(&borsh::to_vec(inner).unwrap())
}

fn sign_partial_encoded_contract_deploys(
&self,
inner: &PartialEncodedContractDeploysInner,
) -> Signature {
self.signer.sign(&borsh::to_vec(inner).unwrap())
}

fn sign_contract_code_request(&self, inner: &ContractCodeRequestInner) -> Signature {
self.signer.sign(&borsh::to_vec(inner).unwrap())
fn sign_bytes(&self, bytes: &[u8]) -> Signature {
self.signer.sign(bytes)
}

fn compute_vrf_with_proof(
Expand Down

0 comments on commit f963732

Please sign in to comment.