getTransactionReceipt - dynamic status generation #634

tzdybal · 2018-06-15T05:32:40Z

No description provided.

whilei · 2018-06-15T05:51:10Z

core/state_transition.go

-	if err != nil {
-		err = nil
+	if vmerr != nil && IsValueTransferErr(vmerr) {
+		return nil, nil, nil, false, InvalidTxError(vmerr)


// if the vmerr was a value transfer error, return immediately // transaction receipt status will be set to TxSuccess

whilei · 2018-06-15T06:15:57Z

eth/api.go

@@ -1197,6 +1215,10 @@ func (s *PublicTransactionPoolAPI) GetTransactionReceipt(txHash common.Hash) (ma
 		fields["contractAddress"] = receipt.ContractAddress
 	}

+	if receipt.Status != types.TxStatusUnknown {
+		fields["status"] = receipt.Status


https://wiki.parity.io/JSONRPC-eth-module#eth_gettransactionreceipt

https://github.com/paritytech/parity/blob/28c731881f2da0ceca4752dbcc1a8f9ad041f988/ethcore/types/src/receipt.rs#L62-L108

Continuing from sorpaas's point 2/3, we need to be careful about changing established eth_ namespaced APIs. So either we'll have to check block number, too, or create a new API method. I'd be keen to use his suggestion of etc_, or our geth_ namespace. (Which one is a question worth talking moar about).

Actually, we're not compatible anyways right now - because we're not returning nil - we're just skipping the status field.

whilei · 2018-06-15T06:26:57Z

core/state_processor.go

@@ -106,6 +109,70 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB) (ty
 	return receipts, allLogs, totalUsedGas, err
 }

+func (p *StateProcessor) ReplayTransaction(txHash common.Hash, statedb *state.StateDB) (*types.Receipt, error) {


I would probably rename this, since "replaying transactions" is taken... EIP155.

Yes, something like trace or playback seems better. Any other ideas?

I like PlayBack.

whilei · 2018-06-15T06:37:03Z

eth/api.go

+	if replayTransactions && receipt.Status == types.TxStatusUnknown {
+		// TODO(tzdybal) - get rid of this nasty casting
+		proc := s.bc.Processor().(*core.StateProcessor)
+		statedb, err := s.bc.StateAt(s.bc.GetBlock(txBlock).Root())


I think you may need to use the state from the parent block.

https://github.com/ethereumproject/go-ethereum/blob/master/core/blockchain.go#L1633

whilei · 2018-06-15T06:39:48Z

eth/api.go

@@ -54,6 +54,9 @@ import (

 const defaultGas = uint64(90000)

+// TODO(tzdybal) - make it configurable parameter
+const replayTransactions = true


w/r/t my later comment, is this intended in order to make the API eth_getTransactionReceipt output configurable? Or?

Yes I was thinking about making it an option. Because, when we add dynamic state fetching, we will have a complicated situation for "fast" clients. After querying transactions, client won't be typically fast, the extra state will require extra disk space. And generation of status by re-executing transactions is extra cost (and maybe someone doesn't need status field).

whilei · 2018-06-15T06:44:40Z

I like the design in general.

whilei · 2018-06-19T09:57:08Z

core/state_transition.go

 	}

 	requiredGas = new(big.Int).Set(self.gasUsed())

 	self.refundGas()
 	self.state.AddBalance(self.env.Coinbase(), new(big.Int).Mul(self.gasUsed(), self.gasPrice))

-	return ret, requiredGas, self.gasUsed(), err
+	return ret, requiredGas, self.gasUsed(), vmerr != nil, err


Seems kinda weird to return requiredGas, when it's just requiredGas = new(big.Int).Set(self.gasUsed())`...

Cleaned up.

whilei · 2018-07-12T12:35:10Z

eth/api.go

+	// Status field was introduced in ETH with EIP-609. For compatibility, we won't return status for
+	// blocks before the fork.
+	fields["status"] = nil
+	if receipt.Status != types.TxStatusUnknown && blockIndex >= 4370000 {


If we keep this, I think it should be configurable via the chain config... somehow.

Since this implementation is only in reference to a fork, but not actually a part of a fork, this is a strange case.

Otherwise, we just eliminate this condition and return status if available for all blocks regardless of chain or consensus schemes (since this deals only with the API, not protocol).

I really don't like keeping this API method 100% compatible with ETH.

IMHO we should keep one method (no extra methods in different namespaces), and "break" the compatibility (we're giving more functionality than ETH, and we're not compatible anyways right now).

Most useful solution will give us cleanest code.

I agree, there is no goal to make it 100% compatible but to add useful feature for users

solution: return new wrapped error inline

whilei · 2018-07-18T16:18:42Z

eth/api.go

@@ -1187,6 +1187,13 @@ func (s *PublicTransactionPoolAPI) GetTransactionReceipt(txHash common.Hash) (ma
 		if err != nil {
 			return nil, err
 		}
+
+		if err := core.WriteReceipts(s.chainDb, receipts); err != nil {
+			glog.V(logger.Warn).Infof("cannot save updated receipts: %v", err)


Do you think Warn/Info is an appropriate level/severity for this? Or even possibly critical failure/error?

whilei

LGTM.

eyfl · 2018-07-25T11:27:12Z

like other QUANTITY fields, like blockNumber ,cumulativeGasUsed and etc, is it better to make type of the field "status" as hex String? As web3 and web3j are both expects hex String , please refer to https://github.com/ethereum/wiki/wiki/JavaScript-API#web3ethgettransactionreceipt
and
https://github.com/web3j/web3j/blob/master/core/src/main/java/org/web3j/protocol/core/methods/response/TransactionReceipt.java and https://github.com/web3j/web3j/blob/master/utils/src/main/java/org/web3j/utils/Numeric.java

`public static BigInteger decodeQuantity(String value) {

    if (!isValidHexQuantity(value)) {
       throw new MessageDecodingException("Value must be in format 0x[1-9]+[0-9]* or 0x0");
    }
    try {
        return new BigInteger(value.substring(2), 16);
    } catch (NumberFormatException e) {
        throw new MessageDecodingException("Negative ", e);
    }
}`

Quantity fields are expected as hex string.
so, current implementation will result in web3 and web3j throw exception.

whilei reviewed Jun 15, 2018

View reviewed changes

whilei reviewed Jun 19, 2018

View reviewed changes

tzdybal force-pushed the feature/tx-status branch 2 times, most recently from 241acf0 to 2bf5b3f Compare July 11, 2018 23:37

whilei suggested changes Jul 12, 2018

View reviewed changes

tzdybal force-pushed the feature/tx-status branch 2 times, most recently from 1d22e1e to 0e3f485 Compare July 18, 2018 11:06

tzdybal changed the title ~~[WIP] getTransactionReceipt - dynamic status generation~~ getTransactionReceipt - dynamic status generation Jul 18, 2018

This was referenced Jul 18, 2018

[WIP] eth_getTransactionReceipt to return optional status field #431

Closed

New method - failed ETCDEVTeam/sputnikvm-ffi#2

Merged

tzdybal and others added 15 commits July 18, 2018 13:55

getTransactionReceipt - dynamic status generation

7a5e3f9

ReplayTransaction function renamed, comment added

d012f6f

EIP-609 block number restriction for tx status

4b03132

Playback of all transactions from a block

f0634b7

Decoding receipt RLP the right way

09a2417

Cleanup & error handling improvement

5ecf32e

problem: shadowed err variable

17dfe0d

solution: return new wrapped error inline

Use proper state root

f65a632

Writing updated receipts to disk.

0ec9f62

Transaction status defaults to Unknown

144c60c

Cleanup (EIP-609 compatibility, config var)

a97a72c

Transaction status from SputnikVM

7a63e21

CircleCI - use SputnikVM from vendor

52bad81

Remove duplicated return value.

d1b0a05

Clean SputnikVM binaries during make clean

584776b

tzdybal force-pushed the feature/tx-status branch from 21069fd to 584776b Compare July 18, 2018 11:55

whilei reviewed Jul 18, 2018

View reviewed changes

whilei added 2 commits July 19, 2018 09:25

Restart CI

9a04559

dep ensure: sputnikvm-ffi branch[vm-failed]->[master]

b65d29e

whilei approved these changes Jul 19, 2018

View reviewed changes

tzdybal merged commit b65d29e into master Jul 19, 2018

sorpaas mentioned this pull request Sep 20, 2018

eth_getTransactionReceipt always return status null for Ethereum Classic chain openethereum/parity-ethereum#9606

Closed

tzdybal mentioned this pull request Sep 25, 2018

How I detect that smart contract call failed ? #560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getTransactionReceipt - dynamic status generation #634

getTransactionReceipt - dynamic status generation #634

tzdybal commented Jun 15, 2018

whilei Jun 15, 2018

whilei Jun 15, 2018 •

edited

Loading

tzdybal Jun 15, 2018

whilei Jun 15, 2018

tzdybal Jun 15, 2018

whilei Jun 17, 2018

whilei Jun 15, 2018

whilei Jun 15, 2018

tzdybal Jun 15, 2018

whilei commented Jun 15, 2018

whilei Jun 19, 2018

tzdybal Jul 18, 2018

whilei Jul 12, 2018

tzdybal Jul 17, 2018

splix Jul 17, 2018

whilei Jul 18, 2018

whilei left a comment

eyfl commented Jul 25, 2018

getTransactionReceipt - dynamic status generation #634

getTransactionReceipt - dynamic status generation #634

Conversation

tzdybal commented Jun 15, 2018

Choose a reason for hiding this comment

whilei Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whilei commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whilei left a comment

Choose a reason for hiding this comment

eyfl commented Jul 25, 2018

whilei Jun 15, 2018 •

edited

Loading