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

feat(rpc) Implement Filecoin.EthGetLogs #4780

Open
wants to merge 83 commits into
base: main
Choose a base branch
from
Open

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented Sep 19, 2024

Summary of changes

Changes introduced in this pull request:

  • Partially implement RPC method Filecoin.EthGetLogs
./forest-tool api compare --lotus /ip4/127.0.0.1/tcp/1234/http --forest /ip4/127.0.0.1/tcp/2345/http forest_snapshot_calibnet_2024-10-03_height_2021383.forest.car.zst --filter EthGetLogs -n 800

| RPC Method                | Forest            | Lotus |
|---------------------------|-------------------|-------|
| Filecoin.EthGetLogs (272) | CustomCheckFailed | Valid |
| Filecoin.EthGetLogs (528) | Valid             | Valid |

This PR introduces a new flag/configuration setting (store_events) to retain events in the Tipset cache.

If neither the RPC API nor the methods related to events are used, a value of false can be passed.

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@@ -1668,16 +1718,21 @@ where

// FVM requires a stack size of 64MiB. The alternative is to use `ThreadedExecutor` from
// FVM, but that introduces some constraints, and possible deadlocks.
stacker::grow(64 << 20, || -> anyhow::Result<(Cid, Cid)> {
stacker::grow(64 << 20, || -> anyhow::Result<StateOutput> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much better than Cid tuple!

@@ -1582,7 +1627,8 @@ pub fn apply_block_messages<DB>(
tipset: Arc<Tipset>,
mut callback: Option<impl FnMut(MessageCallbackCtx<'_>) -> anyhow::Result<()>>,
enable_tracing: VMTrace,
) -> Result<CidPair, anyhow::Error>
store_events: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend not using a boolean here - the method has already plenty of parameters and it's cumbersome for the reader what is foo(true, false). A nicer way is above, with an enum - it'd be more readable to have apply_block_messages(..., StoreEvents::Enabled). Additionally, it's more extendible.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. We can either use another enum. But could we make use of bitflags? (and shave off on param numbers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, an enum is the best fit here.

@LesnyRumcajs
Copy link
Member

Can we have unit tests for this implementation?

@elmattic
Copy link
Contributor Author

Can we have unit tests for this implementation?

Sure.

@ruseinov ruseinov added this pull request to the merge queue Oct 11, 2024
@elmattic elmattic removed this pull request from the merge queue due to a manual request Oct 11, 2024
@elmattic
Copy link
Contributor Author

@ruseinov Not too fast, buster!

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.

4 participants