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: fix GetBlock for null rounds by returning nil #12529

Merged
merged 13 commits into from
Oct 3, 2024

Conversation

virajbhartiya
Copy link
Member

Related Issues

fixes #10909

Proposed Changes

fix GetBlock for non-existent blocks by returning nil

Additional Info

https://github.com/ethereum/go-ethereum
https://docs.infura.io/api/networks/ethereum/json-rpc-methods/eth_getblockbynumber

Checklist

Before you mark the PR ready for review, please make sure that:

@virajbhartiya
Copy link
Member Author

@aarshkshah1992 Can you please give this a view

chain/store/store.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Oct 1, 2024

Can you add something in itests for this?

chain/store/store.go Outdated Show resolved Hide resolved
@aarshkshah1992 aarshkshah1992 changed the title feat: fix GetBlock for non-existent blocks by returning nil feat: fix GetBlock for null rounds by returning nil Oct 1, 2024
@virajbhartiya
Copy link
Member Author

virajbhartiya commented Oct 1, 2024

I have reverted back to the original state, I'll make the changes in getTipsetByBlockNumber. Sorry for the weird commit history

@virajbhartiya
Copy link
Member Author

Null rounds essentially gaps in the blockchain where no blocks are successfully mined for a particular epoch. These occur naturally in the protocol with a designed 0.7% probability due to the network's winner selection mechanism, though they can also happen if all Storage Providers who were selected as potential block miners for that epoch fail to produce or publish blocks. During a null round, while no blockchain data is created, the network's epoch clock continues to advance. This creates a gap in the chain's sequence. When a null round occurs, the next successful block will link back to the last valid tipset before the null round, effectively skipping over the empty epoch. This ensures network progression even when block production temporarily fails. And also null rounds differ from empty tipsets in that they don't exist in the chain state at all.

Null rounds -> when no block gets mined at a given epoch
Empty tipsets -> valid tipsets that contain no messages but have a CID and exist in the chain state

@aarshkshah1992
Copy link
Contributor

@virajbhartiya Great ! One more question -> given an epoch -> how can determine if it's a null round or not in Lotus ?

@virajbhartiya
Copy link
Member Author

That can be done by
Trying to retrieve a tipset for your target epoch, if no tipset exists for that epoch, it's likely a null round and even if you get a tipset back it needs to be verified its height matches the target epoch. If the returned tipset's height doesn't match your target, it's a null round

Since null rounds don't exist in the chain state, we need to look for gaps in the chain. These gaps can be found by failing to find a tipset at that epoch

@virajbhartiya
Copy link
Member Author

To be able to return null values when a block is a null round

- func (a EthModule) EthGetBlockByNumber(ctx context.Context, blkParam string, fullTxInfo bool) (ethtypes.EthBlock, error)
+ func (a EthModule) EthGetBlockByNumber(ctx context.Context, blkParam string, fullTxInfo bool) (*ethtypes.EthBlock, error)

which will result in

- return newEthBlockFromFilecoinTipSet(ctx, ts, fullTxInfo, a.Chain, a.StateAPI)
+ return &block, nil

So the function of the definition of EthGetBlockByNumber will have to be changed everywhere. Is this okay or there's a better way of doing this?

@BigLep BigLep requested a review from aarshkshah1992 October 1, 2024 20:12
@aarshkshah1992
Copy link
Contributor

@virajbhartiya Yes, we need to change the function signature. Document this in the CHANGELOG when you write the change log for this.

@virajbhartiya
Copy link
Member Author

I have updated EthGetBlockByNumber to return nil for a null round and also written itest for it and updated the ones using the function.

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@virajbhartiya Reviewed.

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

lgtm

itests/fevm_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@virajbhartiya Last set of comments.

@virajbhartiya
Copy link
Member Author

@aarshkshah1992 I have made the changes suggested in you review and modified the tests.

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

lgtm

@aarshkshah1992 aarshkshah1992 enabled auto-merge (squash) October 3, 2024 11:46
@aarshkshah1992 aarshkshah1992 merged commit c674554 into filecoin-project:master Oct 3, 2024
80 of 81 checks passed
@virajbhartiya virajbhartiya deleted the getblock branch October 3, 2024 12:27
dumikau pushed a commit to protofire/lotus that referenced this pull request Oct 9, 2024
…#12529)

* update pb version

* docs: update changelog

* fix: remove redundant code in sectors.go

* feat: Update EthGetBlockByNumber to return pointer to EthBlock

* fix TestGetBlockByNumber

* fix: Remove unnecessary assertions tests

* Update eth_api_test.go

* feat: Refactor EthGetBlockByNumber test in fevm_test.go

---------

Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>
dumikau added a commit to protofire/lotus that referenced this pull request Oct 9, 2024
rvagg added a commit that referenced this pull request Oct 25, 2024
@rvagg
Copy link
Member

rvagg commented Oct 25, 2024

#12641

sorry, reverts aren't fun to have done to you

rvagg added a commit that referenced this pull request Oct 28, 2024
rvagg added a commit that referenced this pull request Oct 28, 2024
rjan90 pushed a commit that referenced this pull request Oct 28, 2024
rjan90 pushed a commit that referenced this pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

getBlock return an Error
3 participants