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.

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: Reobserve with custom endpoint #4260

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bruce-riley
Copy link
Contributor

@bruce-riley bruce-riley commented Feb 7, 2025

This PR adds support for the reobserve-with-endpoint admin command.

Don't be alarmed by the size of this PR. 900+ lines are generated protobuf code. Also the code in handleReobservationRequest in Solana and EVM is mostly just code moved into a function so it can be reused.

Tests performed:

  • Sepolia new observations still works
  • Sepolia standard reobservation still works
  • Sepolia with custom endpoint works
  • Solana new observations still works
  • Solana standard reobservation by account still works
  • Solana standard reobservation by transaction ID still works
  • Solana by account ID with custom endpoint works
  • Solana by transaction ID with custom endpoint works
  • Chain that is not enabled returns "chain X does not support reobservation by endpoint"
  • Chain other than Solana or EVM that is enabled returns "chain X does not support reobservation by endpoint"

@bruce-riley bruce-riley force-pushed the node_reobserve_with_endpoint branch 5 times, most recently from d399f23 to ccaca21 Compare February 11, 2025 16:28
@bruce-riley bruce-riley marked this pull request as ready for review February 12, 2025 13:44
@bruce-riley bruce-riley force-pushed the node_reobserve_with_endpoint branch 2 times, most recently from da65817 to 880d508 Compare February 12, 2025 15:20
@bruce-riley bruce-riley force-pushed the node_reobserve_with_endpoint branch from 456fed5 to e724478 Compare February 18, 2025 17:57
Copy link
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Looks good, made some suggestions to simplify the code.

@bruce-riley
Copy link
Contributor Author

bruce-riley commented Feb 20, 2025

Note: Once #4116 is merged, the EVM version of Reobserve should be updated to call verifyEvmChainID before performing the reobservation.

// In the primary watcher flow, this is of no concern since we assume the node
// always sends the head before it sends the logs (implicit synchronization
// by relying on the same websocket connection).
blockNumberU := atomic.LoadUint64(&w.latestFinalizedBlockNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not seem same to assume that the watcher finalized block has any relation to the transaction from another connection. can Reobserve create a new watcher instance?

@@ -341,8 +343,17 @@ func (s *SolanaWatcher) Run(ctx context.Context) error {
ContractAddress: contractAddr,
})

// Don't overwrite these fields if they are already set in case there is an old go routine still using them.
Copy link
Contributor

Choose a reason for hiding this comment

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

does this actually happen somewhere or is this just a defense?

Copy link
Contributor

Choose a reason for hiding this comment

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

This drew my eye as well. Maybe it's worth logging if s.ctx isn't nil? Could be helpful to know if there's a Go routine out there that shouldn't be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not actually seen it happen, it just seems like a theoretical possibility since the watcher spawns go routines to fetch accounts. Maybe those could still be running (and attempting to log or something) when the watcher restarts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants