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

Flaw in transfer #4

Closed
holiman opened this issue Apr 10, 2019 · 7 comments
Closed

Flaw in transfer #4

holiman opened this issue Apr 10, 2019 · 7 comments

Comments

@holiman
Copy link
Contributor

holiman commented Apr 10, 2019

This may totally be a false positive, not sure. However, in block_transfer.go

Verify that we have enough ETH to send, and that after the transfer the balance will be either
exactly zero or at least MIN_DEPOSIT_AMOUNT

I think that can be violated. If I have transfer.Amount + transfer.Fee on account A, and transfer transfer.Amount (let's say 1) to B, then I should have 0 remaining. However, If A==B, then that will leave A with 1, after state.IncreaseBalance(transfer.Recipient, transfer.Amount).

Not sure if that's checked anywhere else?

@protolambda
Copy link
Owner

We need the sender to either have:

  • sufficient balance remaining to keep doing work without having to alter state. (beacon.MIN_DEPOSIT_AMOUNT)
  • exactly zero balance

Not sure if I understand you correctly, but we have 2 accounts, A and B, and A sends 1 to B, then A will have MIN_DEPOSIT_AMOUNT remaining, or exactly 0. If it's something else, an error is thrown, and the transfer is denied.

Also, I know transfers look funky. Especially with the removal of nonces (we use 3 primitives to make nonces unnecessary: dedicated slot + transfer uniqueness in a block + one block per slot).

If there are any inconsistencies with the ZRNT logic compared to the spec, I'm happy to continue discussion here. If not, then let's discuss in an issue on the specs repo.

@holiman
Copy link
Contributor Author

holiman commented Apr 10, 2019

So the case is when you send from A to A. Then it will have 1, neither 0 nor MIN_DEPOSIT_AMOUNT.

@protolambda
Copy link
Owner

@holiman FYI: ZNRT is still in a relatively early phase. Most of my time is spent on spec YAML test generation, and optimized implementations of advanced but isolated features (fork choice, shuffling, ssz implementations). When I do have the time to update ZRNT, it's relatively quick. In ~week we should have state-transition YAML tests from my spec work, and I can start making ZRNT stable and well tested.

@protolambda
Copy link
Owner

@holiman Also, let me know if you want to contribute in any way, or build on top of ZRNT, we can figure something out within EF to align projects.

@protolambda
Copy link
Owner

A to A is a good catch! And I see a possible similar case where A is the proposer and receives a fee.
Yes, probably needs to be fixed. But I heard that transfers may not make it in phase 0, and removed soon.
I'll ask around with research people here in Sydney what their plan is.

Are you in Sydney as well, or maybe up for a call about Go ETH 2.0 plans?

@holiman
Copy link
Contributor Author

holiman commented Apr 10, 2019

Are you in Sydney as well, or maybe up for a call about Go ETH 2.0 plans?

Unfortunately not :(
Regarding future plans, it makes sense to me to base off of zrnt, but at this point I'm still looking/learning. cc @karalabe @fjl @rjl493456442

@protolambda
Copy link
Owner

protolambda commented Apr 14, 2019

Reviewed this again after discussing and thinking it was right (there's many other conditions in there), and you're right, there was a bug after all. Thanks for pointing it out! Fixed in specs, and in ZRNT (commit 167aacd )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants