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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Domain for deposit signatures #3849

Closed
paulhauner opened this issue Oct 24, 2019 · 10 comments

Comments

@paulhauner
Copy link

@paulhauner paulhauner commented Oct 24, 2019

Hey Prysm 馃憢

I'm working to build a genesis state from your deposit contract but all the signatures are failing on our end. After some digging, I suspect there's a mismatch between Prysm and the spec.

In process_deposit, the spec declares:

# Note: Deposits are valid across forks, thus the deposit domain is retrieved directly from `compute_domain`.
domain = compute_domain(DOMAIN_DEPOSIT)

The second parameter to compute_domain is omitted, so it defaults to Version() which I understand to be [0, 0, 0, 0].

In Prysm it seems you're passing through the Fork from the BeaconState:

domain := helpers.Domain(beaconState.Fork, helpers.CurrentEpoch(beaconState), params.BeaconConfig().DomainDeposit)
depositSig := deposit.Data.Signature
if err := verifySigningRoot(deposit.Data, pubKey, depositSig, domain); err != nil {
// Ignore this error as in the spec pseudo code.
log.Errorf("Skipping deposit: could not verify deposit data signature: %v", err)
return beaconState, nil
}

Which in your testnet is set to this:

demoConfig.GenesisForkVersion = []byte{0, 0, 0, 2}

So, as I understand it, the spec calls for [0, 0, 0, 0] but Prysm is using [0, 0, 0, 2]. If this turns out to be the case and you're passing all the v0.8.3 tests, it seems that the EF tests have a coverage gap. This seems feasible if all the tests assume that the Fork is [0, 0, 0, 0].

@terencechain

This comment has been minimized.

Copy link
Member

@terencechain terencechain commented Oct 24, 2019

Ahh yes. thank you for pointing that out. We bumped up the fork version on every test net restarts for critical bug fixes. This is to disconnect with external peers that may no longer be compatible to sync with post critical bug fixes.

Not sure what's the best course forward. Is rolling back to fork version 0 a possibility? @prestonvanloon @rauljordan @nisdas

@paulhauner

This comment has been minimized.

Copy link
Author

@paulhauner paulhauner commented Oct 24, 2019

You're welcome!

I think you can keep bumping the fork version, but deposits need to stay on the 0 fork/domain to stick with the spec. All other non-deposit messages will still fail verification.

Interestingly, keeping the deposits on the 0 fork/domain means that you can replay deposits across different testnets. This doesn't immediately seem like a vuln though, there seems to be no gain to spending x ETH to place a deposit for a key that you don't own. In fact, this might even be a feature! It means we could seamlessly move validators from an existing testnet over to a new one, even if the testnet fork changes.

@terencechain

This comment has been minimized.

Copy link
Member

@terencechain terencechain commented Oct 24, 2019

Interestingly, keeping the deposits on the 0 fork/domain means that you can replay deposits across different testnets.

I'm just happy when we are discovering new "features" along with this experiment ; )

@nisdas

This comment has been minimized.

Copy link
Member

@nisdas nisdas commented Oct 25, 2019

@paulhauner Actually for our tests our fork version is still

[]byte{0, 0, 0, 0}

so that is why we are still compliant with the spectests. The only time the fork version diverges
is during runtime where we use our demoConfig to set our custom parameters. However for the purposes of running our test suite it chooses the default beacon config, which always has the fork version as

[]byte{0, 0, 0, 0}
@paulhauner

This comment has been minimized.

Copy link
Author

@paulhauner paulhauner commented Oct 25, 2019

@nisdas, makes sense. It would also break in the case where the fork version was incremented by a legit hard-fork, though.

@nisdas

This comment has been minimized.

Copy link
Member

@nisdas nisdas commented Oct 25, 2019

Not exactly, if there was a hard-fork, we would increment the fork version in the default config(Mainnet Config) .
https://github.com/prysmaticlabs/prysm/blob/master/shared/params/config.go#L173.

So we would then use this fork version across all our tests, etc.

@paulhauner

This comment has been minimized.

Copy link
Author

@paulhauner paulhauner commented Oct 25, 2019

I mean the client would diverge from consensus at runtime because it would declare all (some?) deposit signatures invalid.

The value you linked me wouldn't be what we increment during a fork, it would be fork.current_version.

I opened an issue to add a test case for this here: ethereum/eth2.0-specs#1445

@nisdas

This comment has been minimized.

Copy link
Member

@nisdas nisdas commented Oct 25, 2019

Ah k. Yes in that case it would declare some deposit signatures as invalid. Processing deposit logs from the eth1 chain would be problematic in this case

@prestonvanloon

This comment has been minimized.

Copy link
Member

@prestonvanloon prestonvanloon commented Oct 29, 2019

What is the resolution for this issue?

@prestonvanloon

This comment has been minimized.

Copy link
Member

@prestonvanloon prestonvanloon commented Nov 5, 2019

I believe this was resolved by #3876. Please reopen if that is not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.