-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add a new RPC method eth_getBlockReceipt
to get a block's "system calls" receipt
#1628
Conversation
52e3072
to
ca9373f
Compare
If systems calls made by celo-blockchain result in logs, we add a receipt including those logs, which lists the block's hash as the transaction hash field on the receipt. This PR adds support to eth_getTransactionReceipt to return this receipt when given a block hash. One key difference, however, is that no receipt is added in case there were no logs, whereas eth_getTransactionReceipt will return an empty receipt in such cases, so that the case of no logs can't be confused with the case of not having the block. We may in the future wish to change it so that the receipt is also created during block processing, but that would be a hard fork change.
ca9373f
to
a90e71f
Compare
Why do we have examples of contract creation and not contract creation? I guess those are regular transactions so should not be affected by this PR? Also, not sure if ease, but when creating an empty receipt it would good to use the txIndex that corresponds to "after the last tx". In the example was zero, but was not clear if the block was empty |
Correct, they're not affected, just including them as part of the testing (to confirm they're not affected). An earlier version I tried had a bug that broke that functionality.
It's ok as is, because the method's code sets that field (and some other "derived fields") in the return value:
So even though it isn't set on the Also, I will check the e2e transfer test which seems to be failing. |
The failing end-to-end-transfer-test is due to #1627 (the tests include sending transactions through light clients, and it seems like web3 polls using |
Updated the example of a block without a system receipt to use block 2937 (which contains one transaction) instead of block 10 (which doesn't contain any transactions). |
eth_getBlockReceipt
to get a block's "system calls" receipt
…hash (which includes 'pending')
Updated the PR to add a new method |
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
==========================================
+ Coverage 56.04% 56.07% +0.02%
==========================================
Files 606 606
Lines 80261 80261
==========================================
+ Hits 44980 45004 +24
+ Misses 31753 31727 -26
- Partials 3528 3530 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
If systems calls made by celo-blockchain result in logs, we add a receipt including those logs, which lists the block's hash as the transaction hash field on the receipt. This PR adds a new method,
eth_getBlockReceipt
to return this receipt when given a block hash or block number.One key difference, however, is that no receipt is added in case there were no logs, whereas
eth_getBlockReceipt
will return an empty receipt in such cases, so that the case of no logs can't be confused with the case of not having the block. We may in the future wish to change it so that the receipt is also created during block processing, but that would be a hard fork change.The method can be added to a web3 instance as follows (where
web3
is aWeb3
instance, as for example the one available in contractkit askit.web3
):Once this is merged, released, and deployed to services such as Forno and Figment, we should consider adding support for it in contractkit.
An earlier version of this PR added support for block receipts to
eth_getTransactionReceipt
, but after more consideration it was found that adding a separate method results in both a cleaner API and a cleaner implementation.Other changes
Factored out parts of
eth_getTransactionReceipt
to a separate function so that it can also be used in the new method.Tested
Manual testing using
geth attach
, against a client running lightest sync mode (light client) and against a client running fast sync mode (non-light client). For the ones using transaction hashes I also compared the values against those you get usingmaster
to see they don't change. Also confirmed that it works via direct RPC calls using http rather than withgeth attach
.Note that the results listed below (from
geth attach
) differ somewhat from the results you get via RPC. For example, some fields which are numbers here are given as hex strings in the RPC (block number, gas used, etc.). This is the same behavior as in other methods such aseth_getTransactionReceipt
,eth_getBlockByNumber
, etc.Mainnet, a block without a system receipt (note this block has one transaction, which is why the fake system receipt's transactionIndex is 1):
Mainnet, a block with a system receipt and no transactions:
Mainnet, a block with a system receipt and regular transactions:
Mainnet, transaction receipt with a contract creation transaction's hash (unchanged):
Mainnet, transaction receipt with a non-contract-creation transaction's hash (unchanged):
Mainnet, using a hash that does not correspond to any block:
Backwards compatibility
No changes to existing functionality, it only adds a new RPC method.