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

Mpool fixes #2847

Closed
wants to merge 36 commits into from
Closed

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented May 9, 2023

Summary of changes

Changes introduced in this pull request:

  • Fix sticking mpool messages in case of "single tipset" errors
  • Add limits for each actors to prevent unwanted spam

Reference issue to close (if applicable)

Closes #2761

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.

@elmattic elmattic mentioned this pull request May 11, 2023
5 tasks
@elmattic elmattic changed the base branch from main to elmattic/revive-mpool-commands May 12, 2023 08:04
@elmattic elmattic marked this pull request as ready for review May 12, 2023 08:59
blockchain/chain/src/store/chain_store.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/chain_store.rs Outdated Show resolved Hide resolved
@@ -70,10 +73,17 @@ impl MsgSet {

/// Add a signed message to the `MsgSet`. Increase `next_sequence` if the
/// message has a sequence greater than any existing message sequence.
pub fn add(&mut self, m: SignedMessage) -> Result<(), Error> {
pub fn add(&mut self, m: SignedMessage, untrusted: bool) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does untrusted mean? And it looks like you always set it to false.

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 did this for future endpoint (if we implement it). Right now we support only Filecoin.MpoolPushMessage.
But it exists a counterpart untrusted one: Filecoin.MpoolPushUntrusted for public gateways.

See lotus PR: filecoin-project/lotus#3915

elmattic and others added 3 commits May 12, 2023 12:07
blockchain/chain/src/store/chain_store.rs Outdated Show resolved Hide resolved
blockchain/message_pool/src/msgpool/mod.rs Outdated Show resolved Hide resolved
Comment on lines 526 to 527
// sleep allows for async block to update mpool's cur_tipset
tokio::time::sleep(Duration::new(2, 0)).await;
Copy link
Member

Choose a reason for hiding this comment

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

I highly dislike sleeps in the tests.

  1. It makes them flaky, yielding failures when run on unexpectedly low-end (or busy) workers.
  2. It makes tests unnecessarily long. Say the update took 5ms - we are waiting for 2s for nothing.

Can you think of a mechanism that would at least partially mitigate those issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I don't like it as well and I'm looking for a way to remove those.

I've put 10ms while waiting for something better.

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've added a new method in Provider so it's possible for TestApi to use await instead of sleep.

@elmattic
Copy link
Contributor Author

elmattic commented Jun 2, 2023

Closing it because of merged in #2740.

@elmattic elmattic closed this Jun 2, 2023
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.

Messages are getting stuck in mpool
4 participants