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

Overhaul "free budget" and migrate non-metered code #1123

Merged
merged 21 commits into from
Oct 26, 2023

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Oct 20, 2023

What

Resolves #1061

  • Overhaul the "free budget":

    • Add two fields internal_limit and internal_total_count to budget dimension for keeping track of internal work being done, which don't get metered for fee, but need to be limited for DOS prevention.
    • Remove the with_free_budget interface and replace it with with_internal_mode (and a with_debug_budget on the host side that wraps around it and asserts host is in DEBUG mode), taking two closures and ensuring 1. the internal_limit is not exceeded 2. any error occurring within is suppressed. The error suppression part is to prevent error-based logic existing outside the closure that accidentally causes divergences between debug and no-debug workflows.
    • Adds a new hostile contract case for testing.
  • A refactoring to split the budget.rs into multiple smaller files. The file was getting big (1200+ lines) and hard to navigate.

  • Migrate all existing non-metered code to metered version, this includes diagnostic logging, diagnostic event, auth preflight code.

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@jayz22 jayz22 force-pushed the debug-budget branch 2 times, most recently from 2939881 to f335934 Compare October 23, 2023 15:12
@jayz22 jayz22 marked this pull request as ready for review October 23, 2023 15:29
@jayz22 jayz22 requested a review from dmkozh October 23, 2023 15:29
@jayz22 jayz22 changed the title wip - debug budget overhaul Overhaul "free budget" and refactor budget.rs Oct 23, 2023
@jayz22 jayz22 marked this pull request as draft October 23, 2023 16:03
@jayz22
Copy link
Contributor Author

jayz22 commented Oct 23, 2023

As I was writing some test, I realize most of the currently diagnostic code are unmetered. I will fix the metering on them. Marked as draft.

@jayz22 jayz22 changed the title Overhaul "free budget" and refactor budget.rs Overhaul "free budget" and migrate non-metered code Oct 24, 2023
@jayz22 jayz22 marked this pull request as ready for review October 24, 2023 18:39
@jayz22
Copy link
Contributor Author

jayz22 commented Oct 24, 2023

All diagnostic workflows have been converted to metered code. Marking this back to ready for review.

soroban-env-host/src/budget/util.rs Outdated Show resolved Hide resolved
soroban-env-host/src/test/budget_metering.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/budget/dimension.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget/dimension.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host.rs Outdated Show resolved Hide resolved
soroban-env-host/src/events/internal.rs Show resolved Hide resolved
@jayz22 jayz22 requested a review from a team as a code owner October 25, 2023 03:41
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.

Looking good! Handful of changes requested but I think this is fairly close to what we should be merging, thanks!

@graydon graydon added this pull request to the merge queue Oct 26, 2023
Merged via the queue into stellar:main with commit 2674d86 Oct 26, 2023
10 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.

Change how 'free' budget works
3 participants