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: Improve add fn key estimation #6794

Merged
merged 2 commits into from
May 13, 2022

Conversation

jakmeier
Copy link
Contributor

Test short and long method names in the gas cost estimation for adding a
function call access. Also subtract the receipt creation cost.

Test short and long method names in the gas cost estimation for adding a
function call access. Also subtract the receipt creation cost.
@jakmeier jakmeier requested a review from matklad May 12, 2022 14:46
@jakmeier jakmeier requested a review from a team as a code owner May 12, 2022 14:46
@jakmeier
Copy link
Contributor Author

Step one for #6716.

I need to run also on gcloud, but the icount metric so far suggests that we might not have to change anything at all. Will update on the linked issue once I have all data points.

let base_cost = action_add_function_access_key_base(ctx) + action_sir_receipt_creation(ctx);

// Set up estimation with varying method length and total bytes.
let mut estimate = |method_len: usize, total_len: usize| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a suggestion, but I generally try to avoid longer lambdas -- they make the code harder to read, because it's not clear what exactly are they closing over. So, I have a preference for declaring a local function and passing params explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otoh, I'd just write this as a loop, which has exactly the same problem :)

let cases = [(max_method_len, max_bytes), ]
let mut estimations = Vec::with_capacity(cases.len());
for (m_len, n_methods) {
   ....
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought. Almost made it a loop, almost put it in a separate function. So I definitely see where you are coming form.

But IMO in this specific case it is just more readable this way. (For programmers that are not spooked by closures, that is.)

Comment on lines +406 to +407
// Nothing prevents a key to list the same method many times. Performance should not be affected.
let method_name = "x".repeat(method_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe still made names different, just in case something does change in the future impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if we make names different, names need to be longer than one character.
But an attacker is not restricted to that today, so it is totally feasible to submit 1000x "x". And we estimate that case as slightly more gas heavy than fewer methods with longer names.

@near-bulldozer near-bulldozer bot merged commit bdefd77 into near:master May 13, 2022
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.

2 participants