Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

interpreter: Fix call depth handling in CALLs#5662

Merged
chfast merged 2 commits intomasterfrom
aleth-interpreter
Jul 10, 2019
Merged

interpreter: Fix call depth handling in CALLs#5662
chfast merged 2 commits intomasterfrom
aleth-interpreter

Conversation

@chfast
Copy link
Member

@chfast chfast commented Jul 9, 2019

Fixes an issue in aleth-interpreter where for call the depth is not increased.
The additional assert now checks if the depth is the same as in ExtVM (will cause a lot of state test failures if the fix is not applied).

@chfast chfast force-pushed the aleth-interpreter branch from 34b729b to 95ab8e2 Compare July 10, 2019 07:05
@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #5662 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5662      +/-   ##
==========================================
- Coverage   62.79%   62.78%   -0.02%     
==========================================
  Files         349      349              
  Lines       29721    29723       +2     
  Branches     3346     3346              
==========================================
- Hits        18663    18661       -2     
- Misses       9845     9848       +3     
- Partials     1213     1214       +1

@chfast chfast requested review from gumb0 and halfalicious July 10, 2019 07:16
@gumb0
Copy link
Member

gumb0 commented Jul 10, 2019

I'm a bit confused about this

Executive e{m_s, envInfo(), m_sealEngine, depth + 1};

EvmCHost::call delegates to ExtVM::call in the end, and there depth is incremented. Wouldn't it be then incremented twice with your fix?

@chfast
Copy link
Member Author

chfast commented Jul 10, 2019

ExtVM has it's own depth counter. We just check if both agree.

@gumb0
Copy link
Member

gumb0 commented Jul 10, 2019

Ok, I think I see. I guess it was working fine, because ExtVM was correctly counting anyway.

@chfast
Copy link
Member Author

chfast commented Jul 10, 2019

I guess it was working fine, because ExtVM was correctly counting anyway.

Exactly.

@chfast chfast merged commit db35d5f into master Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants