Don't post a batch that would cause a reorg due to being outside (or near) the time bounds #2340
Conversation
…near) the time bounds
cla-bot
bot
added
the
s
| if (config.ReorgResistanceMargin > 0 && config.ReorgResistanceMargin < config.MaxDelay) && | ||
| (batchDuration >= config.MaxDelay-config.ReorgResistanceMargin) && | ||
| (batchDuration <= config.MaxDelay || (batchDuration <= config.MaxDelay+config.ReorgResistanceMargin && !config.IgnoreReorgResistanceMargin)) { | ||
| disablePosting = true |
There was a problem hiding this comment.
We should log an error here
arbnode/batch_poster.go
Outdated
| @@ -156,6 +156,8 @@ type BatchPosterConfig struct { | |||
| L1BlockBoundBypass time.Duration `koanf:"l1-block-bound-bypass" reload:"hot"` | |||
| UseAccessLists bool `koanf:"use-access-lists" reload:"hot"` | |||
| GasEstimateBaseFeeMultipleBips arbmath.Bips `koanf:"gas-estimate-base-fee-multiple-bips"` | |||
| ReorgResistanceMargin time.Duration `koanf:"reorg-resistance-margin" reload:"hot"` | |||
| IgnoreReorgResistanceMargin bool `koanf:"ignore-reorg-resistance-margin" reload:"hot"` | |||
There was a problem hiding this comment.
Optional(Nit): Prefer using boolean flags with a positive phrasing. Maybe "EnableReorgResistanceMargin" or "EnforceReorgResistanceMargin". When you have a negative like "Ignore" or "Disable" it means that you have to mentally negate the meaning of the boolean which slightly deteriorates the readability of the code.
arbnode/batch_poster.go
Outdated
| batchDuration := time.Since(firstMsgTime) | ||
| if (config.ReorgResistanceMargin > 0 && config.ReorgResistanceMargin < config.MaxDelay) && | ||
| (batchDuration >= config.MaxDelay-config.ReorgResistanceMargin) && | ||
| (batchDuration <= config.MaxDelay || (batchDuration <= config.MaxDelay+config.ReorgResistanceMargin && !config.IgnoreReorgResistanceMargin)) { |
There was a problem hiding this comment.
I'm personally having trouble reasoning through this compound condition. For example, I wonder why we are only checking config.IgnoreReorgResistanceMargin in the last parenthetical || instead of in the other places where the value of the config.ReorgResistanceMargin is being checked. Like, why don't we only ignore the config.ReorgResistanceMargin when checking <= instead of for both <= and >=?
Maybe some test-cases would make it easier to see the truth table of when this clause should disable posting?
There was a problem hiding this comment.
I agree with @eljobe on this one.
There was a problem hiding this comment.
I agree that the if condition is quite complex, I'll try to split it into multiple stages for clarity.
When the batch's duration is between config.MaxDelay-config.ReorgResistanceMargin and config.MaxDelay we will definitely pause posting of batches.
And when its between config.MaxDelay and config.MaxDelay+config.ReorgResistanceMargin we provide an option to prioritize MaxDelay's purpose over ReorgResistanceMargin's.
IgnoreReorgResistanceMargin flag is used to achieve this.
Line 1123 in 7373a4e
I'll rename IgnoreReorgResistanceMargin flag to something like PrioritizeMaxDelayOverReorgResistance which captures its purpose clearly
Resolves NIT-2524