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.

Sign up for GitHub

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

Adapt fees to declared effective balance #52

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Adapt fees to declared effective balance #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
837331e
adapt fees to decalred effective balance
GalRogozinski Sep 18, 2024
676547a
contracts:track maxeb per cluster
mtabasco Oct 22, 2024
e9fc194
update node
GalRogozinski Oct 23, 2024
6d17ea5
introduce fee per ETH managed
mtabasco Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions all.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
## All SIPS

| SIP # | Title | Status |
|---------------------------------------|-----------------------------|--------|
| [1](./sips/dkg.md) | DKG | open-for-discussion |
| [2](./sips/msg_struct_encoding.md) | Message struct and encoding | open-for-discussion |
| [3](./sips/qbft_sync.md) | QBFT Sync | open-for-discussion |
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| [6](./sips/constant_qbft_timeout.md) | Constant QBFT timeout | open-for-discussion |
| [7](./sips/fork_support.md) | Fork Support | open-for-discussion |
| [8](./sips/pre_consensus_livness.md) | Pre-Consensus livness fix | open-for-discussion |
| [9](./sips/partial_signature_verification_aggregation.md) | Partial Signature Verification Aggregation | spec-merged |
| [10](./sips/qbft_drop_redundant_bls.md) | Drop redundant BLS in QBFT | spec-merged |
| [11](./sips/eliminate_bls.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [12](./sips/topic_by_committe_id.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [13](./sips/cluster_consensus.md) | Cluster-based consensus | spec-merged |
| SIP # | Title | Status |
| --------------------------------------------------------- | ------------------------------------------------------ | ------------------- |
| [1](./sips/dkg.md) | DKG | open-for-discussion |
| [2](./sips/msg_struct_encoding.md) | Message struct and encoding | open-for-discussion |
| [3](./sips/qbft_sync.md) | QBFT Sync | open-for-discussion |
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| [6](./sips/constant_qbft_timeout.md) | Constant QBFT timeout | open-for-discussion |
| [7](./sips/fork_support.md) | Fork Support | open-for-discussion |
| [8](./sips/pre_consensus_livness.md) | Pre-Consensus livness fix | open-for-discussion |
| [9](./sips/partial_signature_verification_aggregation.md) | Partial Signature Verification Aggregation | spec-merged |
| [10](./sips/qbft_drop_redundant_bls.md) | Drop redundant BLS in QBFT | spec-merged |
| [11](./sips/eliminate_bls.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [12](./sips/topic_by_committe_id.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [13](./sips/cluster_consensus.md) | Cluster-based consensus | spec-merged |
| [14](./sips/adapt_fees_to_declared_effective_balance.md) | open-for-discussion |
9 changes: 5 additions & 4 deletions contracts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## Core

| SIP # | Title | Status |
|-------------------------------|-----------------------------|--------|
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| SIP # | Title | Status |
| -------------------------------------------------------- | ---------------------- | ------------------- |
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| [14](./sips/adapt_fees_to_declared_effective_balance.md) | open-for-discussion |
30 changes: 16 additions & 14 deletions core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
## Core

| SIP # | Title | Status |
|------------------------------------|-----------------------------|--------|
| [1](./sips/dkg.md) | DKG | open-for-discussion |
| [2](./sips/msg_struct_encoding.md) | Message struct and encoding | open-for-discussion |
| [3](./sips/qbft_sync.md) | QBFT Sync | open-for-discussion |
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| [6](./sips/constant_qbft_timeout.md) | Constant QBFT timeout | open-for-discussion |
| [7](./sips/fork_support.md) | Fork Support | open-for-discussion |
| [8](./sips/pre_consensus_livness.md) | Pre-Consensus livness fix | open-for-discussion |
| [9](./sips/partial_signature_verification_aggregation.md) | Partial Signature Verification Aggregation | spec-merged |
| [10](./sips/qbft_drop_redundant_bls.md) | Drop redundant BLS in QBFT | spec-merged |
| [11](./sips/eliminate_bls.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [13](./sips/cluster_consensus.md) | Cluster-based consensus | spec-merged |
| SIP # | Title | Status |
| --------------------------------------------------------- | ------------------------------------------------------ | ------------------- |
| [1](./sips/dkg.md) | DKG | open-for-discussion |
| [2](./sips/msg_struct_encoding.md) | Message struct and encoding | open-for-discussion |
| [3](./sips/qbft_sync.md) | QBFT Sync | open-for-discussion |
| [4](./sips/change_operator.md) | Change operators set | open-for-discussion |
| [5](./sips/ecies_share_encryption.md) | ECIES Share Encryption | open-for-discussion |
| [6](./sips/constant_qbft_timeout.md) | Constant QBFT timeout | open-for-discussion |
| [7](./sips/fork_support.md) | Fork Support | open-for-discussion |
| [8](./sips/pre_consensus_livness.md) | Pre-Consensus livness fix | open-for-discussion |
| [9](./sips/partial_signature_verification_aggregation.md) | Partial Signature Verification Aggregation | spec-merged |
| [10](./sips/qbft_drop_redundant_bls.md) | Drop redundant BLS in QBFT | spec-merged |
| [11](./sips/eliminate_bls.md) | Eliminate BLS out of QBFT and change message structure | spec-merged |
| [13](./sips/cluster_consensus.md) | Cluster-based consensus | spec-merged |
| [14](./sips/adapt_fees_to_declared_effective_balance.md) | open-for-discussion |

166 changes: 166 additions & 0 deletions sips/adapt_fees_to_declared_effective_balance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
| Author | Title | Category | Status | Date |
|-------|----------------|--------------------------------------|---------|--------------------|
| Gal Rogozinski & Marco Tabasco | Adapt fees to declared effective balance | Contracts & Core | open-for-discussions |18.09.2024 |


Copy link
Contributor

Choose a reason for hiding this comment

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

what bothers me with this approach is that a user having thousands of validators will need to update each one independently ..

Post pectra all validators start to compound above 32 automatically? if so it will be a nightmare from UX POV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the contract owner (DAO) allows a deviation in percentage. So that the maxEffectiveBalanceUpdate won't have to be too frequent.
There is already a similar UX issue with SSV top ups. The UI can combine them into one UX issue.
It can demand an amount of SSV that should be enough until the user will have to do the next balance update.

If we don't go with this solution we need to think about something smarter like estimating the effective balance compounding (I have a draft for it) or using oracles.

In a conversation with @liorrutenberg we decided we should push the simplest solution.
We can try to dive into the complicated ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be also that updating clusters instead of validators will simplify things

## Summary

Creates a mechanism to incentivize users to record the correct balance of their validator with the contract.
This balance will be used for operator and network fee calculations.

## Motivation

Operators on SSV Network charge validators a fee based on an expected workload. The expected workload is proportional to the the effective balance a validator has. Currently the effective balance is assumed to be a constant 32 ETH by the contract. With the introduction of [EIP-7251](https://eips.ethereum.org/EIPS/eip-7251) the effective balance of a validator could be as high as 2048 ETH.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The workload won't increase linearly in proportion to eff balance... so maybe this should be rephrased


The same reasoning applies for the network fee charged to each validator registered.


## Rationale

Do not perform duties if the validator's effective balance is greater by more than a deviation configured by the contract's owner. This will incentivize validators to always register the correct amount on the contract. Effective balance may grow due to compounding rewards and it is the user's responsibility to update their effective balance before their operators pause. Paused operators will resume when the effective balance is updated on the contract.

## Specification

### Contract
When registering new validators, add a new parameter to indicate the current effective balance of the validator being added. This will be used to update operators' [snapshots](https://github.com/ssvlabs/ssv-network/blob/583b7b7cb1c1abc5d4c3b13bafca59bf315113b6/contracts/interfaces/ISSVNetworkCore.sol#L10) with the right amounts, calculate the network fee applied to the validator, and emit the event with this information to allow the node confirm it.


#### Adapted functions

```solidity
function registerValidator(
bytes calldata publicKey,
uint64[] memory operatorIds,
bytes calldata sharesData,
uint256 ethEffectiveBalance, \\ This is new
uint256 amount,
Cluster memory cluster
) external
```
```solidity
function bulkRegisterValidator(
bytes[] memory publicKeys,
uint64[] memory operatorIds,
bytes[] calldata sharesData,
uint256[] ethEffectiveBalance, \\ This is new
uint256 amount,
Cluster memory cluster
) external
```



#### Adapted event
```solidity
event ValidatorAdded(
address indexed owner,
uint64[] operatorIds,
bytes publicKey,
bytes shares,
uint256 ethEffectiveBalance, //This is new
Cluster cluster);
```

### New functions

```solidity
// Function to update the effective balance for a set of validators
function updateEthEffectiveBalance(
bytes[] memory publicKeys,
uint64[] memory operatorIds,
uint256[] newEffectiveBalance,
Cluster memory cluster
) external
```
```solidity
// Function to update the maximum deviation allowed for the ETH
// balance declared by validator owners
// @dev Using 10000 to represent 2 digit precision: 100% = 10000, 10% = 1000
// @dev Only the owner of the contract can perform the update
function updateAllowedBalanceDeviation(uint64 percentage) external onlyOwner
```

```solidity
// Function to get the maximum ETH balance deviation
Copy link
Contributor

Choose a reason for hiding this comment

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

does it really need to be stored on chain? or can nodes simply parse it from events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

events sounds good to me

@mtabasco wants to comment?

Copy link

Choose a reason for hiding this comment

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

Yeah, if there is no major UX issue removing getAllowedBalanceDeviation(), the ETH balance deviation can be emitted.

function getAllowedBalanceDeviation()
external view returns(uint64 percentage)
```


### New events

```solidity
event ValidatorEffectiveBalanceUpdated(
address indexed owner,
uint64[] operatorIds,
bytes publicKey,
uint256 newEffectiveBalance,
Cluster cluster);
```
```solidity
event AllowedBalanceDeviationUpdated(uint64 percentage);
```

### Current implementation

Operator earnings formula since snapshot:


`(currentBlock - snapshotBlock) * operatorFee * operatorValidatorCount`

### New implementation

Earning for last snpashot + fees for one block

`((currentBlock - snapshotBlock) * (snapshotTotalETH / 32) * operatorFee)`

Where`snapshotTotalETH`: The total ETH declared by the validators managed by this operator.


#### Network fee
##### Current implementation
`(currentBlock - snapshotBlock) * networkFee * totalValidatorCount`

##### New implementation
`(currentBlock - snapshotBlock) * (snapshotTotalETH / 32) * networkFee)`


#### User should always report effective balance when depositing SSVs

```solidity
function deposit(
address clusterOwner,
uint64[] calldata operatorIds,
uint256 amount,
uint256 newEffectiveBalance,
Cluster memory cluster
) external
```

### Node

Node should keep a table of the validator's effective balances reported by the contract's `AllowedDeviationUpdated` events.
This table is updated upon contract events reporting balance updates.

When fetching duties for a validator, query the Eth node for its balance in the `Finalized` state. If the difference between the true balance and the effective balance reported by the user is larger than the allowed deviation do not execute the duty.


```python
def skipDuties(valPK ValidatorPublicKey, deviation_percentage float)
true_balance = GET /eth/v1/beacon/states/finalized/validator_balances?valPK
reported_eff_balance = getFromEvent(valPK)
true_balance = min(2048 ETH, true_balance)
# Skip duties if this statement hold
return RoundDown(true_balance) - reported_eff_balance > deviation_percentage * reported_eff_balance
```

A committee consensus process should only be skipped if all paritciapting validators should be skipped.

Note: we use the actual balance and not effective balance since it is easy to query.

## Issues and alternatives

1. We do not track changing balance due to compounding... meaning users will pay a bit less then what they manage. So the deviation should be configured in a way that allow acceptable slippage while not degrading UX by forcing users to update theor balance too soon.
2. Are users incentivized to split validators? By splitting a 2048 eth validators to two validators, a validator enjoys compounding and pays a reduced fee.
3. There was a considered option to allow contract's owner to update the APR offered by Ethereum. Then estimate the correct fee in the presence of rewards compunding and allow a smaller balance deviation. This proposal favors simplicity.
4. Oracles can help calibrate the calculations of an exact fee... however using a 3rd party oracle will add more complexity and will probably be a paid service.