fix(sequencer): deduplicate forced transactions in pool

[nadeemb53]

Fixes #3776.
Deduplicates pending and completed forced transactions so coordinator redelivery cannot leave stale queue entries that overwrite inclusion status.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

linea-besu/plugins/linea-sequencer/sequencer/src/main/java/lineth/sequencer/forced/LineaForcedTransactionPool.java:379

• recordStatus mutates statusCache and pendingTransactionNumbers, and runs concurrently with addForcedTransactions (RPC-driven) and processForBlock/onBlockAdded (Besu event-driven). Without a shared lock, addForcedTransactions can observe statusCache empty and enqueue a tx while recordStatus is in-flight, reintroducing duplicates.

To make the deduplication robust, guard status recording with the same lock as addForcedTransactions (for example by synchronizing this method) so the cache+set updates cannot interleave with enqueueing.

    statusCache.put(ftx.forcedTransactionNumber(), status);
    pendingTransactionNumbers.remove(ftx.forcedTransactionNumber());
    inclusionResultCounters.get(result).incrementAndGet();

linea-besu/plugins/linea-sequencer/sequencer/src/main/java/lineth/sequencer/forced/LineaForcedTransactionPool.java:156

• The deduplication relies on pendingTransactionNumbers and statusCache, but statusCache can be updated concurrently by onBlockAdded/recordStatus while this loop is running. That creates an interleaving where statusCache becomes non-empty after the getIfPresent check but before pendingQueue.addLast(tx), allowing a forcedTxNumber that already has a final status recorded to be enqueued again.

Consider guarding the add/check/enqueue sequence with the same lock used when recording statuses (e.g., synchronized on the instance or a dedicated lock) so the pending-set and status-cache checks are atomic w.r.t. final status recording.

This issue also appears on line 377 of the same file.

      final long forcedTransactionNumber = tx.forcedTransactionNumber();
      if (!pendingTransactionNumbers.add(forcedTransactionNumber)) {
        continue;
      }
      if (statusCache.getIfPresent(forcedTransactionNumber) != null) {

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Filter94]

I feel like the this becomes bloated. What if we use an ConcurrentSkipListMap here instead of the pendingQueue?

[nadeemb53]

my understanding is that the set keeps the transaction number reserved while the transaction moves from the queue into the status cache. With only a map, removing the entry before recording the status creates a window where a retry can add it again, so we’d need extra locking. That doesn’t end up being simpler.

[Filter94]

What if we avoid this window by reordering the operations? Same as you've done with the pendingTransactionNumbers, removal from the queue after the status is updated

Also, a validation whether a transaction is within the statusCache already with INCLUDED status could also help with the duplicates. And I think we don't care about the resubmission with other, initially rejected statuses

[nadeemb53]

Hmm let me try