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(rpc): implement Filecoin.StateSectorPartition #4381

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jun 3, 2024

Summary of changes

Changes introduced in this pull request:

  • Implement RPC method Filecoin.StateSectorPartition
  • API compare tests
     Running `target/quick/forest-tool api compare /home/me/fr/snapshots/calibnet/forest_snapshot_calibnet_2024-06-03_height_1668748.forest.car.zst --filter StateSectorPartition`
| RPC Method                          | Forest | Lotus |
|-------------------------------------|--------|-------|
| Filecoin.StateSectorPartition (145) | Valid  | Valid |

Reference issue to close (if applicable)

Closes #4380

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review June 3, 2024 08:29
@hanabi1224 hanabi1224 requested a review from a team as a code owner June 3, 2024 08:29
@hanabi1224 hanabi1224 requested review from lemmih and LesnyRumcajs and removed request for a team June 3, 2024 08:29
@@ -0,0 +1,276 @@
// Copyright 2019-2024 ChainSafe Systems
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why you copied https://github.com/ChainSafe/fil-actor-states/blob/main/fil_actor_interface/src/convert.rs here? If you intend to move it out of fil-actor-states, let's have a separate PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods are not publicly exported.

Copy link
Member

Choose a reason for hiding this comment

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

So right now we have two lengthy implementations doing basically the same thing. I'd rather we have only one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@@ -114,4 +118,33 @@ impl MinerStateExt for State {
}
}
}

fn find_sector<BS: Blockstore>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this method to be a part of fil-actor-states just like we do it for other actors. Then we don't need all of those from...* method in forest either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a PR for inspiration: ChainSafe/fil-actor-states#299

Basically we could do the same with find_sector and then just call that in Forest rpc code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer getting rid of the process of updating and releasing fil-actor-states crates during the RPC development and we can backport all missing functions all at once after all RPC work is done. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to you, though this creates unnecessary confusion. I'd rather go through an ordeal of having to reference a working branch in fil-actor-states than introducing these helpers and then moving them to another repo.

This also introduces another potential problem which boils down to potential mistakes when moving the code around and ultimately requires an additional round of testing once the refactoring is done.

Which is why I'm not 100% sure this saves us any hassle, while adding to the noise in forest commit history.

Copy link
Contributor Author

@hanabi1224 hanabi1224 Jun 4, 2024

Choose a reason for hiding this comment

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

find_sector added in ChainSafe/fil-actor-states#303

I'd rather go through an ordeal of having to reference a working branch in fil-actor-states

Referencing a working branch without publishing would block cargo publish --dry-run for forest.

introducing these helpers and then moving them to another repo.

I think the ordeal is because with the current development process of fil_actor_interface, there's no way to make the crate carefully designed, when the requirement of what are needed and what are not needed is unclear, and the rest of the crates in the repo are mostly copy-pasted code. Although I really love the idea of having an interface crate, I doubt the usefulness of fil_actor_interface after spending so much maintenance effort when it's not (and not likely possible to be ) a carefully designed library.

By not a carefully designed library, I mean, e.g. when the other crates are mostly copy-pasted code, why don't we use a separate version number for fil_actor_interface instead of publishing all other crates without changing anything in most of the time

@LesnyRumcajs
Copy link
Member

Let's add changelog entry.

@hanabi1224 hanabi1224 force-pushed the hm/StateSectorPartition branch from 734158b to 5b4bade Compare June 5, 2024 07:34
@ruseinov ruseinov self-requested a review June 5, 2024 10:20
@ruseinov ruseinov added this pull request to the merge queue Jun 5, 2024
Merged via the queue into main with commit 696087a Jun 5, 2024
27 checks passed
@ruseinov ruseinov deleted the hm/StateSectorPartition branch June 5, 2024 10:34
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

Successfully merging this pull request may close these issues.

StateSectorPartition
3 participants