Conversation
| WriteReportInsufficientGasRetry struct { | ||
| basic commoncapbeholder.MetricsCapBasic | ||
| } | ||
| WriteReportGasMismatch struct { |
There was a problem hiding this comment.
are we planning to add monitoring or alerting around this ?
There was a problem hiding this comment.
Yes, In a follow up
|
|
||
| func (r *WriteReportGasMismatch) LogAttributes() []attribute.KeyValue { | ||
| return append([]attribute.KeyValue{ | ||
| attribute.String("receiver", getReceiver(r.Req.GetReceiver())), |
There was a problem hiding this comment.
these attributes will increase the cardinality of the dataset.
Are we relying on this dataset being super small ?
Worth asking infra-obs whether a small count but high cardinality dataset is ok to have or should be avoided.
If it should be avoided, we can emit a counter and warn on increasing rate, and then just log the receiver and hash to investigate.
There was a problem hiding this comment.
I think including hash would still be a high cardinality label they'll want us to avoid. Just receiver might be ok if we only expect there to be a few
| // The minimum amount of gas that the receiver contract must get to process the forwarder report. This is the default value used when the user doesn't specify a gas limit when invoking WriteReport. | ||
| ReceiverGasMinimum uint64 `json:"receiverGasMinimum"` | ||
| ReceiverGasMinimum uint64 `json:"receiverGasMinimum"` | ||
| // Safety margin, in gas, added on top of the forwarder contract's internal gas reservation |
There was a problem hiding this comment.
you will need a corresponding CLD PR to be able to pass this through job specs
There was a problem hiding this comment.
Will do in follow up with bumped capability in core and updated regression e2e test
| // limit minus the forwarder's gas overhead, or the configured receiver gas minimum when no | ||
| // explicit limit was provided. | ||
| func (e *WriteReport) estimateReceiverGasBudget(request *evm.WriteReportRequest) uint64 { | ||
| receiverGasBudget := e.ReceiverGasMinimum + e.forwarderGasOverhead |
There was a problem hiding this comment.
how come we add forwarderGasOverhead to receiverGasBudget here ?
i might be missing something.
from my understanding, receiverGasBudget is the gas that the receiver has available to use. Not being able to understand why we add forwarderGas into that ?
There was a problem hiding this comment.
There are two options:
- User did not provide Gas, then the value we provide is forwarderGasOverhead + receiverGasMinimum. Because we want to cover our internal forwarder logic + some gas for receiver execution(the constants are aligned with what we have on chain)
- User provided gas, further in the code you can see calculations for this case
|
|
||
| // warnGasMismatch reports a prior tx whose onchain gas limit differs from the requested one. | ||
| func (e *WriteReport) warnGasMismatch(ctx context.Context, telemetryContext monitoring.TelemetryContext, request *evm.WriteReportRequest, txHash evmtypes.Hash, requestedGasLimit, actualTxGasLimit uint64) { | ||
| e.lggr.Warnw("Gas mismatch: prior transmission tx gas limit does not match the requested gas limit", |
There was a problem hiding this comment.
Could we potentially report this in valid cases? If we ever enable gas limit estimation for a particular chain, the transmission gas limit may be different than what was requested. Off the top of my head, I don't think we have it enabled on our currently integrated chains but I know some exist like Hedera if we ever have to add it.
There was a problem hiding this comment.
Yes, if we enable gas limit estimation it will be a problem in this case. BBut we also allow users to provide limit, - so I think we should never allow gas limit estimation on txm for CRE.
There was a problem hiding this comment.
True but I think a lot of times users don't provide a gas limit to let us set it for them. We have defaults but it could mean higher costs for chains like Hedera where we don't get the full unused gas refunded. I guess it's not our money and if they're concerned about costs they can set one themselves or use EstimateGas. Just wanted to call out the edge case
|
4 participants
No description provided.