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: Handle Teal programs with manual constant blocks better #4442

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

jannotti
Copy link
Contributor

This PR fixes #4034, by forcing the use of "push*" when mixing manual
constant blocks with the int/byte/addr/method pseudo-ops. (And a
related disassembly bug).

However, to avoid this "deoptimization" when manual blocks are used to
add program metadata, deadcode constant blocks are ignored for this
check.

Tests for the problems in #4034 and additional tests for the deadcode were added.

@jannotti jannotti changed the title Handle Teal programs with manual constant blocks better AVM: Handle Teal programs with manual constant blocks better Aug 22, 2022
This PR fixes algorand#4034, by forcing the use of "push*" when mixing manual
constant blocks with the int/byte/addr/method pseudo-ops. (And a
related disassembly bug).

However, to avoid this "deoptimization" when manual blocks are used to
add program metadata, deadcode constant blocks are ignored for this
check.
@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #4442 (7893fa4) into master (7932403) will decrease coverage by 0.04%.
The diff coverage is 98.36%.

@@            Coverage Diff             @@
##           master    #4442      +/-   ##
==========================================
- Coverage   55.24%   55.19%   -0.05%     
==========================================
  Files         398      398              
  Lines       50165    50196      +31     
==========================================
- Hits        27712    27705       -7     
- Misses      20144    20176      +32     
- Partials     2309     2315       +6     
Impacted Files Coverage Δ
data/transactions/logic/assembler.go 85.82% <98.36%> (+0.70%) ⬆️
util/db/dbutil.go 44.24% <0.00%> (-4.25%) ⬇️
ledger/blockqueue.go 85.63% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
ledger/tracker.go 73.93% <0.00%> (-1.71%) ⬇️
catchup/service.go 68.14% <0.00%> (-1.24%) ⬇️
ledger/acctonline.go 78.36% <0.00%> (-1.06%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/acctupdates.go 69.29% <0.00%> (-0.60%) ⬇️
... and 4 more

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

@jannotti jannotti marked this pull request as ready for review August 22, 2022 10:26
@ahangsu ahangsu mentioned this pull request Aug 26, 2022
michaeldiamant
michaeldiamant previously approved these changes Aug 30, 2022
Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@jannotti Nice work to identify + implement a more correct handling of constant blocks ☕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make assembler work better with code that issues *cblock opcodes.
2 participants