Skip to content

feat(eth)!: add Eth APIs to /v2 + minor improvements & fixes#13026

Merged
masih merged 1 commit intomasterfrom
rvagg/eth-v2
Apr 24, 2025
Merged

feat(eth)!: add Eth APIs to /v2 + minor improvements & fixes#13026
masih merged 1 commit intomasterfrom
rvagg/eth-v2

Conversation

@rvagg
Copy link
Copy Markdown
Member

@rvagg rvagg commented Apr 10, 2025

Very draft for now, mainly focused on keeping /v1 working without molesting it too much. The beginnings of /v2 are in here already, it's just not wired up to the main struct yet .. or tested.

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 11, 2025

Got it all wired up now. I've tried to improve the state of some of the things in here but it's quite tricky.

The most interesting part of this as far as v2 is concerned is in the TipSetResolver in node/impl/eth/tipsetresolver.go which behaves differently if it's given an F3Backend or not. Then in the DI system we construct a V1 and V2 variant of this, and all our Eth modules have both V1 and V2 forms which take the right kind of TipSetResolver.

Misc things I've done in here that apply to v1 as well that I'll need to note in the changelog:

  • Moved api.EthTxReceipt to ethtypes.EthTxReceipt where all its friends are. I'm not sure why it was there, all alone, I'm going to assume a mistake. I've left an alias and a deprecation notice.
  • I've changed the way that EthGetTransactionByHashLimited, EthGetTransactionReceiptLimited and EthGetBlockReceiptsLimited are handled on the gateway. Like was already done for EthSendRawTransactionUntrusted I've made it so they're not usable on the gateway. They weren't really doing any harm on the gateway (unlike EthSendRawTransactionUntrusted but they just don't belong as first-class citizens, so I could remove them from api.Gateway although internally they still exist thanks to DI but they "not supported" error.
  • EthGetBlockTransactionCountByNumber now takes a blkNum string instead of a blkNum ethtypes.EthUint64. It's an odd-man-out in the APIs by taking strictly a Uint64 and not allowing labels ("finalized" etc.). I believe this was an oversight originally, eth_getBlockTransactionCountByNumber is supposed to take a flexible input type.
  • I will also give similar treatment to EthGetLogs because the "fromBlock" and "toBlock" parameters are supposed to take more labels than we allow, but it won't be a breaking change.

Still need to add more test coverage and I also need to exercise the behaviour of F3 for "safe" and "latest" for both v1 and v2.

@rvagg rvagg force-pushed the rvagg/eth-v2 branch 2 times, most recently from 5036b50 to 88ee667 Compare April 14, 2025 23:47
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 14, 2025

Making good progress here but the tests are getting tedious. Half of the impacted endpoints are being tested so far.

@rvagg rvagg marked this pull request as ready for review April 15, 2025 11:13
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 15, 2025

Calling this Ready for Review although I still need to write the CHANGELOG entry (so I'm leaving that failing). I've expanded support for tags in the /v1 API in the process of doing this so I need to make clear what's changing, not just for /v2 additions. But the code and tests I think are good to go.

@rvagg rvagg changed the title feat(eth)!: add Eth APIs to /v2, fix minor incompatibilities (DRAFT) feat(eth)!: add Eth APIs to /v2, fix minor incompatibilities Apr 15, 2025
@rvagg rvagg requested review from aarshkshah1992 and masih April 15, 2025 11:14
@rvagg rvagg force-pushed the rvagg/eth-v2 branch 2 times, most recently from 1d699e9 to 3a4b1f7 Compare April 16, 2025 02:21
Comment thread api/api_full.go
Comment thread api/api_gateway.go
Comment thread api/api_full.go
Comment thread build/buildconstants/params_shared_vals.go
Comment thread node/impl/eth/api.go
Comment thread node/impl/eth/events.go
Comment thread node/impl/eth/tipsetresolver.go Outdated
Comment thread node/impl/eth/transaction.go
Comment thread node/impl/eth/transaction.go
Comment thread node/impl/eth/utils.go
Comment thread node/impl/full/eth.go
@rvagg rvagg changed the title feat(eth)!: add Eth APIs to /v2, fix minor incompatibilities feat(eth)!: add Eth APIs to /v2 + minor improvements & fixes Apr 16, 2025
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 18, 2025

Addressed all of the feedback, and found some more things to deal with too. The major change now is wrt "safe", this is described in the CHANGELOG as:

  • /v2 APIs (New & Recommended): These APIs fully leverage F3 finality, significantly reducing confirmation times compared to the previous fixed delays.
    • "finalized" tag maps directly to the F3 finalized epoch (much closer to the chain head).
    • "safe" tag maps to the F3 finalized epoch or 200 epochs behind head, whichever is more recent.
  • /v1 APIs (Existing): These maintain behavior closer to pre-F3 Lotus for compatibility.
    • "finalized" tag continues to use a fixed 900-epoch delay from the head.
    • "safe" tag uses a 30-epoch delay or the F3 finalized epoch (whichever is more recent). After F3 is fully active on the network, the behavior of "safe" on /v1 will align with /v2's "safe".

i.e. when F3 is activated, both /v1 and /v2's "safe" will be F3-finalized or head-200, whichever is highest. We did discuss what it might mean if F3 was activated but finalizing something older than head-200 and whether it was really "safe", but for now we're going to go with this and maybe adjust if we have reason to believe this isn't a good idea when we have more real-world experience with F3. Maybe this could even be reduced if F3 is behaving well and has solid fallback behaviour when it can't make a decision (or we get the EC finality calculator implemented to help us make decisions).

I've also removed the -1 from "safe" and "finalized", that's a change in behaviour for /v1 too.

Comment thread node/impl/eth/tipsetresolver.go Outdated
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 21, 2025

This is not ready to be merged, latest discussion about what's needed here requires me to make the following changes:

  • NO behaviour change for "safe" for /v1 API as long as F3's manifest is EC.Finalize=false; i.e. we are in passive testing mode. We should not be using F3 to change any /v1 behaviour until it's finalizing.
  • Change the CHANGELOG docs for v2 APIs to be much clearer about the fact that these APIs are going to return F3 results based on the passive testing we're doing and they should not be relied upon for any serious notion of "finalized" or "safe" because what F3 is returning is not impacting EC yet. We should also call out how you can determine when F3 is finalizing by getting the manifest from the API.

@BigLep
Copy link
Copy Markdown
Member

BigLep commented Apr 22, 2025

Lets talk during 2025-04-22 standup on how we get this merged.

My thoughts:

  1. Lets not touch /v1 at all. I understand the desire to reduce difference between what "finalized" and "safe" mean in /v1 and /v2, but that is added scope in my opinion. I think we should be laser focused on providing a way for builders to start using F3-aware APIs as soon as possible.
  2. Having clear docs here makes sense. To be clear, our messaging is:
    • v2 APIs are highly experimental and can change at any point currently without concern for breakage.
    • v2 APIs are F3 aware, regardless if F3 is being used for finalizing. Basically if the F3 subsystem is enabled, then F3 will be consulted regardless of the value of manifest.EC.Finalize
    • These are the steps for seeing whether the F3 subsystem is being used to finalize: ...

@masih
Copy link
Copy Markdown
Member

masih commented Apr 22, 2025

v2 APIs are F3 aware, regardless if F3 is being used for finalizing.

To refine this slightly: V2 APIs are F3 aware as long as F3 is enabled, regardless of F3 activation on mainnet. It really should be obvious that no-one can rely on F3 in mainnet until it is ...activated!

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 23, 2025

Lets not touch /v1 at all

I'm find with this and will adjust, although the process of getting here has made some improvements to both forms of the eth APIs which I'll document but I'll leave "safe" and "finalized" alone for now.

But, we should note the original reason we started touching them here and not lose track of this as a future area of work. The 200 epoch lookback reflects a pessimistic view that if F3 isn't operational or delayed for some reason, then something must not be going well, so we take a very conservative view that EC has the potential for some major reorganisation if F3 is experiencing a hiccup. In that case, we're still going to be offering 30 to /v1 users, which is probably not great. So the proposal here was to match the pessimistic 200 of /v2 but give them finality as a consolation so in normal behaviour their "safe" is actually sooner than it was before.

@BigLep
Copy link
Copy Markdown
Member

BigLep commented Apr 23, 2025

Lets not touch /v1 at all

I was saying this on principle but hadn't looked at the PR. I looked at it more today and I see there is more nuance. Apologies if I'm causing whiplash here - my intent was to simplify the conversation so we could hopefully land something sooner without a lot of discussion.

Going through the changelog:

/v2 "finalized" tag maps directly to the F3 finalized epoch (much closer to the chain head).

👍

/v2 "safe" tag maps to the F3 finalized epoch or 200 epochs behind head, whichever is more recent.

👍

/v1 "finalized" tag continues to use a fixed 900-epoch delay from the head.

👍

"safe" tag uses a 30-epoch delay or the F3 finalized epoch (whichever is more recent). After F3 is fully active on the network, the behavior of "safe" on /v1 will align with /v2's "safe".

The rationale makes sense and maybe this is where we want to end eventually? If the code is easier to reason about if we have consistency between /v1 and /v2 then I'm more apt to leave what you have or at least just simplify to assuming F3 is active since it will be shortly. I can also see the mindset of "don't change the semantics of v1 and we potentially improve it later".

Both: Previously, "finalized" and "safe" tags referred to epochs N-1. This -1 offset has been removed in both V1 and V2.

👍

Both: eth_getBlockTransactionCountByNumber now accepts standard Ethereum block specifiers (hex numbers or labels like "latest", "safe", "finalized").

👍

Both: Methods accepting BlockNumberOrHash now support all standard labels ("pending", "latest", "safe", "finalized"). This includes eth_estimateGas, eth_call, eth_getCode, eth_getStorageAt, eth_getBalance, eth_getTransactionCount, and eth_getBlockReceipts.

👍

Both: Removed internal Eth*Limited methods (e.g., EthGetTransactionByHashLimited) from the supported gateway API surface.

👍

Both: Improved error handling: block selection endpoints now consistently return ErrNullRound (and corresponding JSONRPC errors) for null tipsets.

👍

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 24, 2025

Pushed changes that roll back the use of F3 in /v1 at all but note this may change in the future. I was going to roll back the -1 offset too but decided that it may as well be addressed now since I'm touching all of this and we have new opinions on tipset vs parent for /v2, so I left that.

Worked on the docs a bit too, but feel free to hack at that to make it clearer.

Copy link
Copy Markdown
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Thanks for scoping down the changes @rvagg .


I have one comment about if/when we fallback to EC.


I see /v2 also has support for


I can take a stab at documenting the /v2 APIs for https://www.notion.so/filecoindev/Filecoin-V2-APIs-1d0dc41950c1808b914de5966d501658 (sort like how started on this in #13051 for the safe tag). My approach would be to say soemthing along the lines that /v2 ETH APIs are basically the same to /v1 ETH APIs but that finalized and safe have different meaning. I'd explain what safe and finalized mean in /v2. I'd then link to https://github.com/filecoin-project/lotus/blob/master/documentation/en/api-v2-unstable-methods.md for the API listing rather than generating docs in Notion for all of them.

Comment thread CHANGELOG.md
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Apr 24, 2025

I see /v2 also has support for

.. I've updated the CHANGELOG to have a bit more detail, I was intending it to be implicit that they're all covered. Everything in https://github.com/filecoin-project/lotus/blob/master/api/eth_aliases.go is covered, those are standard Ethereum calls (or Erigon), plus our couple of custom extra addressing ones that we don't alias.

Comment thread CHANGELOG.md
Comment thread CHANGELOG.md
@masih masih enabled auto-merge (squash) April 24, 2025 11:24
@masih masih merged commit 8c51a81 into master Apr 24, 2025
93 of 94 checks passed
@masih masih deleted the rvagg/eth-v2 branch April 24, 2025 11:26
@github-project-automation github-project-automation Bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Apr 24, 2025
@github-project-automation github-project-automation Bot moved this from In review to Done in F3 Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done
Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

5 participants