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

EpochInfoProvider improvements #12542

Open
wacban opened this issue Dec 2, 2024 · 4 comments
Open

EpochInfoProvider improvements #12542

wacban opened this issue Dec 2, 2024 · 4 comments
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on.

Comments

@wacban
Copy link
Contributor

wacban commented Dec 2, 2024

Description

The epoch info provider encapsulates the runtime's needs of epoch manager.

  1. There is some redundancy in its API - the account_id_to_shard_id can be also done via the shard_layout. Please remove the account_id_to_shard_id and replace it's usages. Please also remove the account_id_to_shard_id_map from the MockEpochInfoProvider and instead use an appropriate shard layout and test accounts.
  2. The EpochInfoProvider technically should only be used at the current epoch id. Please peg the provider to EpochId given in the constructor and remove epoch id / block hash from the API methods where possible.
@wacban wacban added the C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. label Dec 2, 2024
@hackpk
Copy link

hackpk commented Dec 6, 2024

Hello @wacban,
I’d like to work on this issue as my first contribution to NEAR. I understand the need to peg EpochInfoProvider to a specific EpochId and simplify the API.
Regarding the second point, is the block hash mentioned referring to the last block hash?
Thank you!

@wacban
Copy link
Contributor Author

wacban commented Dec 6, 2024

Hi @hackpk Thank you and welcome!

It should be the block hash of the block that contains the chunk that is being applied. It should be available in the ApplyState or one of the ProcessingState types that you can find in apply.

@hackpk
Copy link

hackpk commented Dec 13, 2024

Hi @wacban
For the second point, I'm unable to create a constructor in EpochInfoProvider because it's a trait. If I try to do the same in EpochMannager when EpochManager gets called, I don't have Epochid. Can you please guide how should I proceed?

@wacban
Copy link
Contributor Author

wacban commented Dec 13, 2024

  1. If it's too crazy feel free to leave it for now.
  2. You can introduce a new struct e.g. ApplyEpochInfoProvider that implements the trait, holds a reference to the epoch manager and an epoch id.

It would be nice to double check that my assumption about always using the current epoch id is correct. Before getting rid of epoch id from the API can you first just debug_assert that the epoch is is equal to the pegged one in ApplyEpochInfoProvider? We can then push it through the CI tests and see that it passes. I will of course do some due diligence in code review but this could be a quick preliminary check.

btw the name is up for debate, I'll think about it once I see the code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on.
Projects
None yet
Development

No branches or pull requests

2 participants