Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1822 - Per-Peer Transaction Rules #1891

Merged
merged 20 commits into from
Nov 28, 2022
Merged

Conversation

ApexTheory
Copy link
Collaborator

@kushti kushti changed the base branch from master to v5.0.4 November 16, 2022 10:20
Copy link
Member

@kushti kushti left a comment

Choose a reason for hiding this comment

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

I think we need to have both per-block and per-peer-per-block limits

@ApexTheory
Copy link
Collaborator Author

Hey @kushti , I reverted the interblock cost changes. It now considers both Interblock and Per-Peer costs. Not sure what to set the per-peer block limit to though.


//TODO peer should always be set? Throw exception?
val peer = processingResult.transaction.source.get
val peerTxInfo = txInfo(peer)
Copy link
Member

Choose a reason for hiding this comment

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

Exception can be thrown here if txInfo has no a record for peer

@@ -179,17 +189,35 @@ class ErgoNodeViewSynchronizer(networkControllerRef: ActorRef,
// should not be here, and so ReserveCostValue should not be used
log.warn("Cost is empty in processMempoolResult")
}

//TODO peer should always be set? Throw exception?
val peer = processingResult.transaction.source.get
Copy link
Member

Choose a reason for hiding this comment

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

source can be empty if transaction is submitted via local API. In this case we need to skip txInfo update.

case _: DeclinedTransaction => interblockCost.copy(declinedCost = interblockCost.declinedCost + cost)
case _: FailedTransaction => {
interblockCost.copy(invalidatedCost = interblockCost.invalidatedCost + cost)
peerTxInfo.copy(invalidatedCost = peerTxInfo.invalidatedCost + cost)
Copy link
Member

Choose a reason for hiding this comment

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

ng would be peer-related in this branch, and others also

case _: DeclinedTransaction => {
interblockCost.copy(declinedCost = interblockCost.declinedCost + cost)
peerTxInfo.copy(declinedCost = peerTxInfo.declinedCost + cost)
}
}

log.debug(s"Old global cost info: $interblockCost, new $ng, tx processing cache size: ${txProcessingCache.size}")
interblockCost = ng
Copy link
Member

Choose a reason for hiding this comment

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

but then you put ng into interblock (all peers) data

/**
* Max cost of per-peer transactions we are going to process between blocks
*/
private val MempoolPeerCostPerBlock = 12000000
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be lowered (or configurable?) I assume.

if (interblockCost.totalCost < MempoolCostPerBlock) {
if ((peerOpt.isDefined &&
perPeerCost.getOrElse(peerOpt.get, IncomingTxInfo.empty()).totalCost < MempoolPeerCostPerBlock) ||
interblockCost.totalCost < MempoolCostPerBlock) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

perPeerCost.getOrElse may be a bit overkill here as the perPeerCost(peer) should always be set at this point when the peer is defined. But I figured it's better to continue gracefully in case of any edge cases.

val peerCost = perPeerCost.getOrElse(remote, IncomingTxInfo.empty()).totalCost

val (toProcess, toPutIntoCache) =
if (peerCost < MempoolPeerCostPerBlock || interblockCost.totalCost < MempoolCostPerBlock) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this should be && instead of ||...updating.

@kushti kushti merged commit ac4dbc2 into v5.0.4 Nov 28, 2022
@kushti kushti deleted the 1822-per-peer-transaction-rules branch November 28, 2022 21:14
@kushti kushti mentioned this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants