Serve pending transactions through the v2 REST API - #2122
nullPointerEnjoyer wants to merge 27 commits into
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 10 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.
| 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).
| 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.
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.