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

Block eligibility oracle #778

Merged
merged 14 commits into from Apr 4, 2019

Conversation

Projects
None yet
3 participants
@noamnelke
Copy link
Contributor

noamnelke commented Apr 2, 2019

I had to skip TestMultipleNodes since it fails for now, need to fix and unskip before merging.

Major TODOs:

  • Create validator
  • Write tests for oracle/validator
  • Properly initialize ActivationDB
  • Include eligibility proof in block (?)

VRF Signer related (are we doing any of this right now?):

  • Implement VRF signer/validator
  • Generate and store VRF key-pair

@noamnelke noamnelke requested a review from antonlerner Apr 2, 2019

@almogdepaz

This comment has been minimized.

Copy link
Member

almogdepaz commented Apr 2, 2019

TestMultipleNodes is flaky and I'm going to fix it as part of a pr anyway so you can just leave it skipped

@noamnelke noamnelke force-pushed the layer_committee_vrf branch from dff909b to a4d14f4 Apr 3, 2019

Show resolved Hide resolved oracle/blockoracle.go Outdated
Show resolved Hide resolved oracle/blockoracle.go Outdated
Show resolved Hide resolved crypto/vrf.go
//bo := oracle.NewBlockOracleFromClient(oracleClient, int(app.Config.CONSENSUS.NodesPerLayer))
nodesPerLayer := app.Config.CONSENSUS.NodesPerLayer
layersPerEpoch := app.Config.CONSENSUS.LayersPerEpoch
activationDb := &activation.ActivationDb{} // TODO: initialize properly

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 3, 2019

Member

There is an implementation of this in our code, however it is disconnected from all flows. I think we should first run tests on each component separately and then combine them together

noamnelke added some commits Apr 3, 2019

actually validate the vrf 🤭
Thank god for unit tests.
receive atx ID from block, validate ATX, fix bug
The bug was that the layer ID was calculated relative to the start of the epoch, so nobody was eligible outside of epoch 0. We now use the absolute layer ID.

@noamnelke noamnelke marked this pull request as ready for review Apr 4, 2019

@@ -17,7 +17,7 @@ import (
)

This comment has been minimized.

Copy link
@almogdepaz

almogdepaz Apr 4, 2019

Member

this was intended for internal package use i think we should keep it that way
and if possible from sync point of view the api should be
func BlockValid(b block) bool
we can discuss this in person

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 5, 2019

Author Contributor

@almogdepaz just so you don't think I ignored this comment - Anton and I discussed it and it will be addressed in a separate PR on Sunday.

The interface you want (and I agree it's the right one) requires putting the eligibility proofs in the block and implementing a bit more logic, so we wanted to separate it from this PR.

bo.Register(true, pub.String())

err := app.apps[i].initServices(pub.String(), n, store, sgn, bo, bo, numOfInstances)
err := app.apps[i].initServices(pub.String(), n, store, sgn, bo, nil, bo, numOfInstances) // TODO: pass blockValidator and hareOracle

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 4, 2019

Member

if this fails the test, revert to old blockoracle and test

Show resolved Hide resolved crypto/vrf.go
@@ -76,6 +76,10 @@ func (t ActivationTx) Id() AtxId {
return AtxId{crypto.Keccak256Hash(tx)}
}

func (t ActivationTx) Validate() error {

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 4, 2019

Member

maybe validate should return true false - if valid or not

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 4, 2019

Author Contributor

I like this pattern for validation where no error means it passed and if it failed the error tells you why. An additional boolean would just reflect that (if there's an error it should be false, otherwise true).

How is the calling code supposed to handle the result? If the error is not nil -- also check the bool, or just ignore it and assume it's true? If the bool is false and the error is nil -- I don't get a reason that I can log?

Show resolved Hide resolved mesh/mesh.go
}

func ValidateVRF(message, signature, publicKey []byte) error {
if !bytes.Equal(message, signature) {

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 4, 2019

Member

can use some sig scheme and validate it

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 5, 2019

Author Contributor

done in #789

}
hash := sha256.Sum256(sig)
epochOffset := epochNumber * uint64(v.layersPerEpoch)
eligibleLayer := mesh.LayerID(binary.LittleEndian.Uint64(hash[:])%uint64(v.layersPerEpoch) + epochOffset)

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 4, 2019

Member

comment and explain or extract calculation to a function

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 5, 2019

Author Contributor

done in #789

Show resolved Hide resolved oracle/blockoracle.go Outdated
Show resolved Hide resolved oracle/blockoracle.go Outdated

@noamnelke noamnelke merged commit 73e4e08 into develop Apr 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noamnelke noamnelke deleted the layer_committee_vrf branch Apr 4, 2019

@noamnelke noamnelke referenced this pull request Apr 5, 2019

Merged

Block oracle improvements #789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.