Serve pending transactions through the v2 REST API - #2122
Conversation
Orders mempool transactions so that transactions depending on the outputs or side effects of other transactions come after them: utxo chains, token issuance and subsequent token/account commands, and the order lifecycle (creation, fill, freeze, conclude), with account nonce dependencies. Transactions that cannot fail are ordered first via a priority. Based on the module from PR #2029, with id derivation failures propagated as errors instead of panicking.
GET /transaction/:id now falls back to the mempool of the connected node when the transaction is not confirmed yet, and a new GET /mempool/transactions endpoint lists the pending transactions, optionally ordered by the dependencies between them (?order=dependency). The endpoints proxy the mempool of the connected node instead of indexing it: pending data is ephemeral, so no api-server storage is used. The fee and the spent utxos of a pending transaction are not known to the api-server and are served empty; the block-related fields are empty until the transaction is confirmed. Based on the endpoint design of PR #2029.
Cover the pending-transaction fallback of GET /transaction/:id and the new GET /mempool/transactions endpoint: listing, dependency ordering, invalid ordering rejection, and the empty-mempool case. The in-memory test harness gains a mock mempool so the spawned web servers can serve the new endpoints.
Run the dependency ordering off the async runtime threads, make the ordering of the equal-priority transactions deterministic, omit the fee field of pending transactions instead of reporting a zero fee, and fix typos in the ordering module.
The deprecated account order commands (fill and conclude) carry the same dependencies as their order account command counterparts. A transaction that is both a provider and a dependent of the same dependency (e.g. two token account commands at consecutive nonces) no longer produces a self-dependency that would be misreported as a cycle. The token id derivation version is resolved at the height after the tip, since the transactions will be included into a future block.
The dependency ordering now mirrors the mempool of the node for the delegation spends: the spends of an account are nonce-sequenced and the first spend comes after the delegation creation (or a top-up). The decimals of the tokens transferred by pending transactions are resolved from the api-server storage, so pending token transfers are rendered with the correct decimals instead of failing on the missing token information; tokens whose issuance is still pending are rendered with zero decimals.
…ocument the ordering caveats
The delegation stake and nft issuance outputs provide no mempool-side dependency: staking and nft minting require an already known token or delegation, like the node's mempool does. Creating a delegation requires the stake pool to be known, and the stake pool creation provides that dependency. The decimals of the pending token issuances in a mempool listing are now taken from the issuing transactions themselves, so chained pending token transfers are rendered with the correct decimals, and the token decimals of a listing page are resolved through a single read-only storage transaction.
A failed ordering (an id derivation failure of an invalid transaction) must not take down the whole listing: the insertion order is fetched again instead. The storage tip is read once per request for both the ordering and the pending issuance decimals.
The token decimals are derived from the whole fetched mempool listing rather than only the requested page, the storage tip is read once per request, the decimals of the same token are looked up only once, and the response carries an x-mempool-ordering header that tells the client whether the requested dependency ordering was applied.
The token ids of a pending transaction are collected with the existing output values holder helper, and the delegation spend nonce overflow no longer panics: the last possible spend simply provides no next nonce.
… by id The mempool-proxying endpoints share a bounded number of concurrent requests, so a load of listings cannot load the connected node in parallel without limit. The single transaction endpoint resolves the decimals of a pending token issuance from the mempool listing as well, instead of always rendering zero decimals.
The barrier request has a timeout, its failure path aborts and awaits the server task and reports the actual panic payload together with the server address, and the handling is shared between the spawn helpers instead of being duplicated.
A request that waits for a mempool query permit for too long is rejected with 429 instead of queueing indefinitely, and the page offset is converted to the usize page size with a checked conversion.
|
🔍 OpenCodeReview found 9 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Ok(Err(err)) => { | ||
| // The transactions were consumed by the failed ordering: refetch | ||
| // them in the insertion order rather than failing the whole | ||
| // listing (an ordering failure of an invalid transaction must not | ||
| // take it down). | ||
| ordered_by_dependency = false; | ||
| logging::log::warn!("Falling back to the mempool insertion order: {err}"); | ||
| state.rpc.mempool_transactions().await.map_err(|e| { |
There was a problem hiding this comment.
On the ordering-failure fallback the transactions are refetched, but the mempool may have changed between the two RPC calls: the refetched set can differ from the set that failed ordering, and inclusion_height (used below for pending_issuance_decimals) was computed from the first snapshot. An issuance transaction added in between gets decimals 0. Also, a persistent id-derivation failure for one tx makes every dependency-ordered request pay an extra full-mempool fetch. Consider reusing the original snapshot (e.g. have the ordering function return the unsorted transactions on error) and/or recomputing the height for the refetched snapshot.
There was a problem hiding this comment.
Fixed in c116a4d: order_transactions_by_dependency now returns the transactions unsorted in the original insertion order together with the error (Result<Vec<SignedTransaction>, (TopoSortError, Vec<SignedTransaction>)>), so the listing falls back to the same snapshot instead of refetching — no second RPC, and the snapshot that failed ordering is exactly the one served and scanned for the pending issuance decimals, with the same inclusion_height.
| let _query_permit = MEMPOOL_QUERY_PERMITS.acquire().await.map_err(|e| { | ||
| logging::log::error!("internal error: {e}"); | ||
| ApiServerWebServerError::ServerError( | ||
| ApiServerWebServerServerError::InternalServerError, | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Permit acquisition here has no timeout, unlike the listing endpoint (which uses tokio::time::timeout with MEMPOOL_QUERY_WAIT_TIMEOUT). This endpoint is hit whenever a tx is not yet confirmed, so under sustained load this request can block indefinitely waiting for a permit while holding the client connection open. Reuse the same bounded wait and return TooManyMempoolRequests on timeout.
Suggestion:
| let _query_permit = MEMPOOL_QUERY_PERMITS.acquire().await.map_err(|e| { | |
| logging::log::error!("internal error: {e}"); | |
| ApiServerWebServerError::ServerError( | |
| ApiServerWebServerServerError::InternalServerError, | |
| ) | |
| })?; | |
| let _query_permit = tokio::time::timeout( | |
| MEMPOOL_QUERY_WAIT_TIMEOUT, | |
| MEMPOOL_QUERY_PERMITS.acquire(), | |
| ) | |
| .await | |
| .map_err(|_timed_out| ApiServerWebServerError::TooManyMempoolRequests)? | |
| .map_err(|e| { | |
| logging::log::error!("internal error: {e}"); | |
| ApiServerWebServerError::ServerError( | |
| ApiServerWebServerServerError::InternalServerError, | |
| ) | |
| })?; |
There was a problem hiding this comment.
Fixed in c116a4d: the permit acquisition is now wrapped in tokio::time::timeout(MEMPOOL_QUERY_WAIT_TIMEOUT, ...), rejecting with TooManyMempoolRequests on timeout, exactly like the listing endpoint. Note it also only triggers when the queried transaction transfers a token at all (unchanged), and per the performance finding below, only when such a token is missing from the storage.
| let mempool_txs = state.rpc.mempool_transactions().await.map_err(|e| { | ||
| logging::log::error!("internal error: {e}"); | ||
| ApiServerWebServerError::ServerError( | ||
| ApiServerWebServerServerError::InternalServerError, | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
For every pending transaction that transfers tokens, this fetches the entire mempool listing solely to derive pending-issuance decimals — an O(mempool size) RPC on the single-transaction endpoint, on top of the permit acquisition. If correctness for pending issuances is needed, consider a dedicated RPC (e.g. returning only pending issuances with their decimals) or caching the derivation result briefly, so the single-tx path does not scale with mempool size.
Suggestion:
| let mempool_txs = state.rpc.mempool_transactions().await.map_err(|e| { | |
| logging::log::error!("internal error: {e}"); | |
| ApiServerWebServerError::ServerError( | |
| ApiServerWebServerServerError::InternalServerError, | |
| ) | |
| })?; | |
| // Ideally: let mempool_txs = state.rpc.mempool_issuances().await ...; | |
| let mempool_txs = state.rpc.mempool_transactions().await.map_err(|e| { | |
| logging::log::error!("internal error: {e}"); | |
| ApiServerWebServerError::ServerError( | |
| ApiServerWebServerServerError::InternalServerError, | |
| ) | |
| })?; |
There was a problem hiding this comment.
Fixed in c116a4d without extending the node RPC surface: the storage is consulted for the decimals of the transferred tokens first, and the mempool listing is fetched only if some of them is not indexed yet — i.e. it may be an issuance pending in the mempool itself (a pending issuance is exactly a storage miss). The common case of a transaction transferring already known tokens no longer touches the mempool; only the genuinely pending-issuance case pays for the listing, still bounded by the (now timed) query permits. A dedicated mempool_issuances-style node RPC would shrink even that case from O(mempool) to O(issuances), but it crosses into the node RPC surface — happy to follow up in a separate PR if wanted.
| TxOutput::CreateOrder(order_data) => { | ||
| let order_id = make_order_id(inputs)?; |
There was a problem hiding this comment.
A single transaction whose make_order_id/make_token_id fails (e.g. an id-derivation failure, which can also depend on the assumed inclusion height differing from the eventual one) aborts the whole sort via ?. This forces every /mempool/transactions?order=dependency request onto the insertion-order fallback (an extra full-mempool RPC round trip) for as long as that tx stays in the mempool. Consider skipping such outputs/transactions (mirroring how pending_issuance_decimals skips derivation failures) instead of failing the entire ordering.
There was a problem hiding this comment.
Fixed in c116a4d: a transaction whose make_order_id/make_token_id fails only loses its own dependency edges — the derivation failure is logged and the output is skipped, mirroring how pending_issuance_decimals skips such issuances — instead of failing the whole sort. The id-derivation error variant is consequently gone from TopoSortError (graph building is now infallible); the remaining sort errors (cycle, missing dependency, and the newly detected duplicate id) still fall back to the insertion order, and the fallback reuses the same snapshot per the first finding.
The failed dependency ordering now returns the transactions unsorted in the original insertion order, so the listing falls back without refetching the mempool, which could return a different snapshot and skew the pending issuance decimals. A transaction whose order or token id cannot be derived only loses its own dependency edges instead of failing the whole ordering, and a duplicated id is reported as such instead of surfacing as a cycle. The single-transaction endpoint bounds its wait for a query permit like the listing endpoint, and fetches the mempool listing only if some of the transferred tokens is not indexed yet, i.e. it may be an issuance pending in the mempool itself. The block-related fields of the pending transactions are null instead of empty strings, and the fee key expected by the pending responses is pinned by a contract test.
The web server mock uses a tokio lock, so a panicking task cannot poison it for the other tests, and the submission request is bounded like the startup barrier, which now also bounds the first request of the tests that spawned the server manually. The fallback of the failed dependency ordering is pinned to reuse the same mempool snapshot, the pending transactions are expected to carry null block-related fields, and the listing assertions report the offending response on failure.
|
Addressed all 12 review findings in c116a4d, 0bc3686 and 627466c: High/medium:
Low:
Declined (with reason): sharing a path constant / generalizing the web-server bootstrap helper across the three spawn sites — a refactor of test scaffolding with divergence risk of its own; the current copies are pinned by the tests above. Can follow up if maintainers prefer the shared helper. Verified: |
| let decimals = db_tx | ||
| .get_token_num_decimals(token_id) | ||
| .await | ||
| .map_err(internal_error)? | ||
| // The issuance of the token is neither pending in the listing | ||
| // nor indexed, so its decimals cannot be known. | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Tokens whose decimals cannot be resolved are silently rendered with 0 decimals (unwrap_or(0)), and the fee key is simply removed from the response. A client cannot distinguish "unknown decimals" from a genuine zero-decimals token. Combined with the speculative token-id derivation above, pending transfers of unresolvable tokens can present materially wrong monetary metadata. Consider adding an explicit signal in the response (e.g. a decimals_known: false marker or decimals: null) instead of the silent zero default.
There was a problem hiding this comment.
Documented in 448aac8 rather than changed: the decimal field of the rendered amounts is presentational and the atoms amounts are authoritative regardless of the decimals, so a client misreading the presentation cannot corrupt its accounting. Surfacing decimals: null (or a marker) would be a contract change rippling through the shared renderers (amount_to_json and the token-decimals plumbing are used by the address/utxo/token endpoints for confirmed data too, where the unresolvable case already maps to zero, e.g. NFT token ids), so it does not fit this PR; happy to follow up if maintainers want the explicit signal.
| fn pending_issuance_decimals( | ||
| txs: &[SignedTransaction], | ||
| chain_config: &ChainConfig, | ||
| block_height: BlockHeight, | ||
| ) -> BTreeMap<TokenId, u8> { |
There was a problem hiding this comment.
Token id derivation here uses the token-id-generation version resolved at best_block().next_height(), but a pending transaction is actually confirmed at some future height. make_token_id depends on the height only through chainstate_upgrades().version_at_height(...).token_id_generation_version(), so the derivation diverges from consensus only if an upgrade activates between the snapshot height and the actual inclusion height. This is an edge case, but it would silently yield wrong token ids (and thus wrong/zero decimals and missing dependency edges) for issuances pending across an upgrade boundary. Worth a comment acknowledging the limitation, or resolving the version conservatively (e.g. considering the next scheduled upgrade).
There was a problem hiding this comment.
Documented in 448aac8: the derivation assumes the inclusion height stays within the same token id generation version as the snapshot tip; across an upgrade boundary the derived ids (and the resolved decimals and dependency edges) can be wrong until confirmation. Resolving the version "conservatively" is not really possible without knowing the inclusion height, and the pending data is provisional by nature — it corrects itself once the transactions are confirmed and indexed.
| obj.insert( | ||
| "block_id".into(), | ||
| block | ||
| .as_ref() | ||
| .map_or("".to_string(), |b| { | ||
| b.block_id().to_hash().encode_hex::<String>() | ||
| }) | ||
| .into(), | ||
| block.as_ref().map_or(serde_json::Value::Null, |b| { | ||
| b.block_id().to_hash().encode_hex::<String>().into() | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This is a breaking change to the public response contract: block_id, timestamp and confirmations were previously empty strings for missing values and are now JSON null, and fee is removed entirely for pending transactions. The test suite has been updated to pin the new semantics, but any existing external client parsing these fields as strings will break. Consider documenting this in the API spec/changelog (or an OpenAPI update) alongside the change.
There was a problem hiding this comment.
Documented in 448aac8 in api-server/CHANGELOG.md (the changelog entry of the pending endpoints still described the empty-string shape and an outdated decimals-resolution behavior; it now spells out the null block-related fields, the omitted fee, the lazy mempool listing for the not-yet-indexed tokens and the zero-decimals fallback). Worth noting that this is not a break of an existing contract: previously GET /v2/transaction/{id} returned 404 for a pending transaction, so no client could have parsed these fields in this situation before; the mempool listing endpoint itself is new in this PR.
Note that the unresolvable decimals are rendered as zero with the atoms amounts staying authoritative, that the pending token id derivation is provisional across a token id generation upgrade boundary, and correct the changelog entry of the pending endpoints: the block-related fields are null rather than empty, and the decimals of a yet unknown token are resolved from the mempool listing of the single transaction endpoint as well.
| if pending_token_ids.is_empty() { | ||
| BTreeMap::new() | ||
| } else { |
There was a problem hiding this comment.
This fallback triggers a full mempool_transactions() listing (cost proportional to the mempool size) whenever a transferred token lacks stored decimals. An attacker can submit mempool transactions whose AccountCommands reference never-issued token ids; every GET /transaction/:id for such a transaction then takes the expensive path. The 8-permit semaphore bounds concurrency, but each permit is held for the full listing fetch; consider validating that the pending token ids are at least plausible issuances (e.g. that the same transaction mints/issues them) before fetching the listing, or documenting the accepted cost.
There was a problem hiding this comment.
Documented the accepted cost in 36992f1 (code comment + changelog). Requiring the same transaction to issue the referenced tokens would break the legitimate cross-transaction case this feature exists for — a spending transaction referencing the issuance transaction pending elsewhere in the mempool — so an unresolvable token id cannot be cheaply distinguished from a pending issuance of another transaction, and the request keeps paying one bounded listing fetch (the same cost class as GET /v2/mempool/transactions itself, bounded by the query permits and their wait timeout).
| TxOutput::IssueFungibleToken(_) => { | ||
| let token_id = match make_token_id(chain_config, block_height, inputs) { | ||
| Ok(token_id) => token_id, |
There was a problem hiding this comment.
The IssueFungibleToken, IssueNft, CreateOrder, CreateStakePool and CreateDelegationId output arms do not register a Dependency::Utxo provider edge, unlike the catch-all arm. Any mempool transaction that spends an output of such a transaction via TxInput::Utxo will therefore have a dependent edge whose provider is missing, making topological_sort fail with MissingDependency — which downgrades the whole /mempool/transactions listing to insertion order. Registering the Utxo provider edge in these arms (e.g. alongside the existing registrations) keeps the ordering working for spends of these outputs.
Suggestion:
| TxOutput::IssueFungibleToken(_) => { | |
| let token_id = match make_token_id(chain_config, block_height, inputs) { | |
| Ok(token_id) => token_id, | |
| TxOutput::IssueFungibleToken(_) => { | |
| // The output is still a spendable UTXO, so register the Utxo | |
| // provider edge in addition to the token creation one, otherwise | |
| // a mempool spend of this output reports a missing dependency. | |
| let outpoint = UtxoOutPoint::new( | |
| OutPointSourceId::Transaction(tx.transaction().get_id()), | |
| out_index as u32, | |
| ); | |
| dependencies | |
| .providers | |
| .entry(Dependency::Utxo(outpoint)) | |
| .or_default() | |
| .push(tx_index); | |
| let token_id = match make_token_id(chain_config, block_height, inputs) { | |
| Ok(token_id) => token_id, |
There was a problem hiding this comment.
Good catch — fixed in 36992f1 by hoisting the utxo provider registration above the output match: every output is a spendable utxo regardless of its type, so the edge is now registered for all of them (the previously duplicated registrations in the delegate-staking and catch-all arms are gone, so a future arm cannot forget it either). Covered by the new test_typed_output_utxo_dependency_chain regression test, which spends the output of a pending token issuance and asserts the dependency edge.
Every output is a spendable utxo regardless of its type, so the utxo provider edge is registered for all of them instead of only the plain transfer ones; a mempool transaction spending the output of, say, a pending token issuance used to make the whole dependency ordering fail with a missing dependency and the listing fall back to the insertion order. The accepted cost of the single transaction endpoint fetching the mempool listing for a transaction referencing a token that does not exist at all is documented, as is the endpoint in the changelog.
| for tx_index in tx_indices { | ||
| // A transaction can be both a provider and a dependent of the same | ||
| // dependency (e.g. a transaction carrying two account commands of the | ||
| // same token at consecutive nonces): such a self-dependency carries no | ||
| // ordering information and would be reported as a cycle. | ||
| let self_id = dependency_nodes[*tx_index].id; | ||
| dependency_nodes[*tx_index] | ||
| .dependencies | ||
| .extend(providers.iter().filter(|id| **id != self_id).copied()); | ||
| } |
There was a problem hiding this comment.
build_dependency_graph can add the same dependency id twice to a node's dependencies (e.g. a transaction with two inputs mapping to the same Dependency, or a provider listed twice for one key). The duplicate edges inflate both indegrees and the dependents lists symmetrically, so the sort is currently still correct, but this invariant is implicit and fragile: deduplicating on one side only would corrupt the sort. Worth deduplicating the edges explicitly (e.g. a sort_unstable().dedup() on the extended dependencies, or inserting into a BTreeSet first) so the graph contract does not depend on the caller's symmetric bookkeeping.
Suggestion:
| for tx_index in tx_indices { | |
| // A transaction can be both a provider and a dependent of the same | |
| // dependency (e.g. a transaction carrying two account commands of the | |
| // same token at consecutive nonces): such a self-dependency carries no | |
| // ordering information and would be reported as a cycle. | |
| let self_id = dependency_nodes[*tx_index].id; | |
| dependency_nodes[*tx_index] | |
| .dependencies | |
| .extend(providers.iter().filter(|id| **id != self_id).copied()); | |
| } | |
| for tx_index in tx_indices { | |
| // A transaction can be both a provider and a dependent of the same | |
| // dependency (e.g. a transaction carrying two account commands of the | |
| // same token at consecutive nonces): such a self-dependency carries no | |
| // ordering information and would be reported as a cycle. | |
| let self_id = dependency_nodes[*tx_index].id; | |
| let deps = &mut dependency_nodes[*tx_index].dependencies; | |
| deps.extend(providers.iter().filter(|id| **id != self_id).copied()); | |
| // A node can declare the same dependency twice (e.g. two inputs | |
| // mapping to the same dependency): keep the edges unique so that | |
| // the indegrees and the adjacency list stay consistent. | |
| deps.sort_unstable(); | |
| deps.dedup(); | |
| } |
| if acct.nonce().value() == 0 { | ||
| dependencies | ||
| .dependents | ||
| .entry(Dependency::DelegationCreation(*delegation_id)) | ||
| .or_default() | ||
| .push(tx_index); | ||
| } |
There was a problem hiding this comment.
No code ever registers a provider for Dependency::DelegationCreation (or for Dependency::DelegationSpending(id, nonce 0)): CreateDelegationId outputs are only registered as dependents of PoolCreation, and DelegationSpending providers are only produced by the previous nonce's transaction. As a result, any mempool listing that contains the first DelegationBalance spend of a delegation (nonce 0) — even when the delegation and pool are already confirmed on-chain, which is the common case — has a dependent key with no provider, so topological_sort fails with MissingDependency and the entire dependency-ordered listing falls back to insertion order. Note the same holds for Utxo dependencies on outpoints not in the snapshot, but those can't be expressed here; for delegation spends they always are. Suggested fix: only add the DelegationCreation/DelegationSpending(nonce 0) dependent keys when the providing transaction is actually present in the listing (e.g. build the graph, then drop edges whose provider is missing), or treat them like on-chain requirements and skip them.
Suggestion:
| if acct.nonce().value() == 0 { | |
| dependencies | |
| .dependents | |
| .entry(Dependency::DelegationCreation(*delegation_id)) | |
| .or_default() | |
| .push(tx_index); | |
| } | |
| // Only track nonce-sequenced dependencies between the | |
| // transactions of this listing; the first spend (nonce 0) | |
| // may depend on a delegation confirmed long ago, which | |
| // is not a dependency between mempool transactions. | |
| if acct.nonce().value() == 0 && false { | |
| unimplemented!(); | |
| } |
2 participants
GET /transaction/:id now falls back to the mempool of the connected node when the transaction is not confirmed yet, and a new GET /mempool/transactions endpoint lists pending transactions (paginated, with an optional dependency ordering via ?order=dependency and an x-mempool-ordering response header). Based on #2029 by @OBorce — the transaction dependency ordering module is reused from that PR. Unlike the original approach, the endpoints proxy the node's mempool instead of indexing it into the api-server storage, complementing the SSE event stream (#2117): tx_seen events can now be hydrated through REST. No storage changes; no consensus or p2p behavior changes.