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

feat: log poller for vrf v2/v2+ #11174

Merged
merged 22 commits into from
Nov 28, 2023
Merged

feat: log poller for vrf v2/v2+ #11174

merged 22 commits into from
Nov 28, 2023

Conversation

makramkd
Copy link
Contributor

@makramkd makramkd commented Nov 4, 2023

Switch over the VRF V2 listener to use the log poller instead of the log broadcaster.

Summary of Changes

  1. Transform the log processing loop of the VRF listener to the following (pseudocode, see runLogListener in listener_v2_log_listener.go for the gory details):
ticker := time.NewTicker(pollPeriod)
var (
	lastProcessedBlock int64
	startingUp         = true
)
for {
	case <-stop:
		return
	case <-ticker.C:
		err := registerLogPollerFilter()
		if startingUp {
			lastProcessedBlock = initLastProcessedBlock()
			startingUp = false
		}
		pending := pollLogs(lastProcessedBlock)
		// adds sent fulfillments to inflight cache
		processPendingRequests(pending)
		lastProcessedBlock = updateLastProcessedBlock(lastProcessedBlock)
}

The effect of this is that the processing loop of the VRF listener is much simpler, concurrent access to lsn.reqs is no longer needed, and in fact the reqs in-memory queue can be completely removed, as well as any associated data structures to protect it from data races. The function runRequestHandler has also been removed, since pending request processing is in the same loop as the log polling.

  1. Separate the VRF listener code into four files to make grokking things easier:
    a. listener_v2_log_listener.go: this file contains all log polling and listening logic, as well as the main loop runLogListener. This is the entrypoint into the functionality of the VRF listener. This file contains the lion's share of new code in this PR, mainly the revamped runLogListener, and the new initializeLastProcessedBlock, updateLastProcessedBlock, and pollLogs methods.
    b. listener_v2_log_processor.go: this file contains all log processing logic, the entrypoint of which is processPendingVRFRequests. Notably, processPendingVRFRequests marks all in-flight fulfillments in the inflightCache cache, so that these requests don't get reprocessed. Most of the code in this file is unchanged and was simply copied over from listener_v2.go.
    c. listener_v2_helpers.go: contains helper functions and types, extracted from listener_v2.go.

Copy link
Contributor

github-actions bot commented Nov 4, 2023

I see that you haven't updated any README files. Would it make sense to do so?

core/services/vrf/v2/listener_v2.go Outdated Show resolved Hide resolved
core/services/vrf/v2/listener_v2.go Outdated Show resolved Hide resolved
core/services/vrf/v2/listener_v2.go Outdated Show resolved Hide resolved
@makramkd makramkd force-pushed the feature/vrf-log-poller branch from cf415bd to 654c261 Compare November 11, 2023 15:43
@makramkd makramkd marked this pull request as ready for review November 11, 2023 19:30
@makramkd makramkd requested a review from a team as a code owner November 11, 2023 19:30
@makramkd
Copy link
Contributor Author

makramkd commented Nov 12, 2023

After running a short load test it seems that this approach of having everything in a single loop has actually caused a regression in performance on the simulated networks we use for load tests.

I'm not sure of what is causing it, perhaps the log poller interval is too high, and perhaps the VRF job interval is also too high. If so I'll set them appropriately and run the test again.

@makramkd makramkd requested a review from a team as a code owner November 13, 2023 09:53
@makramkd
Copy link
Contributor Author

Update: after bumping log poll interval down to 1s the load tests are looking on-par with the log-broadcaster-based VRF listener in the simulated environment.

@makramkd makramkd force-pushed the feature/vrf-log-poller branch from 1ee78a6 to d6e83b7 Compare November 20, 2023 09:55
Copy link
Contributor

@jinhoonbang jinhoonbang left a comment

Choose a reason for hiding this comment

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

Still reviewing! Mostly looked at the new main LP logic

core/services/vrf/v2/listener_v2_log_listener.go Outdated Show resolved Hide resolved
core/services/vrf/v2/listener_v2_log_listener.go Outdated Show resolved Hide resolved
jinhoonbang
jinhoonbang previously approved these changes Nov 28, 2023
these tests have questionable value; we should probably remove them or
find a better way to test this particular functionality. Perhaps this
should even be removed.
@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 12.3% 12.32% Duplicated Lines (%) on New Code (is greater than 3%)

See analysis details on SonarQube

@jinhoonbang jinhoonbang added this pull request to the merge queue Nov 28, 2023
Merged via the queue into develop with commit 524eafc Nov 28, 2023
87 checks passed
@jinhoonbang jinhoonbang deleted the feature/vrf-log-poller branch November 28, 2023 13:21
fbac pushed a commit that referenced this pull request Dec 14, 2023
* feat: log poller for vrf v2/+

* chore: address PR comments

* fix: RLock() instead of Lock()

* fix: test

* fix: tests still flakey

* fix: lint

* fix: build

* chore: PR comments

* chore: time out old requests

* chore: skip EOA tests

these tests have questionable value; we should probably remove them or
find a better way to test this particular functionality. Perhaps this
should even be removed.

* remove concurrency in processing

seems unnecessary

* fix build

* fix build

* fix lint

* fix test

* fix tests
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.

7 participants