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] data race when adding messages #10755

Closed
Stebalien opened this issue Apr 25, 2023 · 5 comments
Closed

[mpool] data race when adding messages #10755

Stebalien opened this issue Apr 25, 2023 · 5 comments
Assignees
Labels
kind/bug Kind: Bug

Comments

@Stebalien
Copy link
Member

We're calling mp.getStateNonce without holding the lock:

_, _ = mp.getStateNonce(ctx, m.Message.From, tmpCurTs)

This used to be safe, but this method now resolves addresses which can't be done without holding the lock. We should probably just remove this call (mp.getStateNonce) entirely.

We may also be able to get rid of this "warm the cache" logic entirely. We still have a few more instances of GetActorAfter, but I think we can drop those.

@Stebalien Stebalien added the kind/bug Kind: Bug label Apr 25, 2023
@Stebalien
Copy link
Member Author

Nevermind. We still need GetActorAfter if we want to be able to use an account created in the previous tipset. But we can remove the call to mp.getStateNonce (and/or take a lock internally when we resolve the address).

@snissn
Copy link
Contributor

snissn commented Apr 25, 2023

  1. delete
    _, _ = mp.getStateNonce(ctx, m.Message.From, tmpCurTs)
  2. delete
    _, _ = mp.getStateNonce(ctx, m.Message.From, tmpCurTs)
  3. instead of call getActorAfter, call getActorBefore then if it does not exist call getActorAfter -- all we need is actor code CID or sending address, not nonce

@Stebalien
Copy link
Member Author

So, we have a couple of options. Either:

  1. Call GetActorBefore, then GetActorAfter if it turns out that the actor doesn't exist yet.
  2. Drop support for sending messages from an account immediately after creating it. Honestly... this seems reasonable.

For 2, we'd just replace

senderAct, err := mp.api.GetActorAfter(m.Message.From, curTs)
with GetActorBefore and remove all the logic around "warming the cache" in general.

@snissn snissn linked a pull request Apr 25, 2023 that will close this issue
7 tasks
@snissn
Copy link
Contributor

snissn commented Apr 25, 2023

Sounds good! I opened a PR for 2): #10758

@snissn snissn linked a pull request Apr 27, 2023 that will close this issue
7 tasks
@Stebalien
Copy link
Member Author

Turns out this isn't actually an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Kind: Bug
Projects
None yet
2 participants