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

Closed
wacban opened this issue Dec 2, 2024 · 7 comments · Fixed by #12754
Closed

EpochInfoProvider improvements #12542

wacban opened this issue Dec 2, 2024 · 7 comments · Fixed by #12754
Assignees
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

@ssavenko-near
Copy link
Contributor

  1. Seems to be easy. account_id_to_shard_id_map also seems to be already removed.
  2. Not so sure about this one. There are calls to account_id_to_shard_id() with with a "pinned" epoch as well as with some "apply_state" epoch. These are apparently logically different entities, and my understanding of the code is not enough to conclude those are supposed to always be the same (I would actually tend to assume otherwise).

@ssavenko-near
Copy link
Contributor

@wacban with 12754 merged do you think we can close this issue?

@wacban
Copy link
Contributor Author

wacban commented Jan 17, 2025

yep, sounds good, thanks!

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

Successfully merging a pull request may close this issue.

3 participants