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

EIP-150 for Ethereum Classic cpp-ethereum. #10

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

marcusrbrown
Copy link

Pulled from ethereum#3345 with a modification for the gas reprice hard fork block number (2463000 -> 2500000).

This really needs a review other than my own. Ethereum cpp-ethereum is still working on changes to address the DoS attacks, this PR might be blocked by those changes.

I also might hold off on this PR until I can finally add the Classic chain flag and defaults to ETC cpp-ethereum. I don't know yet if any of those changes will be usable/relevant to Ethereum cpp-ethereum.

Copy link
Member

@elaineo elaineo left a comment

Choose a reason for hiding this comment

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

why does this crash cpp eth?

Copy link
Member

@elaineo elaineo left a comment

Choose a reason for hiding this comment

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

@igetgames It looks like some of the EIP150 changes are implemented differently here vs in geth, so it's a bit hard to review. is it passing all the tests? (i don't understand that one commit that adds tests that crash cpp eth)

u256 maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;
callParams->gas = std::min(*m_sp, maxAllowedCallGas);
}

Copy link
Member

Choose a reason for hiding this comment

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

what is the "all but one 64th" rule?

@@ -191,7 +191,7 @@ void VM::interpretCases()

// After EIP150 hard fork charge additional cost of sending
// ethers to non-existing account.
if (!m_schedule->staticCallDepthLimit && !m_ext->exists(dest))
if (m_schedule->suicideChargesNewAccountGas() && !m_ext->exists(dest))
m_runGas += m_schedule->callNewAccountGas;
Copy link
Member

Choose a reason for hiding this comment

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

was this change spec'd in the yellow paper? i don't remember seeing it implemented in geth

@marcusrbrown
Copy link
Author

Thanks for taking a look. I will be syncing to the ETH PR in the morning; those tests no longer crash Eth in the PR merged upstream, so that should be resolved after I sync.

On Oct 20, 2016, at 9:53 PM, Elaine Ou [email protected] wrote:

@elaineo commented on this pull request.

@igetgames It looks like some of the EIP150 changes are implemented differently here vs in geth, so it's a bit hard to review. is it passing all the tests? (i don't understand that one commit that adds tests that crash cpp eth)

In libevm/VMCalls.cpp:

@@ -153,15 +153,20 @@ bool VM::caseCallSetup(CallParameters *callParams)
updateIOGas();

// "Static" costs already applied. Calculate call gas.

  • u256 requestedCallGas = *m_sp;
  • u256 maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;
  • u256 callGas = m_schedule->staticCallDepthLimit() ? requestedCallGas :
  •            std::min(requestedCallGas, maxAllowedCallGas);
    
  • m_runGas = toUint64(callGas);
  • if (m_schedule->staticCallDepthLimit())
  • // With static call depth limit we just charge the provided gas amount.
    
  • callParams->gas = *m_sp;
    
  • else
  • {
  • // Apply "all but one 64th" rule.
    
  • u256 maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;
    
  • callParams->gas = std::min(*m_sp, maxAllowedCallGas);
    
  • }
    +
    what is the "all but one 64th" rule?

In libevm/VM.cpp:

@@ -191,7 +191,7 @@ void VM::interpretCases()

      // After EIP150 hard fork charge additional cost of sending
      // ethers to non-existing account.
  •     if (!m_schedule->staticCallDepthLimit && !m_ext->exists(dest))
    
  •     if (m_schedule->suicideChargesNewAccountGas() && !m_ext->exists(dest))
          m_runGas += m_schedule->callNewAccountGas;
    
    was this change spec'd in the yellow paper? i don't remember seeing it implemented in geth


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

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