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

Charge for hashing init code in CREATE2 #5295

Merged
merged 1 commit into from
Oct 4, 2018
Merged

Charge for hashing init code in CREATE2 #5295

merged 1 commit into from
Oct 4, 2018

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Oct 2, 2018

Changing the gas cost of CREATE2 according to the latest changes in spec ethereum/EIPs#1375

cc @winsvega

@gumb0 gumb0 added this to the Constantinople milestone Oct 2, 2018
@codecov-io
Copy link

codecov-io commented Oct 2, 2018

Codecov Report

Merging #5295 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5295      +/-   ##
=========================================
+ Coverage   61.09%   61.1%   +0.01%     
=========================================
  Files         340     340              
  Lines       27764   27784      +20     
  Branches     3208    3209       +1     
=========================================
+ Hits        16962   16977      +15     
- Misses       9668    9673       +5     
  Partials     1134    1134

@gumb0 gumb0 removed the in progress label Oct 2, 2018
@gumb0
Copy link
Member Author

gumb0 commented Oct 2, 2018

@chfast Any idea what happened to macOS build here?

@gumb0 gumb0 requested review from chfast and halfalicious October 2, 2018 16:25
@chfast
Copy link
Member

chfast commented Oct 2, 2018

It cannot find sha3WordGas symbol :/

@gumb0
Copy link
Member Author

gumb0 commented Oct 4, 2018

Linker can't find the symbol, but it's constexpr, weird. And it's fine with VMSchedule::createGas in the same function 😕

salt = m_SP[3];
// charge for hashing initCode = GSHA3WORD * ceil(len(init_code) / 32)
m_runGas += toInt63((static_cast<u512>(initSize) + 31) / 32 * uint64_t(VMSchedule::sha3WordGas));
Copy link
Member

Choose a reason for hiding this comment

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

Write it as u512(initSize) because there is not data loss here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to avoid C-style casts altogether, than to have a rule when they're safe
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named

But here u512{initSize} can be used, I'll change it.

@gumb0
Copy link
Member Author

gumb0 commented Oct 4, 2018

@chfast Looks like it's obscure C++11 & 14 rules about static constexpr, fixed in C++17
https://stackoverflow.com/questions/40690260/undefined-reference-error-for-static-constexpr-member

@gumb0
Copy link
Member Author

gumb0 commented Oct 4, 2018

So uint64_t{VMSchedule::sha3WordGas} fixes the linker error.

Another option is to add definitions of VMSchedule static members to cpp file like

constexpr int64_t VMSchedule::sha3WordGas;
...

https://en.cppreference.com/w/cpp/language/static#Constant_static_members

@chfast
Copy link
Member

chfast commented Oct 4, 2018

I don't care which workaround is used, but I've found this for SHA3 instructions:

        CASE(SHA3)
        {
            ON_OP();
            constexpr int64_t sha3Gas = VMSchedule::sha3Gas;
            constexpr int64_t sha3WordGas = VMSchedule::sha3WordGas;
            m_runGas = toInt63(sha3Gas + (u512(m_SP[1]) + 31) / 32 * sha3WordGas);

@gumb0
Copy link
Member Author

gumb0 commented Oct 4, 2018

I see, this is third kind of workaround.

So as I understand constexpr int64_t sha3WordGas = VMSchedule::sha3WordGas; works and (u512(m_SP[1]) + 31) / 32 * VMSchedule::sha3WordGas doesn't, because the second one probably tries to pass VMSchedule::sha3WordGas by reference to boost::multiprecision::operator* and creating a reference requires definition outside of class in C++11

@chfast
Copy link
Member

chfast commented Oct 4, 2018

I don't know why it work and I don't want to know. I think it was me who add this but I have not investigated the reasons.

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.

Also maybe squash commits?

salt = m_SP[3];
// charge for hashing initCode = GSHA3WORD * ceil(len(init_code) / 32)
m_runGas += toInt63((static_cast<u512>(initSize) + 31) / 32 * m_schedule->sha3WordGas);
Copy link
Member

Choose a reason for hiding this comment

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

Same static_cast -> u512{} change here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@gumb0 gumb0 merged commit 0fe8de8 into master Oct 4, 2018
@gumb0 gumb0 deleted the create2-cost branch October 4, 2018 14:57
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