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

Charge gas for the tipset_cid syscall #1633

Closed
arajasek opened this issue Feb 2, 2023 · 6 comments · Fixed by #1644
Closed

Charge gas for the tipset_cid syscall #1633

arajasek opened this issue Feb 2, 2023 · 6 comments · Fixed by #1644
Assignees
Labels
P1 P1: Must be resolved

Comments

@arajasek
Copy link
Contributor

arajasek commented Feb 2, 2023

As far as I can tell, there's no explicit gas charged for this operation, beyond the base syscall cost of 14k. That seems like a problem, because it could involve walking up 900 epochs (restricted to 256 in its only use case right now), and performing an uncharged hash (uncharged because the CID computation happens in the client).

@arajasek arajasek added the P1 P1: Must be resolved label Feb 2, 2023
@arajasek arajasek added this to the M2.1 (r12) Carbonado.3 milestone Feb 2, 2023
@arajasek
Copy link
Contributor Author

arajasek commented Feb 2, 2023

Also, why does the Runtime implementation of this method in builtin-actors return an Option, and not a Result?

@Stebalien
Copy link
Member

Well, this is definitely broken. It should be charging the extern cost (21k gas).

But yeah, we're also not charging for the walk. I thought we had a cache, but it looks like the cache doesn't actually provide O(1) lookups.

The simple solution would be to:

  1. Do some gas modeling with a simple test in lotus.
  2. Add 21k to account for crossing the extern boundary.

The right solution would be to test it from the FVM itself.

@Stebalien
Copy link
Member

Also, why does the Runtime implementation of this method in builtin-actors return an Option, and not a Result?

Historical reasons, IIRC. The method now takes the epoch at which to look, so it has two failure cases: "lookback limit exceeded" and "epoch beyond current". Previously, it would take the lookback distance so it only had one failure case and an option was convenient (I'm just making this up, I have no idea what the actual reason was).

@arajasek
Copy link
Contributor Author

arajasek commented Feb 2, 2023

Also, why does the Runtime implementation of this method in builtin-actors return an Option, and not a Result?

Historical reasons, IIRC. The method now takes the epoch at which to look, so it has two failure cases: "lookback limit exceeded" and "epoch beyond current". Previously, it would take the lookback distance so it only had one failure case and an option was convenient (I'm just making this up, I have no idea what the actual reason was).

That sounds like it can / should change to being a Result :P

@Stebalien
Copy link
Member

Yes.

@Stebalien Stebalien self-assigned this Feb 2, 2023
@arajasek
Copy link
Contributor Author

arajasek commented Feb 2, 2023

Yes.

Done #1151

Stebalien added a commit that referenced this issue Feb 6, 2023
Stebalien added a commit that referenced this issue Feb 6, 2023
Stebalien added a commit that referenced this issue Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 P1: Must be resolved
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants