Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node/Solana: Shim watcher cleanup #4269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Node/Solana: Shim watcher cleanup #4269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 40 additions & 12 deletions node/pkg/watchers/solana/shim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ package solana
import (
"bytes"
"encoding/hex"
"errors"
"fmt"
"time"

Expand Down Expand Up @@ -175,6 +176,9 @@ func (s *SolanaWatcher) shimProcessTopLevelInstruction(
isReobservation bool,
) (bool, error) {
topLevelIdx := uint16(topLevelIndex)
if topLevelIdx >= uint16(len(tx.Message.Instructions)) {
return false, fmt.Errorf("topLevelIndex %d is greater than %d", topLevelIdx, len(tx.Message.Instructions))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return false, fmt.Errorf("topLevelIndex %d is greater than %d", topLevelIdx, len(tx.Message.Instructions))
return false, fmt.Errorf("topLevelIndex %d is greater than the total number of instructions in the tx message %d", topLevelIdx, len(tx.Message.Instructions))

}
inst := tx.Message.Instructions[topLevelIdx]

// The only top-level instruction generated by the shim contract is the PostMessage event. Parse that to get
Expand All @@ -192,7 +196,7 @@ func (s *SolanaWatcher) shimProcessTopLevelInstruction(
outerIdx := -1
for idx, inner := range innerInstructions {
if inner.Index == topLevelIdx {
outerIdx = int(idx)
outerIdx = idx
break
}
}
Expand All @@ -202,7 +206,7 @@ func (s *SolanaWatcher) shimProcessTopLevelInstruction(
}

// Process the inner instructions associated with this shim top-level instruction and produce an observation event.
err = s.shimProcessRest(logger, whProgramIndex, shimProgramIndex, tx, innerInstructions[outerIdx].Instructions, outerIdx, 0, postMessage, alreadyProcessed, isReobservation)
err = s.shimProcessRest(logger, whProgramIndex, shimProgramIndex, tx, innerInstructions[outerIdx].Instructions, outerIdx, 0, postMessage, alreadyProcessed, isReobservation, true)
if err != nil {
return false, fmt.Errorf("failed to process inner instructions for top-level shim instruction %d: %w", topLevelIdx, err)
}
Expand All @@ -226,6 +230,10 @@ func (s *SolanaWatcher) shimProcessInnerInstruction(
alreadyProcessed ShimAlreadyProcessed,
isReobservation bool,
) (bool, error) {
if startIdx >= len(innerInstructions) {
return false, fmt.Errorf("startIdx %d is greater than or equal to %d", startIdx, len(innerInstructions))
Copy link
Contributor

@johnsaigle johnsaigle Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return false, fmt.Errorf("startIdx %d is greater than or equal to %d", startIdx, len(innerInstructions))
return false, fmt.Errorf("startIdx %d is out of bounds of slice innerInstructions (length: %d)", startIdx, len(innerInstructions))

From the error log it's not clear why e.g. "startIdx 3 is greater than or equal to 3" is a problem

}

// See if this is a PostMessage event from the shim contract. If so, parse it. If not, bail out now.
postMessage, err := shimParsePostMessage(s.shimPostMessageDiscriminator, innerInstructions[startIdx].Data)
if err != nil {
Expand All @@ -238,7 +246,7 @@ func (s *SolanaWatcher) shimProcessInnerInstruction(

alreadyProcessed.add(outerIdx, startIdx)

err = s.shimProcessRest(logger, whProgramIndex, shimProgramIndex, tx, innerInstructions, outerIdx, startIdx+1, postMessage, alreadyProcessed, isReobservation)
err = s.shimProcessRest(logger, whProgramIndex, shimProgramIndex, tx, innerInstructions, outerIdx, startIdx+1, postMessage, alreadyProcessed, isReobservation, false)
if err != nil {
return false, fmt.Errorf("failed to process inner instructions for inner shim instruction %d: %w", outerIdx, err)
}
Expand All @@ -260,7 +268,12 @@ func (s *SolanaWatcher) shimProcessRest(
postMessage *ShimPostMessageData,
alreadyProcessed ShimAlreadyProcessed,
isReobservation bool,
isTopLevel bool,
) error {
if postMessage == nil {
return errors.New("postMessage is nil")
}

// Loop through the inner instructions after the shim PostMessage and do the following:
// 1) Find the core event and verify it is unreliable with an empty payload.
// 2) Find the shim MessageEvent to get the rest of the fields we need for the observation.
Expand All @@ -269,17 +282,25 @@ func (s *SolanaWatcher) shimProcessRest(
var verifiedCoreEvent bool
var messageEvent *ShimMessageEventData
var err error
coreEventFound := false
for idx := startIdx; idx < len(innerInstructions); idx++ {
inst := innerInstructions[idx]
if inst.ProgramIDIndex == whProgramIndex {
if verifiedCoreEvent, err = shimVerifyCoreMessage(inst.Data); err != nil {
if verifiedCoreEvent {
return fmt.Errorf("detected multiple inner core instructions when there should not be at instruction %d, %d", outerIdx, idx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I believe this check is too general: this returns an error if the inner instruction's program index matches the Wormhole Program's, regardless of the contents. I think instead what should be flagged here is the case where another "verified core event" occurs.

So the more targeted check would be something like:

// Ensure that only one verified core event appears in each set of inner instructions
if foundIt && verifiedCoreEvent {
 // return error 
 }

In practice this might be fine, but maybe there's a valid case (now or down the line) where the Wormhole program needs to be interacted with twice in the same inner instruction set.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it looks pretty restrictive to me in the sense that any sequence of instructions where there's an instruction that returns true on shimVerifyCoreMessage(inst.Data), and then contains any subsequent instruction where the following condition holds inst.ProgramIDIndex == whProgramIndex, will result in an error.

}
foundIt, err := shimVerifyCoreMessage(inst.Data)
if err != nil {
return fmt.Errorf("failed to verify inner core instruction for shim instruction %d, %d: %w", outerIdx, idx, err)
}
alreadyProcessed.add(outerIdx, idx)
coreEventFound = true
if foundIt {
alreadyProcessed.add(outerIdx, idx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to add this the instruction to the alreadyProcessed set even if foundIt is false.

verifiedCoreEvent = true
}
} else if inst.ProgramIDIndex == shimProgramIndex {
if !coreEventFound {
if messageEvent != nil {
return fmt.Errorf("detected multiple shim message event instructions when there should not be at instruction %d, %d", outerIdx, idx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the case above -- this will catch and throw an error for any interaction with the shim program, even if this isn't a second shim event. So, this code might end up looking like this:

		} else if inst.ProgramIDIndex == shimProgramIndex {
			if !verifiedCoreEvent {
				return fmt.Errorf("detected an inner shim message event instruction before the core event for shim instruction %d, %d: %w", outerIdx, idx, err)
			}
			thisEvent, err := shimParseMessageEvent(s.shimMessageEventDiscriminator, inst.Data)
			if err != nil {
				return fmt.Errorf("failed to parse inner shim message event instruction for shim instruction %d, %d: %w", outerIdx, idx, err)
			}
			if messageEvent != nil && thisEvent != nil && len(thisEvent) >  0 {
				return fmt.Errorf("detected multiple shim message event instructions when there should not be at instruction %d, %d", outerIdx, idx)
			}
			alreadyProcessed.add(outerIdx, idx)
		}

}
if !verifiedCoreEvent {
return fmt.Errorf("detected an inner shim message event instruction before the core event for shim instruction %d, %d: %w", outerIdx, idx, err)
}
messageEvent, err = shimParseMessageEvent(s.shimMessageEventDiscriminator, inst.Data)
Expand All @@ -290,16 +311,21 @@ func (s *SolanaWatcher) shimProcessRest(
}

if verifiedCoreEvent && messageEvent != nil {
break
if !isTopLevel {
// We found what we are looking for, and additional shim events in the inner instruction case are allowed, so we can stop looking.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the loop continue in case there are multiple verifiedCoreEvent occurrences? If not, it would be good to reword this to something like "In the integrator (inner instruction) case, additional core event and shim event pairs are allowed."

Basically my concern with this function is to make sure that:

  • there's a one-to-one correspondance of core events and shim events in all cases (i.e. they're always matched)
  • there are no "extra events" -- e.g. in the Direct/top-level case, there should only be one core event and one shim event

break
}

// In the top level instruction case, we should not have any additional shim events in this instruction set.
}
}

if !verifiedCoreEvent {
return fmt.Errorf("failed to find inner core instruction for shim instruction %d", outerIdx)
return fmt.Errorf("failed to find inner core instruction for shim instruction %d, %d", outerIdx, startIdx)
}

if messageEvent == nil {
return fmt.Errorf("failed to find inner shim message event instruction for shim instruction %d", outerIdx)
return fmt.Errorf("failed to find inner shim message event instruction for shim instruction %d, %d", outerIdx, startIdx)
}

commitment, err := postMessage.ConsistencyLevel.Commitment()
Expand Down Expand Up @@ -328,7 +354,9 @@ func (s *SolanaWatcher) shimProcessRest(
Payload: postMessage.Payload,
ConsistencyLevel: uint8(postMessage.ConsistencyLevel),
IsReobservation: isReobservation,
Unreliable: false,

// Shim messages are always reliable.
Unreliable: false,
}

solanaMessagesConfirmed.WithLabelValues(s.networkName).Inc()
Expand Down
Loading
Loading