Expose transaction validation cost in mempool transaction JSON - #2547
Conversation
Block assembly is bounded by maxBlockSize and maxBlockCost, both enforced
by CandidateGenerator.correctLimits. The mempool API returned `size` but
not `cost`, so external block builders could enforce only the size limit
and had to recompute or guess per-transaction cost, even though the node
already measured it on mempool admission.
Add a `cost` field to the mempool transaction JSON, sourced from
UnconfirmedTransaction.lastCost. It is null when the node never measured
the cost: digest state, or a transaction returned to the pool by a
rollback and not yet re-validated by CleanupWorker.
Affected endpoints:
GET /transactions/unconfirmed
GET /transactions/unconfirmed/byTransactionId/{txId}
POST /transactions/unconfirmed/byErgoTree
byTransactionId used ErgoMemPoolReader.modifierById, which returns
Option[ErgoTransaction] and drops the cost. Add unconfirmedById to the
reader, returning the UnconfirmedTransaction wrapper; modifierById is now
defined in terms of it. byErgoTree kept the wrapper out of its collect
blocks, so those become filters. Deduplication is unchanged:
UnconfirmedTransaction.equals is defined on the transaction id.
The digest-state branch of byErgoTree previously serialized a bare
ErgoTransaction, which did not match the ErgoTransactionWithInputBoxes
schema the spec declares for that response. It now goes through the same
JSON builder, so `cost` is never silently absent.
The field is additive and optional, so clients ignoring unknown keys are
unaffected. Confirmed-transaction schemas are untouched, as lastCost only
exists for unconfirmed transactions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jozanek
left a comment
There was a problem hiding this comment.
Thanks for the description — the implementation notes made this quick to follow, and the equals/hashCode reasoning behind the collect → filter change holds up.
I read this as: publish a number the pool already holds, so tooling can reason about the mempool without re-running validation. On that reading the field belongs, the nullable-field shape looks right to me, and the blast radius is small — explorer-backend (circe) and ergo-appkit (Gson) both ignore unknown keys.
Where I got stuck is the stated why. Both justifications came apart against the code they cite:
- Block-budget enforcement.
collectTxsnever readslastCost— it re-runsvalidateWithCostagainst its ownupcomingContext(CandidateGenerator.scala:979-987), and budgetsmaxBlockCost - safeGap, with safeGap up to 500,000 (:680-696).correctLimitsalso counts the fee transaction the node synthesizes itself. A builder summing these againstmaxBlockCostovershoots, andcollectTxstruncates silently rather than rejecting (:1000-1003). - Reproducing FeePerCycle ordering.
mempoolSorting = "random"is the shipped default (application.conf:88) — a per-process coin flip that no endpoint exposes. AndOrderedTxPool.putdiscards the newfeeFactorwhen the id is already present (:101-110), so a cleanup pass moves the reported cost but not the weight.
Neither sinks the field. They point at a different framing — mempool observability: fee-market analysis, simulation, "why isn't my transaction being mined" — which it serves well. Would you be open to re-justifying it that way?
Questions:
- Is there a consumer that would sum these against a budget? If yes, I think the shape needs to change and
/mining/candidateWithTxsis the better vehicle. If every consumer is descriptive, the field is right and the rest is wording. - #2534 describes this change but isn't linked — was that deliberate? It also records
getAll(Seq(id))as an alternative to touching the trait. I lean towardunconfirmedByIdbeing the better call, so I'd mainly like the reasoning captured where the next reader finds it. POST /transactions/checkalready computes this exact number and discards it (ErgoBaseApiRoute.scala:136-141). Worth a line on why the pool-side view was the one you needed?- Base
v6.0.6matches convention, but the carry-forward PR #2473 is currently CONFLICTING — is there a plan for how this reaches 6.0.7?
I couldn't find any third-party request for this: #1854, the wishlist that produced these endpoints, doesn't mention cost. Not evidence against the change, but the constituency isn't visible to a reviewer.
What would move me to approve: corrected wording at the two description sites, a line preserving the required-list tri-state, and the null-cost test pointed at digestReadersRef.
| format: int32 | ||
| cost: | ||
| description: >- | ||
| Validation cost of the transaction, as measured when it entered the mempool. |
There was a problem hiding this comment.
This says cost is measured when the transaction entered the mempool, but CleanupWorker.scala:90-92 overwrites it via head.withCost(txCost) on every re-validation pass for transactions older than mempoolCleanupDuration (30s default) — which is what UnconfirmedTransaction.scala:12 means by "during last check".
An integrator reading this will treat the number as pinned to admission and may cache it. Would you be open to something like "as of this node's most recent validation of this transaction, against the state at that time"? Since the re-check pass is itself budget-limited (CostLimit at CleanupWorker.scala:27), two honest nodes can also report different costs for the same transaction, which may be worth a clause.
There was a problem hiding this comment.
Yea that is fair to change
| description: Size in bytes | ||
| type: integer | ||
| format: int32 | ||
| cost: |
There was a problem hiding this comment.
The tri-state this sets up — key absent means an older node, null means unmeasured, a number means measured — works because cost stays out of the required list above. That reads deliberate to me and I'd keep it exactly as is.
What I think is missing is anything in the file saying so. A later tidy-up that "completes" the required list would quietly remove the version detection the description relies on, and no test would catch it. Would you be open to a sentence in the description: always present on nodes carrying this field, absence indicates an older node?
There was a problem hiding this comment.
That seems like a reasonable way to handle it.
| /** | ||
| * Creates a transaction JSON representation with resolved input boxes. | ||
| * | ||
| * `cost` is the validation cost measured when the transaction entered the pool. It is None |
There was a problem hiding this comment.
Same wording as the spec description — "measured when the transaction entered the pool". Given CleanupWorker refreshes it, the next reader of this method may take the value as fixed for the lifetime of the pool entry.
Would you mind matching whatever wording you settle on in openapi.yaml, so the two don't drift apart later?
| txsWithOutputMatch | ||
| .slice(offset, offset + limit) | ||
| .map(_.asJson) | ||
| .map(utx => createTransactionWithResolvedInputs(utx.transaction, Map.empty, utx.lastCost)) |
There was a problem hiding this comment.
The description presents this as bringing the digest branch into line with the ErgoTransactionWithInputBoxes schema. Reading the schema, I don't think it arrives there: ErgoBoxWithSpendingProof is allOf[ErgoTransactionOutput, ...], and ErgoTransactionOutput requires value, ergoTree, additionalRegisters and creationHeight (openapi.yaml:110-116). With Map.empty the inputs are still {boxId, spendingProof}.
The change itself still looks right to me — it makes this branch consistent with the two sibling endpoints and keeps cost from going missing on one of them. Would you be open to describing it as a consistency fix rather than a conformance fix, so a later spec audit doesn't treat this as closed?
There was a problem hiding this comment.
Sure that makes sense
| } | ||
| } | ||
|
|
||
| it should "report the same validation cost from the list and by-id endpoints" in { |
There was a problem hiding this comment.
Both requests here read lastCost from the same in-process pool object, so I think this can only fail if one of the two tests above it has already failed.
No action needed if it's here to document the invariant — I mostly want to check I'm not missing a path where the list and by-id routes could diverge.
There was a problem hiding this comment.
This was mostly to document the change
| it should "report null validation cost when the node did not measure it" in { | ||
| // Stubs' mempool holds transactions put without a cost, as happens in digest state | ||
| // or for transactions returned to the pool by a rollback. | ||
| Get(prefix + s"/unconfirmed/byTransactionId/${txs.head.id}") ~> route ~> check { |
There was a problem hiding this comment.
The comment describes digest state and rollback, but this request goes to route, which line 42 wires to utxoReadersRef — and txs.head comes from the Stubs pool, built as UnconfirmedTransaction(tx, None) and never validated. So it asserts null on a fixture that was constructed null, rather than on a node that couldn't measure.
digestReadersRef already exists at Stubs.scala:362. Would you be open to pointing this one at it? The digest branch of byErgoTree — the behavior change the implementation notes single out — currently has no coverage at all, and that's the case most likely to regress.
There was a problem hiding this comment.
Yes this makes sense, can fix it
Yes, this will be used to build blocks according to cost budgets, but I am still quite confident that this is the best way to do it. A few reasons:
This is true, but as miners are in a race to build the block, it is best for them to avoid making additional API calls which would require the transaction to be revalidated. As the cost is already computed while the transaction is in the mempool, it is most efficient to just return it as part of the mempool call.
I am mostly focused on getting this ready for 6.0.6 due to the upcoming Lithos launch, as we need both the merkle proof fix in #2463 for proper Lithos functionality and this fix for #2547 to allow proper MEV. But I can look into what is needed for 6.0.7 |
Problem
Block assembly is bounded by two limits,
maxBlockSizeandmaxBlockCost. Both are enforced byCandidateGenerator.correctLimits:The mempool API returns
size, so external tooling can enforce the size limit. It does not returncost, so the cost limit cannot be enforced outside the node.Anyone assembling blocks externally (block template builders, mempool simulators, throughput analysis) has to re-run script validation against a UTXO set, or guess from
sizeand input/output counts. The node already computed the exact value during mempool admission and discarded it at the serialization boundary.Cost is also already load-bearing inside the pool:
SortingOption.FeePerCycleorders by fee-per-cost vialastCostinErgoMemPool.feeFactor. A node in that mode exposes an ordering clients cannot reproduce.Change
Add a
costfield to the mempool transaction JSON, sourced fromUnconfirmedTransaction.lastCost.{ "id": "...", "inputs": [ ... ], "dataInputs": [], "outputs": [ ... ], "size": 344, "cost": 21456 }Affected endpoints:
GET /transactions/unconfirmedGET /transactions/unconfirmed/byTransactionId/{txId}POST /transactions/unconfirmed/byErgoTreeNull semantics
costisnullwhen the node never measured it:lastCostis never set.ErgoNodeViewHolder.updateMemPoolreturns transactions from disconnected blocks withUnconfirmedTransaction(tx, None)and puts them viaput, which skipsvalidateWithCost. They staynulluntilCleanupWorkerre-validates them.The key is always present on a node carrying this change, so clients can distinguish "node too old" (key absent) from "cost unknown" (key
null).Implementation notes
unconfirmedByIdadded toErgoMemPoolReader. The by-id endpoint usedmodifierById, which returnsOption[ErgoTransaction]and drops the wrapper before the route sees it. The new method returns theUnconfirmedTransaction;modifierByIdis now defined in terms of it, so there is one lookup path.ErgoMemPoolimplements it aspool.get(id), identical to whatmodifierByIddid.byErgoTreekeeps the wrapper. Its twocollectblocks mapped totx.transaction, discarding the cost. They becomefilters. Deduplication across the output-match and input-match sets is unchanged, sinceUnconfirmedTransaction.equalsandhashCodeare defined on the transaction id.Digest-state branch of
byErgoTreenow uses the shared JSON builder. It previously serialized a bareErgoTransaction, which did not match theErgoTransactionWithInputBoxesschema the spec declares for that response. Routing it through the same builder fixes that and meanscostis never silently absent from a response that otherwise carries it.Compatibility
Additive and optional. Not in the schema's
requiredlist. Clients that ignore unknown keys are unaffected. Confirmed-transaction schemas (ErgoTransaction,IndexedErgoTransaction,WalletTransaction) are untouched, sincelastCostonly exists for unconfirmed transactions.Tests
Six cases added to
TransactionApiRouteSpec, covering cost reported by id, in the list, viabyErgoTree, agreement between the list and by-id endpoints, thenullcase, and thatsizestill reports correctly alongsidecost.Run locally on
v6.0.6:TransactionApiRouteSpec: 29 passed, 0 failed (23 pre-existing, 6 new)Known follow-up, not in this PR
Rollback-restored transactions report
nulluntil the next cleanup pass. Closing that would mean carrying per-transaction cost from block validation throughupdateMemPool, which is a larger change than exposing a value the pool already holds.🤖 Generated with Claude Code