Bug/1621 tx dropped silently when ctq full#2475
Conversation
|
SUGGESTIONS BEFORE MERGE:
|
There was a problem hiding this comment.
Pull request overview
This PR updates SKALED’s transaction queue admission logic to avoid “successful” imports that later result in silent transaction loss when the current transaction queue (CTQ) is at capacity. It makes queue insertion outcomes explicit, propagates QueueIsFull up to the client/RPC layer, and expands unit tests around CTQ/FTQ behavior.
Changes:
- Refactors
TransactionQueue::import()to require caller-providedstateNonceand an explicitallowFutureQueueflag, and addsImportResult::QueueIsFull. - Updates
Clientto pass the committed sender nonce and convertQueueIsFullinto a newTransactionQueueIsFullexception; maps that exception to an RPC error message. - Updates/extends unit tests to validate CTQ/FTQ admission and full-queue rejection behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
libethereum/TransactionQueue.h |
Changes import API to require allowFutureQueue + stateNonce; adds new internal helpers. |
libethereum/TransactionQueue.cpp |
Implements explicit CTQ/FTQ insertion paths and QueueIsFull handling; adjusts drop/import semantics. |
libethcore/Common.h |
Adds ImportResult::QueueIsFull. |
libethcore/Exceptions.h |
Introduces TransactionQueueIsFull exception. |
libethereum/Client.cpp |
Passes stateNonce/allowFutureQueue into TQ import; throws TransactionQueueIsFull on QueueIsFull. |
libweb3jsonrpc/Eth.cpp |
Maps TransactionQueueIsFull to a user-facing RPC error string. |
test/unittests/libethereum/TransactionQueue.cpp |
Updates existing tests and adds new cases for CTQ-full fallback / FTQ limits / explicit rejection. |
test/unittests/libethereum/SkaleHost.cpp |
Updates test imports to new TQ import signature. |
test/tools/libtesteth/BlockChainHelper.cpp |
Updates helper queue imports to new TQ import signature. |
…om:skalenetwork/skaled into bug/1621-tx-dropped-silently-when-CTQ-full
| * Keeps track of a 'blockedPromotions' mapping used to keep track of txs that were tried | ||
| * to be promoted from FTQ to CTQ, but CTQ was full at the time. | ||
| * This is needed since we only promote FTQ txs from a specific sender that must be in CTQ. | ||
| * blockedPromotions solves the case where a FTQ that hasn't same sender in CTQ to be promoted. |
There was a problem hiding this comment.
The class-level comment about blockedPromotions is hard to understand and has grammatical issues (e.g., “a FTQ that hasn't same sender in CTQ to be promoted”). Please rewrite this comment to clearly describe the invariant and the problem m_blockedPromotions solves (ideally with a short example).
| * Keeps track of a 'blockedPromotions' mapping used to keep track of txs that were tried | |
| * to be promoted from FTQ to CTQ, but CTQ was full at the time. | |
| * This is needed since we only promote FTQ txs from a specific sender that must be in CTQ. | |
| * blockedPromotions solves the case where a FTQ that hasn't same sender in CTQ to be promoted. | |
| * | |
| * FTQ transactions are promoted to CTQ only for senders that already have a transaction in CTQ. | |
| * This preserves per-sender nonce ordering while allowing promotion to start from the sender's | |
| * next executable transaction. | |
| * | |
| * `m_blockedPromotions` remembers FTQ transactions that were ready to move to CTQ but could not | |
| * be promoted because CTQ was full at that moment. Without this bookkeeping, promotion for that | |
| * sender could be missed later if the sender has no transaction left in CTQ to trigger another | |
| * promotion attempt. | |
| * | |
| * Example: sender A has nonce 5 in CTQ and nonce 6 in FTQ. If nonce 5 is removed and nonce 6 is | |
| * ready to be promoted, but CTQ is full, nonce 6 is recorded in `m_blockedPromotions` so it can | |
| * be retried once space becomes available. |
| auto it1 = queue.m_currentByAddressAndNonce.find(_first.transaction.sender()); | ||
| auto it2 = queue.m_currentByAddressAndNonce.find(_second.transaction.sender()); | ||
|
|
||
| if (it1 == queue.m_currentByAddressAndNonce.end() || it2 == queue.m_currentByAddressAndNonce.end()) | ||
| return _first.creationTimeMs < _second.creationTimeMs; |
There was a problem hiding this comment.
These lines look unformatted (missing spaces inside parentheses and a very long if condition). Since CI runs a clang-format check, please re-run clang-format on this file so the comparator block matches repository formatting and stays under the column limit.
| ImportResult ret = ImportResult::QueueIsFull; | ||
| // if compatible with CTQ - try insert in CTQ - may fail due to queue full | ||
| if ( isCurrentNonceCompatible_WITH_LOCK( _transaction, _stateNonce ) ) { | ||
| // may fail insertion if queue full | ||
| ret = insertCurrent_WITH_LOCK( make_pair( _h, _transaction ) ); | ||
| if ( ret == ImportResult::Success ) { | ||
| BOOST_LOG( m_loggerTrace ) << "Queued vaguely legit-looking transaction " << _h; | ||
| m_onReady(); | ||
| } | ||
| // if future -> try insert in FTQ | ||
| } else if ( _allowFutureQueue && _transaction.nonce() > _stateNonce ) { | ||
| // may fail insertion if queue full | ||
| ret = insertFuture_WITH_LOCK( make_pair( _h, _transaction ) ); | ||
| if ( ret == ImportResult::Success ) | ||
| BOOST_LOG( m_loggerTrace ) << "Queued future transaction " << _h; | ||
| } |
There was a problem hiding this comment.
PR description says that when a tx is CTQ-eligible but CTQ is full, the queue will try to insert it into FTQ (when enabled). In this implementation, current-compatible transactions only attempt CTQ insertion and return QueueIsFull if CTQ has no capacity; only future-nonce transactions attempt FTQ insertion. Please align the PR description with the actual behavior, or implement the described CTQ->FTQ fallback here.
Description
This PR fixes a transaction queue issue where a transaction could be silently dropped when the current transaction queue (CTQ) was full - see #1621
Previously, queue insertion happened mostly through the CTQ path. If CTQ exceeded its limits, the queue could drop a transaction internally to recover capacity while still returning success to the caller. That meant the client/RPC layer had no reliable signal that the submitted transaction was not actually accepted.
The new behavior makes queue admission explicit:
QueueIsFull.TransactionQueueIsFullerror, so RPC callers are informed instead of seeing a silent drop.At a logical level, CTQ remains the preferred queue for executable/contiguous transactions, while FTQ acts as the fallback for valid transactions that are either future nonces OR cannot currently fit into CTQ. If neither queue can accept the transaction, the failure is now explicit and observable.
Fixes #1621