Skip to content

Chore: Remove code that was need only during consensus update#6371

Merged
jannotti merged 3 commits intoalgorand:masterfrom
jannotti:simplify-allows-post-consensus
Jun 24, 2025
Merged

Chore: Remove code that was need only during consensus update#6371
jannotti merged 3 commits intoalgorand:masterfrom
jannotti:simplify-allows-post-consensus

Conversation

@jannotti
Copy link
Copy Markdown
Contributor

@jannotti jannotti commented Jun 18, 2025

Summary

This PR removes some code that was needed during a consensus update, but since it is more lenient for transaction processing, the version checking is no longer needed. It was only necessary to have during that consensus release so that we wouldn't have new nodes that were lenient, and old nodes that were not, at the same time.

We now unconditionally put an app's ID into r.createdApps as soon as the app is created (before it begins execution). We had limited ourselves to only doing so after the sharedResources consensus change went into effect, to avoid becoming more lenient at release time instead of vote time. Since we do it unconditionally, we can remove some code that considers r.createdApps and then the current app explicitly.

We keep a special case in r.allows() that short-circuited resource access checks for pre-sharing programs. That short-circuit existed in order to allow apps to use their own appID and address when making an inner call curing creation, even though allows() would stop them because their appID was not yet in createdApps. Now their appID is in r.createdApps, so allows will let them through. Unfortunately, the simulate code is not able to handle the calls to AllowsHolding and AllowsLocals that removing the short-circuit would cause, because simulate has previously only handled cross-product post resourceSharing.

Test Plan

@jannotti jannotti self-assigned this Jun 18, 2025
@jannotti jannotti force-pushed the simplify-allows-post-consensus branch from f31cb7f to 3412f0c Compare June 18, 2025 14:25
algorandskiy
algorandskiy previously approved these changes Jun 18, 2025
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Since it is post consensus and less restrictive we allowed to remove these extra checks.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.46%. Comparing base (807b29a) to head (62acd3c).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6371      +/-   ##
==========================================
- Coverage   50.59%   50.46%   -0.13%     
==========================================
  Files         653      653              
  Lines      110395   110376      -19     
==========================================
- Hits        55859    55706     -153     
- Misses      51661    51813     +152     
+ Partials     2875     2857      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread data/transactions/logic/eval.go
@jannotti jannotti requested review from algorandskiy and gmalouf June 20, 2025 15:47
@jannotti jannotti marked this pull request as ready for review June 23, 2025 15:32
Comment thread data/transactions/logic/eval.go
Comment thread data/transactions/logic/resources.go
Comment thread ledger/apptxn_test.go
Copy link
Copy Markdown
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

I think makes sense, asked a question or two along the way.

@jannotti jannotti merged commit ab77540 into algorand:master Jun 24, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants