rpc,eth/filters,internal/ethapi,core: EIP758

[reductionista]

Introduction

I've been working on implementing EIP758. I now have the basic functionality working, so I figure it's a good time to make an initial PR. Consider this a first draft--there are a couple features I still intend to add, as well as at least one bug fix. In the meantime, I'd like to start getting some feedback on my implementation.

See also my suggested modifications of EIP758, mentally compiled while working on this.

Overview

EIP758 is a proposal to allow rpc clients connecting to a node to receive return data back after sending a transaction which calls a contract method which changes the state on the blockchain. Prior to this, you could write a solidity function that changes state and then returns data, but this data was discarded by Geth (and Parity). This modification allows the connecting client to specify ahead of time that the data should be saved and sent back to the caller instead of being thrown away.

This functions in two different ways, one via the JSON-RPC command eth_subscribe. If the client has a full-duplex connection (ipc or ws), it can issue an eth_subscribe command with params= ["returnData"]. For any transactions submitted by the same client after that, the tx hash is saved internally while the transaction is pending, and the appropriate client is notified when it completes. If the client has only a half-duplex connection (http), then eth_subscribe is not allowed since there are no callbacks. Instead, eth_newReturnDataFilter is sent, the return data of subsequent transactions are stored up in a queue internally, and returned to the client when eth_getFilterChanges is called with the same subscription id.

Read the EIP758 proposal for more details. The only change I have made so far is using eth_newReturnDataFilter instead of eth_newFilter with params=ReturnData. I've done this because it makes the interface more consistent with eth_newBlockFilter and eth_newPendingTransactionFilter.

There is a second change I have proposed making, which I haven't implemented yet but am requesting feedback on here. Currently there is no way to know which transactions were submitted by which http clients, and match that up with who is listening to which filter. So if an http client subscribes to ReturnData, they will see all of the return data coming back from all transactions being executed on the blockchain. My proposal is to add a from field in the params of eth_newReturnDataFilter, so that they only listen to transactions sent from a specific address, or a list of addresses--not everyone's transactions. (I've also mentioned an alternative of adding a subID param to eth_sendTransaction and eth_sendRawTransaction, which would also solve the problem.)

Another improvement I plan to add soon is an optional bool param noEmpty, which can suppress notifications for transactions where there is no return data. For noEmpty=false, the client will still be notified when the requested transactions complete, but the return data field will be [].

Notes

There is potential for confusion between clients subscribing to pending transactions (where they get a notification with a tx receipt), and clients subscribing to completed transactions (where they get a notification containing the return data). Internally, I added a TransactionEvent, so now it co-exists with TransactionPendingEvent. In many cases there were local variables called things like txEvent to mean an event when a transaction is added to the pending txpool. I've renamed those to txPreEvent and just to be clear used txPostEvent for the new kind so there's less chance of confusion.

I've added a new feature of Notifier in rpc/subscription.go. There is now an update channel associated with it where you can send updates to the filter criteria while the filter is running. I've made it general, so it could work in a different way for each type of subscription and handle a different type for each. For ReturnData subscriptions, they start out not listening to anything, just associated with a particular client. Then each time a new transaction is submitted by that client, a new tx hash is added to a set of tx hashes stored in the filter. Only completed transactions in that set of tx hashes will result in a notification.

I've moved the definitions of different kinds of subscriptions from eth/filters/filter_system.go into rpc/subscription.go. They still exist in the first place, and can be accessed the same way, they just reference the ones in rpc/subscription.go now.

Still in Progress

I've tested eth_subscribe, eth_newReturnDataFilter, and eth_getFilterChanges over ipc with nc -U geth.ipc and all 3 appear to work. With the latter two, the only issue is that all new transactions are subscribed to rather than just a particular set--as above, I intend to improve that by adding a from option to params (although I think sometimes listening for all completed transactions could be useful as well). I have a lot more testing to do--and I should add some automated tests for it as well.

Two missing features in the present implementation are: proper handling of chain reorgs, and support for light nodes. I intend to work on those over the next few weeks.

4/25/2018 UPDATE:

I think I've got the logic for chain reorgs working now; need to test it more though, currently working on writing some tests for it. Still missing light client support and from & to filter criteria.

5/24/2018 UPDATE:

from/to filter criteria fully implemented now, including unit tests for that and reorg. Questions have been raised as to whether light client support is even a good idea to add, so this may be finished. Although the author of the EIP has indicated he wants support for including return data with removed=true notifications, to comply with the EIP, so I will also add that (previously I had assumed this would be unnecessary).

7/6/2018 UPDATE:

hasReturnData filter flag fully implemented now, unit test added for it.

Light client support is mostly working, although there is a memory leak bug and possibly at least one other bug I'm aware of with it. Because this is a much bigger change in the core functionality of geth (and therefore more risky) compared to the rest of the EIP, my feeling is it may be better to release it without light client support initially and plan on adding that later. Basically, it makes a light node behave more like a full node, in that it has to process every transaction. I've run some tests to assess how big the impact is on CPU usage, and it appears to be negligible. Nevertheless, it's a big conceptual change for what a light node is expected to do, and may require a bit more network I/O. There are no changes in the disk requirements of a light node however, so this remains as the key distinction between a light and a full node.

Aside from fixing the bugs in the light client support and deciding whether to include it, the only big unfinished task is including return data with removed=true events (which requires saving return data for a while in a cache, and replaying transactions if necessary). There is also the minor task of renaming everything from ReturnData to CompletedTransaction, in compliance with the recent revision to EIP758.

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 7dda3a7a3cd9e212ae1157b1177c09c665bab37d
• Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 7dda3a7a3cd9e212ae1157b1177c09c665bab37d
• Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 7dda3a7a3cd9e212ae1157b1177c09c665bab37d
• Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 7dda3a7a3cd9e212ae1157b1177c09c665bab37d
• Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[holiman]

Looks generally good, with testcases and documentation, and I support the functionality. Would like someone more familiar with the subscription API to give it a review, though.

[holiman]

I'm not sure about this.. As it is implemented here, every single transaction will trigger a go-routine, and I believe it will slow block processing down by quite a lot.

[reductionista]

I don't know whether the slowdown would be significant, but as it turns out I have to change this anyway. In order to handle chain reorgs, I've realized these events need to all get posted after the entire block is done, not individually after each one. I'm rewriting that already, so that this function returns the data and then it gets posted by the caller at the end of the block.

[holiman]

We usually use struct{}{} for lookup-maps, supposedly it's better -- less memory

[holiman]

Would it make sense to have this be a limited map, to prevent simple DoS attacks by stuffing millions of hashes in here via RPC?

[reductionista]

Sounds like a reasonable precaution--I'll take this and your struct{}{} suggestion.

[reductionista]

I changed my mind on this. There is already a limit on the number of pending and queued transactions which can be in the txpool. If the client submits more than that, it will not be accepted and an error is returned before it reaches this code--so I think adding any further limit would be redundant. Still going with struct{} instead of true though.

[holiman]

I must be missing something -- looks like you're setting all returndata to empty byte arrays here??

[reductionista]

Good catch--this line was supposed to be wrapped in if r.Data == nil { }. I made a last minute change to this just before submitting the PR which broke functionality--must have forgotten to retest. Thanks!

[reductionista]

"@reductionista requested review from @fjl @gballet and @gluk256 as code owners an hour ago."

@fjl, @gballet, and @gluk256 please ignore--I think the only reason it did this was because I rebased to master and there was a weird issue with travis.yml which caused a long sequence of commits on master to get appended to my branch. I've since cleaned it up.

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 711ccaf603a486bc58271b6069ac83c5cd1320d4
• Commit subjects should be kept under 100 characters

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[holiman]

We discussed this today. The general feedback is that a better solution to this problem would be for someone to propose an EIP where we put the return data into the recipet (alternatively a hash of the return data).
That would solve the problem for all users, not only the local node, and also historical transcations.

[reductionista]

I agree that putting return data in the receipt would be a better long-term solution. My understanding is that the Augur team knew this from the beginning, but decided to propose the EIP in the way it's written so as to avoid requiring a hard fork.

@holiman Since it's too late to go in Constantinople, does this mean the geth team has decide this should be put off until the next big network upgrade?

One advantage I see of doing it without changing consensus is that different clients can upgrade at different times, and dApp developers can gradually start switching over to a new paradigm where you can write functions that return data instead of just write out to logs. And the teams that could really benefit from it, like Augur, could start using it sooner. (Although ironically, if we'd gone with the receipt approach a year ago, it could have been ready to go out with Constantinople. What seemed like the faster approach ended up taking longer than expected and involving more complexity.)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fjl]

It's now one year later and the EIP is still in draft state. I'm going to close this PR now. If there is any interest to move forward with the change, please shout, and preferably bring up the EIP in an AllCoreDevs call by submitting it to the call agenda in the ethereum/pm repo.