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

Eth API: harden parseBlkParam against null rounds #10426

Closed
raulk opened this issue Mar 8, 2023 · 2 comments
Closed

Eth API: harden parseBlkParam against null rounds #10426

raulk opened this issue Mar 8, 2023 · 2 comments
Assignees
Milestone

Comments

@raulk
Copy link
Member

raulk commented Mar 8, 2023

Problem

Many Eth API calls can take a block parameter, which can be a tagged value (earliest, pending, latest) or a concrete epoch (e.g. 0x1234). When supplied a concrete epoch, this method uses GetTipsetByHeight requesting the next tipset if the requested epoch happened to be a null round.

ts, err := a.Chain.GetTipsetByHeight(ctx, abi.ChainEpoch(num), nil, false)

This logic is not sound since an application traversing the chain with eth_getBlockByNumber will be returned the same tipset more than once, and will likely fail to recognise that edge case. The result may be that the app processes the same transactions more than once.

Additionally, other eth_ methods that access state will return future state, instead of the state prevalent at the requested height.

Solution

  1. Add a "strict" bool param to parseBlkParam.
    • When true, error when the block is a null round.
    • When false, switch to returning the previous non-null tipset, instead of the following one as we do today.
  2. Apply the right strictness on eth_ methods.
    • strict=true
      • eth_getBlockByNumber: strict=true. Return an empty block on a null round.
    • strict=false (all these are state accessors, and the relevant state to return is the past one, not the future one)
      • eth_getTransactionCount
      • eth_getBalance
      • eth_getStorageAt
      • eth_call
      • eth_getCode
      • eth_feeHistory
@arajasek
Copy link
Contributor

arajasek commented Mar 9, 2023

Many Eth API calls can take a block parameter, which can be a tagged value (earliest, pending, latest) or a concrete epoch (e.g. 0x1234). When supplied a concrete epoch, this method uses GetTipsetByHeight requesting the next tipset if the requested epoch happened to be a null round.
...
Additionally, other eth_ methods that access state will return future state, instead of the state prevalent at the requested height.

@raulk I think a lot of these gotchas can be avoided if the EthAPI implementation used the other APIs, in particular the ChainAPI, instead of accessing the ChainStore directly. Those APIs have been hardened around sharp edges like these over the years, and so will avoid a wave of minor issues.

In this case, the ChainAPI's ChainGetTipsetByHeight defaults to always asking for the previous tipset, which is generally what we want

return m.Chain.GetTipsetByHeight(ctx, h, ts, true)

@raulk
Copy link
Member Author

raulk commented Mar 9, 2023

Good flag! I switched the associated PR to use the higher-level API based on the recommendation. For the broader topic of removing the ChainStore, we should evaluate if it's feasible, as in some cases we do need lower-level access. Returning the nonce of an EVM smart contract is an extreme example, but we can probably port others to use high-level APIs, or create high-level equivalents if needed (although this may be superfluous if they're just pass-through).

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

No branches or pull requests

3 participants