Rust: load_trusted_setup() API shouldnt care about BYTES_PER_*_POINT #440
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Right now it's only reth who uses When we merge Regardless of this PR they will most likely have to make changes anyway so that they load the |
jtraglia
reviewed
Jun 23, 2024
bindings/rust/src/bindings/mod.rs
Outdated
| return Err(Error::InvalidTrustedSetup(format!( | ||
| "Invalid number of g1 monomial points in trusted setup. Expected {} got {}", | ||
| FIELD_ELEMENTS_PER_BLOB, | ||
| g1_monomial_bytes.len() | ||
| ))); | ||
| } | ||
| if g1_lagrange_bytes.len() != FIELD_ELEMENTS_PER_BLOB { | ||
| if g1_lagrange_bytes.len() != BYTES_PER_BLOB { |
There was a problem hiding this comment.
For these two checks, I think I would prefer to use FIELD_ELEMENTS_PER_BLOB*BYTES_PER_G1_POINT for clarity and consistency with the G2 points check.
bindings/rust/src/bindings/mod.rs
Outdated
Comment on lines
98
to
100
| /// Initializes a trusted setup from a flat array of `FIELD_ELEMENTS_PER_BLOB` G1 points in | ||
| /// Lagrange form, a flat array of 65 G2 points in monomial form, and a flat array of | ||
| /// `FIELD_ELEMENTS_PER_BLOB` G1 points in monomial form. |
There was a problem hiding this comment.
This comment isn't exactly correct.
It says g1 Lagrange, g2 monomial, g1 monomial.
It should be g1 monomial, g1 Lagrange, then g2 monomial.
asn-d6
added a commit
that referenced
this pull request
Jul 18, 2024
* Add initial implementation of DAS functions * Add rust bindings for DAS functions * Refactor names for types/consts * For go bindings, use heap and pointers * Add verify_sample_proof_batch function I have included Go bindings for this new method. I will add Rust bindings later. This will break the Rust bindings build until then, sorry about that. * Fix a few nits in new function * Add more comments * Add new batch func to Rust bindings * Add multi blob test for batch func * Start to add java bindings * Add batch verification method to java bindings * Use more complex sample structure The sample structure now includes the data, proof, and indices. This should make things easier to read and understand. Bindings will be broken for a little while. Going to remove the 2d functions, as I don't think they are that useful at the moment. * Update go bindings * Clean up go bindings a little * Delete 2d functions * Update java bindings * Use appropriate style in java bindings * Remove references to BLOB_COUNT * Add samplesToBlob and recoverSamples to java bindings * Improve samples_to_blob * Replace calloc with JNI allocation * Rename two functions * Rename SAMPLE_SIZE to FIELD_ELEMENTS_PER_SAMPLE * Update rust bindings * Fix comment * Use fast g1 lincomb implementation * Rename sample to cell * Fix comment indentations for cell params * Add pub methods for cell and export das items (#386) * Add benchmark for verifying columns * Remove complex cell type * Add helper functions for Cell * Remove reference to old BLOB_COUNT variable * Make Cell::new const * Add func to profile compute_cells_and_proofs * Change scale factor from 5 to 7 * Check that recovered cells match input cells * Add methods to cell to retrieve byte representations (#387) * Correct offset for cell 2D array construction (#388) * Rename DATA_POINTS_PER_BLOB -> FIELD_ELEMENTS_PER_EXT_BLOB * Add BITS_PER_FIELD_ELEMENT constant * Revert back to simpler cells_to_blob impl * Add compute_cells() func to bindings * Go back to non-pointer params in Go bindings * Fix minor typo * Replace multiple memory allocs with one * Refactor fk20_multi_da_opt * Speed up proof generation by 2x * Clean things up a little * Fix some minor mistakes dealing with refactor * Use fixed-based MSM for proof generation * Compute random challenge for verify cell proof batch * Filter out zero points in g1_lincomb_fast * Use bool array to check if values are used * Fix minor typo * Allocate polys & use consistent ordering * Add node.js bindings for DAS functions * Remove duplicate constant * Add das functions to c_kzg_4844_jni.h * Use verifyCellProofBatch domain from spec * Rename rowIds & columnIds to *Indices to match spec * Rename recover_cells to recover_polynomial * Fix bug in new g1_lincomb_fast * Add initial set of peerdas reference tests * Fix benchmarks in go bindings * Add VerifyRows benchmark * Rename recover_polynomial to recover_all_cells * Rename CELLS_PER_BLOB to CELLS_PER_EXT_BLOB * Update reference tests * Make some minor fixups in function comments * Clean up verify_kzg_proof_multi_impl * Add new reference tests for Rust bindings * Remove old rust test * Start to add das funcs to python bindings * Add python bindings for compute_cells* * Add python bindings for verify_cell_proof* * Address a few minor nits * Run das reference tests on java bindings * Run das reference tests on nodejs bindings Also, fix a mistake in its recoverAllCells binding! * Begin to add c# das bindings * Add compute methods for c# * Add recoverAllCells method for C# * Update reference tests * Rename "proof" to "kzg_proof" in das functions * Add extra null checks in free_trusted_setup * Fix typo (ftt -> fft) * Begin to add das funcs to nim bindings * Add other das functions for nim bindings * Update uppercase constants * Fix memleak in poly_lagrange_to_monomial * Add function doc and remove poly_monomial_to_lagrange This removed function is not used in production code, only testing code. I don't think it's worthy of a test, so I removed that too. * Convert interpolation_poly from poly_t to fr_t* * Add explicit duplication check in recover_all_cells * Improve scale_poly helper funcs * Revert scale factor swap * Use g1_lincomb_naive in verify_kzg_proof_batch * Simplify fft_g1_fast with iterative implementation * Revert "Simplify fft_g1_fast with iterative implementation" This reverts commit 783ad62. After further consideration, I don't really like it. * Remove da_using_fk20_multi and use less poly_t * Use more FIELD_ELEMENTS_PER_EXT_BLOB * Compute fixed-base MSM scratch size in init_fk20_multi_settings * Clean up function docs * Add inf check to fft_g1 to save 1ms * [nodejs] Update docs & assign alpha version (#421) * feat: enable exceptions * docs: update peerDas comments for publishing * chore: update package version for publish * docs: update links on nodejs README.md --------- Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com> * Update versions in preparation for release (#428) * Make precomputation optional/configurable * Address asn feedback * Change recover_all_cells from Vec to fixed array * Box recover_all_cells return * Skip vector allocation, only do fixed array * Add null check for 2d coeffs before freeing * Add parallel bench test * Add cellsToBlob & computeCells to Nim bindings * Clean up some function docs * Add G1 points in monomial form to the trusted setup This commit will speed up loading the trusted setup by about ~1.5 seconds, the time it takes to convert the G1 points in Lagrange form to monomial form. These values were copied from the trusted setup in the consensus specs. We added the new points to the bottom of the trusted setup so that it is backwards compatiable with previous versions of c-kzg-4844. So clients could start using this trusted setup whenever they want. * Move no-recovery-needed check further down * Rename g1/g2 fields in trusted setup * Fix proof computation bug with other FIELD_ELEMENTS_PER_CELL configs In the precompute=0 path, we were using `k` when we should have been using `FIELD_ELEMENTS_PER_CELL`. When `FIELD_ELEMENTS_PER_CELL` is 64, `k` is also 64. But when `FIELD_ELEMENTS_PER_CELL` is 16, `k` is `256` (wrong). I noticed this when testing out `FIELD_ELEMENTS_PER_CELL` set to 16. Sometimes the tests would pass and sometimes they would fail; weird. After some debugging, I spotted the problem. * Begin to add rust fuzzing * Add differential fuzzer with peerdas-kzg * Add more differential fuzzers * Create corpus dir if it doesn't exist * Fix typo in corpus name * Add more eip7594 fuzzers * Remove duplicate allocation This was causing a memory leak. The first allocation pointer was being overwritten and never freed. * Change recover_all_cells to recover_cells_and_kzg_proofs This adds a convenient function for clients to use. This is not yet exposed to clients. We provide a null value for proofs which tells C to not recover proofs, keeping the same functionality as recover_all_cells. For now. * Check ctx in computeCellsAndProofs and recoverProofs * Add RecoverCellsAndKZGProofs func for Go Warning, the C code is pretty ugly, not documented, and may contain bugs. I expect we will make changes to this in the future. Given <= 3 missing proofs, this method will recompute individual proofs, otherwise it will recompute all of them. See the code comments for a more in-depth rationale. * Remove unused variables * Clean up recover_all_cells_impl some * Do some more clean up * Various clean up * [nim] Correct params for verifyProof (#433) * Move coset_for_cell * Add recover_cells_and_kzg_proofs to Rust bindings * Add recover_cells_and_kzg_proofs to Python bindings * Add recover_cells_and_kzg_proofs to Java bindings * Replace some uint64_t with size_t * Add recover_cells_and_kzg_proofs to Node bindings * Add recover_cells_and_kzg_proofs for Nim bindings * Quick & dirty fix for cell fromHex in Nim * Convert Cell to flat array of bytes Instead of an array of Bytes32, just an array of bytes. * Remove to_bytes() usages in generate-fuzz-corpus code * Fix Go's Cell.UnmarshalText function * Do not expose all constants * Remove old references to recover_all_cells in C * Update rustfuzz * Clean up Rust bindings * Add new recover_cells_and_kzg_proofs tests * Remove cells_to_blob * Update c# reference tests * Update java reference tests * Update python reference tests * Update nodejs reference tests * Update nim reference tests * Delete some recover_all_cells bindings * Update rust reference tests * Remove compute_cells * Specify precompute in base_fuzz.h (#435) * Make updates to rustfuzz * Add new Rust files * Make CELLS_PER_EXT_BLOB in Java protected * some minor improvements of comments * Improve documentation of verify_cell_kzg_proof * Improve documentation and variable names in verify_kzg_proof_multi_impl * Change vanishing poly name to Z(X) + wrap lines in comments later * [rust] Change load_trusted_setup to take flat byte arrays (#440) * In recover_cells_and_kzg_proofs, always do FK20 (#442) * Run C auto-formatter & fix some nits * Update project README * Add 4844 functions to rustfuzz * Fix documentation formatting (#446) * Remove proofs param from recover_cells_and_kzg_proofs * Replace fuzz with rustfuzz * Add new fuzz directory * Update copyright years * Update ref tests & rename cell id to cell index * Remove unused commitment filtering * Run cargo update in fuzz * Change fuzz corpus path from rustfuzz to fuzz * Add profiling funcs for 7594 methods * Start to fuzz against Constantine * Fuzz 4844 funcs against Constantine * Rid ourselves of the CHECK macro * Fix various documentation nits * DAS: Implement coset FFT and use it around the codebase (#449) * Remove idea of point from load_trusted_setup params * In batch cell verification, take commitment for each cell (#451) * Remove verify_cell_kzg_proof * Remove verify_cell_kzg_proof tests * Rename column_indices to cell_indices * Don't use `tmp` variable in `c_kzg_calloc` * Fix Fiat-Shamir computation in verify_cell_kzg_proof_batch() * Fix function docs * Move batch cell verification helper funcs up * Clean up compute_r_powers_for_verify_cell_kzg_proof_batch * Revert "Clean up compute_r_powers_for_verify_cell_kzg_proof_batch" This reverts commit dced524. * Use consistent ordering of new EIP-7594 funcs * Reorder fuzzers * Rename "cas" to "caps" * Clean up java bindings * Remove ComputeCellsTest * Remove G*Data types * Fix some nits * Revert changes to poly_to_kzg_commitment * Revert accidental blst downgrade * Add newline to trusted setup * Replace min with MIN macro to fix name conflict * Add missing constants to Rust benchmarks * Replace __SIZEOF_POINTER__ with sizeof(void *) * Add debug prints to C# test * Add new functions to csharp/ckzg.def * Add debug print for C#'s TestRecoverCellsAndKzgProofs * Change param types for recover from byte * to byte* * Add debug prints * Add more info to the exception * Change ulong * to ulong* * Add more debug prints * Use ulong instead of int * Remove debug prints * Use TestCaseSource for new C# tests * Improve function docs for C and C# * Clarify doc comment * Replace cellCount with len(cells) * Remove unused include in Java bindings * Add C test for verify_cell_kzg_proof_batch * Clean up C tests some more * Make CellsPerExtBlob in Go bindings public I believe this affects how clients see the return types. It appears as a slice instead of an array of a known size. This isn't right. * Revert "Make CellsPerExtBlob in Go bindings public" This reverts commit e689135. * Revert fuzz changes * Revert changes to Rust bindings for new fuzzer * Revert test_files.iter() to test_files * g1_lincomb_fast(): Simplify point at infinity weed out (#454) * Fix formatting * Apply review feedback to Nim's load_trusted_setup * Make some constants public again --------- Co-authored-by: Jacob Kaufmann <jacobkaufmann18@gmail.com> Co-authored-by: Matthew Keil <me@matthewkeil.com> Co-authored-by: Agnish Ghosh <80243668+agnxsh@users.noreply.github.com> Co-authored-by: Antonio Sanso <antonio.sanso@gmail.com> Co-authored-by: b-wagn <crypto@benedikt-wagner.dev> Co-authored-by: George Kadianakis <desnacked@riseup.net> Co-authored-by: Benedikt Wagner <113296072+b-wagn@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rust clients had to know the
BYTES_PER_G1_POINTandBYTES_PER_G2_POINTconstants to be able to useload_trusted_setup(). Ideally this would not be the case. The clients should not be exposed to cryptographic details.This small modification makes the function take flat byte vectors as input which is also what the c-kzg backend expects. This way Rust clients don't need to chunk bytes into group points themselves, they just supply the flat byte vector and c-kzg takes care of the rest.