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

Enables transfers of balance proportions > 32 ETH #936

Merged
merged 14 commits into from
Apr 23, 2019
Merged

Enables transfers of balance proportions > 32 ETH #936

merged 14 commits into from
Apr 23, 2019

Conversation

CarlBeek
Copy link
Contributor

@CarlBeek CarlBeek commented Apr 16, 2019

As per discussions around the new balance schemes, this PR allows the proportion of a validator's balance that is > 32 ETH to be transferred.

This stems from my insistence that validators receive compounding growth. If this was not the case, I argued that validators would be incentivised to transfer frequently causing frequent validator set changes. I argue this point fully in the recently closed #730.

The rationale behind this PR is to instead embrace transfers and to leverage them to provide the growth functionality we desire. By allowing validators to instantly transfer any portion of the balance that is > 32 ETH, validators can simulate the desired growth profile through repeated transfers. This has the the enhancement of improving the liquidity of validators' stakes allowing them to syphon off the rewards they accrue to either instantiate new validator instances or to be put into alternate Defi instruments. This has the further enhancement of removing the necessity of enforcing maximum deposit sizes. :)

@hwwhww
Copy link
Contributor

hwwhww commented Apr 17, 2019

  • Do we want to also enable initial deposit > 32 ETH in deposit contract? I remember the answer is no.
  • If we still want to restrict that initial deposit <= 32 ETH in contract, then we need to (i) keep the constant MAX_DEPOSIT_AMOUNT in spec or (ii) maybe for specifically, add MAX_INITIAL_DEPOSIT_AMOUNT.

@JustinDrake
Copy link
Collaborator

Do we want to also enable initial deposit > 32 ETH in deposit contract?

Now that we have transfers in phase 1 I don't think it's that bad :) Also, unlimited deposits mean that we may not need another contract to send Eth1 ETH to shards.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

  • Fixed a minor bug (get_balance(transfer.sender) -> get_balance(state, transfer.sender)
  • added a bunch of transfer tests

Can you add a bit more rationale to this PR for implementers?

@CarlBeek
Copy link
Contributor Author

Can you add a bit more rationale to this PR for implementers?

Absolutely! My bad for not including it from the outset.
This stems from my insistence that validators receive compounding growth. If this was not the case, I argued that validators would be incentivised to transfer frequently causing frequent validator set changes. I argue this point fully in the recently closed #730.

The rationale behind this PR is to instead embrace transfers and to leverage them to provide the growth functionality we desire. By allowing validators to instantly transfer any portion of the balance that is > 32 ETH, validators can simulate the desired growth profile through repeated transfers. This has the the enhancement of improving the liquidity of validators' stakes allowing them to syphon off the rewards they accrue to either instantiate new validator instances or to be put into alternate Defi instruments. This has the further enhancement of removing the necessity of enforcing maximum deposit sizes. :)

@djrtwo
Copy link
Contributor

djrtwo commented Apr 19, 2019

This needs an associated PR allowing deposits > MAX_EFFECTIVE on the deposit contract before merge

@CarlBeek
Copy link
Contributor Author

This needs an associated PR allowing deposits > MAX_EFFECTIVE on the deposit contract before merge

This is not strictly needed. The deposit contract just asserts the deposit is <=MAX_EFFECTIVE, but everything will work without this change. That said, I'll make the deposit contract PR now.

@hwwhww
Copy link
Contributor

hwwhww commented Apr 19, 2019

@CarlBeek

This is not strictly needed. The deposit contract just asserts the deposit is <=MAX_EFFECTIVE, but everything will work without this change.

It needs deposit contract updates since you changed:

Every Ethereum 1.0 deposit, of size between MIN_DEPOSIT_AMOUNT and MAX_DEPOSIT_AMOUNT

to

Every Ethereum 1.0 deposit, of size at least MIN_DEPOSIT_AMOUNT


@JustinDrake

unlimited deposits mean that we may not need another contract to send Eth1 ETH to shards.

I remember the original reasons that we bound it are:

  1. Simple: having a clear condition if deposit_amount == MAX_DEPOSIT_AMOUNT to trigger the full_deposit_count increment .
  2. UX and fat-fingered proof: no chance to top-up 320 ETH
  • I suppose we can loose condition to amount >= MAX_EFFECTIVE_BALANCE for (1).
  • The deposit contract was designed to be as simple as possible (compare to SMC and Casper FFG contract). Do we want it to be the phase 2 contract as well? I'm not against it, just wondering. :)

@JustinDrake
Copy link
Collaborator

JustinDrake commented Apr 19, 2019

having a clear condition if deposit_amount == MAX_DEPOSIT_AMOUNT to trigger the full_deposit_count increment

The condition can become if deposit_amount >= MAX_DEPOSIT_AMOUNT, right? [Edit: Oh, I see you suggested that 😄. ]

UX and fat-fingered proof: no chance to top-up 320 ETH

With transfers fat fingers are not too bad. Whoever deposited 320 ETH can do the following:

  1. Sell the fat-finger BETH for ETH (e.g. on an exchange) using a transfer.
  2. Make 1 ETH deposits to register new validators and transfer them each 31 BETH for activation.

So the fat-finger BETH is definitely not unrecoverable or stuck with transfers.

Do we want it to be the phase 2 contract as well?

I think we have our cake (simple deposit contract) and eat it too (use it for phase 2).

@djrtwo
Copy link
Contributor

djrtwo commented Apr 19, 2019

I think we have our cake (simple deposit contract) and eat it too (use it for phase 2).

not sure we can commit to this. The deposit contract has no notion of a destination shard and also requires a minimum of 1eth deposit. We should have a mechanism to reduce dust transfers but 1eth seems very high for a user transfer.

@djrtwo djrtwo merged commit 7b7b867 into dev Apr 23, 2019
@djrtwo djrtwo deleted the carl-patch-1 branch April 23, 2019 18:24
@@ -347,7 +347,7 @@ def get_valid_transfer(state, slot=None, sender_index=None, amount=None, fee=Non
fee=fee,
slot=slot,
pubkey=transfer_pubkey,
signature=EMPTY_SIGNATURE,
signature=ZERO_HASH,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong. This line should go away.

hwwhww added a commit that referenced this pull request Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling compounding growth for validators' stakes
4 participants