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

feat(batching): Data result store #357

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Conversation

hacheigriega
Copy link
Member

@hacheigriega hacheigriega commented Sep 11, 2024

Explanation of Changes

This PR migrates the data result store from the Core Contract to the batching module. Once the tally module's end blocker completes a tally of a given data request, it will construct and store the data result keyed by its hash. This means that the tally end blocker will no longer send data result data to the contract. Instead, it will simply notify the contract of the tally completion of a given data request by sending its data request ID.

Task list beyond this PR

  • Precisely define DataResult type and its hash, which is also its ID, for the protocol
  • Update seda-common-rs so that the post data result sudo message only contains the data request ID field.
  • Contract: Remove data result store.
  • Contract: Sudo call should only take a data request ID so that the corresponding item can be removed from the data request store in the contract. The contract no longer needs any information about data results.

Closes #355

@hacheigriega hacheigriega changed the base branch from main to hy/batching September 11, 2024 01:56
@hacheigriega hacheigriega force-pushed the hy/batching-data-results branch from f3244ae to f0caf14 Compare September 25, 2024 20:30
@hacheigriega hacheigriega marked this pull request as ready for review September 30, 2024 17:21
@hacheigriega hacheigriega force-pushed the hy/batching-data-results branch 2 times, most recently from b9f9bce to a263130 Compare October 1, 2024 15:13
@hacheigriega hacheigriega requested a review from a team October 1, 2024 20:32
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

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

Maybe not part of this PR, but how does a solver go from DR_ID to result to batch? In terms of lookups that seems difficult right now.

@hacheigriega hacheigriega force-pushed the hy/batching branch 2 times, most recently from 10ad9f2 to a9bc161 Compare October 3, 2024 18:20
Base automatically changed from hy/batching to main October 3, 2024 18:21
@hacheigriega hacheigriega force-pushed the hy/batching-data-results branch from 827dec5 to 91b5452 Compare October 3, 2024 18:26
// block_height is the height at which the data request was tallied.
uint64 block_height = 4;
// exit_code is the exit code of the tally wasm binary execution.
uint32 exit_code = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oopa, I realize seda-wasm-vm actually returns an i32 from the rust side and an int on the go side. But tbh we should change this to a u8/uint8 I think. No reason to have so many options for an exit code imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should be uint8, but protobuf doesn't have that type :/
seda-wasm-vm should be fixed to return a u8 from the rust side and uint8 from the go side, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can make an issue for that, or maybe I'll just do it in the logging pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh... wasmer returns an i32 as an exit code I see...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see in the wasi codebase where it could be negative but I asked a question to the wasi team. Let's see what they say and maybe we should be keeping it as an i32.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I am just casting it to uint8 on the module side for now. Franklin mentioned that process::exit can only use 8 bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm waiting on a response here wasmerio/wasmer#5131 for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we discussed this, instead on the tally VM we should take this exit code from WASM and put that exit code in the string and reserve a number for WASMER exit codes.

Copy link
Member Author

Choose a reason for hiding this comment

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

link to the issue: sedaprotocol/seda-wasm-vm#30

@hacheigriega hacheigriega requested a review from gluax October 3, 2024 21:41
@hacheigriega hacheigriega force-pushed the hy/batching-data-results branch from 4b0f3e2 to f04a1c2 Compare October 8, 2024 18:50
@hacheigriega hacheigriega merged commit f04a1c2 into main Oct 8, 2024
8 of 10 checks passed
@hacheigriega hacheigriega deleted the hy/batching-data-results branch October 8, 2024 18:50
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.

✨ Add data result storage and complete data result tree
3 participants