Report L1 base fee set by L2s #111

matYang · 2023-09-03T07:05:29Z

Motivation

On optimistic rollups, a major portion of tx cost is data availability fee (DA fee). DA fee = DA gas * L1 gas price.
DA gas can be calculated from message's calldata size.
L1 gas price needs to be read from the L2's L1 gas oracle.

Solution

Read L2-specific L1 gas price, encode it to PriceRegistry-compatible int format, where higher-order 112 bits are L1 gas price, and lower-order 112 bits are exec gas price.

…from-l2

connorwstein · 2023-09-21T17:18:37Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go


 	for _, obs := range observations {
-		if obs.SourceGasPriceUSD != nil {
+		if obs.SourceGasPriceUSD.notNil() {


aside: we should move this validation into getParseableObservations and we should similarly move the > r.F check into the top level report instead of spread out in 3 places. Just consider observations all or nothing

seems tricky, since interval | token price | gas price are validated individually, e.g. an Observation where gas price is nil can contain valid values for token price and/or interval, I think it'd still make sense to check them separately

Communicated on Slack, decision is to reject Observations if any price field is nil, since with current impl, non-faulty nodes should not produce observations with nil fields.

connorwstein · 2023-09-21T17:32:34Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

+		NativeGasPrice: sourceNativeGasPriceUSD,
+	}
+
+	// If l1 oracle exists, l1 gas price is a price component of overall tx, need to fetch and encode l1 gas price.


seems very hard to follow what will happen with different versions here and the plugin doesn't need to know any of these details. I think we can abstract all of this into 2 helper components (which wrap gas.EvmGasEstimator) one for 1.0/1.1 and one for 1.2 which implement a common interface that the plugin uses. Something like:

type GasPriceEstimator interface { EstimateGasPrice() (GasPrice, error) // plugin calls this then EncodeGasPrice in Observe EncodeGasPrice(GasPrice) ([]byte, error) // encodes according to version DecodeGasPrice([]byte)(GasPrice, error) // decodes according to version } type GasPrice interface { String() string // for logging Cmp(GasPrice) int // Used for deviation and median }

When we instantiate the plugin:

var gasPriceEstimator GasPriceEstimator switch commitVersion case 1_0, 1_1 { gasPriceEstimator = NewV1GasPriceEstimator() // native fees only } case 1_2 { gasPriceEstimator = NewV2GasPriceEstimator() // native fees and da } default { error } commitPlugin{gasPriceEstimator: gasPriceEstimator...}

Has an added benefit of improving testability, smaller interface injected vs the whole gas.EvmFeeEstimator and it will help reduce duplicate code in the exec plugin I think

connorwstein · 2023-09-21T17:33:57Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

 		}

-		if gasPriceUpdate.value != nil {
-			r.lggr.Infow("Latest gas price from inflight", "gasPriceUpdateVal", gasPriceUpdate.value, "gasPriceUpdateTs", gasPriceUpdate.timestamp)
+		if gasUpdate.value.notNil() {


I think clarity would be improved if we just explicitly return a boolean indicating whether there is a prior gas price update and remove this "notNil means no update"

Can change getLatestInflightGasPriceUpdate to return (update, bool) as opposed to *update.
Per my understanding a priori gas price update where chain selector is not 0 should never contain a gas value that's nil, was this nil check more intended as defensive code?

Checks removed

…from-l2

github-actions · 2023-09-24T19:44:50Z

LCOV of commit `71992a3` during Solidity Foundry #687

Summary coverage rate:
  lines......: 98.9% (911 of 921 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.1% (359 of 394 branches)

Files changed coverage rate: n/a

RensR · 2023-09-25T08:59:09Z

core/services/ocr2/plugins/ccip/commit_plugin.go

@@ -66,6 +67,14 @@ func NewCommitServices(lggr logger.Logger, jb job.Job, chainSet evm.LegacyChainC
 	if err != nil {
 		return nil, errors.Wrap(err, "failed getting the static config from the commitStore")
 	}
+	typeAndVersion, err := commitStore.TypeAndVersion(&bind.CallOpts{})


Maybe we should change the loaders to also return the version, as we already parse it in LoadCommitStore

RensR · 2023-09-25T09:03:01Z

core/services/ocr2/plugins/ccip/config/offchain_config.go

+	return nil
+}
+
+type CommitOffchainConfigV1 struct {


nit: add a comment to indicate the version range this is valid for

github-actions · 2023-09-26T05:09:23Z

LCOV of commit `9b52b04` during Solidity Foundry #706

Summary coverage rate:
  lines......: 98.9% (911 of 921 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.1% (359 of 394 branches)

Files changed coverage rate: n/a

github-actions · 2023-09-26T14:16:18Z

LCOV of commit `3367405` during Solidity Foundry #714

Summary coverage rate:
  lines......: 98.9% (911 of 921 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.1% (359 of 394 branches)

Files changed coverage rate: n/a

github-actions · 2023-09-27T19:27:39Z

LCOV of commit `337a715` during Solidity Foundry #744

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

github-actions · 2023-09-28T04:04:46Z

LCOV of commit `9717f92` during Solidity Foundry #746

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

…from-l2

github-actions · 2023-09-28T20:40:17Z

LCOV of commit `c4f9718` during Solidity Foundry #753

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

core/services/ocr2/plugins/ccip/commit_inflight.go

dimkouv · 2023-09-29T14:33:00Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

@@ -402,19 +418,18 @@ func (r *CommitReportingPlugin) getLatestTokenPriceUpdates(ctx context.Context,
 	return latestUpdates, nil
 }

-// Gets the latest gas price updates based on logs within the heartbeat
-func (r *CommitReportingPlugin) getLatestGasPriceUpdate(ctx context.Context, now time.Time, checkInflight bool) (gasPriceUpdate update, error error) {
+// getLatestGasPriceUpdate returns the latest gas price updates based on logs within the heartbeat.


typo: gas price update based

dimkouv · 2023-09-29T14:43:17Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

+// An observation is rejected if any of its gas price or token price is nil. With current CommitObservation implementation, prices
+// are checked to ensure no nil values before adding to Observation, hence an observation that contains nil values comes from a faulty node.
+func (r *CommitReportingPlugin) validateObservations(ctx context.Context, lggr logger.Logger, observations []CommitObservation) (validObs []CommitObservation, err error) {
+	tokenDecimals, err := r.tokenDecimalsCache.Get(ctx)


I think it's somewhat better to read from token decimals cache in the caller and pass the map to this function, that way it's easier to test and easier to re-use the map.

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

dimkouv · 2023-09-29T15:02:04Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

+		for token, price := range obs.TokenPricesUSD {
+			if price == nil {
+				lggr.Warnw("Nil value in observed TokenPricesUSD", "token", token.Hex())
+				skipObservation = true


Maybe it's safe to stop on the first skipped observation?

i.e. with something like this

func skipObservation() bool { for token, price := range obs.TokenPricesUSD { if price==nil { return false } if _, exists := tokenDecimals[token]; !exists { return false } } return true }

yeah agreed I think we can just continue on first invalidation

This was on purpose, as nil prices should not happen - it indicates a bug or a faulty node, normally this should never trigger, when it does, printing all nil values will help with triaging.

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

core/services/ocr2/plugins/ccip/config/offchain_config.go

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

core/services/ocr2/plugins/ccip/prices/da_price_estimator.go

connorwstein · 2023-09-29T19:06:30Z

core/services/ocr2/plugins/ccip/prices/exec_price_estimator.go

+func (g ExecGasPriceEstimator) Median(gasPrices []GasPrice) (GasPrice, error) {
+	var prices []*big.Int
+	for _, p := range gasPrices {
+		if p != nil {


the rest of the implementations assume non-nil input. I think we either make the input trusted to be not-nil (i.e. document that in the interface) and don't check it or we check it in all the impls and return an error. Probably the former since we validate input in the plugin

connorwstein · 2023-09-29T19:17:18Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

+		for token, price := range obs.TokenPricesUSD {
+			if price == nil {
+				lggr.Warnw("Nil value in observed TokenPricesUSD", "token", token.Hex())
+				skipObservation = true


yeah agreed I think we can just continue on first invalidation

connorwstein · 2023-09-29T19:31:39Z

core/services/ocr2/plugins/ccip/prices/gas_price_estimator.go

+	// Updating EVM2EVMMessage involves an offchain upgrade, safe to keep this as constant in code.
+	EvmMessageFixedBytes     = 448
+	EvmMessageBytesPerToken  = 128          // Byte size of each token transfer, consisting of 1 EVMTokenAmount and 1 bytes, excl length of bytes
+	DAMultiplierBase         = int64(10000) // DA multiplier is in multiples of 0.0001, i.e. 1/DAMultiplierBase


shouldn't need to expose these version specific details from the prices package?

github-actions · 2023-09-29T22:35:53Z

LCOV of commit `f619ede` during Solidity Foundry #767

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

github-actions · 2023-09-30T03:02:54Z

LCOV of commit `42fa3bd` during Solidity Foundry #771

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

…from-l2

github-actions · 2023-10-02T06:02:22Z

LCOV of commit `9d7673f` during Solidity Foundry #773

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

github-actions · 2023-10-02T06:24:18Z

LCOV of commit `db7be89` during Solidity Foundry #775

Summary coverage rate:
  lines......: 98.9% (915 of 925 lines)
  functions..: 96.4% (190 of 197 functions)
  branches...: 91.2% (363 of 398 branches)

Files changed coverage rate: n/a

connorwstein · 2023-10-02T14:14:26Z

docs/CHANGELOG_CCIP.md

+  - the new `sourceTokenData` field is added to the hash.
+  - fixed-size fields are hashed in nested hash function.
+- CommitStore OffchainConfig fields updated.
+  - New fields `GasPriceHeartBeat`, `DAGasPriceDeviationPPB`, `ExecGasPriceDeviationPPB`, `TokenPriceHeartBeat`, `TokenPriceDeviationPPB` added


could use some more detail on the usage of these new params, can be follow up to unblock

* CCIP plugin code sketch * revert models changes * update gas price encoding and deviation * udpate exec cost estimation * Update exec cost estimation * update offchain config * use gas price struct instead of encoding where applicable * do not use v2 suffix for latest version of offchain config * add nil check for gas update * Adding price estimator interfaces * update commit plugin * update name to prices * update exec plugin * comments and nits * change the loaders to also return version * address comments * making tests build * validate observed token prices match token with decimals * fixed commit reporting tests * add test for validateObservations * pass tests * add test for exec getPrice * couple more tests in exec price estimator * complete exec price estimator test * add da price estimator test * resolve exec logpoller fiter test * address comments * remove price estimator deviation and computeCost opts * fix 1.21 import

CCIP plugin code sketch

c35425e

matYang added the do not merge Do not merge at this time label Sep 3, 2023

matYang changed the title ~~[WIP] Report L1 base fee set by L2s~~ [WIP Sketch] Report L1 base fee set by L2s Sep 3, 2023

matYang removed the do not merge Do not merge at this time label Sep 3, 2023

matYang added 2 commits September 19, 2023 17:56

Merge branch 'ccip-develop' into feature/offchain-report-l1-base-fee-…

0978d8f

…from-l2

revert models changes

27d328d

matYang force-pushed the feature/offchain-report-l1-base-fee-from-l2 branch from 87eee3c to 27d328d Compare September 19, 2023 21:59

matYang added 7 commits September 20, 2023 11:09

update gas price encoding and deviation

6fd2486

udpate exec cost estimation

4288094

Update exec cost estimation

65ec92f

update offchain config

d68a786

use gas price struct instead of encoding where applicable

fb7b0af

do not use v2 suffix for latest version of offchain config

b2c5866

add nil check for gas update

3985918

connorwstein reviewed Sep 21, 2023

View reviewed changes

matYang added 5 commits September 22, 2023 02:05

Merge branch 'ccip-develop' into feature/offchain-report-l1-base-fee-…

159cc13

…from-l2

Adding price estimator interfaces

bc94b46

update commit plugin

78ab531

update name to prices

a6cd7c4

update exec plugin

71992a3

RensR reviewed Sep 25, 2023

View reviewed changes

matYang added 3 commits September 25, 2023 09:31

comments and nits

436eef3

change the loaders to also return version

5e4ec20

address comments

9b52b04

making tests build

3367405

validate observed token prices match token with decimals

5fb58a2

add da price estimator test

9717f92

matYang changed the title ~~[WIP Sketch] Report L1 base fee set by L2s~~ Report L1 base fee set by L2s Sep 28, 2023

matYang requested review from RensR, connorwstein and dimkouv September 28, 2023 04:30

matYang marked this pull request as ready for review September 28, 2023 04:30

matYang requested review from a team as code owners September 28, 2023 04:30

matYang added 2 commits September 28, 2023 16:08

Merge branch 'ccip-develop' into feature/offchain-report-l1-base-fee-…

d795be9

…from-l2

resolve exec logpoller fiter test

c4f9718

dimkouv reviewed Sep 29, 2023

View reviewed changes

connorwstein reviewed Sep 29, 2023

View reviewed changes

address comments

f619ede

remove price estimator deviation and computeCost opts

42fa3bd

matYang requested review from connorwstein and dimkouv September 30, 2023 03:46

Merge branch 'ccip-develop' into feature/offchain-report-l1-base-fee-…

9d7673f

…from-l2

fix 1.21 import

db7be89

connorwstein approved these changes Oct 2, 2023

View reviewed changes

AnieeG approved these changes Oct 2, 2023

View reviewed changes

matYang merged commit 37b87f0 into ccip-develop Oct 2, 2023
49 checks passed

matYang deleted the feature/offchain-report-l1-base-fee-from-l2 branch October 2, 2023 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report L1 base fee set by L2s #111

Report L1 base fee set by L2s #111

matYang commented Sep 3, 2023 •

edited

Loading

connorwstein Sep 21, 2023

matYang Sep 25, 2023

matYang Sep 26, 2023

connorwstein Sep 21, 2023 •

edited

Loading

connorwstein Sep 21, 2023

matYang Sep 25, 2023

matYang Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 24, 2023

RensR Sep 25, 2023

RensR Sep 25, 2023

github-actions bot commented Sep 26, 2023

github-actions bot commented Sep 26, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

dimkouv Sep 29, 2023

dimkouv Sep 29, 2023

dimkouv Sep 29, 2023

connorwstein Sep 29, 2023 •

edited by matYang

Loading

matYang Sep 29, 2023 •

edited

Loading

connorwstein Sep 29, 2023

connorwstein Sep 29, 2023 •

edited by matYang

Loading

connorwstein Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 30, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

connorwstein Oct 2, 2023

Report L1 base fee set by L2s #111

Report L1 base fee set by L2s #111

Conversation

matYang commented Sep 3, 2023 • edited Loading

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matYang Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2023

LCOV of commit 71992a3 during Solidity Foundry #687

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2023

LCOV of commit 9b52b04 during Solidity Foundry #706

github-actions bot commented Sep 26, 2023

LCOV of commit 3367405 during Solidity Foundry #714

github-actions bot commented Sep 27, 2023

LCOV of commit 337a715 during Solidity Foundry #744

github-actions bot commented Sep 28, 2023

LCOV of commit 9717f92 during Solidity Foundry #746

github-actions bot commented Sep 28, 2023

LCOV of commit c4f9718 during Solidity Foundry #753

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein Sep 29, 2023 • edited by matYang Loading

Choose a reason for hiding this comment

matYang Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein Sep 29, 2023 • edited by matYang Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2023

LCOV of commit f619ede during Solidity Foundry #767

github-actions bot commented Sep 30, 2023

LCOV of commit 42fa3bd during Solidity Foundry #771

github-actions bot commented Oct 2, 2023

LCOV of commit 9d7673f during Solidity Foundry #773

github-actions bot commented Oct 2, 2023

LCOV of commit db7be89 during Solidity Foundry #775

Choose a reason for hiding this comment

matYang commented Sep 3, 2023 •

edited

Loading

connorwstein Sep 21, 2023 •

edited

Loading

matYang Sep 26, 2023 •

edited

Loading

LCOV of commit `71992a3` during Solidity Foundry #687

LCOV of commit `9b52b04` during Solidity Foundry #706

LCOV of commit `3367405` during Solidity Foundry #714

LCOV of commit `337a715` during Solidity Foundry #744

LCOV of commit `9717f92` during Solidity Foundry #746

LCOV of commit `c4f9718` during Solidity Foundry #753

connorwstein Sep 29, 2023 •

edited by matYang

Loading

matYang Sep 29, 2023 •

edited

Loading

connorwstein Sep 29, 2023 •

edited by matYang

Loading

LCOV of commit `f619ede` during Solidity Foundry #767

LCOV of commit `42fa3bd` during Solidity Foundry #771

LCOV of commit `9d7673f` during Solidity Foundry #773

LCOV of commit `db7be89` during Solidity Foundry #775