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: update deal filter to use ExpectedSealDuration and MaxDealStartDelay #1609

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

LexLuthr
Copy link
Collaborator

@LexLuthr LexLuthr commented Aug 7, 2023

Fixes #1608

This PR has the following changes:

  1. Update BasicDealFilter to not use cfg params as it is not required.
  2. Add checks for ExpectedSealDuration and MaxDealStartDelay
  3. Update MaxDealStartDelay to only factor in online deals. This was done keeping in mind that offline deals can take days to weeks for shipping & importing the data. We don't want them to be rejected because deal start epoch is too far away.
  4. Deal Publisher should use cfg.Dealmaking.StartEpochSealingBuffer instead of cfg.LotusDealmaking.StartEpochSealingBuffer for Boost deals.
  • Test on devnet

@LexLuthr LexLuthr requested a review from nonsense August 7, 2023 13:06
@LexLuthr
Copy link
Collaborator Author

LexLuthr commented Aug 7, 2023

Both filters are working as expected:
Screenshot 2023-08-07 at 6 19 23 PM

@LexLuthr LexLuthr requested a review from dirkmc August 7, 2023 17:39
@@ -77,6 +77,7 @@ fi
## Override config options
echo Updating config values
sed -i 's|ServiceApiInfo = ""|ServiceApiInfo = "ws://localhost:8042"|g' $BOOST_PATH/config.toml
sed -i 's|ExpectedSealDuration = "0h0m10s"|ExpectedSealDuration = "0h0m10s"|g' $BOOST_PATH/config.toml
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading the code correctly it looks like this line doesn't do anything 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -628,7 +628,7 @@ func ConfigBoost(cfg *config.Boost) Option {
Override(new(*storageadapter.DealPublisher), storageadapter.NewDealPublisher(&legacyFees, storageadapter.PublishMsgConfig{
Period: time.Duration(cfg.LotusDealmaking.PublishMsgPeriod),
MaxDealsPerMsg: cfg.LotusDealmaking.MaxDealsPerPublishMsg,
StartEpochSealingBuffer: cfg.LotusDealmaking.StartEpochSealingBuffer,
StartEpochSealingBuffer: cfg.Dealmaking.StartEpochSealingBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include a comment about this change in the release announcement when this commit lands in a release.
Some SPs may be relying on this config flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack, I will make an announcement.

@LexLuthr LexLuthr merged commit 848a505 into main Aug 8, 2023
@LexLuthr LexLuthr deleted the fix/dealFilters branch August 8, 2023 09:19
LexLuthr added a commit that referenced this pull request Aug 8, 2023
…rtDelay` (#1609)

* update deal filters

* fix itests

* adjust devnet values

* Update docker/devnet/boost/entrypoint.sh

fix devnet
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.

Check ExpectedSealDuration and MaxDealStartDelay for boost deals
2 participants