types, rawdb, core, miner: add block location fields to receipt #17662

bmperrea · 2018-09-12T21:19:55Z

Solves #15210 without changing consensus, in a backwards compatible way, by adding the block location fields from ReadReceipt to the Receipt struct.

Update: In particular this improves consistency between the go client interface and the json rpc spec and allows go clients to use transaction receipts to watch for block confirmations for particular transactions - see #15210.

bmperrea · 2018-09-25T15:29:49Z

@karalabe @holiman any chance for a review on this 32 line pr?

bmperrea · 2018-10-01T16:13:16Z

@karalabe @holiman are there any concerns with getting this through as it is now?

favadi · 2018-10-02T02:13:32Z

@bmperrea probably they just had no time to do a review yet.

jgiles · 2018-10-12T11:47:51Z

@karalabe any chance for a review on this?

holiman

In general, I approve, but I don't know enough about how the different kinds of marshalling occurs, and whether this may break something. I'll have to defer to @fjl or @karalabe

holiman · 2018-10-15T12:16:10Z

core/types/gen_receipt_json.go

@@ -47,6 +54,9 @@ func (r *Receipt) UnmarshalJSON(input []byte) error {
 		TxHash            *common.Hash    `json:"transactionHash" gencodec:"required"`
 		ContractAddress   *common.Address `json:"contractAddress"`
 		GasUsed           *hexutil.Uint64 `json:"gasUsed" gencodec:"required"`
+		BlockHash         *common.Hash    `json:"blockHash"`
+		BlockNumber       *hexutil.Big    `json:"blockNumber"`
+		TransactionIndex  *hexutil.Big    `json:"transactionIndex"`


Perhaps good to have omitEmpty on the json-part?

Great idea. Adding it now.

core/types/receipt.go

bmperrea · 2018-10-15T22:00:44Z

Thanks for the helpful review @holiman ! Do you still want to wait on a response from @fjl or @karalabe ?

holiman · 2018-10-25T13:29:24Z

That would be good, yes, ping @fjl

bmperrea · 2018-11-14T20:37:51Z

@karalabe @fjl @holiman Could we get a decisive review on this one? Thanks!

karalabe

This PR adds a few fields to the receipt which only ever get initialized when read from the database. What happens when we receive a receipt from the network? The fields will be empty. What happens if we mine and generate a receipt ourselves? The fields will be empty.

These incomplete receipts will end up in various caches and will be returned on our APIs. Unless you ensure all paths that ever create a receipt also correctly initializes these fields, it's not possible to accept with PR.

You also updated the JSON output with extra fields, but the JSON is adhering to the standard Ethereum JSON RPC spec. Adding anything only in Geth will fragment the ecosystem because all of a sudden we have information other clients don't report. Or alternatively, our ethclient expects fields that other clients don't provide. The correct way here is to open an EIP that chnges the RPC spec so all clients implement it and we can also enforce it properly.

bmperrea · 2018-11-16T12:59:20Z

thanks @karalabe 👍. Is the rpc spec you're referring to documented somewhere? I see block location fields are included here https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_gettransactionreceipt

bmperrea · 2018-11-17T05:17:10Z

Great point about mined and downloaded receipts @karalabe . I'm excited to improve the geth go-client interface to make sure we keep consistent with the rpc spec which includes these location fields. I've updated the PR to add code to take care of these cases in miner/worker.go and core/blockchain.go respectively. These are the same place where location-based fields are added to the log objects within the receipt.

This pr makes the receipt object in the go-client interface more consistent with the rpc spec while also keeping consistent with how location fields are set for log entries.

bmperrea · 2018-11-27T14:56:31Z

In general there are significant differences between the rpc api and the ethclient go client api. For instance
TransactionByHash(ctx context.Context, txHash common.Hash) (tx *types.Transaction, isPending bool, err error) of the go client (interfaces.go) vs
func (s *PublicTransactionPoolAPI) GetTransactionByHash(ctx context.Context, hash common.Hash) *RPCTransaction (ethapi/api.go)

@karalabe is right that these missing block location fields, as well as the differences between RPCTransaction and types.Transaction are a problem. They are fragmenting the ecosystem because we are missing information that other clients report.

This PR is only the beginning of fixing the broad issue.
We should put together and EIP to discuss the best way to match the ethclient interface to the RPC spec. One long-term option would be to make the json rpc a serialized version of the same struct returned by the ethclient.

holiman · 2018-12-13T09:43:01Z

@karalabe it's your call, please decide on how to move forward

rjl493456442 · 2019-01-22T09:56:35Z

Hi, actually I don't think we need to add these additional fields into the receipt struct. We can retrieve these data in txLookup structure. Maybe we need this additional information in the RPC side, we can just retrieve these from the database and assemble the response.

And actually what we are doing now is "try to cut some unnecessary fields" to save disk usage. Like in your PR you add three new fields which occupy 48bytes for each receipt. It can abuse the disk storage. For more information check #17106

holiman · 2019-02-21T09:45:09Z

We'll add this after #17106 is in

karalabe

In core/state_processor.go we are creating new receipts when applying a transaction. You need to add these computed fields there too.

bmperrea · 2019-02-21T19:39:11Z

Thanks @holiman @karalabe . I rebased and added the block location fields in state_processor.

To add the transaction index in core/state_processor.go I ended up exposing the current transaction index from the statedb. The other option would be to add that index as an argument to statedb.ApplyTransaction since each caller of that function needs to have the transaction index to call statedb.Prepare right before to setup that data. I think exposing from statedb is a reasonable approach given that the current pattern passes the transaction index in Prepare and the getter is not so different from other methods in core/state/statedb.go.

Good work @rjl493456442 on #17106 . The separation of Receipt struct from the corresponding rlp and db objects allows for storage optimization while still allowing us to add context that a client would expect to have available when a Receipt object is returned, and the same goes for other types returned by ethclient.

fjl · 2019-03-21T12:01:01Z

This change is going to help with ethclient because it will make it possible to fetch inclusion info through TransactionReceipt

fjl · 2019-03-21T12:02:47Z

core/types/receipt.go

+	// Block location fields - not part of the rlp
+	BlockHash        common.Hash `json:"blockHash,omitempty"`
+	BlockNumber      *big.Int    `json:"blockNumber,omitempty"`
+	TransactionIndex *big.Int    `json:"transactionIndex,omitempty"`


TransactionIndex can just be a regular uint. Maybe it should be called BlockIndex to make it clear that it's the index in the block.

~~I'll change the type~~ Thanks for changing the type 👍

I got the name TransactionIndex from this RPC spec. I'm hoping we can continue toward ethclient implementing that entire spec.

Yeah, it can be called TransactionIndex, it's fine. ethclient will never implement the full RPC API because it is based on core/types and some of the concepts in the web3 RPC API cannot be represented using the types. One example is transaction sender and inclusion info: the RPC API returns inclusion info and sender in the transaction object, but it would be silly to add those fields to types.Transaction).

I would rather see us separate the interface struct from the storage struct than compromise the interface. ethclient could return everything in rpcTransaction. Otherwise it's dropping fields that are returned by the geth server.

It would be good to have a go client that is as functional as rpc clients in other languages and I think ethclient should be that interface.

I'll try to explain the motivation for the current API in a different way: when making ethclient I wanted an API that could be implemented on the server side as well as the client side. The interfaces in the root directory should be general enough to allow efficient implementation on both sides. From that PoV the web3 RPC interface is stupid: it requires us to dig up receipts when transactions are queried to add inclusion info, etc. If you are building an app in Go, you should just be able to embed the light client in your app and access the blockchain using efficient operations. ethclient was supposed to be the drop-in replacement for the light client in cases where you want to get the same info from an RPC provider like Infura.

Unfortunately we never got around to implementing the interfaces natively, without RPC in the middle. That's why we're stuck with an API that doesn't match the RPC API.

I still want to get to the native implementation done. If you're seriously interested in improving Go developer experience using go-ethereum as a library, I'd be super happy if you could help us make that happen by implementing the existing interfaces on the server side.

holiman · 2019-03-22T10:57:40Z

core/state_processor.go

@@ -121,6 +121,9 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo
 	// Set the receipt logs and create a bloom for filtering
 	receipt.Logs = statedb.GetLogs(tx.Hash())
 	receipt.Bloom = types.CreateBloom(types.Receipts{receipt})
+	receipt.BlockHash = header.Hash()
+	receipt.BlockNumber = header.Number
+	receipt.TransactionIndex = uint(statedb.TxIndex())


A better solution might be to set them in Process, since you can reuse the hash instead of recalculating the header hash for every tx.

The drawback, I guess, is that direct usage of ApplytTransaction won't get it, which I guess applies to the miner, the txpool (?) and various users of chain_makers in tests.

@holiman fixed it now by adding an accessor for block hash to StateDB. The PR already adds an accessor for TxIndex, one more can't hurt.

holiman · 2019-03-25T17:47:16Z

Lgtm!

zulhfreelancer · 2019-03-28T04:28:22Z

Finally. Thank you guys!

Solves ethereum#15210 without changing consensus, in a backwards compatible way, by adding tx inclusion information to the Receipt struct.

bmperrea requested review from holiman and karalabe as code owners September 12, 2018 21:19

bmperrea mentioned this pull request Sep 13, 2018

Get BlockNumber from Transaction #15210

Closed

holiman reviewed Oct 15, 2018

View reviewed changes

bmperrea force-pushed the bmperrea/receipt-with-blocknumber branch from 5d00090 to 444dd6f Compare October 15, 2018 17:30

karalabe requested changes Nov 16, 2018

View reviewed changes

bmperrea force-pushed the bmperrea/receipt-with-blocknumber branch from 444dd6f to 9b2bbe8 Compare November 17, 2018 05:03

bmperrea changed the title ~~types, rawdb: add block location fields to receipt~~ types, rawdb, core, miner: add block location fields to receipt Nov 17, 2018

holiman assigned karalabe Dec 13, 2018

holiman added this to the 1.9.0 milestone Feb 21, 2019

karalabe requested changes Feb 21, 2019

View reviewed changes

types, rawdb, core, miner: add block location fields to receipt

d797d25

bmperrea force-pushed the bmperrea/receipt-with-blocknumber branch from 9b2bbe8 to d797d25 Compare February 21, 2019 19:36

fjl reviewed Mar 21, 2019

View reviewed changes

fjl mentioned this pull request Mar 21, 2019

Add a way to get the BlockNumber from ethclient. Fixes #15210 #15847

Closed

Change transaction index to uint

a6a9df1

holiman reviewed Mar 22, 2019

View reviewed changes

core/state: add accessor for block hash to remove extra header hash

9a02115

holiman approved these changes Mar 26, 2019

View reviewed changes

fjl merged commit 7fb8969 into ethereum:master Mar 27, 2019

gzliudan mentioned this pull request Apr 11, 2024

types, core, miner: add block location fields to receipt XinFinOrg/XDPoSChain#520

Merged

19 tasks

gzliudan mentioned this pull request Aug 19, 2024

reduce chain database size XinFinOrg/XDPoSChain#570

Open

gzliudan mentioned this pull request Jan 23, 2025

all: reduce the chain db size XinFinOrg/XDPoSChain#816

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types, rawdb, core, miner: add block location fields to receipt #17662

types, rawdb, core, miner: add block location fields to receipt #17662

bmperrea commented Sep 12, 2018 •

edited

Loading

bmperrea commented Sep 25, 2018

bmperrea commented Oct 1, 2018

favadi commented Oct 2, 2018

jgiles commented Oct 12, 2018

holiman left a comment

holiman Oct 15, 2018

bmperrea Oct 15, 2018

bmperrea commented Oct 15, 2018

holiman commented Oct 25, 2018

bmperrea commented Nov 14, 2018

karalabe left a comment

bmperrea commented Nov 16, 2018

bmperrea commented Nov 17, 2018

bmperrea commented Nov 27, 2018

holiman commented Dec 13, 2018

rjl493456442 commented Jan 22, 2019

holiman commented Feb 21, 2019

karalabe left a comment

bmperrea commented Feb 21, 2019

fjl commented Mar 21, 2019

fjl Mar 21, 2019

bmperrea Mar 22, 2019 •

edited

Loading

fjl Mar 25, 2019

bmperrea Mar 25, 2019 •

edited

Loading

fjl Mar 25, 2019 •

edited

Loading

fjl Mar 25, 2019 •

edited

Loading

holiman Mar 22, 2019

fjl Mar 25, 2019

holiman commented Mar 25, 2019

zulhfreelancer commented Mar 28, 2019

types, rawdb, core, miner: add block location fields to receipt #17662

types, rawdb, core, miner: add block location fields to receipt #17662

Conversation

bmperrea commented Sep 12, 2018 • edited Loading

bmperrea commented Sep 25, 2018

bmperrea commented Oct 1, 2018

favadi commented Oct 2, 2018

jgiles commented Oct 12, 2018

holiman left a comment

Choose a reason for hiding this comment

holiman Oct 15, 2018

Choose a reason for hiding this comment

bmperrea Oct 15, 2018

Choose a reason for hiding this comment

bmperrea commented Oct 15, 2018

holiman commented Oct 25, 2018

bmperrea commented Nov 14, 2018

karalabe left a comment

Choose a reason for hiding this comment

bmperrea commented Nov 16, 2018

bmperrea commented Nov 17, 2018

bmperrea commented Nov 27, 2018

holiman commented Dec 13, 2018

rjl493456442 commented Jan 22, 2019

holiman commented Feb 21, 2019

karalabe left a comment

Choose a reason for hiding this comment

bmperrea commented Feb 21, 2019

fjl commented Mar 21, 2019

fjl Mar 21, 2019

Choose a reason for hiding this comment

bmperrea Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

fjl Mar 25, 2019

Choose a reason for hiding this comment

bmperrea Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

fjl Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

fjl Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

holiman Mar 22, 2019

Choose a reason for hiding this comment

fjl Mar 25, 2019

Choose a reason for hiding this comment

holiman commented Mar 25, 2019

zulhfreelancer commented Mar 28, 2019

bmperrea commented Sep 12, 2018 •

edited

Loading

bmperrea Mar 22, 2019 •

edited

Loading

bmperrea Mar 25, 2019 •

edited

Loading

fjl Mar 25, 2019 •

edited

Loading

fjl Mar 25, 2019 •

edited

Loading