From 871426afde8bf8f414c52abb3eea3820435122c4 Mon Sep 17 00:00:00 2001 From: Jan Ferdinand Sauer Date: Wed, 4 Dec 2024 16:15:57 +0100 Subject: [PATCH] feat: Introduce snippet sign-off functionality --- tasm-lib/src/test_helpers.rs | 6 + tasm-lib/src/traits/basic_snippet.rs | 247 +++++++++++++++++++++++++-- 2 files changed, 241 insertions(+), 12 deletions(-) diff --git a/tasm-lib/src/test_helpers.rs b/tasm-lib/src/test_helpers.rs index 084b45a4..64718df7 100644 --- a/tasm-lib/src/test_helpers.rs +++ b/tasm-lib/src/test_helpers.rs @@ -11,6 +11,7 @@ use crate::execute_with_terminal_state; use crate::exported_snippets; use crate::library::Library; use crate::traits::basic_snippet::BasicSnippet; +use crate::traits::basic_snippet::SignedOffSnippet; use crate::traits::deprecated_snippet::DeprecatedSnippet; use crate::traits::rust_shadow::RustShadow; use crate::InitVmState; @@ -414,6 +415,11 @@ pub fn test_rust_equivalence_given_complete_state( sponge: &Option, expected_final_stack: Option<&[BFieldElement]>, ) -> VMState { + shadowed_snippet + .inner() + .borrow() + .assert_all_sign_offs_are_up_to_date(); + let init_stack = stack.to_vec(); let rust = rust_final_state(shadowed_snippet, stack, stdin, nondeterminism, sponge); diff --git a/tasm-lib/src/traits/basic_snippet.rs b/tasm-lib/src/traits/basic_snippet.rs index 5cd1f6d9..517a859d 100644 --- a/tasm-lib/src/traits/basic_snippet.rs +++ b/tasm-lib/src/traits/basic_snippet.rs @@ -1,10 +1,22 @@ +use std::collections::HashMap; +use std::fmt::Display; +use std::fmt::Formatter; +use std::hash::Hash; +use std::hash::Hasher; + use num_traits::Zero; +use triton_vm::isa::instruction::AnInstruction; use triton_vm::isa::op_stack::NUM_OP_STACK_REGISTERS; use triton_vm::prelude::*; use crate::data_type::DataType; use crate::library::Library; +/// ### Dyn-Compatibility +/// +/// This trait is [dyn-compatible] (previously known as “object safe”). +/// +/// [dyn-compatible]: https://doc.rust-lang.org/reference/items/traits.html#object-safety pub trait BasicSnippet { fn inputs(&self) -> Vec<(DataType, String)>; fn outputs(&self) -> Vec<(DataType, String)>; @@ -122,24 +134,191 @@ pub trait BasicSnippet { } diff } + + /// Contains an entry for every sign off. + /// + /// Many of the snippets defined in this TASM library are critical for the + /// consensus logic of the blockchain [Neptune Cash](https://neptune.cash). + /// Therefore, it is paramount that the snippets are free of errors. In order + /// to catch as many errors as possible, the snippets are reviewed by as many + /// developers as possible. The requirements of such a review are listed here. + /// + /// A reviewer can (and should) sign off on any snippet they have reviewed and + /// for which they found no defects. This is done by adding that snippet's + /// [fingerprint] (at the time) to the overriding implementation of this method + /// on that snippet. + /// + /// Together with the tools [`git blame`][blame] and cryptographic + /// [signing] of commits, this makes sign-offs traceable. It also guarantees + /// that changes to snippets that have already been signed-off are easy to + /// detect. + /// + /// # For Reviewers + /// + /// ## Modifying snippets + /// + /// While the primary intention of the review process is to _review_ a snippet, + /// there are circumstances under which modifying it is acceptable. + /// + /// Modifying a snippet to simplify reviewing that snippet is fair game. A + /// common example of this case is replacing a `swap`-juggle chain with a few + /// `pick`s & `place`s. + /// + /// Modifying a snippet in order to improve performance should only happen if + /// the performance impact is meaningful. The currently agreed-upon threshold + /// is 0.5% of at least one consensus program. + /// + /// It is acceptable, and can be desired, to modify a snippet by including + /// assumption checks. For example, if the snippet's pre-conditions require + /// some input to fall within a certain range, it is fine to add a corresponding + /// range check to the snippet. + /// Removing existing checks of such nature is considered bad practice. + /// + /// In either case, modifying a snippet that has already been reviewed and + /// signed off by someone else in a way that alters its [fingerprint] requires + /// their consent. + /// + /// ## Checklist + /// + /// Use the following checklist to guide your review. Signing off on a snippet + /// means that in your eyes, all points on this checklist are true. + /// + /// - the snippet's documentation lists pre- and post-conditions + /// - the snippet makes no assumptions outside the stated pre-conditions + /// - given all pre-conditions, all post-conditions are met + /// - whenever this snippet calls another snippet, all of that other snippet's + /// pre-conditions are met + /// - all dynamic memory offsets are range-checked before they are used + /// - each field accessor is used at most once per struct instance, or + /// range-checked before each use + /// - reading from non-deterministically initialized memory only happens from + /// the region specified in the [memory convention] + /// - memory-writes only happen outside of page 0 (see [memory convention]) + /// + /// ## Non-Unit Structs + /// + /// Most, but not all types implementing [BasicSnippet] are unit structs. + /// [Fingerprinting][fingerprint] gets more difficult for non-unit structs. + /// In such cases, a default instantiation should be selected and signed off. + /// + /// ## Overriding this Method + /// + /// This default implementation _is_ intended to be overridden for any snippet + /// that has been signed off, but _should not_ call the [fingerprint] method. + /// + /// [fingerprint]: SignedOffSnippet::fingerprint + /// [memory convention]: crate::memory + /// [blame]: https://git-scm.com/docs/git-blame + /// [signing]: https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work + fn sign_offs(&self) -> HashMap { + HashMap::default() + } +} + +/// Extension trait for [`BasicSnippet`] related to +/// [signing off](BasicSnippet::sign_offs). Contains methods that are callable, +/// but for which the provided default implementation cannot be overridden. +/// +/// ### Dyn-Compatibility +/// +/// This trait is [dyn-compatible] (previously known as “object safe”). +/// +/// [dyn-compatible]: https://doc.rust-lang.org/reference/items/traits.html#object-safety +// +// Because `$[final]` trait methods are in pre-RFC phase [0], and trait +// sealing [1] would be clumsy, use this workaround. +// +// [0]: https://internals.rust-lang.org/t/pre-rfc-final-trait-methods/18407 +// [1]: https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust +pub trait SignedOffSnippet: BasicSnippet { + /// The unique fingerprint as used for [signing off][BasicSnippet::sign_offs] on + /// this snippet. + fn fingerprint(&self) -> SignOffFingerprint { + let mut hasher = std::hash::DefaultHasher::new(); + triton_vm::proof::CURRENT_VERSION.hash(&mut hasher); + + for instruction in self.code(&mut Library::new()) { + let LabelledInstruction::Instruction(instruction) = instruction else { + continue; + }; + + if let AnInstruction::Call(_) = instruction { + AnInstruction::Call("").opcode().hash(&mut hasher); + } else { + instruction.hash(&mut hasher) + } + } + + SignOffFingerprint(hasher.finish()) + } + + /// Panics if any [sign-offs](BasicSnippet::sign_offs) disagree with the actual + /// [fingerprint](Self::fingerprint). + fn assert_all_sign_offs_are_up_to_date(&self) { + let fingerprint = self.fingerprint(); + let mut out_of_date_sign_offs = self + .sign_offs() + .into_iter() + .filter(|(_, fp)| fp != &fingerprint) + .peekable(); + + if out_of_date_sign_offs.peek().is_none() { + return; + } + + let name = self.entrypoint(); + for (reviewer, fp) in out_of_date_sign_offs { + eprintln!("reviewer {reviewer} of snippet “{name}” has signed off on fingerprint {fp}") + } + panic!("A sign-off is out of date. Current fingerprint of “{name}”: {fingerprint}"); + } +} + +// Blanket implementation conflicts with any other implementation, making the +// provided defaults final. +impl SignedOffSnippet for T {} + +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct Reviewer(pub &'static str); + +impl Display for Reviewer { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +/// A fingerprint as used for [signing off][BasicSnippet::sign_offs] snippets. +/// +/// While this fingerprint can be used to distinguish [`BasicSnippets`], it is +/// not cryptographically secure. +#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct SignOffFingerprint(pub(crate) u64); + +impl Display for SignOffFingerprint { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{:x}", self.0) + } +} + +impl From for SignOffFingerprint { + fn from(value: u64) -> Self { + Self(value) + } } #[cfg(test)] mod tests { - use triton_vm::prelude::triton_asm; - use triton_vm::prelude::Program; - - use crate::traits::basic_snippet::BasicSnippet; + use super::*; #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] struct DummySnippet; impl BasicSnippet for DummySnippet { - fn inputs(&self) -> Vec<(crate::data_type::DataType, String)> { + fn inputs(&self) -> Vec<(DataType, String)> { vec![] } - fn outputs(&self) -> Vec<(crate::data_type::DataType, String)> { + fn outputs(&self) -> Vec<(DataType, String)> { vec![] } @@ -147,10 +326,7 @@ mod tests { "____dummy_snippet_test".to_string() } - fn code( - &self, - _library: &mut crate::library::Library, - ) -> Vec { + fn code(&self, _library: &mut Library) -> Vec { triton_asm!( {self.entrypoint()}: push 14 @@ -168,9 +344,56 @@ mod tests { let calculated_init_stack = DummySnippet.init_stack_for_isolated_run(); let program = DummySnippet.link_for_isolated_run(); let program = Program::new(&program); - let init_vm_state = - triton_vm::vm::VMState::new(program, Default::default(), Default::default()); + let init_vm_state = VMState::new(program, Default::default(), Default::default()); assert_eq!(init_vm_state.op_stack.stack, calculated_init_stack); } + + #[test] + fn defined_traits_are_dyn_compatible() { + fn basic_snippet_is_dyn_compatible(snippet: Box) { + snippet.fingerprint(); + } + + fn signed_off_snippet_is_dyn_compatible(snippet: Box) { + snippet.fingerprint(); + } + + basic_snippet_is_dyn_compatible(Box::new(DummySnippet)); + signed_off_snippet_is_dyn_compatible(Box::new(DummySnippet)); + } + + macro_rules! dummy_snippet { + ($name:ident: $($instr:tt)+) => { + struct $name; + + impl BasicSnippet for $name { + fn inputs(&self) -> Vec<(DataType, String)> { vec![] } + fn outputs(&self) -> Vec<(DataType, String)> { vec![] } + fn entrypoint(&self) -> String { + stringify!($name).to_ascii_lowercase() + } + + fn code(&self, _: &mut Library) -> Vec { + triton_asm!($($instr)+) + } + } + }; + } + + #[test] + fn snippets_differing_only_in_call_targets_have_identical_fingerprints() { + dummy_snippet!(SomeLabel: call some_label); + dummy_snippet!(OtherLabel: call other_label); + + assert_eq!(SomeLabel.fingerprint(), OtherLabel.fingerprint()); + } + + #[test] + fn snippets_differing_only_in_instruction_args_have_different_fingerprints() { + dummy_snippet!(Push20: push 20); + dummy_snippet!(Push42: push 42); + + assert_ne!(Push20.fingerprint(), Push42.fingerprint()); + } }