In the FailedAuthorizations limits, there was code that intentionally ignored errLimitDisabled errors (errors.Is(err, errLimitDisabled)). However, that that resulted in those functions later using a returned limit value that was invalid (i.e. its zero value). That happened to trigger some later checks in validateTransaction. Specifically this check failed:
if txn.cost > txn.limit.Burst {
// error
When txt.limit.Burst is zero, this will always fail.
This problem doesn't really show up in prod, where all the limits are configured. But it showed up in tests, specifically TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit, where the limits are constructed using a simplified config that leaves most of them disabled.
In this change, I tried to make handling of errLimitDisabled more consistent, and always return an allow-only transaction as early as possible instead of falling through the error condition.
Where that wasn't possible, I used a boolean to record whether the result of builder.getLimit() was valid before referencing any of its fields.
I also added some "shouldn't happen" errors to catch this problem earlier if it recurs.
I removed some "skip disabled limit" comments because those say "what the code does" (which the code also says), not "why the code does it".
In the FailedAuthorizations limits, there was code that intentionally ignored errLimitDisabled errors (
errors.Is(err, errLimitDisabled)). However, that that resulted in those functions later using a returnedlimitvalue that was invalid (i.e. its zero value). That happened to trigger some later checks in validateTransaction. Specifically this check failed:When txt.limit.Burst is zero, this will always fail.
This problem doesn't really show up in prod, where all the limits are configured. But it showed up in tests, specifically TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit, where the limits are constructed using a simplified config that leaves most of them disabled.
In this change, I tried to make handling of errLimitDisabled more consistent, and always return an allow-only transaction as early as possible instead of falling through the error condition.
Where that wasn't possible, I used a boolean to record whether the result of
builder.getLimit()was valid before referencing any of its fields.I also added some "shouldn't happen" errors to catch this problem earlier if it recurs.
I removed some "skip disabled limit" comments because those say "what the code does" (which the code also says), not "why the code does it".
Fixes the test failures in #7797.