cartesi / dave

Cartesi fraud-proof system
Apache License 2.0
19 stars 11 forks source link

Embed Cartesi Machine #28

Closed GCdePaula closed 4 months ago

GCdePaula commented 6 months ago

Attempt at embedding libcartesi.a in cartesi-machine-sys. There's a feature flag for linking with libcartesi_jsonrpc.a instead.

This is using an unmerged branch of the emulator that has the ongoing output unification.

GCdePaula commented 6 months ago

I'll need help testing, documenting and general cleanups. Please give all the feedbacks!

stephenctw commented 6 months ago

This is looking great! I did something similar and smaller on my local branch so I can run the tests with newer emulator version. But I used emulator v0.16.0 instead of the output-unification branch. Why did you use the unmerged branch? I noticed that output-unification somehow can't pass Dave tests, not sure if it's a bug or something else, I've been getting this error from both simple and stress tests:

[#2][03/11/2024 09:37:06] join tournament reverted: ./lua_node/blockchain/sender.lua:76: Send transaction `joinTournament(bytes32,bytes32[],bytes32,bytes32)` reverted:
Error: 
(code: 3, message: execution reverted: tournament doesn't have contested final state, data: Some(String("0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002d746f75726e616d656e7420646f65736e2774206861766520636f6e7465737465642066696e616c20737461746500000000000000000000000000000000000000")))

Also another concern is that we have a few PRs pending right now, some of them are modifying the same code. We might want to prioritize merging them in proper order to avoid greater trouble in the future.

GCdePaula commented 6 months ago

Hey Stephen! Thanks :)

Why did you use the unmerged branch?

This branch (Marcelo's) has the output unification that we'll need for Dave rollups. That's the main reason.

On the tests, I think it may have something to do changes in the emulator itself. I expect that that images we're testing with are not compatible. Just a guess, I'll investigate it!

We might want to prioritize merging them in proper order

You're absolutely right.

stephenctw commented 6 months ago

This branch (Marcelo's) has the output unification that we'll need for Dave rollups. That's the main reason.

I can rebase all my current work on this branch, but it's annoying that the tests are broken.

GCdePaula commented 6 months ago

I can rebase all my current work on this branch

You're using the jsonRPC bindings, right? I don't think this affects you.

In any case, I think we should merge your work to main first, then rebase this PR on top of main.

stephenctw commented 6 months ago

I can rebase all my current work on this branch

You're using the jsonRPC bindings, right? I don't think this affects you.

In any case, I think we should merge your work to main first, then rebase this PR on top of main.

I want to use machine-bindings for new emulator release so I can work on the reset feature. jsonRPC is outdated and I thought we agreed to discard it?

GCdePaula commented 6 months ago

True, you need the bindings for the reset.

The compute branch is currently using the jsonRPC bindings? Have you already started the reset?

I think these Rust bindings Sofia did were already at Marcelo's branch (maybe not the most recent commit).

We could make this binding for the 0.16 release of the machine. Not sure if there are many changes to the interface itself. Maybe they renamed rollups to cmio (Cartesi Machine IO).

If the changes are small, it's possible just checking out the right emulator branch can work. Otherwise, we may have to do some changes, but I think a simple regex can work.

We could ask Fusco to generate a docker image at that branch for us too.

I think the issue is that the CM images we have were built by an incompatible emulator version. In that case, if we want to use the bindings at Marcelo's branch, we'd have to generate those CM images again with the emulator at his branch.

But another possibility is that the bindings have some bug.

Also, with this PR, I think we can run the machine without docker. We may only need docker to create CM images.

This comment is a bit jumbled, I have a plane to catch soon. Let me know what you think, I'm not sure what's the best path forward.

stephenctw commented 6 months ago

Yes, the compute branch is using jsonRPC binding. I switched to use the Rust bindings Sofia made on my local reset branch and made some small fixes so now things are working with emulator v0.16.0. My concern is that the binding now can conflict because of the changes you made and the changes I made were based on different tag/branch.

GCdePaula commented 6 months ago

Ok! Your work is the priority. Let's finish it first, then worry about this PR and sort out the conflicts.

You don't need anything from this PR, right?

stephenctw commented 6 months ago

Ok! Your work is the priority. Let's finish it first, then worry about this PR and sort out the conflicts.

You don't need anything from this PR, right?

For the rust-base and rust-compute PR, I don't need anything from this PR. For the uarch-reset PR, I can use the embedded machine crate, but I can deal with it later.

stephenctw commented 4 months ago

This looks great! I just have some questions about the machine-runner.