refactor: Receipt handling to be done in VM logic #6468
Conversation
There was a problem hiding this comment.
This looks beautiful!
I have only one overall comment -- let's just make receipts a public field of VMOutcome (burdening the caller with the task of converting them to actual recipts). Other than that, it looks basically good to me! Though, I didn't do a super thorough review, as this is still a draft.
austinabell
changed the title
| .iter() | ||
| .map(|(_, weight)| weight) | ||
| // Saturating sum of weights | ||
| .fold(0, |sum, val| sum.checked_add(*val).unwrap_or(u64::MAX)); |
There was a problem hiding this comment.
Should this be just sum (ie, panic on overflow)
There was a problem hiding this comment.
The implementation doesn't panic though, so this would cause inconsistency (one of the test cases has larger than u64::MAX ratio sum)
| if let Some(Action::FunctionCall(FunctionCallAction { ref mut gas, .. })) = self | ||
| .action_receipts | ||
| .0 | ||
| .get_mut(*receipt_index) |
There was a problem hiding this comment.
Let's extract this bit into fn get_fuction_call_action_mut function, it'll more clearl y explain the purpose of FunctionCallIndex construct.
There was a problem hiding this comment.
The reason I didn't do this is because this closure is used within code blocks where self is borrowed immutably, so this wouldn't be able to be used. I'll extract it into a helper function but I'm unsure that's better
| let mut distributed = 0; | ||
| for (action_index, GasWeight(weight)) in &self.gas_weights { | ||
| // This can't overflow because the gas_per_weight is floor division | ||
| // of the weight sum. | ||
| let assigned_gas = gas_per_weight * weight; | ||
|
|
||
| distribute_gas(action_index, assigned_gas); | ||
|
|
||
| distributed += assigned_gas | ||
| } | ||
|
|
||
| // Distribute remaining gas to final action. | ||
| if let Some((last_idx, _)) = self.gas_weights.last() { | ||
| distribute_gas(last_idx, gas - distributed); | ||
| } |
There was a problem hiding this comment.
🤔 I am still not happy about this algorithm. A bad example here would be if we want to distribute 10 gas across 10 function calls evenly, but where each weight is 1000. In that situation, gas_per_weight will be zero, and only the last call will get the gas.
I suggest tackling this separately though, let's first land this much needed refactor.
There was a problem hiding this comment.
So with this, this is more of an edge case where the weight is greater or close to the amount of gas remaining.
The downside to this is just that it requires extra logic and computation to handle for this edge case. I'm also wondering in the case of splitting 12 gas among the 10 function calls, do the last two functions receive an extra 1 gas or how will this happen?
I don't have an opinion on which direction this goes, I was just defaulting to this for simplicity rather than creating a bunch of semantics that only does something subjectively meaningful with improper usage.
There was a problem hiding this comment.
I don't think handlig this particular cases requires that much excar logic. I think what we need here is
let assigned_gas = ((gas_to_distribute as u128 * weight as u128) / sum_weights) as u64;
That is, we just needlessly loosing precision here.
Distributing the remainder evenly is a different, smaller problem, that I think would require more complex logic.
There was a problem hiding this comment.
This is what I'm referring to, the remainder distribution being more calculations.
My intuition is that it would divide the amount of remaining gas by the number of individual weights to get the distribution of gas per function call, then get the remainder of that division (x) to attach the remaining gas to final x number of function calls.
Seems like unnecessary logic and computation for what will be trivial amounts of gas unless specifically misused by specifying gas weights larger than the amount of gas, but I'm happy to make the change if there is consensus on this :)
There was a problem hiding this comment.
From what I see, here we choose between
1) let assigned_gas = ((gas_to_distribute as u128 * weight as u128) / sum_weights) as u64;
2) let assigned_gas = (gas_to_distribute / sum_weights) * weight;
I don't see a significant difference for regular usecases, so I would also choose (2) because it allows to make less operations.
|
This looks good to me! As this is a larger refactor, I think it'll benefit from a second pair of eyes doing the review as well. @Longarithm would you have time to take a look here? |
Refactors receipt handling from being done in all
Externalimplementations to being done in vm-logic. A few refactoring patterns were explored but this one solved the main issues of testability and deduplication the best I've found.The tests are somewhat of a property-based test with all common cases included, but this can be expanded to be fuzzed as well. (might do this in separate PR though)