feat: CheckTX Priority #10507
feat: CheckTX Priority #10507
Conversation
|
unblocked |
|
A fix to this would be using only one fee denom to determine the priority. Or if we don't want to add configuration, using the min integer value of all the demons in the fee so it's not easily gameable. |
|
@aaronc I don't really understand the concern. What do you mean by "but isn't even a fee denom."? In any case, I implemented and even documented that this is a naive approach, which will work just fine for the Hub btw. Applications that need a more sophisticated model would just implement their own middleware. In fact, they could do so and release updates and changes to this middleware in point releases (non-breaking). This PR wasn't intended to solve most uses cases, but rather just get the ball rolling on a basic approach to tx priority. |
|
|
||
| // GetTxPriority returns a naive tx priority based on the total sum of all fees | ||
| // provided in a transaction. | ||
| func GetTxPriority(fee sdk.Coins) int64 { |
There was a problem hiding this comment.
how about renaming to ComputeTxPriority?
|
Aaron concerns is about rebasing the fees (if there are fees in multiple tokens) to a common denominator. |
I'm not sure why this is so complicated. I suggested a very simple approach to make this non-gameable. I'll make a code suggestion with my approach. |
| func GetTxPriority(fee sdk.Coins) int64 { | ||
| var priority int64 | ||
| for _, c := range fee { | ||
| priority += c.Amount.Int64() |
There was a problem hiding this comment.
| priority += c.Amount.Int64() | |
| p := c.Amount.Int64() | |
| if priority == 0 || p < priority { | |
| priority = p | |
| } |
There was a problem hiding this comment.
I know you're just taking the minimum here. What does this really buy you? If all fees provided are valid denominations, what is the issue exactly?
There was a problem hiding this comment.
but the denominations have different prices and settling on the minimum makes it hard to use a cheaper denom alongside the primary denom to boost a tx. the primary denom will take precedence with the minimum
There was a problem hiding this comment.
I would say this is out of scope of a naive implementation. Its either we spend time designing a new way to do this, which we have pre open for but no reviews, or we document teams how do this and say they have to do it based on their use case. We don't have the man power to design yet another thing right now
Maybe adding a warning godoc could benefit this.
There was a problem hiding this comment.
But why is there a lot of design needed? It seems like just taking the min solves a lot to potential issues and is a very simple code change
There was a problem hiding this comment.
I agree with Aaron, what if one denom has exponent = 2, and another one 6? Adding 2 = 0.2 (fee from first denom) to 500 = 0.0005 (fee from the secon denom) doesn't really make sense. So at least we should document / add a comment, that his is a naive implementation.
Set the
PriorityinResponseCheckTxto prioritize txs in Tendermint's mempool. For now, we compute the tx priority naively as the total sum of all fees in a tx.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change