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

Fix DynVolatileRamTable::max_len #716

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

matthiasgoergens
Copy link
Collaborator

@matthiasgoergens matthiasgoergens commented Dec 9, 2024

The previous logic only ever worked for powers of two.

@matthiasgoergens
Copy link
Collaborator Author

I think I can fix this one relatively easily via something like max_size.next_power_of_two().div_ceil(WORD_SIZE as u32). But need to confirm that this is a problem, first.

params.platform.ram.start = 0;
params.platform.ram.end = 100;
let max_len_bytes = 4 * DynMemTable::max_len(&params);
assert_eq!(max_len_bytes, 128);
Copy link
Collaborator

Choose a reason for hiding this comment

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

correct result is 64. In dynamic memory case, we want to get prev_power_of_2

Copy link
Collaborator Author

@matthiasgoergens matthiasgoergens Dec 9, 2024

Choose a reason for hiding this comment

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

Could you please help me craft a comment to explain the logic we want here?

What is the supposed meaning of the result of the trait function max_len? It seems rather confusing to me.

It looks like it should be the number of rows we need to initialise the memory? Or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just illustrate with pic
image

next_pow2 is just to padding memory in used per proof to make sumcheck implementation work
prev_pow2 is to make (end - offset) aligned with power of 2. So the maximal in used mem can't exceed this range.

So current max_len logic fit above design, so we dont need this PR actually

Copy link
Collaborator Author

@matthiasgoergens matthiasgoergens Dec 10, 2024

Choose a reason for hiding this comment

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

Thanks for the explanation. You are pointing to a flaw in our design.

See in ceno_emul we define our 'platform':

/// The Platform struct holds the parameters of the VM.
/// It defines:
/// - the layout of virtual memory,
/// - special addresses, such as the initial PC,
/// - codes of environment calls.
#[derive(Clone, Debug)]
pub struct Platform {
    pub rom: Range<Addr>,
    pub ram: Range<Addr>,
    pub public_io: Range<Addr>,
    pub hints: Range<Addr>,
    pub stack_top: Addr,
    /// If true, ecall instructions are no-op instead of trap. Testing only.
    pub unsafe_ecall_nop: bool,
}

The doc comment suggests that everything in Platform::ram is available for read-write memory. And nothing in ceno_emul contradicts that.

However the logic in ceno_zkvm that you kindly explained means that in general, ie for ram sizes not powers of two, a program using the promised ram will run just fine in the emulator, but will fail to prove.

In the spriti of #630 I suggest we change the circuits so that an execution staying strictly within the promised platform capabilities will both run and prove.

(Concretely here, if the platform promises 100 bytes of RAM, we should deliver at least 100 bytes of RAM, not 64.)

Please correct me, if I am wrong.

Copy link
Collaborator

@hero78119 hero78119 Dec 10, 2024

Choose a reason for hiding this comment

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

I pretty agree that this is a good case that we should align both emul and circuit.
Circuit & proving system currently rely on pow 2 to succinctly evaluate address polynomial with respect to a random point.
So it will be better add check in toolchain/emulator to assure memory space are aligned with power 2 address.

Copy link
Collaborator Author

@matthiasgoergens matthiasgoergens Dec 10, 2024

Choose a reason for hiding this comment

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

Yes, we have two possibilities here:

  • Replace the whole computation here with an assertion that we are using a power of two. (And probably add an assertion in the emulator, too.)
  • Or make the change I suggested to round up to the next power of two.

Either way works for me. You seem to be happier with the assertions?

However, I would suggest that the circuits should only prepare RAM of the size that was actually used.

So even if our maximum RAM available is, say, 1GiB, the prover should look at the span between maximum and minimum address actually accessed as RAM during the execution and only do work accordingly (rounded up to the next power of two, of course).

What do you think?

Copy link
Collaborator

@hero78119 hero78119 Dec 10, 2024

Choose a reason for hiding this comment

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

[...] the prover should look at the span between maximum and minimum address actually accessed as RAM during the execution and only do work accordingly [...]

we already support this feature (non uniform design) now (seems I keep spreading this message in many PR 😅 ) max_len only call in circuit setup phase and those information will be extract to constrain system, encoding to verifier key for check

Replace the whole computation here with an assertion that we are using a power of two. (And probably add an assertion in the emulator, too.)

+1 as this make spirit align, plus keeping verifier logic simple

So this PR probably can probably go toward this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

[...] the prover should look at the span between maximum and minimum address actually accessed as RAM during the execution and only do work accordingly [...]

we already support this feature (non uniform design) now (seems I keep spreading this message in many PR 😅 ) max_len only call in circuit setup phase and those information will be extract to constrain system, encoding to verifier key for check

Ok, now I'm really confused! If we already have this feature, why do we even look at this max_len derived from the platform here, and not just look at what the execution actually used?

What's the point of the value we are computing here? Why don't we use the actual memory used for the circuit setup?

Copy link
Collaborator

@hero78119 hero78119 Dec 10, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@matthiasgoergens matthiasgoergens changed the title Check: is DynVolatileRamTable::max_len broken? FxDynVolatileRamTable::max_len Dec 9, 2024
@matthiasgoergens matthiasgoergens changed the title FxDynVolatileRamTable::max_len FixDynVolatileRamTable::max_len Dec 12, 2024
@matthiasgoergens matthiasgoergens changed the title FixDynVolatileRamTable::max_len Fix DynVolatileRamTable::max_len Dec 12, 2024
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