Leader lane multi-chain gas price reports #1071

matYang · 2024-06-24T05:15:08Z

Previously, leader lane ORM was added to enabled Commit plugin to read multi-chain gas prices during Observation. This PR updates Report phase so Commit plugin can now report multi-chain gas prices.

Important changes are:

make use of the PriceReportingDisabled flag to skip price reports on non-leader lanes
update gas price logic during Report to handle multi-chain gas prices

makramkd · 2024-06-24T06:52:18Z

.changeset/sour-elephants-sell.md

We're not committing any changesets anymore

makramkd · 2024-06-24T06:52:44Z

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

 	if err != nil {
 		return nil, err
 	}

+	// For backwards compatibility with the order release during phased rollout, include the default gas price on this lane


Suggested change

// For backwards compatibility with the order release during phased rollout, include the default gas price on this lane

// For backwards compatibility with the older release during phased rollout, include the default gas price on this lane

makramkd · 2024-06-24T06:54:42Z

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

+	ctx context.Context,
+) (gasPricesUSD map[uint64]*big.Int, tokenPricesUSD map[cciptypes.Address]*big.Int, err error) {
+	// Do not observe prices if price reporting is disabled. Price reporting will be disabled for lanes that are not leader lanes.
+	if r.offchainConfig.PriceReportingDisabled {


Is it worth logging in these scenarios or nah?

Also sidenote I wish this was PriceReportingEnabled instead

yeah me too, this was defined in April under a different leader lane architecture that got killed due to LOOPPS, but Engops already update the tooling to include this flag, didn't wanna change it again

makramkd · 2024-06-24T06:57:49Z

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

+		// During phased-rollout where some honest nodes may not have started observing the token yet, it requires 5 malicious node with 1 being the leader to successfully alter price.
+		// During regular operation, it requires 3 malicious nodes with 1 being the leader to temporarily delay price update for the token.
+		if len(perChainPriceObservations) < (2*(f-1) + 1) {
+			lggr.Warnf("Skipping chain with selector %d due to not enough valid observations: #obs=%d, f=%d", selector, len(perChainPriceObservations), f)


Worth logging the actual 2f-1 value?

makramkd · 2024-06-24T07:00:03Z

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

+				return nil, nil, err
+			}
+			if gasPriceUpdatedRecently && !gasPriceDeviated {
+				r.lggr.Debugw("gas price was updated recently, skipping the update",


To reflect the condition, maybe add and not deviated sufficiently?

makramkd · 2024-06-24T07:00:55Z

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

-		gasPriceUpdatedRecently := time.Since(latestGasPrice.timestamp) < r.offchainConfig.GasPriceHeartBeat
-		gasPriceDeviated, err := r.gasPriceEstimator.Deviates(newGasPrice, latestGasPrice.value)
+	for chainSelector, gasPriceObservations := range gasPriceObs {
+		newGasPrice, err := r.gasPriceEstimator.Median(gasPriceObservations) // Compute the median price
 		if err != nil {
 			return nil, nil, err


Do these returned err values have enough context to figure out where we failed exactly? This has happened before where its hard to deduce the exact place things stopped working

makramkd · 2024-06-24T07:02:43Z

core/services/ocr2/plugins/ccip/internal/ccipdb/price_service.go

@@ -38,6 +38,7 @@ type PriceService interface {

 	// GetGasAndTokenPrices fetches source chain gas prices and relevant token prices from all lanes that touch the given dest chain.
 	// The prices have been written into the DB by each lane's PriceService in the background. The prices are denoted in USD.
+	// If the results are empty, they are in the form of empty maps, as opposed to being nil.


This is an interesting API contract

cleaned up, added nil checks on ocr2.go

makramkd · 2024-06-24T07:03:38Z

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

+	}
+
+	// Fetches multi-lane gas prices and token prices, for the given dest chain
+	gasPricesUSD, tokenPricesUSD, err = r.priceService.GetGasAndTokenPrices(ctx, r.destChainSelector)


Worth checking that gasPricesUSD and tokenPricesUSD are not nil and erroring if they are? Thats a breakage in the API contract

after the API cleanup, added explicit nil check

dimkouv · 2024-06-26T07:42:45Z

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

 	if err != nil {
 		return nil, err
 	}
+	// Set prices to empty maps if nil to be friendlier to JSON encoding
+	if gasPricesUSD == nil {
+		gasPricesUSD = map[uint64]*big.Int{}


maybe better to do this inside r.observePriceUpdates(ctx)

dimkouv · 2024-06-26T07:42:53Z

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

+		gasPricesUSD = map[uint64]*big.Int{}
+	}
+	if tokenPricesUSD == nil {
+		tokenPricesUSD = map[cciptypes.Address]*big.Int{}


dimkouv · 2024-06-26T07:43:43Z

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

+		tokenPricesUSD = map[cciptypes.Address]*big.Int{}
+	}
+	// For backwards compatibility with the older release during phased rollout, set the default gas price on this lane
+	defaultGasPricesUSD := gasPricesUSD[r.sourceChainSelector]


rename var to defaultGasPriceUSD and check if it exists in the map?

renamed to sourceGasPriceUSD, it's ok if it does not exist, nil for this value is supported

dimkouv · 2024-06-26T07:45:03Z

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

+	// Fetches multi-lane gas prices and token prices, for the given dest chain
+	gasPricesUSD, tokenPricesUSD, err = r.priceService.GetGasAndTokenPrices(ctx, r.destChainSelector)
+	if err != nil {
+		return nil, nil, err


dimkouv · 2024-06-26T07:45:30Z

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

-		if !timestamp.Before(gasUpdate.timestamp) {
-			gasUpdate = update{
+		timestamp := time.Unix(priceUpdate.TimestampUnixSec.Int64(), 0)
+		if priceUpdate.Value != nil && !timestamp.Before(latestUpdates[priceUpdate.DestChainSelector].timestamp) {


check if latestUpdates[priceUpdate.DestChainSelector] exists?

update is a struct, I think it's intended that latestUpdates[priceUpdate.DestChainSelector] may not exist, if it doesn't, then timestamp is 0 so !timestamp.Before is always true, and that initializes the value

Previously, leader lane ORM was added to enabled Commit plugin to read multi-chain gas prices during `Observation`. This PR updates `Report` phase so Commit plugin can now report multi-chain gas prices. Important changes are: 1. make use of the PriceReportingDisabled flag to skip price reports on non-leader lanes 2. update gas price logic during Report to handle multi-chain gas prices

… in Test (#1120) ## Motivation This [PR](#1071) introduces leader lane feature. Setting leader lane will avoid duplicate gas price fetch for the same destination from different source chain. Here in this PR, the expectation is to add a test where leader lane is setup and verify every commitstore report transaction hash emitted two `UsdPerUnitGasUpdated` events. ## Solution Introduce config variable to enable leader lane setup and based on network pair, automatically define set of lanes as leader and non-leader lanes, track the tx hash from the event `UsdPerUnitGasUpdated` and ensure transaction count matches to the number of sources chain selector expected.

matYang added 9 commits June 19, 2024 17:34

fetch prices from price service

500ca7c

multi gas price commit reports

a20be77

use the PriceReporting disabled flag

beddb0d

update tets for getLatestGasPriceUpdates

cbf5796

calculatePriceUpdates

f4349d3

commit ocr2 report test

26d0f3d

complete tests

8056127

Merge branch 'ccip-develop' into leader-lane-obs

6265f2a

changeset

ea9d914

matYang requested review from connorwstein and dimkouv June 24, 2024 05:15

matYang temporarily deployed to sdlc June 24, 2024 05:15 — with GitHub Actions Inactive

add deprecated comment

3a24c59

matYang temporarily deployed to sdlc June 24, 2024 05:17 — with GitHub Actions Inactive

goimports

b7fcbc3

matYang temporarily deployed to sdlc June 24, 2024 05:28 — with GitHub Actions Inactive

matYang marked this pull request as ready for review June 24, 2024 05:28

matYang requested a review from a team as a code owner June 24, 2024 05:28

makramkd reviewed Jun 24, 2024

View reviewed changes

address comments

b5fe222

matYang temporarily deployed to sdlc June 24, 2024 22:51 — with GitHub Actions Inactive

Merge branch 'ccip-develop' into leader-lane-obs

7b52af0

matYang temporarily deployed to sdlc June 25, 2024 03:33 — with GitHub Actions Inactive

matYang requested a review from makramkd June 25, 2024 03:39

fix obs tests

2ef2404

matYang temporarily deployed to sdlc June 25, 2024 15:05 — with GitHub Actions Inactive

Merge branch 'ccip-develop' into leader-lane-obs

4dfa1fe

matYang temporarily deployed to sdlc June 25, 2024 18:05 — with GitHub Actions Inactive

makramkd approved these changes Jun 25, 2024

View reviewed changes

dimkouv reviewed Jun 26, 2024

View reviewed changes

address comments

99e7a08

matYang temporarily deployed to sdlc June 27, 2024 06:33 — with GitHub Actions Inactive

matYang requested a review from dimkouv June 27, 2024 06:34

dimkouv approved these changes Jun 27, 2024

View reviewed changes

matYang enabled auto-merge (squash) June 27, 2024 13:41

Merge branch 'ccip-develop' into leader-lane-obs

5c98b2f

matYang temporarily deployed to sdlc June 27, 2024 13:41 — with GitHub Actions Inactive

matYang merged commit dc8599f into ccip-develop Jun 27, 2024
100 checks passed

matYang deleted the leader-lane-obs branch June 27, 2024 13:56

b-gopalswami mentioned this pull request Jul 8, 2024

CCIP-2645: Introducing leader lane and setting PriceReportingDisabled in Test #1120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leader lane multi-chain gas price reports #1071

Leader lane multi-chain gas price reports #1071

matYang commented Jun 24, 2024 •

edited

Loading

makramkd Jun 24, 2024

makramkd Jun 24, 2024

makramkd Jun 24, 2024

makramkd Jun 24, 2024

matYang Jun 24, 2024

makramkd Jun 24, 2024

makramkd Jun 24, 2024

makramkd Jun 24, 2024

makramkd Jun 24, 2024

matYang Jun 24, 2024

makramkd Jun 24, 2024

matYang Jun 24, 2024

dimkouv Jun 26, 2024

dimkouv Jun 26, 2024

dimkouv Jun 26, 2024

matYang Jun 27, 2024

dimkouv Jun 26, 2024

dimkouv Jun 26, 2024

matYang Jun 27, 2024

	// For backwards compatibility with the order release during phased rollout, include the default gas price on this lane
	// For backwards compatibility with the older release during phased rollout, include the default gas price on this lane

Leader lane multi-chain gas price reports #1071

Leader lane multi-chain gas price reports #1071

Conversation

matYang commented Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matYang commented Jun 24, 2024 •

edited

Loading