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

core: add status as a consensus field in receipt #15014

Merged
merged 3 commits into from
Aug 23, 2017

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Aug 21, 2017

Implements Ethereum EIP #658

Motivation:

Current receipt doesn't include a field to indicate the transaction execution status. User can only get execution result by replay transaction. While this method is only useful in a full node, fast nodes can only do this for nodes after their pivot point, and light nodes cannot do this at all. So the field in receipt is helpful.

So a new field Failed is added to receipt, if this field be set with false, means the state revert happened in the outermost call(any internal state revert can be discarded).

While in consensus recepit, failed is converted to a field named status, 0x01 means transaction been executed successfully, while 0x00 means failure occurred.

Whatsmore, the Failed can help to estimate gas cost more accurately, and expose more information in evm runner

@rjl493456442 rjl493456442 force-pushed the metropolis-eip658 branch 4 times, most recently from 5b18f24 to 3b9420f Compare August 21, 2017 12:21
@rjl493456442 rjl493456442 changed the title [WIP]core/types: add status as a consensus field in receipt core: add status as a consensus field in receipt Aug 21, 2017
@rjl493456442
Copy link
Member Author

@karalabe Could you please review the changes?

@karalabe karalabe added this to the 1.7.0 milestone Aug 21, 2017
// Receipt represents the results of a transaction.
type Receipt struct {
// Consensus fields
PostState []byte `json:"root"`
Status byte `json:"status"`
Copy link
Member

Choose a reason for hiding this comment

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

Since the status can only ever be "failed" or "succeeded", it would make more sense to me to keep this field as a bool called Failed. The actual serialization will of course remain the 0x00 or 0x01 byte, but it seems cleaner to work with a boolean in Go code if nothing else is possible (or will be in the planned future).

const (
ReceiptStatusSuccess = byte(0x01)
ReceiptStatusFailure = byte(0x00)
)
Copy link
Member

Choose a reason for hiding this comment

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

These should be private (since we want to work with bools API wise) and please document them.

statusStr = "Failure"
} else {
statusStr = "Undefined"
}
Copy link
Member

Choose a reason for hiding this comment

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

If we switch over to using bools, this undefined cannot happen any more.

if parent, ok := self.caller.(*Contract); ok {
parent.SetVmError(err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this or the contract.VMError field. All the Call* and Create methods already return the same error, and we only care about the outermost one, so internal ones can be discarded without any problems.

core/vm/evm.go Outdated
@@ -127,18 +127,18 @@ func (evm *EVM) Cancel() {
// parameters. It also handles any necessary value transfer required and takes
// the necessary steps to create accounts and reverses the state in case of an
// execution error or failed value transfer.
func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas uint64, value *big.Int) (ret []byte, leftOverGas uint64, err error) {
func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas uint64, value *big.Int) (ret []byte, leftOverGas uint64, vmErr error, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, I don't think we need another error return, the existing one can be used for the desired purpose.

core/vm/evm.go Outdated
@@ -210,6 +211,7 @@ func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte,
ret, err = run(evm, snapshot, contract, input)
if err != nil {
evm.StateDB.RevertToSnapshot(snapshot)
contract.SetVmError(err)
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason to use such a mechanism, since the error is returned anyway from this function.

core/vm/evm.go Outdated
@@ -243,6 +245,7 @@ func (evm *EVM) DelegateCall(caller ContractRef, addr common.Address, input []by
ret, err = run(evm, snapshot, contract, input)
if err != nil {
evm.StateDB.RevertToSnapshot(snapshot)
contract.SetVmError(err)
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason to use such a mechanism, since the error is returned anyway from this function.

core/vm/evm.go Outdated
@@ -286,6 +289,7 @@ func (evm *EVM) StaticCall(caller ContractRef, addr common.Address, input []byte
ret, err = run(evm, snapshot, contract, input)
if err != nil {
evm.StateDB.RevertToSnapshot(snapshot)
contract.SetVmError(err)
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason to use such a mechanism, since the error is returned anyway from this function.

core/vm/evm.go Outdated
@@ -294,18 +298,18 @@ func (evm *EVM) StaticCall(caller ContractRef, addr common.Address, input []byte
}

// Create creates a new contract using code as deployment code.
func (evm *EVM) Create(caller ContractRef, code []byte, gas uint64, value *big.Int) (ret []byte, contractAddr common.Address, leftOverGas uint64, err error) {
func (evm *EVM) Create(caller ContractRef, code []byte, gas uint64, value *big.Int) (ret []byte, contractAddr common.Address, leftOverGas uint64, vmErr error, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, I don't think we need another error return, the existing one can be used for the desired purpose.

} else {
statusStr = "Undefined"
}
return fmt.Sprintf("receipt{status=%s cgas=%v bloom=%x logs=%v}", statusStr, r.CumulativeGasUsed, r.Bloom, r.Logs)
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use failed instead of status and just pass in the bool.


evm *vm.EVM
evm *vm.EVM
vmErr error // vmErr represents all errors occurred during the execution
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason to have this field, it can be returned as a function result when needed.

// can only return the error message called at level 0.
// Sub error will not been assigned to err, except for insufficient
// balance error.
suberr error
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a reason for this second suberr, since returning a single error from Create or Call is enough. Below you can check if it's vm.ErrInsufficientBalance and return nil, nil, nil, nil, vmerr in such a case, and return ret, requiredGas, st.gasUsed(), vmerr, nil in the case of the happy path.

Copy link
Member

Choose a reason for hiding this comment

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

I.e. instead of saving the vmerr inside the StateTransition object, might as well simply return it (since you're doing that at the upper call level anyway).

st := NewStateTransition(evm, msg, gp)

ret, _, gasUsed, err := st.TransitionDb()
return ret, gasUsed, err
return ret, gasUsed, st.vmErr, err
Copy link
Member

Choose a reason for hiding this comment

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

Just return vmerr from st.TransitionDb() instead of accessing it as a member field.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It's almost good, but can be simplified a bit.

@karalabe
Copy link
Member

Note, an internal reversion doesn't mean the receipt's state should be set to failed. Only the outermost revertal/failure should impact the receipt's new field.

@rjl493456442
Copy link
Member Author

Thanks for your time! If only the outermost failure should take care, it can be more simplified. It will fix these tomorrow.

@rjl493456442 rjl493456442 changed the title core: add status as a consensus field in receipt [WIP]core: add status as a consensus field in receipt Aug 22, 2017
@rjl493456442 rjl493456442 force-pushed the metropolis-eip658 branch 2 times, most recently from 9775459 to fd2d594 Compare August 22, 2017 08:44
@rjl493456442 rjl493456442 changed the title [WIP]core: add status as a consensus field in receipt core: add status as a consensus field in receipt Aug 22, 2017
status = receiptStatusSuccessful
} else {
status = receiptStatusFailed
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be done a bit more compactly with:

status := receiptStatusSuccessful
if r.Failed {
  status = receiptStatusFailed
}

// Metropolis receipts have 3 components
// Deserialize based on the first component type.
switch {
case kind == rlp.Byte || kind == rlp.String && len(cnt) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the string magic here, it's only byte we're working with.

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 definitely be removed, or else we'll have a potential consensus failure by accepting an invalid receipt.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Arachnid Since now the empty value is encoded as 0x80. When we use rlp.Split, it returns a String type instead of the byte type we expected. So add these statements in order to deal with this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Arachnid This is another solution to fix it. Change receipt status type from byte to []byte. Since rlp will treat byte as uint8, and byte(0x00) will be encoded as 0x80, which make confused. If we use []byte to represent receipt status, []byte{0x00} can be encoded as 0x00 correctly. But the receipt definition can cause ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

I think this part is correct, since 0x01 is decoded as a byte, but 0x80 as an empty string (0 number). It's a bit weird, but that's the way RLP guesses the type unless it's given explicitly.

One thing that did occur to me however, is that we don't enforce the status byte to be 0 or 1. That's a possible consensus problem. I'll add a fix for this.

r.Failed = false
} else {
r.Failed = true
}
Copy link
Member

Choose a reason for hiding this comment

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

You can set the failed flag with a single statement: r.Failed = metro.Status == receiptStatusFailed

@@ -33,6 +33,7 @@ var (
errWriteProtection = errors.New("evm: write protection")
errReturnDataOutOfBounds = errors.New("evm: return data out of bounds")
errExecutionReverted = errors.New("evm: execution reverted")
errMaxCodeSizeExceeded = errors.New("contract code size exceed the max")
Copy link
Member

Choose a reason for hiding this comment

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

errors.New("evm: max code size exceeded")

failed = true
}

return ret, requiredGas, st.gasUsed(), failed, err
Copy link
Member

@karalabe karalabe Aug 22, 2017

Choose a reason for hiding this comment

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

This whole block can be replaced imho with:

return ret, requiredGas, st.gasUsed(), vmerr != nil, nil

i.e. the failed flag is only ever used upward from Metropolis, so we don't really care if it's true or false in Frontier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it

@karalabe
Copy link
Member

karalabe commented Aug 22, 2017

One tiny error, otherwise it works

 diff --git a/common/bytes.go b/common/bytes.go
index c445968..66577bb 100644
--- a/common/bytes.go
+++ b/common/bytes.go
@@ -47,6 +47,9 @@ func FromHex(s string) []byte {
 //
 // Returns an exact copy of the provided bytes
 func CopyBytes(b []byte) (copiedBytes []byte) {
+       if b == nil {
+               return nil
+       }
        copiedBytes = make([]byte, len(b))
        copy(copiedBytes, b)

Without this, the PostState of a receipt is always set to []byte{} on Metropolis, which causes it to encode as Homestead (since PostState != nil).

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM (I've squashed it and pulled in a test suite update)

default:
return fmt.Errorf("invalid status byte: 0x%x", metro.Status)
}
r.Failed = metro.Status == receiptStatusFailed
Copy link
Contributor

Choose a reason for hiding this comment

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

We're already setting the r.Failed in the switch above.

Copy link
Member

Choose a reason for hiding this comment

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

God damn it, I left this in.

@karalabe
Copy link
Member

@holiman Fixed, PTAL.

@holiman
Copy link
Contributor

holiman commented Aug 23, 2017

LGTM

@karalabe karalabe merged commit 3c48a25 into ethereum:master Aug 23, 2017
@@ -158,7 +158,7 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
evm.Transfer(evm.StateDB, caller.Address(), to.Address(), value)

// 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.

what a great typo - love this word!-)

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.

5 participants