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: implement getInsecureTime precompile #278

Merged

Conversation

pistomat
Copy link
Contributor

📝 Summary

Implementation of the getInsecureTime precompile that returns the Kettle Unix time.

Considerations

  • The description, name and address of the precompile are up for debate.
  • When the precompile is used in a normal Suave transaction, it should probably return block time.
  • Is the E2E test enough?

📚 References


  • I have seen and agree to CONTRIBUTING.md

@@ -475,3 +475,11 @@ functions:
- name: message
type: bytes
description: "Decrypted message"
- name: getInsecureTime
address: "0x000000000000000000000000000000007770000c"
description: "Returns the current Kettle Unix time in seconds."
Copy link
Member

Choose a reason for hiding this comment

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

would be good to add ", insecure because it assumes trust in Kettle's clock" or something like that to the description plz

Copy link
Member

@zeroXbrock zeroXbrock left a comment

Choose a reason for hiding this comment

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

To really ensure the precompile isn't returning a bogus number (although the code is sound; I'm sure it won't), you could also check the system time after calling the precompile, so that you could ultimately assert start_time $\le$ precompile_time $\le$ now_time.

But otherwise the code is solid. Thanks for the PR @pistomat!

@dmarzzz
Copy link
Member

dmarzzz commented Aug 29, 2024

To really ensure the precompile isn't returning a bogus number (although the code is sound; I'm sure it won't), you could also check the system time after calling the precompile, so that you could ultimately assert start_timeprecompile_timenow_time.

Good idea, maybe we should add some basic checks also to the precompile so even if the TEE operator does mess with it, there are bounds? ideally shouldnt be a time before chain genesis time or outside of latest_block_num * 2 seconds + genesis_start_time ? It's not great because the operator could stop any blocks coming in, but it does make it slightly harder to manipulate I would argue. There's probably more we could do to make it harder also

@pistomat
Copy link
Contributor Author

Nice idea!

The bounds should be suave block.timestamp <= value returned from the precompile <= suave block.timestamp + 2 seconds, right?

Also FYI I chose uint256 as return value because that is the type of block.timestamp.

Copy link
Collaborator

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

Good call on the boundary checks. LGTM otherwise.

@pistomat
Copy link
Contributor Author

pistomat commented Aug 30, 2024

Is this precompile also be available in normal Suave onchain transactions (not CCR)? @dmarzzz @ferranbt If so, I think we should also set the return value to the suave block.timestamp for that case. Because otherwise all validating nodes will get different values for this precompile which will probably break stuff.

This does not matter for CCRs, because they are processed only on a single Kettle, as opposed to Suave blocks, where all precompile return values should be deterministic (otherwise you can't validate blocks).

@ferranbt
Copy link
Collaborator

ferranbt commented Sep 4, 2024

Is this precompile also be available in normal Suave onchain transactions (not CCR)? @dmarzzz @ferranbt If so, I think we should also set the return value to the suave block.timestamp for that case. Because otherwise all validating nodes will get different values for this precompile which will probably break stuff.

This does not matter for CCRs, because they are processed only on a single Kettle, as opposed to Suave blocks, where all precompile return values should be deterministic (otherwise you can't validate blocks).

This precompile is not available on the Suave chain, only in the CCR context.

@pistomat
Copy link
Contributor Author

pistomat commented Sep 4, 2024

Updated the description @dmarzzz.

In the end I did not add any bounds for the time returned, because the lower bound of block.timestamp can be easily checked in Solidity and the upper bound cannot be simply adding block frequency (3 seconds) because of missed slots (which for Toliman happen like 6 times a day on average). So for me it is ready to merge.

P.S. I will add a PR to the suave-std repo to call this precompile after this one is merged.

@ferranbt
Copy link
Collaborator

ferranbt commented Sep 4, 2024

P.S. I will add a PR to the suave-std repo to call this precompile after this one is merged.

Nice! I will run the CI and merge if it passes.

You do not need to update suave-std, it is done automatically.

@pistomat
Copy link
Contributor Author

pistomat commented Sep 5, 2024

Updated the precompile to use milliseconds instead of seconds

@ferranbt ferranbt merged commit 0ca15b7 into flashbots:main Sep 5, 2024
4 checks passed
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.

4 participants