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

Improved transfer replay protection, and removal of transfers in phase 0 #1274

Closed
JustinDrake opened this issue Jul 5, 2019 · 16 comments
Closed
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets

Comments

@JustinDrake
Copy link
Contributor

JustinDrake commented Jul 5, 2019

Replace the current slot-based transfer replay protection mechanism by adding state.transfer_roots: Vector[Hash, MAX_TRANSFERS * SLOTS_IN_REPLAY_PROTECTION_PERIOD]. We can also completely move transfers from phase 0 to phase 1.

@JustinDrake JustinDrake added the post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets label Jul 5, 2019
@vbuterin
Copy link
Contributor

vbuterin commented Jul 7, 2019

So the only substantive thing here is removing transfers from the block structure, right?

@vbuterin
Copy link
Contributor

vbuterin commented Jul 7, 2019

It's also worth noting that this mechanism could be reused in phase 2 to prevent replay of withdrawals from execution environments into other execution environments, though in that case SLOTS_IN_REPLAY_PROTECTION_PERIOD would need to be really long for safety (a year?).

@djrtwo
Copy link
Contributor

djrtwo commented Jul 9, 2019

Do you still have to specify a slot or epoch?

I'm not certain what problem this is actually solving. If a validator has a Transfer for a slot and it is not included in a chain and then broadcasts a repeat for slot + N, then both of these can still be replayed on a reorged chain even though the initial transfer was not included in the pre-reorged chain.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 9, 2019

Or are you suggesting removing slot from Transfer? If this is the case, then I can just replay a transfer after the replay protection period.

@JustinDrake
Copy link
Contributor Author

Do you still have to specify a slot or epoch?

Yes.

@JustinDrake
Copy link
Contributor Author

Or are you suggesting removing slot from Transfer?

As discussed on our call, slot stays there (or can be replaced with epoch).

@djrtwo
Copy link
Contributor

djrtwo commented Jul 9, 2019

So is the only thing this solving the ability to expand it to epoch instead of slot?

@JustinDrake
Copy link
Contributor Author

If a validator has a Transfer for a slot and it is not included in a chain and then broadcasts a repeat for slot + N

The point is that the re-broadcast would not have to modify slot + N, hence removing the risk of having two transfers accidentally go through.

@JustinDrake
Copy link
Contributor Author

JustinDrake commented Jul 9, 2019

So is the only thing this solving the ability to expand it to epoch instead of slot?

It expands slot to SLOTS_IN_REPLAY_PROTECTION_PERIOD. The intent is that SLOTS_IN_REPLAY_PROTECTION_PERIOD is much larger than SLOTS_PER_EPOCH.

@protolambda
Copy link
Contributor

We can also completely move transfers from phase 0 to phase 1.

Then we move it to a new phase-1 beacon-chain updates document? Or will it live with the updated beacon-block body definition in the custody game doc?

@JustinDrake
Copy link
Contributor Author

Then we move it to a new phase-1 beacon-chain updates document?

A new phase 1 beacon chain update document makes sense. (Mixing transfers with the custody game is a bit awkward.) My guess is longer term the miscellaneous update documents will be collapsed into a unified beacon chain state transition document.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 10, 2019

I'm thinking about organizing specs into new sub-directories (will break links... :/). These directories would be grouped by fork and ordered. This would open up easily putting transfer into it's own fork defined as a beacon chain file in forks/

That said, don't want to do this surgery until we discuss more

@djrtwo
Copy link
Contributor

djrtwo commented Jul 10, 2019

I'm still pro doing a transfer-enabling fork for practice independent of phase 1 fork

@protolambda
Copy link
Contributor

Another replay case (although much rarer) is when validators change index after an Eth 1 re-org. Credits to @sorpaas for surfacing it and bringing it to our attention on discord.
A transfer could then be replayed (or re-used in a clean but unexpected state really...) to send the funds to the new validator now taking the receiver index.

This may be solved by modifying the other replay protection measures. Or we replace the receiver index with the receiver pubkey. And, as mentioned by Justin. When we have eth2 -> eth2 deposits, indices can get much more unstable, and transfers will need this replay protection. Something to keep in mind when revisiting the transfer design.

@JustinDrake
Copy link
Contributor Author

@protolambda My favourite conservative fix is to replace indices in transfers by pubkeys.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 12, 2019

transfers removed!

@djrtwo djrtwo closed this as completed Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets
Projects
None yet
Development

No branches or pull requests

4 participants