Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Precompile Backend #562

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Conversation

Roee-87
Copy link
Contributor

@Roee-87 Roee-87 commented Jan 16, 2025

The purpose of this PR is to add code to support precompiles in the zkVM.

if self.precompile_outputs.len() <= internal_address {
self.precompile_outputs.resize(internal_address + 1, 0);
}

self.outputs[internal_address] = value;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store method only takes one value as an input. Should there be additional Option inputs for precompile input and output?


let mut virtual_trace = vec![];

let precompile_output: &[u32; 16] = &[0u32; 16]; // compute precompile output based on t0 register
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the precompile output going to be stored in rs1?

@Roee-87 Roee-87 marked this pull request as ready for review February 6, 2025 08:53
Copy link
Collaborator

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I think we're almost there

pub input:[u32; 16] // 512 bits
pub output:[u32; 16] // 512 bits
}
struct PrecompileWitness {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's instead make PrecompileWitness an associated type on the Precompile trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added PrecompileWitness and PrecompileProof as associated types with an example implementation (for PrecompileWitness) in the Bn254.rs file: 37da785

Should PrecompileIO also be an associated type? Or will it be identical for all precompiles?

Comment on lines 1154 to 1165
/// Gets the precompile input from the correct location in memory designated for precompile inputs.
pub fn get_precompile_input_word(&mut self, v_address: u64) -> Result<u32, Trap> {
// Load the world from virtual memory.
let input_word = self.load_word(v_address);
input_word
}

/// Sets the precompile output to the correct location in memory designated for precompile outputs.
pub fn set_precompile_output_word(&mut self, output_word: u32, p_address: u64) {
// Store the word in the correct precompile output memory location
self.store_word(p_address, output_word);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we should do here is use self.jolt_device.memory_layout (which contains the start/end addresses of the precompile input/output regions) to determine where to read/write the precompile inputs/outputs.

That way, we don't need to pass in v_address or p_address to these methods; we'll always read/write 512 bits (16 words) from the respective memory regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed using fields in memory_layout: 0782e6e

Comment on lines 2523 to 2527
let precompile_input_address = 0_u64; // @TODO: which address should be supplied to fetch the input?
let inputs = cpu.mmu.get_precompile_input(precompile_input_address); // which address should be supplied to fetch the input?
let output = precompile.execute(input);
let precompile_output_address = 0_u64; // @TODO: which address should be supplied to store the output?
cpu.get_mut_mmu.set_precompile_output(output, precompile_output_address);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: which addresses to supply, see comment below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set using memory_layout struct: 0782e6e


let mut virtual_trace = vec![];

let precompile_output: &[u32; 16] = &[0u32; 16]; // compute precompile output based on t0 register
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we'll just be writing the precompile output to memory

Separately, we'll need to invoke the precompile (e.g. bn254_add) here on the input.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I guess we'll need to add some fields on RVTraceRow to capture the inputs during tracing. Namely, we should add an Option<[u32; 16]> for precompile inputs
And we'll probably need an Option<u64> for the precompile output address

Comment on lines 84 to 85
pub fn prove(&self, witness: PrecompileWitness) -> JoltProof; // Need to include Preprocessing struct as an input?
pub fn verify(&self, proof: JoltProof) -> Result<(), ProofVerifyError>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of JoltProof, I think we'll need another associated type PrecompileProof

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to make any changes to this file for the time being; the main prove function can just invoke the individual precompile provers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted changes: 5407655

Comment on lines 22 to 24
// Ecall source registers
let r_x = trace_row.instruction.rs1;
let r_y = trace_row.instruction.rs2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should only be on source register (t0), specifying which precompile to execute

Comment on lines 25 to 42
// Virtual registers used in sequence
let v_0 = Some(virtual_register_index(0));
let v_ao0: Option<u64> = Some(virtual_register_index(1));
let v_ao1: Option<u64> = Some(virtual_register_index(2));
let v_ao2: Option<u64> = Some(virtual_register_index(3));
let v_ao3: Option<u64> = Some(virtual_register_index(4));
let v_ao4: Option<u64> = Some(virtual_register_index(5));
let v_ao5: Option<u64> = Some(virtual_register_index(6));
let v_ao6: Option<u64> = Some(virtual_register_index(7));
let v_ao7: Option<u64> = Some(virtual_register_index(8));
let v_ao8: Option<u64> = Some(virtual_register_index(9));
let v_ao9: Option<u64> = Some(virtual_register_index(10));
let v_ao10: Option<u64> = Some(virtual_register_index(11));
let v_ao11: Option<u64> = Some(virtual_register_index(12));
let v_ao12: Option<u64> = Some(virtual_register_index(13));
let v_ao13: Option<u64> = Some(virtual_register_index(14));
let v_ao14: Option<u64> = Some(virtual_register_index(15));
let v_ao15: Option<u64> = Some(virtual_register_index(16));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't need any virtual registers, as we'll be writing the outputs to memory, not registers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed and updated the virtual instruction to write the relevant precompile output word to memory: 8864710

virtual_trace.push(RVTraceRow {
instruction: ELFInstruction {
address: trace_row.instruction.address,
opcode: RV32IM::VIRTUAL_ADVICE,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was wrong before; we will in fact need a new RV32IM::VIRTUAL_ADVICE_SW instruction that writes a word to memory, as opposed to VIRTUAL_ADVICE which writes to a register. This also entails some changes in rv_trace.rs for the new instruction which we can talk about

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants