Move Rate Limiter

[Primata]

Description

Rate Limits the Relayer capability of calling complete_bridge_transfer based on an insurance fund address, similar to the bridge counterparty on Solidity. movementlabsxyz/movement#836

How Has This Been Tested?

Mirrors testing of rate limit on Solidity but without fuzzing

Key Areas to Review

Test coverage

Type of Change

• New feature
• Bug fix
• Breaking change
• Performance improvement
• Refactoring
• Dependency update
• Documentation update
• Tests

Which Components or Systems Does This Change Impact?

• Validator Node
• Full Node (API, Indexer, etc.)
• Move/Aptos Virtual Machine
• Aptos Framework
• Aptos CLI/SDK
• Developer Infrastructure
• Move Compiler
• Other (specify)

Checklist

• I have read and followed the CONTRIBUTING doc
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I identified and added all stakeholders and component owners affected by this change as reviewers
• I tested both happy and unhappy path of the functionality
• I have made corresponding changes to the documentation

[andygolay]

Thanks for this, @Primata

Got this test failure with movement move test:

Failures in 0x1::native_bridge:

┌── test_complete_bridge_with_erroneous_bridge_id_by_relayer ──────
│ error[E11001]: test failure
│    ┌─ /Users/andygmove/Downloads/repos/aptos-core/aptos-move/framework/aptos-framework/sources/timestamp.move:62:9
│    │
│ 61 │     public fun now_microseconds(): u64 acquires CurrentTimeMicroseconds {
│    │                ---------------- In this function in 0x1::timestamp
│ 62 │         borrow_global<CurrentTimeMicroseconds>(@aptos_framework).microseconds
│    │         ^^^^^^^^^^^^^ Test did not error as expected. Expected test to abort with code 2 originating in the module 0000000000000000000000000000000000000000000000000000000000000001::native_bridge but instead it gave a MISSING_DATA (code 4008) error with error message: "Failed to borrow global resource from 0000000000000000000000000000000000000000000000000000000000000001". Error originating in the module 0000000000000000000000000000000000000000000000000000000000000001::timestamp rooted here
│ 
│ 
│ stack trace
│       timestamp::now_seconds(/Users/andygmove/Downloads/repos/aptos-core/aptos-move/framework/aptos-framework/sources/timestamp.move:68)
│       native_bridge::assert_rate_limit_budget_not_exceeded(/Users/andygmove/Downloads/repos/aptos-core/aptos-move/framework/aptos-framework/sources/native_bridge.move:660)
│       native_bridge::complete_bridge_transfer(/Users/andygmove/Downloads/repos/aptos-core/aptos-move/framework/aptos-framework/sources/native_bridge.move:504)
│       native_bridge::test_complete_bridge_with_erroneous_bridge_id_by_relayer(/Users/andygmove/Downloads/repos/aptos-core/aptos-move/framework/aptos-framework/sources/native_bridge.move:1023-1030)
│ 
└──────────────────

Test result: FAILED. Total tests: 513; passed: 512; failed: 1
{
  "Error": "Move unit tests failed"
}

Putting in draft until all unit tests pass.

Also needs to be clearly aligned with MIP-58, ideally with reviews from Research team, before merging.

[andygolay]

• Move unit tests and E2E Move tests pass ✅
• LGTM for testing, with a note that a proper MIP and review from Research is needed for the entire rate limiter before considering this ready for production.

[0xmovses]

@andygolay This Rate Limiting implementation is already specified in MIP-58 https://github.com/movementlabsxyz/MIP/blob/161ed87aba691992b83f7d3fb1641d58bd5fe352/MIP/mip-58/README.md

The new suggestions and updates are to be laid out in a new MIP-74. The review and approval of that MIP don't block this from being merged.

This is so that we can deploy this initial version of the Native Bridge MIP-58 to Bardock and unblock front end testing.

cc. @Primata

[andygolay]

@andygolay This Rate Limiting implementation is already specified in MIP-58 https://github.com/movementlabsxyz/MIP/blob/161ed87aba691992b83f7d3fb1641d58bd5fe352/MIP/mip-58/README.md

The new suggestions and updates are to be laid out in a new MIP-74. The review and approval of that MIP don't block this from being merged.

This is so that we can deploy this initial version of the Native Bridge MIP-58 to Bardock and unblock front end testing.

cc. @Primata

Right, sounds like we're on the same page about this being fine to merge for front end testing.

MIP-58 mentions rate limiting but it doesn't seem totally clear nor is it consistent with this implementation from what I can tell. For example MIP-58 mentions, "Single-sided rate limiting, Rate limiting should be implemented on the L1 and maps each day to a budget, for each direction." Maybe I'm misunderstanding the wording or implementation. But here we have rate limiting implemented on the L2. So that's a big difference. Likewise there's no mention of the denominator in MIP-58, despite it being an important factor in the implementation.

That said, yes I agree this is good for testing. And look forward to a more fully-fleshed out and aligned spec and revisiting when it comes time to integrate everything. I haven't been able to focus on it as I'm occupied with operations tasks and want to be sure to prioritize those. I don't want to downplay this progress but rather emphasize that it's still in my opinion an early iteration that needs more formal input and consideration before being deemed production-ready (which it sounds like we agree on).

I already approved, LGTM to merge once we get another approval.

[apenzk]

@andygolay MIP-74 now requests that rate limit is applied symmetrical. The circumstances have changed compared to the HTLC-type bridge.

The idea boils down to

• We have a Governance Operator which operates the Insurance Fund and sets parameters, such as reaction time
• We have a Relayer which is a software component
• We trust the Governance Operator more than the Relayer

Consequently,

• We protect against abuse or error of minting on the target chain (protect the supply)
• We protect the user from sending transfers that will take a long time or never complete on the source chain (protect the user and Relayer)

There may be dispute as to how the rate limits are set.. For example does the insurance fund need to be on both chains (target chain for either direction, i.e. L1->L2 and L2->L1) or is it sufficient to be on one chain, as the Governance Operator is supposed to be aware of both chains, e.g. through the Informer.

[andygolay]

@andygolay MIP-74 now requests that rate limit is applied symmetrical. The circumstances have changed compared to the HTLC-type bridge.

The idea boils down to

• We have a Governance Operator which operates the Insurance Fund and sets parameters, such as reaction time
• We have a Relayer which is a software component
• We trust the Governance Operator more than the Relayer

Consequently,

• We protect against abuse or error of minting on the target chain (protect the supply)
• We protect the user from sending transfers that will take a long time or never complete on the source chain (protect the user and Relayer)

There may be dispute as to how the rate limits are set.. For example does the insurance fund need to be on both chains (target chain for either direction, i.e. L1->L2 and L2->L1) or is it sufficient to be on one chain, as the Governance Operator is supposed to be aware of both chains, e.g. through the Informer.

MIP-58 wasn't for the HTLC bridge. For the most part, discussion of the rate limiting specification details is probably best kept to the relevant MIP itself. My point in my approving comment on this PR was that this code should be considered an early iteration, for testing, while a proper MIP for rate limiting, eg MIP-74, is worked out. Then the code on both sides should be inspected to be sure that it actually implements the MIP. Hopefully we all agree on that; I don't see it as a particularly controversial or objectionable stance.

I already approved this PR. All that needs to happen here is for someone else to review and approve it, then it can be merged. I don't think we should get into the weeds about the design in the PR conversation, but rather in MIP conversation.

[apenzk]

What is the risk denominator? i assume it is the reaction time or window.

you could provide the equation context here (rate_limit=....)

[apenzk]

There seems to be an error in the units

the left value is a budget unit, whereas rate_limit is a budget/time-unit (here max_budget/day) ..

for example one_day_budget = rate_limit * 1 (days)

however the budget can be consumed within a risk_period, which could be 8 hours, 24hours, 48 hours, etc.

maybe we should approach this different:

• window = risk_denominator ( = reaction time )
• budget_for_window = insurance_fund
• assert ( current_budget + amount < budget_for_window )

[apenzk]

The window length of budgets probably may or even will change. specifically the Governance Operator may set the window to e.g. 8 hours, which means in a first approach (without moving window) the max_budget would reset after the new window time (8 hours)

[apenzk]

left some comments

[apenzk]

I don't think we should get into the weeds about the design in the PR conversation, but rather in MIP conversation.

@andygolay I agree that higher level design discussions should happen at the MIP. I was providing context and addressing your comment for your convenience. :)

[andygolay]

I don't think we should get into the weeds about the design in the PR conversation, but rather in MIP conversation.

@andygolay I agree that higher level design discussions should happen at the MIP. I was providing context and addressing your comment for your convenience. :)

The MIP context should be given in the PR description. If anything we should be encouraging PR authors to do that, in my opinion. I think if you look through the discussion, you'll see there's an abundance of confusion about which MIP this PR is even meant to address. I hope pushing code like this without linking to an MIP is a practice we can avoid going forward, in favor of proper software engineering practices.