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

Return rpc error in eth_getLogs on unknown block hash #8051

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonson
Copy link

@jonson jonson commented Dec 18, 2024

PR description

Fixed Issue(s)

Fixes #8007

The issue describes the problem well, so I won't repeat it here.

The root cause is org.hyperledger.besu.ethereum.api.query.BlockchainQueries.matchingLogs detects an unknown hash and returns an empty list. This was the desired behaviour at some point in time, as there was even a test to validate it.

I have updated the method to throw an exception in this situation now, as IMO it really is a bad input if it's being called with an unknown hash. This has potentially larger impacts as there are other callers of this function, but to my knowledge they are all passing in a valid hash (acquired via a BlockHeader). An alternative approach could be to just patch EthGetLogs and check the block header prior to calling matchingLogs, which limits the impact.

I'm not sure if it's ok to be throwing an InvalidJsonRpcParameters from within the org.hyperledger.besu.ethereum.api.query package, there do not seem to be any other imports, so let me know if that should be cleaned up (I originally was going to raise a new exception type here, but I also noticed this package doesn't have any custom exceptions either).

Finally I am re-using the existing RpcErrorType.BLOCK_NOT_FOUND for this response, as I did not want to introduce a type to keep the change small. But I can if that is better.

ℹ️ I could not run the acceptance tests locally, they killed my (high-end) laptop.

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@jonson jonson force-pushed the eth_getlogs-error-on-unknown-hash branch from 8b7fb2f to a61efeb Compare December 18, 2024 22:45
@macfarla
Copy link
Contributor

change looks OK. see if you can follow the instructions to fix the DCO https://github.com/hyperledger/besu/pull/8051/checks?check_run_id=34624508989

Comment on lines +412 to +420
try {
queries.matchingLogs(Hash.ZERO, new LogsQuery.Builder().build(), () -> true);
} catch (final InvalidJsonRpcParameters e) {
assertThat(e.getMessage()).isEqualTo("Unknown block hash");
assertThat(e.getRpcErrorType()).isEqualTo(RpcErrorType.BLOCK_NOT_FOUND);
return;
}

fail("Expected InvalidJsonRpcParameters exception");
Copy link
Contributor

Choose a reason for hiding this comment

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

assertThrows is used in other parts of the codebase to assert exceptions

    var exception =
        assertThrows(
            InvalidJsonRpcParameters.class,
            () -> queries.matchingLogs(Hash.ZERO, new LogsQuery.Builder().build(), () -> true));
    assertThat(exception.getMessage()).isEqualTo("Unknown block hash");
    assertThat(exception.getRpcErrorType()).isEqualTo(RpcErrorType.BLOCK_NOT_FOUND);

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.

eth_getLogs must return "block unknown" when blockHash is not found
3 participants