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 write fees for read-only rent entry bumps. #996

Closed
wants to merge 8 commits into from

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Aug 15, 2023

What

Charge write fees for read-only rent entry bumps.

Write entry bumps are already charged separately as a part of the total write fees.

Why

RO entry bumps need to be written in ledger, so there is always some 'flat' fee component due to the write (so bumping the entry by 1 ledger is not just 1 stroop).

Known limitations

N/A

Write entry bumps are already charged separately as a part of the total write fees.
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Mostly looks good! One change request: can you change the arithmetic here to be saturating_mul / saturating_add based?

@dmkozh
Copy link
Contributor Author

dmkozh commented Aug 15, 2023

One change request: can you change the arithmetic here to be saturating_mul / saturating_add based?

FWIW this is supposed to only be called on sanitized small inputs (everything is controlled by the protocol-imposed limits), so I wanted to save some typing/cycles on this. But I guess it's not really significant, so might as well just have safer ops everywhere.

@@ -258,20 +288,20 @@ fn rent_fee_per_entry_change(
entry_change.new_expiration_ledger
- entry_change.old_expiration_ledger.max(current_ledger - 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can underflow if current_ledger > new_expiration_ledger > old_expiration_ledger

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 not sure if a saturating sub is better here or hoisting the max up to the if condition that guards this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_ledger > new_expiration_ledger > old_expiration_ledger should never happen - again, this is not a library to use for arbitrary inputs, but a helper to use with well-behaved state managed by core or preflight. These shouldn't ever pass the expired entries to the host.

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Couple more minor ones..

@dmkozh
Copy link
Contributor Author

dmkozh commented Aug 16, 2023

Given that we might be changing how the rent works, I'll close this for the time being as it probably will become irrelevant.

@dmkozh dmkozh closed this Aug 17, 2023
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