feat(node): txn prioritization based on gas parameters #1185
Conversation
fendermint/app/src/app.rs
Outdated
| ) | ||
| .context("error creating check state")? | ||
| } | ||
| None => self.create_check_state()?, |
There was a problem hiding this comment.
We can avoid loading the 5 blocks state params for check_tx, to do this, we must change the commit to hold some historical check_state instead of purging everything. Not sure if we should do in this PR.
raulk
changed the title
| if msg.gas_fee_cap >= base_fee { | ||
| let i = msg.gas_fee_cap.clone() - base_fee + msg.gas_premium.clone(); | ||
| i.atto() | ||
| .min(&BigInt::from(i64::MAX)) | ||
| .to_i64() | ||
| .expect("clipped to i64 max") | ||
| } else { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
-
These kinds of conditionals are easier to follow when written as an early return. If the gas_fee_cap is below the base fee, return 0.
-
Are we sure 0 is the right value here? Why not return i64::MIN if the message is non-includable? Right now messages with gas_fee_cap == base_fee && gas_premium == 0 would rank exactly the same as non-includable messages. Seems wrong?
-
Adding the gas premium to the delta between the gas_fee_cap and the base fee is incorrect. The gas_fee_cap includes the payable base fee and the gas premium. When the message is includable, our priority should be proportional to the effective gas premium. See how the effective gas premium is calculated in ref-fvm; copy the logic here.
-
i64::MAX atto is equal to 9.223372036854775807 base units. We are probably safe in clipping the effective premium to i64::MAX, since nobody sane will set such a high premium, but I'm uncomfortable with the panic here. I suggest this instead, which saturates to i64::MAX:
effective_premium.atto().to_i64().unwrap_or(i64::MAX);
|
@raulk Updated, please help take another look. |
fendermint/app/src/app.rs
Outdated
| let mut t = to_check_tx(ret); | ||
| t.priority = priority; | ||
| t |
There was a problem hiding this comment.
nit: feed the priority as a param to to_check_tx?
| #[derive(Clone, Debug)] | ||
| pub struct TxnPriorityCalculator { | ||
| /// Ring buffer of base fee history | ||
| base_fee_history: Vec<Option<TokenAmount>>, |
There was a problem hiding this comment.
Can you explain why we need to keep a base fee history? In my head, this component would just know the current base fee and prioritise inclusion based on it -- past base fees are irrelevant. A transaction is either includable or not includable now. And we know for certain that after a block is committed, CometBFT will recheck all pending transactions before proposing the next block, right?
There was a problem hiding this comment.
I think this was quite early discussion on one of the huddles to use the lowest base fee in recent history to let slightly more transaction to go through to avoid empty blocks. An array is also tuneable in how base fee can be selected. The requirement is updated slightly and base fee selection is now includable/non-includable. Then the most recent base fee should be enough. I can update this accordingly.
There was a problem hiding this comment.
We'll need to fix this. The base fee determines inclusion and the burn rate (network fee). By no means should a transaction be included if its gas fee cap is below the current base fee for the block it's included in. That's a consensus fault.
|
@raulk updated accordingly. Now the priority calculation is based on the latest base fee from the block gas market reading. Anything below will have priority i64::MIN. |
raulk
left a comment
raulk
left a comment
There was a problem hiding this comment.
An integration test here would be great.
- Submit many transactions with high gas limit to stuff blocks and make the base fee spike to X or above.
- Submit a transaction with gas fee cap way below X but above the minimum base fee.
- Stop the flood so the base fee decreases, and ensure the transaction does not appear until the base fee is below the gas fee cap.
I will merge this and open an issue for these extra tests.
|
@sanderpick feel free to integrate this fix on your side. There are still some edge cases for us to deal with (e.g. rejecting transactions below minimum base fee). |
great, ty! |
3 participants
This is a follow up to gas market feature and closes #1183.
The implementation follows the method 2 described. A high-level review is as follows:
ipc/fendermint/vm/interpreter/src/fvm/state/priority.rs
Line 47 in 17796a0