Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1668 +/- ##
==========================================
+ Coverage 81.58% 82.84% +1.25%
==========================================
Files 22 23 +1
Lines 994 1067 +73
Branches 184 197 +13
==========================================
+ Hits 811 884 +73
Misses 166 166
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Cool. I assume that initially we would top up the wrapper contract ourselves, and that we would only refund the BEEFY relayer for the transaction cost of the consensus update. We may also want to allow end users to tip for a message that is included in a relay-chain block but has not yet been finalized by BEEFY. The tip storage could be a map from I assume this would be the main incentive mechanism for the consensus relay: relayers receive additional rewards from end users only when there is a message to relay. |
|
I think this can be done way simpler. You just need a refund target, which is a number in blocks, say 300 blocks (30 minutes). You always refund, but scale it according to how much progress the relayer made. If a relayer chooses to relay every 30 blocks, they will only get 10% of gas returned as refund. If a relayer waits 300 blocks or more, it gets 100% of its gas as refund. There is no round robin, relayers must monitor open tickets and see if they will get a refund based on the pending tickets. If there is a race condition where two relayers compete, this scaled refund makes sure each one gets paid a portion based on progress made. Remember relayers can abandon tickets if there is a race condition where another relayer competes with them. This same idea can be extended to rewards/tips. You could choose a reward target, which is a number in blocks, say 2400 (4 hours). You refund as per the above logic until 300 block refund target, for every block over, up untill 2400 you scale by amount of blocks progressed and pay a portion of the reward. So if a relayer makes 600 blocks progress, you give them a full refund for meeting the refund target (300/300), and then a 12.5% portion of the reward (300/2400). |
|
@alistair-singh this sounds cool, and is simpler. Relayers might frequently lose gas spent on submitInitial, if more than one relayer submits at the same time. What do you think about this? And do we remove the relayer whitelist then? |
contracts/src/BeefyClientWrapper.sol
Outdated
| * @dev Forwards BeefyClient submissions and refunds gas costs to whitelisted relayers. | ||
| * Implements soft round-robin scheduling to prevent competition. | ||
| */ | ||
| contract BeefyClientWrapper is IInitializable, IUpgradable { |
There was a problem hiding this comment.
Not sure why we need proxy pattern and upgradable here. We may need it, but we can get away with pointing the wrapper at the Gateway contract instead of the Beefy contract. It can read the current beefy client from the Gateway. This means when we upgrade the Beefy client on the gateway, the wrapper is automatically picks up the new Beefy Instance.
Since this is a wrapper we should also strive to rather throw it away and deploy a new wrapper if we ever have to upgrade it, avoiding forms of governance if possible. We would only really need to migrate any Eth held by the contract. So Proxy Pattern seems to heavy for this requirement.
There was a problem hiding this comment.
I didn't have a proxy pattern at first, but because this contract holds state I figured it would be better, esp if we want to change some of the config. I don't have a strong opinion about this either way.
There was a problem hiding this comment.
Removed proxy.
contracts/src/BeefyClientWrapper.sol
Outdated
| error AlreadyInitialized(); | ||
| error TicketAlreadyActive(); | ||
|
|
||
| address public owner; |
There was a problem hiding this comment.
Who is the owner? Does our team own this key?
There was a problem hiding this comment.
Yeah, could be our SAFE multisig.
There was a problem hiding this comment.
We should make this permissionless and stateless. See https://www.notion.so/Relayer-Decentralization-Strategy-2e8296aaabef8031af9ff87151bd3d5c?d=2e9296aaabef8079a230001cb2543324&source=copy_link#2e9296aaabef80bd9b23d3fe2b62d7d6
There was a problem hiding this comment.
The tradeoff is that deposited funds in the contract can be never be withdrawn. Is this OK? Say we deposit funds and then discover a bug, we can move the funds to a new contract. Seems less than ideal to me. It does need some state, for example, to capture the gas spent in a session.
There was a problem hiding this comment.
I’d expect the funds deposited in this contract to be very limited. Given the low gas prices these days, even a small initial deposit can last a long time, so we wouldn’t need to recharge it frequently.
Even if the contract were somehow exploited, we could simply abandon it and deploy a new, fixed version. The state impact would be limited to the ongoing session, which is also acceptable.
Ideally, we’d like to remove all permission-related calls if possible.
I think it might happen occasionally, but we need some system where we can tune the parameters to encourage all the relayers to pipeline consensus updates as opposed to use manually configuring and whitelisting relayers. I dont think it needs to be a perfect system either, we must trust relayers to act in their best interest. At worst, they will abandon the ticket and post another. This edge case is something that can be addressed in the wrapper as well, by reverting early if some condition is not met. For example you can do something like compare and swap operation. e.g. The current latest ticket open is for block X, I want to relay block X+300, assuring my 100% refund, when I submit initial to claim a ticket via the wrapper i submit both X and X+300, and the submit initial in the wrapper can revert if X has changed onchain. So it still costs, but less than submit initial. The only time X will change onchain is if two relayers submit and it gets included in the same block. We can introduce random in the relayer to offset this in practise, so generate a random number N between 1-20, and the relay block X+300+N. So relayers never try to relay at the exact interval for example. We could even do something like we do for message relayers and co-ordinate round robin offchain. |
Agreed. If the goal is simply to have multiple BEEFY relayers cooperate to update the light client, then on-chain or off-chain round-robin (RR) coordination may not be necessary. As we’ve observed with the Flashbots RPC, multiple relayers can submit updates concurrently, and only one will succeed—the others will fail without incurring any fees. This effectively provides a form of round-robin behavior at the RPC level. |
contracts/src/BeefyClientWrapper.sol
Outdated
| event FundsDeposited(address indexed depositor, uint256 amount); | ||
| event FundsWithdrawn(address indexed recipient, uint256 amount); |
There was a problem hiding this comment.
Perhaps these are not necessary.
There was a problem hiding this comment.
Agree, I don't think these events are nessessary.
| function submitFiatShamir( | ||
| IBeefyClient.Commitment calldata commitment, | ||
| uint256[] calldata bitfield, | ||
| IBeefyClient.ValidatorProof[] calldata proofs, | ||
| IBeefyClient.MMRLeaf calldata leaf, | ||
| bytes32[] calldata leafProof, | ||
| uint256 leafProofOrder | ||
| ) external { | ||
| beefyClient.submitFiatShamir(commitment, bitfield, proofs, leaf, leafProof, leafProofOrder); | ||
|
|
||
| // Clear highest pending block if light client has caught up | ||
| if (beefyClient.latestBeefyBlock() >= highestPendingBlock) { | ||
| highestPendingBlock = 0; | ||
| highestPendingBlockTimestamp = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why we aren't refunding for Fiat-Shamir?
There was a problem hiding this comment.
I thought the user covers the cost of the transaction in a separate flow?
There was a problem hiding this comment.
We might consider switching to Fiat–Shamir as the default update mechanism in future. If that’s the case, we would need to add refund support for it.
There was a problem hiding this comment.
Refunding Fiat Shamir while an interactive protocol session is on-going will cause the interactive protocol relayer to forfeit their gas refund. Is that alright? Otherwise we should refund both the Fiat Shamir and the interactive protocol relayer for submissions up to that point, and then clear they ticket?
There was a problem hiding this comment.
A relayer could game this by submitting a SubmitInitial and then a FiatShamir though, and get refunded for both.
There was a problem hiding this comment.
I’d prefer to keep things simple and not include 8b8ec69. In my opinion, refunding an unfinished interactive update seems unnecessary.
contracts/src/BeefyClientWrapper.sol
Outdated
| error AlreadyInitialized(); | ||
| error TicketAlreadyActive(); | ||
|
|
||
| address public owner; |
There was a problem hiding this comment.
We should make this permissionless and stateless. See https://www.notion.so/Relayer-Decentralization-Strategy-2e8296aaabef8031af9ff87151bd3d5c?d=2e9296aaabef8079a230001cb2543324&source=copy_link#2e9296aaabef80bd9b23d3fe2b62d7d6
contracts/src/BeefyClientWrapper.sol
Outdated
| uint256 totalGasUsed = currentGas + previousGas; | ||
| uint256 effectiveGasPrice = tx.gasprice < maxGasPrice ? tx.gasprice : maxGasPrice; |
There was a problem hiding this comment.
Calculation is incorrect. I assumes that the gasprice for the submitInitial is the same as the gas price for the submitFinal. You need to snapshot the previousGas and gasPrice at time of submitInitial
contracts/src/BeefyClientWrapper.sol
Outdated
| uint256 public highestPendingBlockTimestamp; | ||
|
|
||
| constructor( | ||
| address _beefyClient, |
There was a problem hiding this comment.
What happens when the beefyClient/gateway is updated? We should rather get the beefyClient address from the Gateway by initializing the Wrapper with the GatewayProxy address, that way after an upgrade, this automatically points to the new implementations.
| function _refundWithProgress(uint256 startGas, uint256 previousGas, uint256 progress) internal { | ||
| if (progress < refundTarget) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This means that if another relayer relays a block such that my relayer does not make enough progress, that relayer is "rewarded" with a 100% refund, and my relayer is punished by receiving a 0% refund. It is better the scale the refund by the progress made and refund each relayer accordingly. This way the other relayer is also punished by not receiving the 100% refund.
There was a problem hiding this comment.
I moved this to off-chain checking, the relayer checks if there is a session is progress and if their update will be eligible for the refund. If not, they don't start the session. I think this is simpler, and moves the complexity off-chain.
There was a problem hiding this comment.
Happy with this being offchain. We should do the same check on submitInitial to avoid race condition and save cost for relayers.
| * Credited cost is forfeited when clearing a ticket. | ||
| */ | ||
| function clearTicket(bytes32 commitmentHash) external { | ||
| if (ticketOwner[commitmentHash] != msg.sender) { |
There was a problem hiding this comment.
There is no real incentive for a relayer to call this method if the credited cost is forfeit. So maybe remove this method completely or credit the relayer. If we revert on submitInitial if the initial does not make enough progress, then we also dont need this method.
| } | ||
|
|
||
| if (refundAmount > 0 && address(this).balance >= refundAmount) { | ||
| (bool success,) = payable(msg.sender).call{value: refundAmount}(""); |
There was a problem hiding this comment.
This is stateful, it means we have to hold funds in this contract which means if we abandon it we also abandon the funds in it.
This may be ok though if its a small amount, like if we are only moving $ 100 and we top it up in small increments. But we need to take care because funds in this contract are essentially blackholed. Current prices are like $ 0.06 per submitInitial/submitFinal cycle, so $100 is 166 cycles at current price.
There was a problem hiding this comment.
Yes, that is intended, I am not really sure what else we can do? The options are:
- Stateful contract owner (can move funds if we deploy a different contract)
- Proxy pattern (to upgrade)
- No owner, but needs to hold funds (because the whole point of this contract is to refund)
I understood we want the latter.
| uint256[] calldata bitfield, | ||
| IBeefyClient.ValidatorProof calldata proof | ||
| ) external { | ||
| uint256 startGas = gasleft(); |
There was a problem hiding this comment.
Since we are not payout refunds on clearTickets (abandoned submitInitials) and we are not scaling refunds based on progress, We should revert here early if its under the minimum block progress. That way in a race condition where two relayers submit, one will fail atleast with minimum cost to itself.
submitInitial costs like 130k gas, plus storage costs for ticket, credit etc. So I think its quite substantial.
| interface IBeefyClient { | ||
| /* Types */ | ||
|
|
||
| struct PayloadItem { | ||
| bytes2 payloadID; | ||
| bytes data; | ||
| } | ||
|
|
||
| struct Commitment { | ||
| uint32 blockNumber; | ||
| uint64 validatorSetID; | ||
| PayloadItem[] payload; | ||
| } | ||
|
|
||
| struct ValidatorProof { | ||
| uint8 v; | ||
| bytes32 r; | ||
| bytes32 s; | ||
| uint256 index; | ||
| address account; |
There was a problem hiding this comment.
We’re duplicating types here with BeefyClient. It might be better to extract the shared types, or alternatively, I’d prefer the wrapper to import BeefyClient directly rather than adding a new interface.
| if (ticketOwner[commitmentHash] != address(0)) { | ||
| revert TicketAlreadyOwned(); | ||
| } |
There was a problem hiding this comment.
This check is necessary to prevent race conditions. However, there is a scenario where a malicious relayer could submit submitInitial but never complete the second phase (submitFinal).
In that case, an honest/canonical relayer should ideally be smart enough to skip this commitment and move on to a more recent one. Alternatively, we could include timing information so that expired tickets can be overwritten after a timeout period.
For example:
struct PendingTicket {
address owner;
uint256 creditedCost; // needed for refund calculation
uint32 blockNumber; // optional, from commitment; helps relayers query
uint64 createdAt; // optional, block.timestamp when submitInitial was executed, useful for timeout-based handling
}
mapping(bytes32 => PendingTicket) public pendingTickets;
This would allow timeout-based handling and prevent stalled tickets from blocking progress indefinitely.
SubmitInitial,CommitRandao,SubmitFinal) once a successfulSubmitFinalhas been submitted.highestPendingBlockin the wrapper contract if a submission would result in a gas refund. If thehighestPendingBlockis stale for 40 minutes, submit a new session.Resolves: SNO-1668