Fix: simplify ibc client update construction (#68)

relayer/chains/cosmos/cosmos_chain_processor.go

@@ -551,7 +551,7 @@ func (ccp *CosmosChainProcessor) queryCycle(
 	}
 
 	for _, pp := range ccp.pathProcessors {
-		clientID := pp.RelevantClientID(chainID)
+		clientID := pp.ClientIDForClientForCounterparty(chainID)
 		clientState, err := ccp.clientState(ctx, clientID)
 		if err != nil {
 			ccp.log.Error("Fetching client state.",

relayer/client.go

@@ -325,6 +325,7 @@ func MsgUpdateClient(
 	eg.Go(func() error {
 		return retry.Do(func() error {
 			var err error
+			// query h+1 because we need the header whose validator set corresponds to the nextValidatorsHash of the last trusted height
 			dstTrustedHeader, err = src.ChainProvider.QueryIBCHeader(egCtx, int64(dstClientState.GetLatestHeight().GetRevisionHeight())+1)
 			return err
 		}, retry.Context(egCtx), RtyAtt, RtyDel, RtyErr, retry.OnRetry(func(n uint, err error) {

relayer/processor/message_processor.go

@@ -1,7 +1,6 @@
 package processor
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -105,7 +104,7 @@ func (mp *messageProcessor) processMessages(
 	var needsClientUpdate bool
 
 	// Localhost IBC does not permit client updates
-	if !isLocalhostClient(src.clientState.ClientID, dst.clientState.ClientID) {
+	if !isLocalhostClient(src.lastObservedClientState.ClientID, dst.lastObservedClientState.ClientID) {
 		var err error
 
 		// need to update dst with a more recent view of src first?
@@ -143,15 +142,15 @@ func isLocalhostClient(srcClientID, dstClientID string) bool {
 func (mp *messageProcessor) shouldUpdateClientNow(ctx context.Context, src, dst *pathEndRuntime) (bool, error) {
 	var consensusHeightTime time.Time
 
-	if dst.clientState.ConsensusTime.IsZero() {
-		height := int64(dst.clientState.LatestHeight.RevisionHeight)
+	if dst.lastObservedClientState.ConsensusTime.IsZero() {
+		height := int64(dst.lastObservedClientState.LatestHeight.RevisionHeight)
 		h, err := src.chainProvider.QueryIBCHeader(ctx, height)
 		if err != nil {
 			return false, fmt.Errorf("query ibc header: chain id: %s: height: %d: %w", src.chainProvider.ChainId(), height, err)
 		}
 		consensusHeightTime = time.Unix(0, int64(h.ConsensusState().GetTimestamp()))
 	} else {
-		consensusHeightTime = dst.clientState.ConsensusTime
+		consensusHeightTime = dst.lastObservedClientState.ConsensusTime
 	}
 
 	clientUpdateThresholdMs := mp.clientUpdateThresholdTime.Milliseconds()
@@ -160,10 +159,10 @@ func (mp *messageProcessor) shouldUpdateClientNow(ctx context.Context, src, dst
 	enoughBlocksPassed := (dst.latestBlock.Height - blocksToRetrySendAfter) > dst.lastClientUpdateHeight
 	dst.lastClientUpdateHeightMu.Unlock()
 
-	twoThirdsTrustingPeriodMs := float64(dst.clientState.TrustingPeriod.Milliseconds()) * 2 / 3
+	twoThirdsTrustingPeriodMs := float64(dst.lastObservedClientState.TrustingPeriod.Milliseconds()) * 2 / 3
 	timeSinceLastClientUpdateMs := float64(time.Since(consensusHeightTime).Milliseconds())
 
-	pastTwoThirdsTrustingPeriod := dst.clientState.TrustingPeriod > 0 &&
+	pastTwoThirdsTrustingPeriod := dst.lastObservedClientState.TrustingPeriod > 0 &&
 		timeSinceLastClientUpdateMs > twoThirdsTrustingPeriodMs
 
 	pastConfiguredClientUpdateThreshold := clientUpdateThresholdMs > 0 &&
@@ -172,9 +171,9 @@ func (mp *messageProcessor) shouldUpdateClientNow(ctx context.Context, src, dst
 	shouldUpdateClientNow := enoughBlocksPassed && (pastTwoThirdsTrustingPeriod || pastConfiguredClientUpdateThreshold)
 
 	if mp.metrics != nil {
-		timeToExpiration := dst.clientState.TrustingPeriod - time.Since(consensusHeightTime)
-		mp.metrics.SetClientExpiration(src.info.PathName, dst.info.ChainID, dst.clientState.ClientID, fmt.Sprint(dst.clientState.TrustingPeriod.String()), timeToExpiration)
-		mp.metrics.SetClientTrustingPeriod(src.info.PathName, dst.info.ChainID, dst.info.ClientID, time.Duration(dst.clientState.TrustingPeriod))
+		timeToExpiration := dst.lastObservedClientState.TrustingPeriod - time.Since(consensusHeightTime)
+		mp.metrics.SetClientExpiration(src.info.PathName, dst.info.ChainID, dst.lastObservedClientState.ClientID, fmt.Sprint(dst.lastObservedClientState.TrustingPeriod.String()), timeToExpiration)
+		mp.metrics.SetClientTrustingPeriod(src.info.PathName, dst.info.ChainID, dst.info.ClientID, time.Duration(dst.lastObservedClientState.TrustingPeriod))
 	}
 
 	return shouldUpdateClientNow, nil
@@ -250,72 +249,50 @@ func (mp *messageProcessor) assembleMessage(
 }
 
 // assembleMsgUpdateClient uses the ChainProvider from both pathEnds to assemble the client update header
-// from the source and then assemble the update client message in the correct format for the destination.
-func (mp *messageProcessor) assembleMsgUpdateClient(ctx context.Context, src, dst *pathEndRuntime) error {
-	clientID := dst.info.ClientID
-	clientLatestHeight := dst.clientState.LatestHeight
-	trustedHeight := dst.clientTrustedState.ClientState.LatestHeight
-
-	var trustedNextValHash []byte
-	if dst.clientTrustedState.IBCHeader != nil {
-		trustedNextValHash = dst.clientTrustedState.IBCHeader.NextValidatorsHash()
-	}
-
-	// If the client state height is not equal to the client trusted state height and the client state height is
-	// the latest block, we cannot send a MsgUpdateClient until another block is observed on the counterparty.
-	// If the client state height is in the past, beyond ibcHeadersToCache, then we need to query for it.
-	if !trustedHeight.EQ(clientLatestHeight) {
-		// TODO: looks like dupe code with updateClientTrustedState
-
-		deltaConsensusHeight := int64(clientLatestHeight.RevisionHeight) - int64(trustedHeight.RevisionHeight)
-		if trustedHeight.RevisionHeight != 0 && deltaConsensusHeight <= clientConsensusHeightUpdateThresholdBlocks {
-			return fmt.Errorf("observed client trusted height does not equal latest client state height: trusted: %d: latest %d",
-				trustedHeight.RevisionHeight, clientLatestHeight.RevisionHeight)
-		}
-
-		header, err := src.chainProvider.QueryIBCHeader(ctx, int64(clientLatestHeight.RevisionHeight+1))
-		if err != nil {
-			return fmt.Errorf("query IBC header at height: %d: chain_id: %s, %w",
-				clientLatestHeight.RevisionHeight+1, src.info.ChainID, err)
-		}
-
-		mp.log.Debug("Queried for client trusted IBC header",
-			zap.String("path_name", src.info.PathName),
-			zap.String("chain_id", src.info.ChainID),
-			zap.String("counterparty_chain_id", dst.info.ChainID),
-			zap.String("counterparty_client_id", clientID),
-			zap.Uint64("height", clientLatestHeight.RevisionHeight+1),
-			zap.Uint64("latest_height", src.latestBlock.Height),
-		)
-
-		dst.clientTrustedState = provider.ClientTrustedState{
-			ClientState: dst.clientState,
-			IBCHeader:   header,
-		}
-
-		trustedHeight = clientLatestHeight
-		trustedNextValHash = header.NextValidatorsHash()
+// from the counterparty and then assemble the update client message in the correct format for the updatee.
+func (mp *messageProcessor) assembleMsgUpdateClient(ctx context.Context, counterparty, updatee *pathEndRuntime) error {
+	/*
+		Reminder how IBC update works:
+			Update contains:
+				- signed header
+				- validator set that signed the header (at least 2/3)
+				- trusted height
+				- trusted header
+					Must have a trusted validator set exactly corresponding to trustedHeight.nextValidatorsHash
+
+			Checks (among other things), that:
+				- validators that signed trusted header correspond to trustedHeight.nextValidatorsHash
+	*/
+
+	clientID := updatee.info.ClientID
+	latestH := updatee.lastObservedClientState.LatestHeight
+
+	nextHeader, err := counterparty.chainProvider.QueryIBCHeader(ctx, int64(latestH.RevisionHeight+1))
+	if err != nil {
+		return fmt.Errorf("query IBC nextHeader at height: %d: chain_id: %s, %w",
+			latestH.RevisionHeight+1, counterparty.info.ChainID, err)
 	}
 
-	if src.latestHeader.Height() == trustedHeight.RevisionHeight &&
-		// TODO: wth?
-		!bytes.Equal(src.latestHeader.NextValidatorsHash(), trustedNextValHash) {
-		return fmt.Errorf("latest header height is equal to the client trusted height: %d, "+
-			"need to wait for next block's header before we can assemble and send a new MsgUpdateClient",
-			trustedHeight.RevisionHeight)
-	}
+	mp.log.Debug("Queried for client trusted IBC nextHeader",
+		zap.String("path_name", counterparty.info.PathName),
+		zap.String("chain_id", counterparty.info.ChainID),
+		zap.String("counterparty_chain_id", updatee.info.ChainID),
+		zap.String("counterparty_client_id", clientID),
+		zap.Uint64("height", latestH.RevisionHeight+1),
+		zap.Uint64("latest_height", counterparty.latestBlock.Height),
+	)
 
 	// get the header to update with a new trusted, base on what we have for trusted
-	msgUpdateClientHeader, err := src.chainProvider.MsgUpdateClientHeader(
-		src.latestHeader,
-		trustedHeight,
-		dst.clientTrustedState.IBCHeader,
+	msgUpdateClientHeader, err := counterparty.chainProvider.MsgUpdateClientHeader(
+		counterparty.latestHeader,
+		latestH,
+		nextHeader,
 	)
 	if err != nil {
 		return fmt.Errorf("msg update client header: %w", err)
 	}
 
-	msgUpdateClient, err := dst.chainProvider.MsgUpdateClient(clientID, msgUpdateClientHeader)
+	msgUpdateClient, err := updatee.chainProvider.MsgUpdateClient(clientID, msgUpdateClientHeader)
 	if err != nil {
 		return fmt.Errorf("msg update client: %w", err)
 	}

relayer/processor/path_end_runtime.go

@@ -30,11 +30,11 @@ type pathEndRuntime struct {
 	latestBlock  provider.LatestBlock
 	messageCache IBCMessagesCache
 
-	// This is the actual state of the light client for the counterparty
-	clientState provider.ClientState
-	// This is a copy of clientState but it also might have a header which can be used as a trusted header
-	// when submitted updates.
-	clientTrustedState   provider.ClientTrustedState
+	// TODO: clear up next two fields
+
+	// This is some recently queried client state for the light client for the counterparty chain, on this chain
+	lastObservedClientState provider.ClientState
+
 	connectionStateCache ConnectionStateCache
 	channelStateCache    ChannelStateCache
 	channelStateCacheMu  sync.RWMutex
@@ -517,21 +517,24 @@ func (pathEnd *pathEndRuntime) mergeCacheData(
 	pathEnd.inSync = d.InSync
 
 	pathEnd.latestHeader = d.LatestHeader
-	pathEnd.clientState = d.ClientState
+	pathEnd.lastObservedClientState = d.ClientState
 
-	terminate, err := pathEnd.checkForMisbehaviour(ctx, pathEnd.clientState, counterParty)
+	terminate, err := pathEnd.checkForMisbehaviour(ctx, pathEnd.lastObservedClientState, counterParty)
 	if err != nil {
 		pathEnd.log.Error("Check for misbehaviour.",
 			zap.String("client_id", pathEnd.info.ClientID),
 			zap.Error(err),
 		)
 	}
 
-	if d.ClientState.LatestHeight != pathEnd.clientState.LatestHeight {
-		pathEnd.clientState = d.ClientState
-		ibcHeader, ok := counterParty.ibcHeaderCache[d.ClientState.LatestHeight.RevisionHeight]
-		if ok {
-			pathEnd.clientState.ConsensusTime = time.Unix(0, int64(ibcHeader.ConsensusState().GetTimestamp()))
+	{
+		// TODO: why is this block needed? Seems useless as pathEnd.lastObservedClientState is set above
+		if d.ClientState.LatestHeight != pathEnd.lastObservedClientState.LatestHeight {
+			pathEnd.lastObservedClientState = d.ClientState
+			ibcHeader, ok := counterParty.ibcHeaderCache[d.ClientState.LatestHeight.RevisionHeight]
+			if ok {
+				pathEnd.lastObservedClientState.ConsensusTime = time.Unix(0, int64(ibcHeader.ConsensusState().GetTimestamp()))
+			}
 		}
 	}
 

relayer/processor/path_processor.go

@@ -169,8 +169,8 @@ type channelPair struct {
 	pathEnd2ChannelKey ChannelKey
 }
 
-// RelevantClientID returns the relevant client ID or panics
-func (pp *PathProcessor) RelevantClientID(chainID string) string {
+// returns client id for the client of the counterparty chain on the chain specified with chainID
+func (pp *PathProcessor) ClientIDForClientForCounterparty(chainID string) string {
 	if pp.pathEnd1.info.ChainID == chainID {
 		return pp.pathEnd1.info.ClientID
 	}

relayer/processor/path_processor_internal.go

@@ -692,43 +692,6 @@ ClientICQLoop:
 	return res
 }
 
-func (pp *PathProcessor) updateClientTrustedState(src *pathEndRuntime, dst *pathEndRuntime) {
-	if src.clientTrustedState.ClientState.LatestHeight.GTE(src.clientState.LatestHeight) {
-		return
-	}
-	// need to assemble new trusted state
-	// We see if header for H+1 exists because the client trusts H and has the nextValidatorsHash of it (+1)
-	ibcHeader, ok := dst.ibcHeaderCache[src.clientState.LatestHeight.RevisionHeight+1]
-	if !ok {
-
-		// We dont have H+1
-		// Maybe we can use H?
-
-		// DYMENSION: changed from upstream. Think there was a bug there. Now looking at src.clientTrustedState
-
-		if ibcHeaderCurrent, ok := dst.ibcHeaderCache[src.clientState.LatestHeight.RevisionHeight]; ok {
-			if src.clientTrustedState.IBCHeader != nil && bytes.Equal(src.clientTrustedState.IBCHeader.NextValidatorsHash(), ibcHeaderCurrent.NextValidatorsHash()) {
-				src.clientTrustedState = provider.ClientTrustedState{
-					ClientState: src.clientState,
-					IBCHeader:   ibcHeaderCurrent,
-				}
-				return
-			}
-		}
-		pp.log.Debug("No cached IBC header for client trusted height",
-			zap.String("chain_id", src.info.ChainID),
-			zap.String("client_id", src.info.ClientID),
-			zap.Uint64("height", src.clientState.LatestHeight.RevisionHeight+1),
-		)
-		return
-
-	}
-	src.clientTrustedState = provider.ClientTrustedState{
-		ClientState: src.clientState,
-		IBCHeader:   ibcHeader,
-	}
-}
-
 var observedEventTypeForDesiredMessage = map[string]string{
 	conntypes.EventTypeConnectionOpenConfirm: conntypes.EventTypeConnectionOpenAck,
 	conntypes.EventTypeConnectionOpenAck:     conntypes.EventTypeConnectionOpenTry,
@@ -921,9 +884,6 @@ func (pp *PathProcessor) queuePreInitMessages(cancel func()) {
 
 // messages from both pathEnds are needed in order to determine what needs to be relayed for a single pathEnd
 func (pp *PathProcessor) processLatestMessages(ctx context.Context, cancel func()) error {
-	// Update trusted client state for both pathends
-	pp.updateClientTrustedState(pp.pathEnd1, pp.pathEnd2)
-	pp.updateClientTrustedState(pp.pathEnd2, pp.pathEnd1)
 
 	channelPairs := pp.channelPairs()
 

relayer/processor/rotation_solver.go

@@ -59,7 +59,7 @@ func (s *rotationSolver) solve(ctx c.Context) error {
 }
 
 func (s *rotationSolver) hubClientValset(ctx c.Context) (uint64, []byte, error) {
-	h := s.hub.clientState.LatestHeight.GetRevisionHeight()
+	h := s.hub.lastObservedClientState.LatestHeight.GetRevisionHeight()
 	header, err := s.ra.chainProvider.QueryIBCHeader(ctx, int64(h))
 	if err != nil {
 		return 0, nil, fmt.Errorf("query ibc header: %w", err)
@@ -149,14 +149,14 @@ func search(sleep time.Duration, l, r uint64, direction func(uint64) (int, error
 
 // a = h, b = h+1 where valhash changes in between
 func (s *rotationSolver) sendUpdatesV2(ctx c.Context, a, b provider.IBCHeader) error {
-	trusted, err := s.ra.chainProvider.QueryIBCHeader(ctx, int64(s.hub.clientState.LatestHeight.GetRevisionHeight())+1)
+	trusted, err := s.ra.chainProvider.QueryIBCHeader(ctx, int64(s.hub.lastObservedClientState.LatestHeight.GetRevisionHeight())+1)
 	if err != nil {
 		return fmt.Errorf("query ibc header: %w", err)
 	}
 
 	u1, err := s.ra.chainProvider.MsgUpdateClientHeader(
 		a,
-		s.hub.clientState.LatestHeight,
+		s.hub.lastObservedClientState.LatestHeight,
 		trusted, // latest+1
 	)
 	if err != nil {
@@ -169,7 +169,7 @@ func (s *rotationSolver) sendUpdatesV2(ctx c.Context, a, b provider.IBCHeader) e
 	}
 
 	aHeight := clienttypes.Height{
-		RevisionNumber: s.hub.clientState.LatestHeight.RevisionNumber,
+		RevisionNumber: s.hub.lastObservedClientState.LatestHeight.RevisionNumber,
 		RevisionHeight: a.Height(),
 	}
 

relayer/processor/types.go

@@ -319,9 +319,13 @@ func (c ConnectionStateCache) FilterForClient(clientID string) ConnectionStateCa
 // ChainProcessorCacheData is the data sent from the ChainProcessors to the PathProcessors
 // to keep the PathProcessors up to date with the latest info from the chains.
 type ChainProcessorCacheData struct {
-	IBCMessagesCache     IBCMessagesCache
-	InSync               bool
-	ClientState          provider.ClientState
+	IBCMessagesCache IBCMessagesCache
+	InSync           bool
+
+	// Continuously updated: the client state for the counterparty chain on this chain, at the
+	// height defined by .LatestBlock
+	ClientState provider.ClientState
+
 	ConnectionStateCache ConnectionStateCache
 	ChannelStateCache    ChannelStateCache
 	LatestBlock          provider.LatestBlock

relayer/processor/types_internal.go

@@ -78,7 +78,7 @@ func (msg packetIBCMessage) assemble(
 	default:
 		return nil, fmt.Errorf("unexepected packet message eventType for message assembly: %s", msg.eventType)
 	}
-	if src.clientState.ClientID == ibcexported.LocalhostClientID {
+	if src.lastObservedClientState.ClientID == ibcexported.LocalhostClientID {
 		packetProof = src.localhostSentinelProofPacket
 	}
 
@@ -170,7 +170,7 @@ func (msg channelIBCMessage) assemble(
 	default:
 		return nil, fmt.Errorf("unexepected channel message eventType for message assembly: %s", msg.eventType)
 	}
-	if src.clientState.ClientID == ibcexported.LocalhostClientID {
+	if src.lastObservedClientState.ClientID == ibcexported.LocalhostClientID {
 		chanProof = src.localhostSentinelProofChannel
 	}
 

relayer/provider/provider.go

@@ -75,11 +75,9 @@ type ClientState struct {
 	Header []byte
 }
 
-// ClientTrustedState holds the current state of a client from the perspective of both involved chains,
-// i.e. ClientState enriched with the trusted IBC header of the counterparty chain.
-type ClientTrustedState struct {
-	ClientState ClientState
-	IBCHeader   IBCHeader
+type ClientStateWithNextHeader struct {
+	ClientState
+	NextHeader IBCHeader // IBC header corresponds to H+1 of ClientState, because it should match the nextValidatorsHash of the last header
 }
 
 // PacketInfo contains any relevant properties from packet flow messages