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

Dummy Circuit - Basic ECALL #369

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Dummy Circuit - Basic ECALL #369

merged 6 commits into from
Nov 8, 2024

Conversation

naure
Copy link
Collaborator

@naure naure commented Oct 12, 2024

Issue #359 and #567

  • The DummyInstruction implements all the communications of a step: state, fetch, registers, memory. But it does not verify calculations: any value can be written out.

  • Placeholder circuits for missing implementations, including unknown ecalls.

  • More precise register op assignment, see Feat/ecall nop #570.

@hero78119
Copy link
Collaborator

hero78119 commented Oct 14, 2024

Hi, may I know what the expected use case of dummy circuit, e.g. for padding execution trace?

@naure
Copy link
Collaborator Author

naure commented Oct 14, 2024

Hi, may I know what the expected use case of dummy circuit, e.g. for padding execution trace?

To support instructions that don’t have a complete implementation yet, but still connect all the state together (register writes, etc).

@kunxian-xia
Copy link
Collaborator

Moreover, we can build on top of this to support compute-bound syscall like sha256_compress in a unconstrained way. This will allow us to support real world example like ssz-withdrawal.

@naure naure enabled auto-merge (squash) October 14, 2024 14:57
@matthiasgoergens
Copy link
Collaborator

What's the status of this?

@naure
Copy link
Collaborator Author

naure commented Oct 21, 2024

Ready.

@kunxian-xia
Copy link
Collaborator

I plan to build a dummy circuit for syscall like ed_add on top of this pr.

@naure
Copy link
Collaborator Author

naure commented Oct 23, 2024

I updated this. You can merge it.

@matthiasgoergens
Copy link
Collaborator

What's the status of this one? Do you still want to merge it? I'm happy to review after you bring it up to date with current master and fix the CI problems.

@naure naure requested review from matthiasgoergens and kunxian-xia and removed request for kunxian-xia November 6, 2024 18:27
@naure naure mentioned this pull request Nov 7, 2024
@naure naure changed the title Dummy Circuit Dummy Circuit (incl. ecall) Nov 7, 2024
@naure naure force-pushed the dummy branch 2 times, most recently from 1ffcdb1 to 4f6a0e3 Compare November 7, 2024 08:15
@naure
Copy link
Collaborator Author

naure commented Nov 7, 2024

Updated + now with ecall support.

This was linked to issues Nov 7, 2024
@naure naure requested a review from hero78119 November 7, 2024 10:53
codes: InsnCodes,
) -> Result<Self, ZKVMError> {
let (with_rs1, with_rs2, with_rd) = match (codes.format, codes.kind) {
(_, InsnKind::EANY) => (true, true, false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

For ecall instructions, I think they need to read the rd register to get syscall_id.

Copy link
Collaborator Author

@naure naure Nov 8, 2024

Choose a reason for hiding this comment

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

It has to match VMState::ecall. That’s not true in this PR, I fixed it in another branch. I can backport it here to be consistent.

Copy link
Collaborator Author

@naure naure Nov 8, 2024

Choose a reason for hiding this comment

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

Better idea: just treat it as an I instruction. I’ll make it so in the guest-example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After reworking this a bit: the ignored ecalls just read the syscall_id and do nothing else. Added comments to explain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also turned it into a parameter: Platform.unsafe_ecall_nop

Comment on lines 73 to 77
(InsnFormat::I, _) => (true, false, true),
(InsnFormat::S, _) => (true, true, false),
(InsnFormat::B, _) => (true, true, false),
(InsnFormat::U, _) => (false, false, true),
(InsnFormat::J, _) => (false, false, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can get rid of them as opcodes that are not supported (i.e. DIV / REM / REMU) right now belong to the R category.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we have implemented almost everything in the mean time 😄 . Maybe there will be other use-cases: test, benchmarks, demos ?

@hero78119
Copy link
Collaborator

hero78119 commented Nov 8, 2024

DummyCircuit implementation LGTM!
Do you think we better make a feature toggle e.g. "dummy" and make it as default feature now?
Via a feature toggling means, at some point, we can assure there is no dummy opcode by toggle off, so in case there are some un-intentional and silently leaking.

@naure
Copy link
Collaborator Author

naure commented Nov 8, 2024

@hero78119 This is a gadget that an example or a benchmark can use. If you don’t use it, it’s not there. Maybe you meant a kill switch that makes it impossible to be used at all (assert_eq!(cheating, false)) 😃

A variant of #369 with all-powerful ecall instead of NOP.

---------

Co-authored-by: Aurélien Nicolas <[email protected]>
@naure naure changed the title Dummy Circuit (incl. ecall) Dummy Circuit - Basic ECALL Nov 8, 2024
@naure naure mentioned this pull request Nov 8, 2024
3 tasks
Copy link
Collaborator

@kunxian-xia kunxian-xia left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's merge it to unblock the integration test #368. Any remaining issue can be addressed in follow-up PRs.

@naure naure merged commit 8e00028 into master Nov 8, 2024
6 checks passed
@naure naure deleted the dummy branch November 8, 2024 13:32
naure added a commit that referenced this pull request Nov 11, 2024
#368 + #369

---------

Co-authored-by: Aurélien Nicolas <[email protected]>
mcalancea added a commit that referenced this pull request Nov 13, 2024
commit dfc85dc
Author: Mihai <[email protected]>
Date:   Wed Nov 13 15:30:17 2024 +0200

    clean experimental code

commit 618965a
Merge: a21a628 82af85a
Author: Mihai <[email protected]>
Date:   Wed Nov 13 15:27:20 2024 +0200

    merge master

commit a21a628
Author: Mihai <[email protected]>
Date:   Wed Nov 13 13:47:25 2024 +0200

    add stats.rs

commit deb0bd3
Author: Mihai <[email protected]>
Date:   Wed Nov 13 13:45:52 2024 +0200

    stash

commit c200f2f
Author: Mihai <[email protected]>
Date:   Tue Nov 12 17:15:45 2024 +0200

    stash

commit fc45251
Author: Mihai <[email protected]>
Date:   Tue Nov 12 16:21:39 2024 +0200

    stash

commit e2408b3
Author: Mihai <[email protected]>
Date:   Tue Nov 12 16:02:37 2024 +0200

    more stash

commit 5cccc29
Author: Mihai <[email protected]>
Date:   Tue Nov 12 12:32:55 2024 +0200

    stash

commit 4129e52
Author: Mihai <[email protected]>
Date:   Tue Nov 12 10:32:28 2024 +0200

    add ConstraintStats

commit 82af85a
Author: mcalancea <[email protected]>
Date:   Tue Nov 12 09:35:07 2024 +0200

    Extend profiling using `tracing` (#572)

    Improve profiling efforts by:
    - refactoring tracing spans
    - addressing a pitfall regarding spawned threads
    - changing some subscriber configs

commit 54c8114
Author: Matthias Görgens <[email protected]>
Date:   Tue Nov 12 07:34:18 2024 +0700

    Remove some redundant `.into_iter()` (#581)

    Just a minor clean-up while I'm reading through our code.

commit 85f8dd8
Author: Mihai <[email protected]>
Date:   Mon Nov 11 18:43:12 2024 +0200

    stash

commit 4acd8c9
Author: Matthias Görgens <[email protected]>
Date:   Mon Nov 11 16:28:43 2024 +0700

    Remove unused file (#580)

    This finishes work done in #201

commit 311d79e
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Nov 11 08:12:12 2024 +0000

    Bump tempfile from 3.13.0 to 3.14.0 (#578)

commit 0389112
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Nov 11 08:11:57 2024 +0000

    Bump anyhow from 1.0.92 to 1.0.93 (#579)

commit 853f1ba
Author: Cyte Zhang <[email protected]>
Date:   Mon Nov 11 15:55:49 2024 +0800

    Change BaseFold trim API to consume pp & truncate public parameters directly in `trim` (#248)

    Considering that BaseFold trim is almost always used only once in each
    program execution, consume the input public parameter instead of clone
    it to save memory.

    This API change also allows the `trim` function to directly truncate the
    input public parameters.

commit 8e00028
Author: naure <[email protected]>
Date:   Fri Nov 8 14:32:07 2024 +0100

    Dummy Circuit - Basic ECALL (#369)

    _Issue #359 and #567_

    * The `DummyInstruction` implements all the communications of a step:
    state, fetch, registers, memory. But it does not verify calculations:
    any value can be written out.

    * Placeholder circuits for missing implementations, including unknown
    ecalls.

    * More precise register op assignment, see #570.

    ---------

    Co-authored-by: Aurélien Nicolas <[email protected]>

commit a060e15
Author: Matthias Görgens <[email protected]>
Date:   Fri Nov 8 18:33:25 2024 +0800

    Remove unimplmented and unused functions (#576)
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.

Dummy ecall circuit Dummy Instruction Circuit
4 participants