Skip to content
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 eth_getBlockReceipts() JSON/RPC method #5771

Merged
merged 13 commits into from
Aug 24, 2023

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Aug 18, 2023

PR description

Adds eth_getBlockReceipts JSON/RPC method as described by Ethereum API specification ethereum/execution-apis#438

Notes for PR reviewer & docs:

  • The specification says that the correct response for a block that cannot be found is an empty array, e.g. https://github.com/ethereum/execution-apis/blob/main/tests/eth_getBlockReceipts/get-block-receipts-future.io Besu appears to have a convention of returning a -32000 error code with message Block not found for such cases. I have done the same with this new JSON/RPC method but welcome any thoughts on this from the PR reviewer.
  • I don't think this query requires any changes/additions to graphql. The current schema covers the basic EVM objects (block, transaction, log etc) and is already designed to provide an alternative way of doing what this new JSON/RPC method adds (getting all receipts for a block in a single call) so I haven't made any graphql changes.

Fixed Issue(s)

Closes #5751

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001 matthew1001 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 21, 2023
@matthew1001 matthew1001 force-pushed the get-block-receipts branch 3 times, most recently from 468ddd8 to 084c740 Compare August 21, 2023 16:07
@matthew1001 matthew1001 changed the title Initial commit with new JSON/RPC method Add eth_getBlockReceipts() JSON/RPC method Aug 21, 2023
@matthew1001 matthew1001 marked this pull request as ready for review August 22, 2023 10:30
Signed-off-by: Matthew Whitehead <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved

@Test
public void blockNotFoundForRandomHash() {
/* Valid random hash - should result in block not found (effectively impossible for it to be a valid block) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the risk is low but this still makes me uneasy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah fair point, I'll remove that test for now.

TransactionReceiptResult tx1 = result.getResults().get(0);
assertThat(tx1.getBlockNumber()).isEqualTo("0x1");
assertThat(tx1.getEffectiveGasPrice()).isEqualTo("0x4a7ebf2e");
assertThat(tx1.getTo()).isEqualTo("0x6ada2e11049e5fc54fcdf2a97996d9b2aa80fe71");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want the tx values hard coded in the test since it makes it brittle - eg if you change the BLOCKCHAIN_LENGTH, these tests will fail. you can get the values from the generated blockchain itself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final Transaction expectedTx1 = blockchain.getBlockByNumber(BLOCKCHAIN_LENGTH - 1).get().getBody().getTransactions().get(0);

// Check TX1 receipt is correct
TransactionReceiptResult tx1 = result.getResults().get(0);
assertThat(tx1.getBlockNumber()).isEqualTo("0x"+(BLOCKCHAIN_LENGTH-1));
assertThat(tx1.getEffectiveGasPrice()).isNotEmpty();
assertThat(tx1.getType()).isEqualTo(expectedTx1.getType());
assertThat(tx1.getTo()).isEqualTo(expectedTx1.getTo().get().toString());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point. I'll make those suggested changes so the test is more reliable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit updates the unit tests as you've suggested @macfarla. I added getEthSerializedType() to TransactionType so there is a single place in the codebase where FRONTIER transactions are mapped from their enum value of 0xf8 to their eth serialized value of 0x00.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also have a look at EthJsonRpcHttpBySpecTest - maybe add some "by spec" tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer. I'd found the graphql spec tests but not spotted general JSON/RPC spec tests. Latest commit adds a variety to cover by hash, by number, and by tag.

matthew1001 and others added 4 commits August 23, 2023 08:40
…sonrpc/internal/methods/EthGetBlockReceiptsTest.java

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
…sactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001
Copy link
Contributor Author

I think all of the latest comments are addressed @macfarla I'm away for a few days now, feel free to merge if you're happy or if you have more comments I'm happy to take a look when I'm back.

Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla enabled auto-merge (squash) August 24, 2023 01:50
@macfarla macfarla merged commit 9a0f35c into hyperledger:main Aug 24, 2023
8 checks passed
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
* Initial commit with new JSON/RPC method

Signed-off-by: Matthew Whitehead <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>

* Update unit tests to check receipts against generated blockchain transactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>

* Add spec JSON/RPC tests

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
* Initial commit with new JSON/RPC method

Signed-off-by: Matthew Whitehead <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>

* Update unit tests to check receipts against generated blockchain transactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>

* Add spec JSON/RPC tests

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
* Initial commit with new JSON/RPC method

Signed-off-by: Matthew Whitehead <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>

* Update unit tests to check receipts against generated blockchain transactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>

* Add spec JSON/RPC tests

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: garyschulte <[email protected]>
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 31, 2023
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* Initial commit with new JSON/RPC method

Signed-off-by: Matthew Whitehead <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>

* Update unit tests to check receipts against generated blockchain transactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>

* Add spec JSON/RPC tests

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
* Initial commit with new JSON/RPC method

Signed-off-by: Matthew Whitehead <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>

* Update unit tests to check receipts against generated blockchain transactions at runtime. Add getEthSerializedType() utility to TransactionType.

Signed-off-by: Matthew Whitehead <[email protected]>

* Add spec JSON/RPC tests

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement eth_getBlockReceipts
3 participants