Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[review] MIP-74: Rate-Limiter for the Lock/Mint-type Native Bridge + MD-74: Rate-Limiter for the Lock/Mint-type Native Bridge #74
base: main
Are you sure you want to change the base?
[review] MIP-74: Rate-Limiter for the Lock/Mint-type Native Bridge + MD-74: Rate-Limiter for the Lock/Mint-type Native Bridge #74
Changes from 12 commits
cc0fe99
d168ef6
5a83b4a
a725ac4
046b70d
ac3efab
59c70ef
004ed90
dcad5e0
49be30c
003dc8a
7071a7c
ecbcdb9
88dd2e7
e91a3c9
bb95565
4a65007
9679333
f81f244
dc8cab3
76c1c3f
6ac10e6
0006da6
0f13196
5503feb
62def7a
2a83a53
17897df
846e67e
8034e18
f1ac661
eb38182
d367149
2fa892b
0a4b7e2
16cc7e0
ad96d14
e40537c
f434253
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe that we need to change the role that sets the rate limit to DEFAULT_ADMIN instead of RELAYER. We won't have access to setting limit because the relayer won't be accessible to us. There are other ADMIN functionalities that are at the same risk level. We can have a hot role for emergency PAUSER_ROLE. It could be assigned to all engineers so that anyone could pause initiate_bridge_transfer calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the Bridge Operator should also set the rate limit.
the relayer could read programmatically the required data from L1, and thus set the rate limit programmatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not also, it should be exclusively done by DEFAULT ADMIN, that's what I meant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
In Figure 1, It is not clear to me what "sets parameters" means vs "sets limit".
I would simplify the figure as follows:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk The figure has a few glitches:
I have added () around thr Native attribute in the text, so that may fix 1. but I have not modified the figure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated figure and text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to contradict that the relayer is trusted.
If it can be faulty, it is not trusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"partially trusted" ?
"trusted (to a large extend)" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
Would it makes sense to list the expected properties (as per above) and what they guarantee to the operator and the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is the "Risks and mitigation strategy" section. Do you think it is to convoluted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
Can you confirm this is correct?
And add anything you think is relevant to tis section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have added several points to the "Objectives" section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
The important part seems to be that the insurance funds (total) limits the rate.
Is it necessary to consider two sides the L1 fund and the L2 fund or could we collapse them into one (at the design level)?
I understand that if we need to compensate users we may need two pools, oneon L1 and one on L2, but this may be separate to the actual logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason there are two pools is that each direction needs to be covered by an own value. the total rate in both direction is limited by the total amount in the insurance fund. we cannot use the insurance fund to secure twice and thus if we want to secure X in each direction the insurance fund must be 2X (if it would be on one Level only)
we initially had it on one side and i think this was advocated for by @Primata and @l-monninger .
the governance operator decides about the amount in the insurance fund as well as the reaction time. thus in principle we could simply trust it and allow it to set the value directly. however it adds additional assumptions on that the governance operator does not make errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk So in principle, the values for pools on L1 and L2 should be the same? We split the total insurance fund into the two pools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we would want to have the same max rate in either direction, yes. But i dont think this is a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
I suggest to break this into two independent parts:
At the moment I find this section confusing with the Rate limier, the Operator etc so it may need to be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried to improve, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it the inetrval [0,1] or 0 or 1?
In smart contracts, it is usually not easy to use real numbers (Solidity/EVM does not support it) and MoveVM does but computation is bit more expensive if I am correct than integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk
We may need to justify why there is a need for a rate limit on the source chain.
Could we have an example fo what can happen?
My understanding was that it is needed because of delays between transfers from L2 -> L1 (or vice versa) so we need to know what is requested to bridge and what is completed (the sum of the two should be lower than the insurance funds).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have added a para
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rate limiter must react to chain based data, it is not possible for the source chain contract to know about the completed transactions nor the insurance fund on target. Thus we assume that clocks are correct and
requested transfers < completed transfers
AND trust that the operator updates on source the rate limit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk So it looks like there is a need for synchronising the rate limits on both the source and traget and the relayer can do that. We could use this sync mechanism to simplify the rate limiter I think.
I would assume that as long as less than$S$ is currently bridged over (pending to be completed) to L1 and $S \leq $ $k$ , is successfully completed we can decrease $S$ and update it to $S := S - k$ .
insurance_fund1
is currently "in-flight" we are OK, so we may try to measure that.Assume we are bridging from L2 to L1.
If we know that an amount
insurance_fund1
then we can cover the loss (there is no need for a time window).To know that this the case, we need to keep track of the completed Txs and another conponent can do that (or the Relayer).
When a bridge Tx from L2 to L1, anount
On another note, I am a bit confused about the wording/naming here. Why do we need a rate?
If we have a unique Tx with the maximum amount
insurance_fund1
we should first clear this Tx before we continue to process new ones.Would it be the role of the Informer to do this kind of things (syncing L2 <-> L1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed above with @Primata sliding windows or reseting budget based on completing transactions seems out of scope for this version. I suggest we move more complicated versions over to a new MIP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth noting:
There is a valid case where the relayer should set the rate limit on the source chain: if the operator fails to adjust the rate limit on the source chain when ajusting the rate limit on the target chain (by changing insurance fund or reaction time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main thing for the relayer to implement is holding transaction until it's able to complete on target chain.
in-flight
value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the rate limit on the source chain is much larger than on the target chain, it is could be much more than 24h.
but yes, the relayer would need to be able to hold. or in a simple implementation he would just retry continuously which is handled by MIP-61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is not clear to me is how the time window is updated. And is it a sliding window? e.g the last 40 txs? or the last 10 minutes? Or the last ten blocks?
I would think that a per Tx window makes sense, does it? e.g., every 1000 tx we go to w new time window/interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets assume the simplest non-optimized case first : fixed timewindows, lets call them epochs. then (if not too expensive) every initiate transaction checks and resets the budget IF it is in the new epoch. would this work?
a sliding window seems more difficult. the easiest and most gas efficient may be that we approximate a sliding window. E.g. split the epoch into smaller parts, but maybe not the most elegant approach. We are restricted by the user having to update the bridge contract state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easiest and cheapest implementation is, taking the block timestamp. Divide it by 24hrs and you get a number, that is your time allocation to be checked against. IF we change the 24hrs fixed value it leads to an issue with time allocation, that's why I'd advise against it because to fix it we need some extra operations and logic that might lead to unexpected behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with @Primata and suggest we leave sliding windows and such for future MIPs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes to use MIP-58's design C, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i have moved the moved the alternative designs described in MIP-58 to the appendix here and added you consequently as an author.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apenzk I am not sure we need to do anything with the Relayer. If a transaction is not processed within a given amount of time (for any reason, network down etc), the Relayer should re-send it to ensure it evetually succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to avoid excessive costs to the relayer incase the budget is consumed on the target chain but is not consumed on the source chain. i have removed it as it is an optimisation that we can handle once we discover it actually is an issue.