Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/resources/api/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,16 @@ components:
description: Size in bytes
type: integer
format: int32
cost:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a reasonable way to handle it.

description: >-
Validation cost of the transaction, as measured when it entered the mempool.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that is fair to change

Null when the cost is not known to the node, i.e. when the node is running a
digest state, or when the transaction was returned to the pool by a rollback
and has not been re-validated yet.
type: integer
format: int32
nullable: true
example: 21456

IndexedErgoTransaction:
type: object
Expand Down
36 changes: 22 additions & 14 deletions src/main/scala/org/ergoplatform/http/api/TransactionsApiRoute.scala
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,14 @@ case class TransactionsApiRoute(readersHolder: ActorRef,

/**
* Creates a transaction JSON representation with resolved input boxes.
*
* `cost` is the validation cost measured when the transaction entered the pool. It is None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* when no script validation was done for it, i.e. when the node runs a digest state, or when
* the transaction was returned to the pool by a rollback and has not been re-checked yet.
*/
private def createTransactionWithResolvedInputs(tx: ErgoTransaction, resolvedInputs: Map[BoxId, ErgoBox]): Json = {
private def createTransactionWithResolvedInputs(tx: ErgoTransaction,
resolvedInputs: Map[BoxId, ErgoBox],
cost: Option[Int]): Json = {

val enrichedInputs = tx.inputs.map { input =>
val baseInput = Json.obj(
Expand All @@ -123,7 +129,8 @@ case class TransactionsApiRoute(readersHolder: ActorRef,
"inputs" -> enrichedInputs.asJson,
"dataInputs" -> tx.dataInputs.asJson,
"outputs" -> tx.outputs.asJson,
"size" -> tx.size.asJson
"size" -> tx.size.asJson,
"cost" -> cost.asJson
)
}

Expand All @@ -137,19 +144,20 @@ case class TransactionsApiRoute(readersHolder: ActorRef,
val enrichedTxs = transactions.map { unconfirmedTx =>
val tx = unconfirmedTx.transaction
val resolvedInputs = resolveTransactionInputs(tx.inputs, state, pool)
createTransactionWithResolvedInputs(tx, resolvedInputs)
createTransactionWithResolvedInputs(tx, resolvedInputs, unconfirmedTx.lastCost)
}
enrichedTxs.asJson
}

/**
* Resolves inputs for a single transaction and returns it with resolved inputs.
*/
private def getUnconfirmedTransactionWithResolvedInputs(transaction: ErgoTransaction): Future[Json] =
private def getUnconfirmedTransactionWithResolvedInputs(unconfirmedTx: UnconfirmedTransaction): Future[Json] =
getStateAndPool.map {
case (state, pool) =>
val transaction = unconfirmedTx.transaction
val resolvedInputs = resolveTransactionInputs(transaction.inputs, state, pool)
createTransactionWithResolvedInputs(transaction, resolvedInputs)
createTransactionWithResolvedInputs(transaction, resolvedInputs, unconfirmedTx.lastCost)
}

private def getUnconfirmedTransactions(offset: Int, limit: Int): Future[Json] = getUnconfirmedTransactionsWithResolvedInputs(offset, limit)
Expand Down Expand Up @@ -247,7 +255,7 @@ case class TransactionsApiRoute(readersHolder: ActorRef,
(pathPrefix("unconfirmed" / "byTransactionId") & get & modifierId) { modifierId =>
ApiResponse(
getMemPool.flatMap { pool =>
pool.modifierById(modifierId) match {
pool.unconfirmedById(modifierId) match {
case Some(unconfirmedTx) =>
getUnconfirmedTransactionWithResolvedInputs(unconfirmedTx)
case None =>
Expand Down Expand Up @@ -292,25 +300,25 @@ case class TransactionsApiRoute(readersHolder: ActorRef,
val allTxs = pool.getAll
val txsWithOutputMatch =
allTxs
.collect { case tx if tx.transaction.outputs.exists(_.ergoTree.bytesHex == ergoTree) =>
tx.transaction
}.toSet
.filter(_.transaction.outputs.exists(_.ergoTree.bytesHex == ergoTree))
.toSet

getState.flatMap {
case state: UtxoStateReader =>
val txWithInputMatch =
allTxs
.collect { case tx if
tx.transaction.inputs.exists(i => state.boxById(i.boxId).exists(_.ergoTree.bytesHex == ergoTree)) =>
tx.transaction
}
.filter(
_.transaction.inputs.exists(i => state.boxById(i.boxId).exists(_.ergoTree.bytesHex == ergoTree))
)
val allMatchingTxs = (txsWithOutputMatch ++ txWithInputMatch).toSeq.slice(offset, offset + limit)
Future.sequence(allMatchingTxs.map(getUnconfirmedTransactionWithResolvedInputs)).map(_.asJson)
case _ =>
// digest state: inputs cannot be resolved, but the response keeps the shape
// declared by the ErgoTransactionWithInputBoxes schema, `cost` included
Future.successful(
txsWithOutputMatch
.slice(offset, offset + limit)
.map(_.asJson)
.map(utx => createTransactionWithResolvedInputs(utx.transaction, Map.empty, utx.lastCost))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that makes sense

.asJson
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ class ErgoMemPool private[mempool](private[mempool] val pool: OrderedTxPool,
override def size: Int = pool.size

override def modifierById(modifierId: ModifierId): Option[ErgoTransaction] = {
pool.get(modifierId).map(unconfirmedTx => unconfirmedTx.transaction)
unconfirmedById(modifierId).map(unconfirmedTx => unconfirmedTx.transaction)
}

override def unconfirmedById(modifierId: ModifierId): Option[UnconfirmedTransaction] = {
pool.get(modifierId)
}

override def contains(modifierId: ModifierId): Boolean = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ trait ErgoMemPoolReader extends NodeViewComponent with ContainsModifiers[ErgoTra

def modifierById(modifierId: ModifierId): Option[ErgoTransaction]

/**
* Returns the pooled transaction along with the data the pool keeps for it
* (validation cost, timestamps, source peer), unlike `modifierById` which returns
* the transaction only.
*
* @param modifierId - transaction id
* @return unconfirmed transaction wrapper, or None if the pool does not hold it
*/
def unconfirmedById(modifierId: ModifierId): Option[UnconfirmedTransaction]

/**
* Returns transaction ids with weights. Weight depends on a fee a transaction is paying.
* Resulting transactions are sorted by weight in descending order.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import org.ergoplatform.ErgoBox.{AdditionalRegisters, NonMandatoryRegisterId, To
import org.ergoplatform.http.api.{ApiCodecs, TransactionsApiRoute}
import org.ergoplatform.modifiers.mempool.{ErgoTransaction, UnconfirmedTransaction}
import org.ergoplatform.nodeView.ErgoReadersHolder.{GetDataFromHistory, GetReaders, Readers}
import org.ergoplatform.nodeView.mempool.ErgoMemPool
import org.ergoplatform.settings.RESTApiSettings
import org.ergoplatform.utils.Stubs
import org.ergoplatform.{DataInput, ErgoBox, ErgoBoxCandidate, Input}
Expand Down Expand Up @@ -73,6 +74,21 @@ class TransactionApiRouteSpec extends AnyFlatSpec
TransactionsApiRoute(readers2, nodeViewRef, settings).route
}

val txCost = 21456

// Pool holding exactly `tx`, with a known validation cost, so `cost` is deterministic.
val costedRoute: Route = {
val mp = ErgoMemPool.empty(settings).put(UnconfirmedTransaction(tx, None).withCost(txCost))
class CostedReadersStub extends Actor {
def receive: PartialFunction[Any, Unit] = {
case GetReaders => sender() ! Readers(history, utxoState, mp, wallet)
case GetDataFromHistory(f) => sender() ! f(history)
}
}
val readers = system.actorOf(Props(new CostedReadersStub))
TransactionsApiRoute(readers, nodeViewRef, settings).route
}

it should "post transaction" in {
Post(prefix, tx.asJson) ~> route ~> check {
status shouldBe StatusCodes.OK
Expand Down Expand Up @@ -319,4 +335,56 @@ class TransactionApiRouteSpec extends AnyFlatSpec
}
}

it should "report validation cost of unconfirmed tx by id" in {
Get(prefix + s"/unconfirmed/byTransactionId/${tx.id}") ~> costedRoute ~> check {
status shouldBe StatusCodes.OK
responseAs[Json].hcursor.downField("cost").as[Int] shouldEqual Right(txCost)
}
}

it should "report validation cost in the unconfirmed tx list" in {
Get(prefix + "/unconfirmed") ~> costedRoute ~> check {
status shouldBe StatusCodes.OK
val costs = responseAs[List[Json]].map(_.hcursor.downField("cost").as[Int])
costs shouldEqual List(Right(txCost))
}
}

it should "report the same validation cost from the list and by-id endpoints" in {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mostly to document the change

val listed = Get(prefix + "/unconfirmed") ~> costedRoute ~> check {
responseAs[List[Json]].map(_.hcursor.downField("cost").as[Int])
}
val byId = Get(prefix + s"/unconfirmed/byTransactionId/${tx.id}") ~> costedRoute ~> check {
responseAs[Json].hcursor.downField("cost").as[Int]
}
listed shouldEqual List(byId)
}

it should "report validation cost when searching unconfirmed txs by ergoTree" in {
val searchedTree = tx.outputs.head.ergoTree.bytesHex
Post(prefix + s"/unconfirmed/byErgoTree", searchedTree) ~> costedRoute ~> check {
status shouldBe StatusCodes.OK
val costs = responseAs[List[Json]].map(_.hcursor.downField("cost").as[Int])
costs shouldEqual List(Right(txCost))
}
}

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this makes sense, can fix it

status shouldBe StatusCodes.OK
val cursor = responseAs[Json].hcursor
cursor.keys.map(_.toList).getOrElse(Nil) should contain("cost")
cursor.downField("cost").focus shouldEqual Some(Json.Null)
}
}

it should "keep reporting size alongside cost" in {
Get(prefix + s"/unconfirmed/byTransactionId/${tx.id}") ~> costedRoute ~> check {
status shouldBe StatusCodes.OK
responseAs[Json].hcursor.downField("size").as[Int] shouldEqual Right(tx.size)
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ trait MempoolTestHelpers {

override def modifierById(modifierId: ModifierId): Option[ErgoTransaction] = ???

override def unconfirmedById(modifierId: ModifierId): Option[UnconfirmedTransaction] = ???

override def getAll(ids: Seq[ModifierId]): Seq[UnconfirmedTransaction] = ???

override def size: Int = ???
Expand Down
Loading