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

fix(market): load proposals and states roots once in on_miner_sectors_terminate #1585

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

Abhay-2811
Copy link
Contributor

Fixes #1498

Changes made:

  1. Caching proposals and states before iterating through deals.

  2. Helper functions to query deal proposal and deal state from cached data.

actors/market/src/state.rs Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Oct 22, 2024

@Abhay-2811 this repo has fairly opinionated rules about formatting and your changes are currently failing. Running rustfmt should fix it, hopefully you can get that integrated into your editor (I use VS Code and and running "Format Document" applies the rules). There's also other checks that clippy applies. The best way to completely check your changes is running make check before pushing, which is what CI is doing and why it's failing.

@Stebalien
Copy link
Member

Also note:

  • cargo fmt --all will format everything.
  • cargo clippy --all will lint everything.

@Abhay-2811
Copy link
Contributor Author

Ran fmt and clippy commands, make check returns errors but it's unrelated to changes I made should I push?

   --> actors/market/src/lib.rs:856:45
    |
856 |                     let dcid = deal_cid(rt, &deal)?;
    |                                             ^^^^^ help: change this to: `deal`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> actors/market/src/lib.rs:861:70
    |
861 |                 total_slashed += st.process_slashed_deal(rt.store(), &deal, &state)?;
    |                                                                      ^^^^^ help: change this to: `deal`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

error: could not compile `fil_actor_market` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `fil_actor_market` (lib test) due to 2 previous errors
make: *** [Makefile:12: check] Error 101```

@rvagg
Copy link
Member

rvagg commented Oct 22, 2024

Oh, they are related, because you have an Option<&DealProposal> instead of the original Option<DealProposal>. You could make it like the original by adding a ?.cloned(); to both of the new ones.

@Stebalien is there any point cloning here? We're not mutating the DealProposal, but we are mutating the DealState, should we just clone the latter and leave the former alone?

@Abhay-2811
Copy link
Contributor Author

@rvagg fixed CI-related issues in the latest commit

@Abhay-2811 Abhay-2811 requested a review from rvagg October 22, 2024 18:05
@rvagg
Copy link
Member

rvagg commented Oct 23, 2024

rustfmt still not happy, thanks to the new cloned addition

@Abhay-2811
Copy link
Contributor Author

Hey @rvagg pushed the fix but should I be aware of any other CI checks, I ran make check and make rustfmt works fine

@rvagg rvagg changed the title Caching proposals & states to avoid querying root repeatedly fix: load proposals and states roots once in on_miner_sectors_terminate Oct 28, 2024
@rvagg rvagg changed the title fix: load proposals and states roots once in on_miner_sectors_terminate fix(miner): load proposals and states roots once in on_miner_sectors_terminate Oct 28, 2024
@rvagg rvagg changed the title fix(miner): load proposals and states roots once in on_miner_sectors_terminate fix(market): load proposals and states roots once in on_miner_sectors_terminate Oct 28, 2024
@rvagg rvagg added this pull request to the merge queue Oct 28, 2024
Merged via the queue into filecoin-project:master with commit ca31c60 Oct 28, 2024
10 checks passed
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.

Improve market actor efficiency handling terminated sectors
3 participants