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

move transaction verification checks to sealEngine #4039

Merged
merged 4 commits into from
Apr 19, 2017

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Apr 11, 2017

connects to #4038

refactor envInfo
split LastHashes and envInfo

next step is to replace envInfo with BlockHeader

updated test branch
ethereum/tests#151

@vbuterin
all of the state tests that used blockhash are now useless and should be migrated into blockchain tests.

@winsvega winsvega requested review from chfast and gumb0 April 11, 2017 16:15
}

// Check gas cost is enough.
int64_t baseGasRequired = _t.baseGasRequired(evmSchedule(_env));
Copy link
Member

Choose a reason for hiding this comment

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

This check is already here on line 161

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm. let me check. there was an issue when we removed this check in the last PR

@chfast
Copy link
Member

chfast commented Apr 13, 2017

I have a lots of state tests errors.

And please remove the commented out code.

@codecov-io
Copy link

codecov-io commented Apr 18, 2017

Codecov Report

Merging #4039 into develop will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4039      +/-   ##
===========================================
+ Coverage    65.63%   65.71%   +0.07%     
===========================================
  Files          309      309              
  Lines        22722    22698      -24     
===========================================
+ Hits         14914    14915       +1     
+ Misses        7808     7783      -25
Impacted Files Coverage Δ
libethashseal/Ethash.cpp 86.3% <100%> (+0.28%) ⬆️
libevm/ExtVMFace.h 79.1% <100%> (+7.87%) ⬆️
libevm/JitVM.cpp 90% <100%> (ø) ⬆️
test/tools/jsontests/TransactionTests.cpp 64.53% <100%> (+0.25%) ⬆️
libethereum/Executive.cpp 57.43% <100%> (-0.34%) ⬇️
test/tools/libtesteth/TestHelper.cpp 52.27% <0%> (-0.31%) ⬇️
libethereum/Block.cpp 84.43% <0%> (+1.02%) ⬆️
test/tools/libtesteth/ImportTest.cpp 53.8% <0%> (+1.61%) ⬆️

u256 startGasUsed = _env.gasUsed();
if (startGasUsed + (bigint)_t.gas() > _env.gasLimit())
{
//clog(ExecutiveWarnChannel) << "Cannot fit tx in block" << _env.number() << ": Require <" << (_env.gasLimit() - startGasUsed) << " Got" << _t.gas();
Copy link
Member

Choose a reason for hiding this comment

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

remove this

@gumb0
Copy link
Member

gumb0 commented Apr 19, 2017

EVMJIT build is failing now

Copy link
Member

@chfast chfast 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, just one comment about a field name.

u256 m_timestamp;
u256 m_difficulty;
int64_t m_gasLimit;
BlockHeader m_headerInfo;
Copy link
Member

Choose a reason for hiding this comment

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

I would just name it as m_header.

@winsvega winsvega merged commit c348678 into ethereum:develop Apr 19, 2017
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.

4 participants