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

AVM: No low resources #5328

Merged
merged 20 commits into from
May 5, 2023
Merged

Conversation

jannotti
Copy link
Contributor

New approach that allows resource opcodes to maintain compatibility in v9, which allows group resource sharing. Obsoletes a prior approach that deprecated a slew of opcodes and made new ones that could not access by static array slot.

v9 changes several "resource accessing" opcodes to no longer accept
"foreign array slots" as arguments - the resource id (for apps or
assets) or the address (when accessing accounts) must be given.

In the case of asset ids, this change could slip past contract authors
pretty easily unless the opcode names are changed. So this PR changes
opcode names. To use v9, authors will need to make explicit changes to
their contracts.

Resource access changes are the only thing in v9. So authors may feel
free to delay this change.

The complete list of changes, bucketed by the resource type being touched.

App
 app_params_get      app_get
 app_global_get_ex   global_state_get_ex

Account & App
 app_opted_in        opted_in
 app_local_get_ex    local_state_get_ex

Asset
 asset_params_get    asset_get

Account & Asset
 asset_holding_get   holding_get

Account
 balance             _deprecated_
 min_balance         _deprecated_
 app_local_get       local_state_get
 app_local_put       local_state_put
 app_local_del       local_state_del
 acct_params_get     acct_get

None, but for consistency
 app_global_get      global_state_get
 app_global_put      global_state_put
 app_global_del      global_state_del
While doing so, ban AVM access to resources < 256, to keep rules for
some opcodes simpler.
@jannotti jannotti changed the title No low resources AVM: No low resources Apr 22, 2023
@jannotti jannotti force-pushed the no-low-resources branch 2 times, most recently from 4c01e9c to e8e7daf Compare April 23, 2023 01:00
@jannotti jannotti marked this pull request as ready for review April 24, 2023 17:50
@ahangsu ahangsu self-requested a review April 25, 2023 14:50
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I somewhat understood of what eval.go is doing, and the related changes for low resource banning.

Just some question here and there. I will look at testing later on.

data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
jasonpaulos
jasonpaulos previously approved these changes May 1, 2023
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good, but I do have a minor comment about function names

data/transactions/logic/eval.go Show resolved Hide resolved
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

ok finally I understood what are the tests doing in evalStateful_test.go, and I am close to approval.

data/transactions/logic/evalStateful_test.go Show resolved Hide resolved
ahangsu
ahangsu previously approved these changes May 4, 2023
bbroder-algo
bbroder-algo previously approved these changes May 5, 2023
@bbroder-algo bbroder-algo dismissed stale reviews from ahangsu and themself via da70459 May 5, 2023 20:02
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #5328 (da70459) into master (d939705) will increase coverage by 0.01%.
The diff coverage is 94.78%.

@@            Coverage Diff             @@
##           master    #5328      +/-   ##
==========================================
+ Coverage   55.28%   55.30%   +0.01%     
==========================================
  Files         454      454              
  Lines       63713    63748      +35     
==========================================
+ Hits        35225    35257      +32     
+ Misses      26096    26086      -10     
- Partials     2392     2405      +13     
Impacted Files Coverage Δ
data/bookkeeping/genesis.go 26.38% <0.00%> (-0.76%) ⬇️
data/transactions/logic/opcodes.go 87.38% <ø> (ø)
data/transactions/transaction.go 42.73% <ø> (ø)
data/transactions/logic/assembler.go 90.23% <88.88%> (-0.05%) ⬇️
data/transactions/logic/eval.go 91.66% <97.80%> (-0.02%) ⬇️
config/consensus.go 85.82% <100.00%> (+0.03%) ⬆️
data/transactions/logic/resources.go 82.95% <100.00%> (ø)
ledger/apply/application.go 73.10% <100.00%> (ø)

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

4 participants