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 events API timeout handling for nil blocks #7184

Merged

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Aug 26, 2021

The events API timeout handler does not take into account nil blocks.
This PR fixes it, and adds a test for deal expiry.

@dirkmc dirkmc requested a review from a team as a code owner August 26, 2021 10:23
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

This is an awesome find! Great that we also extended the test coverage significantly for this edge case 👏

Just need to fix the lint and the test failures, which complain about deal expiration bounds. Not sure if this is related to us modifying the miner actor configuration. It shouldn't be because each test runs in a separate process in CI, so there shouldn't be any cross interaction. Regardless, we should probably reset the value of minerN.DealMinDuration to the original values with a t.Cleanup().

Maybe related to the introduction of MinBlocksDuration? Although not sure how, because everywhere continues using the zero value.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

I just noticed that this will end up conflicting with #7000, which does an entire rewrite of these components.

@dirkmc -- just to have an extra data point, could you try running the test in here against #7000?

@dirkmc
Copy link
Contributor Author

dirkmc commented Aug 26, 2021

Oh hmm good point 👍

I tried running the test against #7000 and it failed, so I guess we will need the fix here in addition to those changes.

@magik6k magik6k requested a review from Stebalien August 26, 2021 14:13
@Stebalien
Copy link
Member

Oh, wow. I didn't even notice this issue.

@Stebalien
Copy link
Member

Any reason not to merge this directly to master?

@raulk raulk merged commit 78b9929 into refactor/mkts-deal-exp-params Aug 26, 2021
@raulk raulk deleted the refactor/mkts-deal-exp-params-test branch August 26, 2021 15:36
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.

3 participants