CongestionTracker: #21164
CongestionTracker: #21164
Conversation
mystenmark
commented
Feb 10, 2025
mystenmark
commented
- Witnesses congestion failures from checkpointed effects
- Returns a suggested gas price given a set of input objects
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
| if state.is_fullnode(epoch_store) { | ||
| state.congestion_tracker.process_checkpoint_effects( | ||
| transaction_cache_reader, | ||
| &checkpoint, | ||
| &effects, | ||
| ); | ||
| } |
There was a problem hiding this comment.
we don't care that this won't be called in-order right?
There was a problem hiding this comment.
If you want you can move this logic down into where we already have a checkpoint data loaded in order to eliminate redundant loads that'll happen here as well as down there.
There was a problem hiding this comment.
I'm not worried about the out of order-ness - we already use the checkpoint timestamps to ensure we have recent data, and up-to-date FNs will naturally process checkpoints in order anyway. If the FN isn't up to date the data will be useless no matter what.
| } else { | ||
| cleared_events.push(( | ||
| gas_price, | ||
| effect | ||
| .input_shared_objects() | ||
| .into_iter() | ||
| .filter_map(|object| match object { | ||
| InputSharedObject::Mutate((id, _, _)) => Some(id), | ||
| InputSharedObject::Cancelled(_, _) | ||
| | InputSharedObject::ReadOnly(_) | ||
| | InputSharedObject::ReadDeleted(_, _) | ||
| | InputSharedObject::MutateDeleted(_, _) => None, | ||
| }) | ||
| .collect::<Vec<_>>(), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
I assume we only care if we don't have congested objects, not just that the txn succeeded (which is why this logic doesn't actually look at the success status)
There was a problem hiding this comment.
congested objects indicates that the tx was aborted due to congestion.
There was a problem hiding this comment.
Sorry for the late review @mystenmark . This LGTM with some minor comments!
| } | ||
| std::cmp::Ordering::Equal => { | ||
| // there were both successes and cancellations. | ||
| info.lowest_executed_gas_price |
There was a problem hiding this comment.
Can highest_cancelled_gas_price > lowest_executed_gas_price? If so, I wonder if this should be max(lowest_executed_gas_price, highest_cancelled_gas_price)
There was a problem hiding this comment.
In this case the last success and last cancel time indicate that the events are from the same checkpoint. So no, I don't believe the highest cancelled could be higher than the lowest executed
There was a problem hiding this comment.
Actually, I guess it could, because a transaction could have been cancelled because it was also using some other even more expensive object. But in this case lowest executed is still probably the right thing.
| } | ||
|
|
||
| fn update_for_cancellation(&mut self, now: CheckpointTimestamp, gas_price: u64) { | ||
| self.last_cancellation_time = now; |
There was a problem hiding this comment.
Does now ever differ from self.last_cancellation_time? Seems this is only called in the scope of one checkpoint.
There was a problem hiding this comment.
It can differ because there may not be any cancellations in the most recently processed checkpoint, in which case this wouldn't be called.
| entry.into_mut().update_for_cancellation(now, *gas_price); | ||
| } | ||
| Entry::Vacant(entry) => { | ||
| let info = CongestionInfo { |
There was a problem hiding this comment.
Should this branch take self.get_congestion_info(*object) into consideration?
There was a problem hiding this comment.
I don't think so since that would be from the previous checkpoint. We consult it in the second loop to determine whether cleared events should be added, as explained in the comment.
There was a problem hiding this comment.
If previous checkpoint is not considered, meaning all the CongestionInfo are just created in this loop, does "now" ever differ from self.last_cancellation_time when calling update_for_cancellation (from my other comment)? Or do I miss something?
There was a problem hiding this comment.
i'm not sure I understand the question - I'm setting last_cancellation_time equal to now so no it would not differ?
mystenmark
force-pushed
the
mlogan-gas-info
branch
from
eba279e to
8237004
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
- Witnesses congestion failures from checkpointed effects - Returns a suggested gas price given a set of input objects
mystenmark
force-pushed
the
mlogan-gas-info
branch
from
8237004 to
57ab249
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| entry.into_mut().update_for_cancellation(now, *gas_price); | ||
| } | ||
| Entry::Vacant(entry) => { | ||
| let info = CongestionInfo { |
There was a problem hiding this comment.
If previous checkpoint is not considered, meaning all the CongestionInfo are just created in this loop, does "now" ever differ from self.last_cancellation_time when calling update_for_cancellation (from my other comment)? Or do I miss something?
