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

scroll-dev-1019 -> v3.0.0 #19

Draft
wants to merge 1 commit into
base: upstream/v3.0.0
Choose a base branch
from
Draft

Conversation

lispc
Copy link

@lispc lispc commented Oct 19, 2024

it is too difficult to keep history.. since upstream make a single huge commit with 20k lines of new codes.. for history, check #18

known issues

  1. VERIFY_VK=false now. Seems we should regenerate vk and shape bin files?
  2. double check we don't use SP1_DEV=true in real envs

refactor

useless files, delete them?

  1. crates/zkvm/entrypoint/src/memcpy.c. (we should only use memcpy syscall inside poseidon. general memcpy syscall at most gives 2% prover performance. Not worthy. )
  2. crates/core/machine/src/syscall/precompiles/bn254_scalar/general_field_op.rs (an old design, not used)

features needed

  1. add WarpBn254 for enum SP1Proof? convenient for our sp1-halo2 wrapper.

@lispc lispc changed the title scroll-dev-1019: squash all old diffs scroll-dev-1019 -> v3.0.0 Oct 19, 2024
Comment on lines +54 to +56
#[allow(unused_variables)]
#[no_mangle]
pub extern "C" fn syscall_bn254_scalar_mul(p: *mut u32, q: *const u32) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

*p *= *q


#[allow(unused_variables)]
#[no_mangle]
pub extern "C" fn syscall_bn254_scalar_mac(ret: *mut u32, a: *const u32, b: *const u32) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAC means multiply-accumulate.

*ret += (*a)*(*b).

Comment on lines +57 to +59
fn num_extra_cycles(&self) -> u32 {
1
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In offline memory check,

  • each memory read operation will put one read record (addr, value, prev_cycle) in the read set, one write record (addr, value, cycle) in the write set. prev_cycle < cycle is enforced.
  • each memory write operation will put one read record (addr, prev_value, prev_cycle) in the read set, one write record (addr, value, cycle) in the write set. prev_cycle < cycle is also enforced.

If p == q, q is accessed in cycle; then p will be accessed in cycle+1.

Comment on lines +30 to +32
fn num_extra_cycles(&self) -> u32 {
1
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • The address of scalar field element a is located at q[0];
  • The address of scalar field element b is located at q[1];

We should return 3 as num_extra_cycles() because we read 4 times into memory as any of them can overlap with each other.

channel: T,
nonce: T,
clk: T,
p_ptr: T,
Copy link
Collaborator

Choose a reason for hiding this comment

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

p_ptr is the value in register X10. And q_ptr is the value in register X11.

Comment on lines +236 to +241
let a_ptr = arg2.0[0..4]
.iter()
.rev()
.cloned()
.map(|v| v.into())
.fold(AB::Expr::zero(), |acc, b| acc * AB::Expr::from_canonical_u16(0x100) + b);
Copy link
Collaborator

Choose a reason for hiding this comment

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

reconstruct the value of a_ptr from 4 bytes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

warning: a[0] + a[1]*2^8 + a[2]*2^16 + a[3]*2^24 might overflow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use the BabyBearWordRangeChecker to ensure that a_ptr is always a valid baby bear word like CPU chip did for memory access opcodes.


builder.eval_memory_access_slice(
local.shard,
local.clk.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be clk + 3.


builder.eval_memory_access_slice(
local.shard,
local.clk.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should clk + 2.

Comment on lines +228 to +234
builder.eval_memory_access_slice(
local.shard,
local.clk.into(),
local.arg2_ptr,
&local.arg2_access,
local.is_real,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move up to L220.


builder.eval_memory_access_slice(
local.shard,
local.clk.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be clk + 2.

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