ethereum / interfaces

Interface Specifications inside Ethereum
36 stars 175 forks source link

Out of gas im improvements #1

Open frozeman opened 8 years ago

frozeman commented 8 years ago

This issue is a reminder and discussion thread for the following improvements:

Error code EIP https://github.com/ethereum/EIPs/issues/136

frozeman commented 7 years ago

We should implement this ASAP @bas-vk @chriseth @debris

debris commented 7 years ago

cc @tomusdrw @jacogr

axic commented 7 years ago

Agree with eth_call and eth_estimateGas.

@kumavis what do you think, which makes more sense, null or RPC error?

frozeman commented 7 years ago

Linking the custom error codes we would need to implement for that https://github.com/ethereum/EIPs/issues/136 (Discussion still ongoing)

kumavis commented 7 years ago

eth_getTransactionReceipt should contain an outOfGas property, so that devs can see if a tx threw.

A TransactionReceipt is an ethereum obj that is part of the consensus. I don't think we should decorate that with extra information. Though, on the other hand, when we lookup a tx, it includes the block reference, which is not part of the normal tx object.

{ eth_call, eth_estimateGas } should return an RPC error

agree

frozeman commented 7 years ago

The receipt returned over RPC is not necessarily the same as the one send over the p2p network. As its mainly used by dapps to deal with the network, we should add things which are basic to understand what happened in this network.

Though i agree we shouldn't decorate it with a lot of unrelated info.

frozeman commented 7 years ago

@bas-vk @obscuren is the outOfGas property already in the receipt?

kumavis commented 7 years ago

@frozeman canonical txReceipt is:

https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/runBlock.js#L117-L122

frozeman commented 7 years ago

Sure but the RPC endpoint is not necessarily the cananonical receipt but instead a tool for Dapp devs to understand their tx execution and properly understand the e execution without much hassle.

kumavis commented 7 years ago

@bas-vk @obscuren is the outOfGas property already in the receipt?

@frozeman oh did you mean the eth_getTransactionReceipt RPC result? Top-level OOG is not included in the RPC result.

response from parity it looks like this:

├─ jsonrpc: 2.0
├─ result
│  ├─ blockHash: 0x37bdc90cc344d9e0f1abc1e7630ce097f8a751cecbfac4d375780fe365045370
│  ├─ blockNumber: 0x2ed304
│  ├─ contractAddress
│  ├─ cumulativeGasUsed: 0x159f5
│  ├─ gasUsed: 0xc807
│  ├─ logs
│  │  └─ 0
│  │     ├─ address: 0x2aa6222e3f4304d25ad73906832912ef0b3d3dda
│  │     ├─ blockHash: 0x37bdc90cc344d9e0f1abc1e7630ce097f8a751cecbfac4d375780fe365045370
│  │     ├─ blockNumber: 0x2ed304
│  │     ├─ data: 0x00000000000000000000000070ad465e0bab6504002ad58c744ed89c7da3852400000000000000000000000000000000000000000000000000b1a2bc2ec5000000000000000000000000000004cb1ade2bd72acd758d4f7abacbc633fc53285700000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000000
│  │     ├─ logIndex: 0x0
│  │     ├─ topics
│  │     │  └─ 0: 0x92ca3a80853e6663fa31fa10b99225f18d4902939b4c53a9caae9043f6efd004
│  │     ├─ transactionHash: 0x5c25b138dda6b02b67c31b61256f609934c74b6c2e62e01defc58325c2625e61
│  │     ├─ transactionIndex: 0x1
│  │     └─ type: mined
│  ├─ logsBloom: 0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000020000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000040000000000000000001000000000000000000000000000000000000000000000000000000000000000000000000000000000
│  ├─ root: 0x005a674291dabc7ab4c060a77fc57d5835e8ca0a7a74429333faf98f8aebb100
│  ├─ transactionHash: 0x5c25b138dda6b02b67c31b61256f609934c74b6c2e62e01defc58325c2625e61
│  └─ transactionIndex: 0x1
└─ id: 120
frozeman commented 7 years ago

this is the interface repo. So yes it is about the rpc :) Thanks for the parity response. The proposal issue here is a request to add it.

@arkadiy @debris can you guys add this?

debris commented 7 years ago

@frozeman I haven't been recently working on rpc... I think it's more sensible to ask @jacogr or @tomusdrw ;)

kumavis commented 7 years ago

For better OOG detection when using eth_call and eth_estimateGas see https://github.com/ethereum/interfaces/issues/8

bas-vk commented 7 years ago

eth_estimateGas should return an RPC error if the all the gas was used

To clarify, users can optionally specify gas as an argument. This method should return an error when the user has specified gas and the gas amount isn't sufficient? If the user has not specified the gas amount geth uses the pending block gas limit as a limit. Should this method also return an OOG error if it runs out of gas in that case?

eth_getTransactionReceipt should contain a outOfGas property, so that devs can see if a tx threw.

There are more reasons a transaction can throw. Out of gas is one of them. In the simulateTransaction proposal a didError indication. Maybe that is what you are looking for? Problem is that it doesn't give a context what went wrong and users must do a trace.