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

Replicates AccountsDataMeter in TransactionContext #26438

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Jul 6, 2022

Problem

Spin-off from #25899.

Summary of Changes

Adds these two properties to TransactionContext:

  • total_resize_limit: Maximum number of bytes which can be allocated in this transaction (measured at the beginning of the transaction).
  • total_resize_delta: Negative bytes numbers mean the total allocation decreased, positive bytes numbers mean it increased.

Also adds the method TransactionContext::get_total_resize_remaining() which calculates the difference between these two properties.

@Lichtso Lichtso requested a review from brooksprumo July 6, 2022 10:59
@Lichtso Lichtso force-pushed the refactor/move_accounts_data_meter_into_transaction_context branch from 584b144 to 0c977b7 Compare July 6, 2022 14:30
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@Lichtso Lichtso merged commit 06ebfa1 into solana-labs:master Jul 6, 2022
@Lichtso Lichtso deleted the refactor/move_accounts_data_meter_into_transaction_context branch July 6, 2022 17:27
@@ -249,6 +254,18 @@ impl TransactionContext {
pub fn get_instruction_trace(&self) -> &InstructionTrace {
&self.instruction_trace
}

/// Returns (in bytes) how much data can still be allocated
pub fn get_total_resize_remaining(&self) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

If used incorrectly, this method could lead to indeterminism due to how the total_resize_limit is set when the bank constructs the TransactionContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is not accessible to BPF programs yet anyway. And for builtin programs we could mark it as do not use / only for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "limit" part of the AccountsDataMeter (and this move into TransactionContext) will be removed, so this fn will be removed. Since instructions will not check/throw errors based on the global accounts data size, the "how much is remaining" won't need to be checked.

Copy link
Member

Choose a reason for hiding this comment

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

And for builtin programs we could mark it as do not use / only for testing.

This would be great, can you please do that for now?

The "limit" part of the AccountsDataMeter (and this move into TransactionContext) will be removed, so this fn will be removed

Ok, then why did we add this fn 2 days ago if we are about to remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

And for builtin programs we could mark it as do not use / only for testing.

This would be great, can you please do that for now?

I think it would be better to instead remove the "limit" part from TransactionContext, and thus remove get_total_resize_remaining().

The "limit" part of the AccountsDataMeter (and this move into TransactionContext) will be removed, so this fn will be removed

Ok, then why did we add this fn 2 days ago if we are about to remove it?

Just ordering. I was looking at this PR and #25899 before #26439.

Copy link
Contributor

@brooksprumo brooksprumo Jul 8, 2022

Choose a reason for hiding this comment

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

PR #26504 will remove get_total_resize_remaining() and total_resize_limit from TransactionContext.

(The similar code needs to be removed from AccountsDataMeter as well. That is not part of this PR on purpose to keep it smaller. (Plus all of AccountsDataMeter will be removed, not just the "limit" parts.))

@jstarry
Copy link
Member

jstarry commented Jul 8, 2022

I don't understand how this change helps, what's the purpose of it? I think we should hold off on changes related to the accounts data meter until the indeterminism issues are solved.

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.

3 participants