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

consensus, core/*: metropolis features #14337

Closed
wants to merge 8 commits into from

Conversation

obscuren
Copy link
Contributor

@obscuren obscuren commented Apr 16, 2017

Todos:

  • 96 Blockhash refactoring
    • Peter will implement this
  • Felix will fix general state tests

Done:

  • 98 Removal of intermediate state roots from receipts
  • 100 Change difficulty adjustment
  • 140 REVERT instruction
  • 196 Precompiled contracts for addition and scalar multiplication on the elliptic curve
  • 197 Precompiled contracts for optimal Ate pairing check on the elliptic curve alt_bn128 Buterin
  • 198 Precompiled contract for bigint modular exponentiation
  • 211 New opcodes: RETURNDATASIZE and RETURNDATACOPY
  • 214 New opcode STATICCALL
  • 86 Abstraction of transaction origin and signature


func isAbstractSigner(tx *Transaction) bool {
v, r, s := tx.RawSignatureValues()
return v.BitLen() == 0 && r.BitLen() == 0 && s.BitLen() == 0
Copy link
Member

Choose a reason for hiding this comment

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

The EIP has changed and v here needs to be the chain ID.

x = big.NewInt(2)
}
z := new(big.Int).Sub(bigTime, bigParentTime)
z.Div(x, big9)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be z.Div(z, big9)?

// may me left uninitialised and will be set the default
// table.
JumpTable [256]operation
}

// Interpreter is used to run Ethereum based contracts and will utilise the
// passed environment to query external sources for state information.
// passed evmironment to query external sources for state information.
Copy link
Member

Choose a reason for hiding this comment

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

That's a fancy word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-P screwed up find and replace

// Create a new receipt for the transaction, storing the intermediate root and gas used by the tx
// based on the eip phase, we're passing wether the root touch-delete accounts.
receipt := types.NewReceipt(statedb.IntermediateRoot(config.IsEIP158(header.Number)).Bytes(), usedGas)
receipt := types.NewReceipt(root, usedGas)
Copy link
Member

@pirapira pirapira May 11, 2017

Choose a reason for hiding this comment

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

Since the option 1 of EIP98 was chosen, so it's not enough to make root the empty sequence. The receipt has one less elements in Metropolis. See a log of a coredev meeting. Ah, no, this seems all fine.

common.BytesToAddress([]byte{4}): &dataCopy{},
common.BytesToAddress([]byte{5}): &bigModexp{},
common.BytesToAddress([]byte{6}): &bn256Add{},
common.BytesToAddress([]byte{6}): &bn256ScalarMul{},
Copy link
Member

Choose a reason for hiding this comment

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

Address 6 is used twice, it seems.

}
mod := new(big.Int).SetBytes(input[:modLen])

return base.Exp(base, exp, mod).Bytes(), nil
Copy link
Member

Choose a reason for hiding this comment

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

The length of the result needs to be the same as that of mod. See https://github.com/ethereum/EIPs/pull/198/files#diff-b02d15001884061861b0e92a735e7cefR11

core/vm/evm.go Outdated
@@ -79,25 +104,72 @@ type EVM struct {
abort int32
}

// NewEVM retutrns a new EVM evmironment.
// NewEVM retutrns a new EVM evmironment. The returned EVM is not thread safe
Copy link
Member

Choose a reason for hiding this comment

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

Now my brain thinks evmironment is a word.

core/vm/evm.go Outdated
}

// initialise a new contract and set the code that is to be used by the
// E The contract is a scoped evmironment for this execution context
Copy link
Member

Choose a reason for hiding this comment

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

I guess this E is EVM.

ret, err = evm.interpreter.Run(snapshot, contract, input)
// When an error was returned by the EVM or when setting the creation code
// above we revert to the snapshot and consume any gas remaining. Additionally
// when we're in homestead this also counts for code storage gas errors.
Copy link
Member

Choose a reason for hiding this comment

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

I guess we never hit here during Homestead.

// if the interpreter is operating in readonly mode, make sure no
// state-modifying operation is performed.
if operation.writes ||
((op == CALL || op == CALLCODE) && stack.Back(3).BitLen() > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

LOG0, LOG1, LOG2, LOG3, LOG4 should be banned here as well. Currently their .writes value is false.

Copy link
Member

Choose a reason for hiding this comment

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

This has been resolved. These instructions now have .writes == true.

}

func (c *bn256Add) Run(in []byte) ([]byte, error) {
if len(in) != 96 {
Copy link
Member

Choose a reason for hiding this comment

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

If the input is too short, it should be padded. I think this changed at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe when I wrote this it was still up for debate, including changing the semantics of CALLDATALOAD (i.e. that it should throw)

return 0 // TODO
}

func (c *bn256Add) Run(in []byte) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The type says bn256Add but this function calls ScalarMult.

return 0 // TODO
}

func (c *bn256ScalarMul) Run(in []byte) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The type says bn256ScalarMul but this function performs addition.

}

func (c *bn256ScalarMul) Run(in []byte) ([]byte, error) {
if len(in) != 128 {
Copy link
Member

Choose a reason for hiding this comment

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

The input should be padded if it's too short.

@obscuren
Copy link
Contributor Author

@pirapira thanks for the review. I'm awaiting review and approval on #14336 before I continue.

@pirapira
Copy link
Member

@obscuren yes, I know. I've been just reading the interesting parts (which was a reasonable thing to do after doing YP).

@obscuren obscuren force-pushed the metropolis-finalisation branch 2 times, most recently from 2cff572 to 046fc67 Compare May 29, 2017 07:11
@fjl fjl modified the milestone: 1.6.3 May 31, 2017
@karalabe karalabe modified the milestones: 1.6.4, 1.6.3 Jun 1, 2017
@obscuren
Copy link
Contributor Author

obscuren commented Jun 3, 2017

@pirapira I've addressed your comments. Thanks again.

@holiman
Copy link
Contributor

holiman commented Jun 7, 2017

I was trying to run in on Hive, but build fails:

# github.com/ethereum/go-ethereum/core/vm
core/vm/interpreter.go:76: undefined: metropolisInstructionSet

@@ -729,7 +729,7 @@ func opReturnDataCopy(pc *uint64, evm *EVM, contract *Contract, memory *Memory,
cOff = stack.pop()
l = stack.pop()
)
memory.Set(mOff.Uint64(), l.Uint64(), getData(memory.lastReturn, cOff, l))
memory.Set(mOff.Uint64(), l.Uint64(), getData(evm.interpreter.returnData, cOff, l))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should generate an error if the operation tries to read out of bounds, as agreed on the last core dev call.

@@ -228,7 +234,7 @@ func (in *Interpreter) Run(snapshot int, contract *Contract, input []byte) (ret
// if the operation returned a value make sure that is also set
// the last return data.
if res != nil {
Copy link
Contributor

@holiman holiman Jun 7, 2017

Choose a reason for hiding this comment

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

I'm not sure this is sufficient.

In the case of CALL, the res may be nil for various reasons, e.g. the balance being to low:

	if !evm.Context.CanTransfer(evm.StateDB, caller.Address(), value) {
		return nil, gas, ErrInsufficientBalance
	}

The way it's written, I don't think RETURNDATA would be cleared. I believe the returndata should be cleared immediately when CALL is invoked.

Further, I believe that also CREATE should clear the RETURNDATA. But I'm not 100% certain in this case. @pirapira what's your take ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better with

if operations.clearsReturndata{
   in.returnData = res
}

Copy link
Contributor

Choose a reason for hiding this comment

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

References: ethereum/EIPs#211 (comment)

To summarize: Any opcode that attempts to create a new call stack frame resets the buffer of the current stack frame right before it executes, even if that opcode fails.

Also, from the EIP itself -
https://github.com/ethereum/EIPs/blob/aeaffcca26fa688f5015da7ce059c886e90ef374/EIPS/returndatacopy.md:

As an optimization, it is possible to share the return data buffer across call frames because only one will be non-empty at any time.

// to the chain identifier and r and s being 0.
func isAbstractSigner(tx *Transaction, chainId *big.Int) bool {
v, r, s := tx.RawSignatureValues()
return v.Cmp(chainId) == 0 && r.BitLen() == 0 && s.BitLen() == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

A requirement for it being an abstract-signer tx, is that gasprice = 0, nonce = 0, value = 0. Is this checked anywhere?

spec

}
instructionSet[RETURNDATASIZE] = operation{
execute: opReturnDataSize,
gasCost: constGasFunc(0), // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://github.com/ethereum/EIPs/blob/aeaffcca26fa688f5015da7ce059c886e90ef374/EIPS/returndatacopy.md#specification:

Gas costs: 2 (same as CALLDATASIZE)

Or rather, GasQuickStep.

cOff = stack.pop()
l = stack.pop()
)
memory.Set(mOff.Uint64(), l.Uint64(), getData(evm.interpreter.returnData, cOff, l))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should raise error in case the requested data is out of bounds - getData pads with zeroes, afaik.

// if the interpreter is operating in readonly mode, make sure no
// state-modifying operation is performed.
if operation.writes ||
((op == CALL || op == CALLCODE) && stack.Back(3).BitLen() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment would be in order here to shed some light on what magic is contained within stack.Back(3)

@holiman
Copy link
Contributor

holiman commented Jun 20, 2017

At https://github.com/obscuren/go-ethereum/blob/metropolis-finalisation/core/vm/evm.go#L195:

		if PrecompiledContracts[addr] == nil && evm.ChainConfig().IsEIP158(evm.BlockNumber) && value.Sign() == 0 {

You need to check for Metro, and if so, use PrecompiledContractsMetropolis instead of PrecompiledContracts.

I think. I'm actually not 100%.

obscuren and others added 8 commits June 24, 2017 09:26
Reimplemented RETURNDATA such that the returndata buffer is isolated
inside the interpreter rather than the memory object.
* Right pad return data of  native contract big mod exp to same length
as the modulo input
* Spelling
* Make LOG{n} write operations so the throw during readonly
* Right pad native contracts input with zeros
* Fixed bn256{add, mul}
* Changed the gas for RETURNDATASIZE
* Added comments for read only check
* Fixed opReturnDataCopy
@holiman
Copy link
Contributor

holiman commented Jun 26, 2017

@obscuren there's also an error in bn256Add - see obscuren@ec8ff51

(but maybe ignore the gas calculation, I'm not sure it's correct)

@karalabe
Copy link
Member

Superseded by #14726 (which itself is superseded already).

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

Successfully merging this pull request may close these issues.

6 participants