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

Lower governor limits of chains based upon usage #4102

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

mdulin2
Copy link
Contributor

@mdulin2 mdulin2 commented Aug 29, 2024

I did some analysis of the token bridge for various chains over the course of 90 days. If the governor limit was not close to being reached (lower than maximum usage of 90% within time frame), then they do not need to be as high. Lowering the Governor limit is a good security win for limiting the impact of an exploit.

From my analysis, here is the maximum Governance usage that I found within the 90 day period:

  • Aurora (0%)
  • Blast (0%)
  • Xpla (0%)
  • XLayer (0%)
  • Mantle (0.90%)
  • Acala (2.2%)
  • Terra2 (3.6%)
  • Near (5.39%)
  • Terra (5.54%)
  • Karura (7.09%)
  • Oasis (9.45%)
  • Avalanche (13.62%)
  • Polygon (14.64%)

@mdulin2 mdulin2 marked this pull request as ready for review September 3, 2024 16:06
bruce-riley
bruce-riley previously approved these changes Sep 3, 2024
Copy link
Contributor

@bruce-riley bruce-riley left a comment

Choose a reason for hiding this comment

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

This looks good to me as long as the rest of the security team agrees.

djb15
djb15 previously approved these changes Sep 3, 2024
@johnsaigle johnsaigle force-pushed the governor_limit_lowering_aug_2024 branch from 1821c66 to cf2cfcf Compare September 3, 2024 20:11
@johnsaigle
Copy link
Contributor

@mdulin2 I'm getting test failures running locally, e.g.:

go test -ldflags '-extldflags "-Wl,-ld_classic " ' ./pkg/governor

--- FAIL: TestChainDailyLimitRange (0.00s)
    --- FAIL: TestChainDailyLimitRange/aurora (0.00s)
        mainnet_chains_test.go:39:
                Error Trace:    /Users/john/coding/wormhole/node/pkg/governor/mainnet_chains_test.go:39
                Error:          "0" is not greater than "0"
                Test:           TestChainDailyLimitRange/aurora
--- FAIL: TestChainListBigTransfers (0.00s)
    mainnet_chains_test.go:66:
                Error Trace:    /Users/john/coding/wormhole/node/pkg/governor/mainnet_chains_test.go:66
                Error:          "0" is not less than "0"
                Test:           TestChainListBigTransfers
    mainnet_chains_test.go:69:
                Error Trace:    /Users/john/coding/wormhole/node/pkg/governor/mainnet_chains_test.go:69
                Error:          "0" is not less than "0"
                Test:           TestChainListBigTransfers
FAIL
FAIL    github.com/certusone/wormhole/node/pkg/governor 0.329s
FAIL

Probably because we've never used a chain limit of 0. It's worth going into these tests and figuring out acceptable modifications for limits of 0.

@mdulin2
Copy link
Contributor Author

mdulin2 commented Sep 5, 2024

Good catch John. I'll see what the issue is and make some changes.

How do you run all of the tests locally? I always get annoying build errors when I try to run the entire test suite.

@johnsaigle
Copy link
Contributor

johnsaigle commented Sep 5, 2024

@mdulin2 You can use this for unit testing the governor:

cd node/
go test -ldflags '-extldflags "-Wl,-ld_classic " ' ./pkg/governor

See also node/Makefile. I recently committed some info there to help with arm64-based testing as a follow-up to #4031

@mdulin2 mdulin2 dismissed stale reviews from bruce-riley and djb15 via 08037e8 September 5, 2024 19:55
@mdulin2
Copy link
Contributor Author

mdulin2 commented Sep 5, 2024

@johnsaigle Tests should be fixed now. @kcsongor says that changing the tests to allow for a limit of 0 seems fine with the new consideration of deprecating chains.

@mdulin2 mdulin2 force-pushed the governor_limit_lowering_aug_2024 branch from 08037e8 to 0c245d6 Compare September 5, 2024 20:21
@mdulin2 mdulin2 force-pushed the governor_limit_lowering_aug_2024 branch from 0c245d6 to d8eabfa Compare September 5, 2024 20:23
@johnsaigle johnsaigle merged commit 6b810ac into main Sep 10, 2024
24 checks passed
@johnsaigle johnsaigle deleted the governor_limit_lowering_aug_2024 branch September 10, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants