From d9ccf0dbba21cbddad462d5384958d41335ff678 Mon Sep 17 00:00:00 2001 From: Ian Joiner <14581281+iajoiner@users.noreply.github.com> Date: Mon, 11 Nov 2024 16:27:20 -0500 Subject: [PATCH 1/2] fix!: address NITs in PR #361 --- crates/proof-of-sql/src/base/database/mod.rs | 3 - .../src/base/database/test_accessor.rs | 60 +------------------ .../proof-of-sql/src/sql/proof/proof_plan.rs | 11 +--- .../src/sql/proof/query_proof_test.rs | 5 +- .../src/sql/proof/verifiable_query_result.rs | 12 +++- .../sql/proof/verifiable_query_result_test.rs | 6 +- .../src/sql/proof_plans/dyn_proof_plan.rs | 3 +- 7 files changed, 18 insertions(+), 82 deletions(-) diff --git a/crates/proof-of-sql/src/base/database/mod.rs b/crates/proof-of-sql/src/base/database/mod.rs index eb726b930..a8cc8fcd2 100644 --- a/crates/proof-of-sql/src/base/database/mod.rs +++ b/crates/proof-of-sql/src/base/database/mod.rs @@ -81,9 +81,6 @@ pub use expression_evaluation_error::{ExpressionEvaluationError, ExpressionEvalu mod test_accessor; pub use test_accessor::TestAccessor; -#[cfg(test)] -#[allow(unused_imports)] -pub(crate) use test_accessor::UnimplementedTestAccessor; #[cfg(test)] mod test_schema_accessor; diff --git a/crates/proof-of-sql/src/base/database/test_accessor.rs b/crates/proof-of-sql/src/base/database/test_accessor.rs index 177e27cbb..5b7827ac8 100644 --- a/crates/proof-of-sql/src/base/database/test_accessor.rs +++ b/crates/proof-of-sql/src/base/database/test_accessor.rs @@ -1,11 +1,6 @@ -use super::{ - Column, ColumnRef, ColumnType, CommitmentAccessor, DataAccessor, MetadataAccessor, - SchemaAccessor, TableRef, -}; -use crate::base::{commitment::Commitment, scalar::Curve25519Scalar}; +use super::{CommitmentAccessor, DataAccessor, MetadataAccessor, SchemaAccessor, TableRef}; +use crate::base::commitment::Commitment; use alloc::vec::Vec; -use curve25519_dalek::ristretto::RistrettoPoint; -use proof_of_sql_parser::Identifier; /// A trait that defines the interface for a combined metadata, schema, commitment, and data accessor for unit testing or example purposes. pub trait TestAccessor: @@ -31,54 +26,3 @@ pub trait TestAccessor: /// Update the table offset alongside its column commitments fn update_offset(&mut self, table_ref: TableRef, new_offset: usize); } - -#[derive(Clone, Default)] -/// A test accessor that leaves all of the required methods except `new` `unimplemented!()`. -pub struct UnimplementedTestAccessor; -impl TestAccessor for UnimplementedTestAccessor { - type Table = (); - - fn new_empty() -> Self { - UnimplementedTestAccessor - } - - fn add_table(&mut self, _table_ref: TableRef, _data: (), _table_offset: usize) { - unimplemented!() - } - - fn get_column_names(&self, _table_ref: TableRef) -> Vec<&str> { - unimplemented!() - } - - fn update_offset(&mut self, _table_ref: TableRef, _new_offset: usize) { - unimplemented!() - } -} -impl DataAccessor for UnimplementedTestAccessor { - fn get_column(&self, _column: ColumnRef) -> Column { - unimplemented!() - } -} -impl CommitmentAccessor for UnimplementedTestAccessor { - fn get_commitment(&self, _column: ColumnRef) -> RistrettoPoint { - unimplemented!() - } -} -impl MetadataAccessor for UnimplementedTestAccessor { - fn get_length(&self, _table_ref: TableRef) -> usize { - unimplemented!() - } - - fn get_offset(&self, _table_ref: TableRef) -> usize { - unimplemented!() - } -} -impl SchemaAccessor for UnimplementedTestAccessor { - fn lookup_column(&self, _table_ref: TableRef, _column_id: Identifier) -> Option { - unimplemented!() - } - - fn lookup_schema(&self, _table_ref: TableRef) -> Vec<(Identifier, ColumnType)> { - unimplemented!() - } -} diff --git a/crates/proof-of-sql/src/sql/proof/proof_plan.rs b/crates/proof-of-sql/src/sql/proof/proof_plan.rs index d2cda0b80..ac73f183e 100644 --- a/crates/proof-of-sql/src/sql/proof/proof_plan.rs +++ b/crates/proof-of-sql/src/sql/proof/proof_plan.rs @@ -2,8 +2,7 @@ use super::{CountBuilder, FinalRoundBuilder, FirstRoundBuilder, VerificationBuil use crate::base::{ commitment::Commitment, database::{ - Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, MetadataAccessor, - OwnedTable, TableRef, + Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, OwnedTable, TableRef, }, map::IndexSet, proof::ProofError, @@ -19,14 +18,6 @@ pub trait ProofPlan: Debug + Send + Sync + ProverEvaluate { /// Count terms used within the Query's proof fn count(&self, builder: &mut CountBuilder) -> Result<(), ProofError>; - /// Check if the input table is empty - fn is_empty(&self, accessor: &dyn MetadataAccessor) -> bool { - let table_refs = self.get_table_references(); - table_refs - .iter() - .all(|table_ref| accessor.get_length(*table_ref) == 0) - } - /// Form components needed to verify and proof store into `VerificationBuilder` fn verifier_evaluate( &self, diff --git a/crates/proof-of-sql/src/sql/proof/query_proof_test.rs b/crates/proof-of-sql/src/sql/proof/query_proof_test.rs index 96db177d5..22046721f 100644 --- a/crates/proof-of-sql/src/sql/proof/query_proof_test.rs +++ b/crates/proof-of-sql/src/sql/proof/query_proof_test.rs @@ -112,7 +112,7 @@ fn verify_a_trivial_query_proof_with_given_offset(n: usize, offset_generators: u let column: Vec = vec![0_i64; n]; let accessor = OwnedTableTestAccessor::::new_from_table( "sxt.test".parse().unwrap(), - owned_table([bigint("a1", column)]), + owned_table([bigint("a1", column.clone())]), offset_generators, (), ); @@ -122,8 +122,7 @@ fn verify_a_trivial_query_proof_with_given_offset(n: usize, offset_generators: u table, } = proof.verify(&expr, &accessor, &result, &()).unwrap(); assert_ne!(verification_hash, [0; 32]); - let expected_col = vec![0_i64; n]; - let expected_result = owned_table([bigint("a1", expected_col)]); + let expected_result = owned_table([bigint("a1", column)]); assert_eq!(table, expected_result); } diff --git a/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs b/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs index d3faf4b06..de535b316 100644 --- a/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs +++ b/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs @@ -88,7 +88,11 @@ impl VerifiableQueryResult { // have been rejected at the parsing stage. // handle the empty case - if expr.is_empty(accessor) { + let table_refs = expr.get_table_references(); + if table_refs + .iter() + .all(|table_ref| accessor.get_length(*table_ref) == 0) + { return VerifiableQueryResult { provable_result: None, proof: None, @@ -124,7 +128,11 @@ impl VerifiableQueryResult { // have been rejected at the parsing stage. // handle the empty case - if expr.is_empty(accessor) { + let table_refs = expr.get_table_references(); + if table_refs + .iter() + .all(|table_ref| accessor.get_length(*table_ref) == 0) + { if self.provable_result.is_some() || self.proof.is_some() { return Err(ProofError::VerificationError { error: "zero sumcheck variables but non-empty result", diff --git a/crates/proof-of-sql/src/sql/proof/verifiable_query_result_test.rs b/crates/proof-of-sql/src/sql/proof/verifiable_query_result_test.rs index ab739c372..03c07837b 100644 --- a/crates/proof-of-sql/src/sql/proof/verifiable_query_result_test.rs +++ b/crates/proof-of-sql/src/sql/proof/verifiable_query_result_test.rs @@ -91,10 +91,9 @@ fn we_can_verify_queries_on_an_empty_table() { columns: 1, ..Default::default() }; - let column: Vec = vec![0_i64; 0]; let accessor = OwnedTableTestAccessor::::new_from_table( "sxt.test".parse().unwrap(), - owned_table([bigint("a1", column)]), + owned_table([bigint("a1", [0_i64; 0])]), 0, (), ); @@ -113,10 +112,9 @@ fn empty_verification_fails_if_the_result_contains_non_null_members() { columns: 1, ..Default::default() }; - let column: Vec = vec![0_i64; 0]; let accessor = OwnedTableTestAccessor::::new_from_table( "sxt.test".parse().unwrap(), - owned_table([bigint("a1", column)]), + owned_table([bigint("a1", [0_i64; 0])]), 0, (), ); diff --git a/crates/proof-of-sql/src/sql/proof_plans/dyn_proof_plan.rs b/crates/proof-of-sql/src/sql/proof_plans/dyn_proof_plan.rs index 490867ecb..51d5bb7c1 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/dyn_proof_plan.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/dyn_proof_plan.rs @@ -3,8 +3,7 @@ use crate::{ base::{ commitment::Commitment, database::{ - Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, MetadataAccessor, - OwnedTable, TableRef, + Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, OwnedTable, TableRef, }, map::IndexSet, proof::ProofError, From d74ef29ae7d5c334db2ee32afe83f32225a8fc1c Mon Sep 17 00:00:00 2001 From: Ian Joiner <14581281+iajoiner@users.noreply.github.com> Date: Tue, 12 Nov 2024 10:50:32 -0500 Subject: [PATCH 2/2] fix: consume `table_refs` in `verifiable_query_result.rs` --- .../proof-of-sql/src/sql/proof/verifiable_query_result.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs b/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs index de535b316..10eb50989 100644 --- a/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs +++ b/crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs @@ -90,8 +90,8 @@ impl VerifiableQueryResult { // handle the empty case let table_refs = expr.get_table_references(); if table_refs - .iter() - .all(|table_ref| accessor.get_length(*table_ref) == 0) + .into_iter() + .all(|table_ref| accessor.get_length(table_ref) == 0) { return VerifiableQueryResult { provable_result: None, @@ -130,8 +130,8 @@ impl VerifiableQueryResult { // handle the empty case let table_refs = expr.get_table_references(); if table_refs - .iter() - .all(|table_ref| accessor.get_length(*table_ref) == 0) + .into_iter() + .all(|table_ref| accessor.get_length(table_ref) == 0) { if self.provable_result.is_some() || self.proof.is_some() { return Err(ProofError::VerificationError {