[build-295] Improve Builder submission rate #70

Wazzymandias · 2023-05-09T13:13:30Z

📝 Summary

Fix rate limit bug that allowed more builder submissions than desired
Expose FLASHBOTS_BUILDER_RATE_LIMIT_DURATION and FLASHBOTS_BUILDER_BURST_LIMIT environment variables
Update builder submission logic such that submissions are sent near end of slot

🧪 Testing

Testing was done with test builder deployed on production mainnet

Without builder submission change

With builder submission change

TL;DR

Prior to change, builder submissions happened roughly between ~t-11 to ~t+3 where t is the time of slot s
After change, builder submissions happen roughly between ~t-4 to ~t+3, which is desired

📚 References

I have seen and agree to CONTRIBUTING.md

… limit, update submit logic such that submissions happen near end of slot

builder/builder.go

builder/resubmit_utils.go

builder/service.go

builder/builder.go

README.md

…te limit settings

builder/resubmit_utils.go

cmd/utils/flags.go

…ssion-rate

…riable

cmd/geth/main.go

Ruteri · 2023-05-23T11:11:21Z

builder/builder.go

+		args.limiter = rate.NewLimiter(rate.Every(RateLimitIntervalDefault), RateLimitBurstDefault)
+	}
+
+	if args.builderBlockResubmitInterval == 0 {


I wonder if this shouldn't be the caller's responsibility (same for the above)

I wonder if this shouldn't be the caller's responsibility (same for the above)

I have it in both places. In general I agree with you re: caller's responsibility. However I would want to return an error for invalid inputs but the function signature would need to be updated. I didn't want to do that work in this PR - happy to discuss

I'd make it the responsibility of the caller still, since you don't really validate the value

I'd make it the responsibility of the caller still, since you don't really validate the value

For now what's wrong with having it in both places? I'm worried that without the defensive check in the receiver, we can easily have a resubmit interval of 0. A resubmit interval of 0 could cause some very strange and difficult to debug behavior since we're using it for retry loop. My preference would rather be to refactor the function signature in the future such that a 0 value returns an error.

builder/builder.go

cmd/utils/flags.go

builder/resubmit_utils.go

…unit tests

Ruteri · 2023-05-25T08:55:30Z

builder/builder.go

+		args.limiter = rate.NewLimiter(rate.Every(RateLimitIntervalDefault), RateLimitBurstDefault)
+	}
+
+	if args.builderBlockResubmitInterval == 0 {


I'd make it the responsibility of the caller still, since you don't really validate the value

Ruteri · 2023-05-25T08:56:22Z

builder/builder.go

 	if parentBlock == nil {
-		log.Warn("Block hash not found in blocktree", "head block hash", attrs.HeadHash)
-		return errors.New("parent block not found in blocktree")
+		return fmt.Errorf("parent block hash not found in block tree given head block hash %s", attrs.HeadHash)


Is HeadHash a string? Same for vd.Pubkey couple lines above

Ruteri · 2023-05-25T09:01:05Z

builder/builder.go

+	// Avoid submitting early into a given slot. For example if slots have 12 second interval, submissions should
+	// not begin until 8 seconds into the slot.
+	slotTime := time.Unix(int64(attrs.Timestamp), 0).UTC()
+	submitTime := slotTime.Add(-SubmissionDelaySecondsDefault)


I find this a bit strange
I'm not sure if the semantics work out, since we don't actually know what's the submit time - here it's defined as slot time - submission delay, which could use a different name. Maybe slot sumission start time? Also the time is not really a delay, it's actually the other way around - its something like a ramp up time
I'd also add a sanity check for attrs.Timestamp < SubmissionDelaySecondsDefault.

builder/resubmit_utils.go

Ruteri · 2023-05-25T09:08:02Z

builder/resubmit_utils.go

+		waitUntilSubmitTime = func(waitUntil time.Time) (ok bool) {
+			now := time.Now().UTC()
+			if waitUntil.UTC().Before(now) {
+				waitUntil = now


shouldn't this just be return true?

not necessarily i think - in the time the function is invoked i want to make sure the context hasn't been cancelled

builder/resubmit_utils.go

cmd/geth/main.go

cmd/utils/flags.go

- Prior to change, builder submissions happened roughly between ~t-11 to ~t+3 where t is the time of slot s - After change, builder submissions happen roughly between ~t-4 to ~t+3, which is desired * Add bug fix for rate limit * Expose environment variables that adjust builder rate limit and burst limit, update submit logic such that submissions happen near end of slot * Update submit loop function * Update conditional in resubmit loop * Update rate limit variable name * Create constant for default burst on builder rate limit * Use CLI flags instead of bespoke environment variables for builder rate limit settings * Fix typo * Update logs for more data analysis * Expose builder block resubmit interval as CLI flag and environment variable * Update variable name for default builder block resubmit interval * Fix wait time when timestamp is not passed in to address failures in unit tests * Update README * Update comments * Fix when error log occurs * Update log * Update check * Update go mod

Wazzymandias added 2 commits May 8, 2023 23:33

Add bug fix for rate limit

b2fd880

Expose environment variables that adjust builder rate limit and burst…

e33c3ca

… limit, update submit logic such that submissions happen near end of slot

Wazzymandias requested review from avalonche, dvush and metachris May 9, 2023 13:13

Wazzymandias changed the title ~~Wasif/build 295 submission rate~~ [build-295] Improve Builder submission rate May 9, 2023

Update submit loop function

ff739f4

metachris reviewed May 9, 2023

View reviewed changes

builder/builder.go Outdated Show resolved Hide resolved

metachris reviewed May 9, 2023

View reviewed changes

builder/resubmit_utils.go Outdated Show resolved Hide resolved

metachris reviewed May 9, 2023

View reviewed changes

builder/service.go Outdated Show resolved Hide resolved

Wazzymandias added 2 commits May 9, 2023 16:45

Update conditional in resubmit loop

bb721a2

Update rate limit variable name

7d99113

avalonche reviewed May 9, 2023

View reviewed changes

builder/builder.go Outdated Show resolved Hide resolved

avalonche reviewed May 9, 2023

View reviewed changes

builder/builder.go Outdated Show resolved Hide resolved

Create constant for default burst on builder rate limit

2d2ea47

dvush reviewed May 10, 2023

View reviewed changes

README.md Show resolved Hide resolved

Wazzymandias added 3 commits May 11, 2023 14:30

Use CLI flags instead of bespoke environment variables for builder ra…

99af9da

…te limit settings

Fix typo

21ef625

Update logs for more data analysis

74d9ec2

dvush reviewed May 15, 2023

View reviewed changes

builder/resubmit_utils.go Outdated Show resolved Hide resolved

builder/resubmit_utils.go Outdated Show resolved Hide resolved

builder/resubmit_utils.go Outdated Show resolved Hide resolved

cmd/utils/flags.go Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into wasif/build-295-submi…

f365afc

…ssion-rate

Wazzymandias marked this pull request as ready for review May 22, 2023 18:39

Wazzymandias requested review from Ruteri, avalonche and metachris May 22, 2023 18:45

Expose builder block resubmit interval as CLI flag and environment va…

c1e942f

…riable

Wazzymandias requested a review from dvush May 22, 2023 19:00

Update variable name for default builder block resubmit interval

95beccf

Ruteri reviewed May 23, 2023

View reviewed changes

Address PR feedback

b70bf10

Wazzymandias added 2 commits May 24, 2023 12:28

Address PR feedback

c0590ba

Fix wait time when timestamp is not passed in to address failures in …

cc58b37

…unit tests

Wazzymandias requested a review from Ruteri May 25, 2023 00:16

Ruteri reviewed May 25, 2023

View reviewed changes

dvush approved these changes May 25, 2023

View reviewed changes

Wazzymandias added 6 commits May 25, 2023 08:51

Update README

5d7fec9

Update comments

2e823dd

Fix when error log occurs

78cd053

Update log

a6d6810

Update check

1acc069

Update go mod

1b60817

avalonche approved these changes May 26, 2023

View reviewed changes

Wazzymandias merged commit 66893df into main May 26, 2023

Wazzymandias deleted the wasif/build-295-submission-rate branch May 26, 2023 20:12

[build-295] Improve Builder submission rate #70

[build-295] Improve Builder submission rate #70

Uh oh!

Conversation

Wazzymandias commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

🧪 Testing

Without builder submission change

With builder submission change

TL;DR

📚 References

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wazzymandias May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Wazzymandias commented May 9, 2023 •

edited

Loading

Wazzymandias May 25, 2023 •

edited

Loading