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(api): lock simultaneous tx insertsion with mutex #3616

Conversation

perekopskiy
Copy link
Contributor

@perekopskiy perekopskiy commented Feb 17, 2025

What ❔

Fixes the lock for simultaneous tx insertion with mutex.

Why ❔

The previous approach could lead to a "stuck" state if the task panics/get canceled before hashmap entry is cleaned.
This shouldn't happen now with guard removing entry on drop.

Is this a breaking change?

  • Yes
  • No

Operational changes

No changes

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zkstack dev fmt and zkstack dev lint.

@perekopskiy perekopskiy requested a review from slowli February 17, 2025 08:45
Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure I understand how the code change correlates with the PR description 🙃 AFAIU, with the change, the lock for address_and_nonce now lasts longer (it's alive across DB manipulations), so the code doesn't just implement RAII semantics for a lock. If the only goal is to implement such semantics, I'd argue it'd be easier to wrap inflight_requests in an Arc and have a guard removing the entry on drop (probably by spawning a task since it has async logic):

struct Guard {
    inflight_requests: Weak<Mutex<HashMap<(Address, Nonce), H256>>>,
    address_and_nonce: (Address, Nonce),
}

impl Drop for Guard {
    fn drop(&mut self) {
        tokio::spawn(async move {
            if let Some(inflight_requests) = self.inflight_requests.upgrade() {
                inflight_requests.lock().await.remove(&self.address_and_nonce);
            }
        });
    }
}

core/node/api_server/src/tx_sender/master_pool_sink.rs Outdated Show resolved Hide resolved
@perekopskiy perekopskiy added this pull request to the merge queue Feb 17, 2025
Merged via the queue into main with commit 644b621 Feb 17, 2025
34 checks passed
@perekopskiy perekopskiy deleted the perekopskiy-zks-992-bug-in-tracking-inflight-txs-inserting-in-api branch February 17, 2025 17:22
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.

2 participants