Skip to content

Conversation

@mateeullahmalik
Copy link
Collaborator

Summary

Add thread-safe mutex locking to the action module to prevent race conditions when submitting concurrent transactions to the blockchain mempool.

Problem

The action module was experiencing race conditions when multiple goroutines attempted to execute transactions concurrently (e.g., multiple RequestAction or
FinalizeCascadeAction calls). This caused errors during transaction creation, signing, and broadcasting to the mempool, potentially leading to:

  • Nonce conflicts (multiple transactions using the same sequence number)
  • Transaction signing failures due to concurrent key access
  • Inconsistent transaction state during mempool submission
  • Potential data races in the transaction helper's internal state

Solution

This PR introduces a sync.Mutex to the action module struct, ensuring that transaction execution is serialized. The sync.Mutex is a Go synchronization
primitive that provides mutual exclusion - only one goroutine can hold the lock at a time, forcing all others to wait until the lock is released.

Changes

  • Import sync package: Add sync for mutex support
  • Add mutex field: Include sync.Mutex in the module struct
  • Protect RequestAction: Lock/unlock mutex around transaction execution to prevent concurrent action requests from interfering
  • Protect FinalizeCascadeAction: Lock/unlock mutex around transaction execution to prevent concurrent finalization calls from interfering

Technical Details

Each transaction goes through several steps:

  1. Creating the message with the correct creator/signer
  2. Signing the transaction with the keyring
  3. Broadcasting to the blockchain mempool

Without synchronization, concurrent calls could:

  • Retrieve the same account sequence number (nonce)
  • Sign transactions with conflicting nonces
  • Cause mempool rejection or unpredictable transaction ordering

return nil, err
}

m.mu.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need lock here... RequestAction signed by user, not supernode.
This works just fine now with lots of accounts concurrently.

Copy link
Contributor

@akobrin1 akobrin1 left a comment

Choose a reason for hiding this comment

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

I think we don't need lock for RequestAction

@a-ok123 a-ok123 requested a review from Copilot November 20, 2025 18:20
Copilot finished reviewing on behalf of a-ok123 November 20, 2025 18:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces thread-safe mutex locking to the action module to prevent race conditions during concurrent transaction submissions. The primary concern is preventing nonce conflicts and transaction signing failures when multiple goroutines execute transactions simultaneously.

Key Changes:

  • Added sync.Mutex to the action module struct for mutual exclusion
  • Protected RequestAction and FinalizeCascadeAction methods with mutex locks around transaction execution
  • Lock is acquired after parameter validation and held during the entire transaction flow (account info retrieval, message creation, signing, and broadcasting)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 17 to 21
type module struct {
client actiontypes.MsgClient
txHelper *txmod.TxHelper
mu sync.Mutex
}
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The SetTxHelperConfig and GetTxHelper methods access m.txHelper without mutex protection, which could cause race conditions if called concurrently with the now-protected RequestAction or FinalizeCascadeAction methods. Additionally, SimulateFinalizeCascadeAction also accesses m.txHelper without protection.

While the mutex correctly protects the transaction execution in the modified methods, consider whether these other methods that access the shared txHelper state should also be protected to ensure complete thread safety across the module.

Copilot uses AI. Check for mistakes.
@roomote
Copy link

roomote bot commented Nov 20, 2025

Rooviewer Clock   See task on Roo Cloud

Review completed: no blocking issues found in the action locking changes.

  • Review action module transaction locking
  • Verify tx helper and action module interaction
  • Confirm no additional changes required for this PR

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

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