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
Merged

Block eligibility oracle #778

merged 14 commits into from Apr 4, 2019

Conversation

noamnelke
Copy link
Member

@noamnelke 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

@almogdepaz
Copy link
Contributor

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

oracle/blockoracle.go Outdated Show resolved Hide resolved
oracle/blockoracle.go Outdated Show resolved Hide resolved
//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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 noamnelke marked this pull request as ready for review April 4, 2019 10:40
@@ -17,7 +17,7 @@ import (
)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -76,6 +76,10 @@ func (t ActivationTx) Id() AtxId {
return AtxId{crypto.Keccak256Hash(tx)}
}

func (t ActivationTx) Validate() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe validate should return true false - if valid or not

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

}

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

Choose a reason for hiding this comment

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

can use some sig scheme and validate it

Copy link
Member Author

Choose a reason for hiding this comment

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

done in #789

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

Choose a reason for hiding this comment

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

comment and explain or extract calculation to a function

Copy link
Member Author

Choose a reason for hiding this comment

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

done in #789

oracle/blockoracle.go Outdated Show resolved Hide resolved
oracle/blockoracle.go Outdated Show resolved Hide resolved
@noamnelke noamnelke merged commit 73e4e08 into develop Apr 4, 2019
@noamnelke noamnelke deleted the layer_committee_vrf branch April 4, 2019 14:57
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

Successfully merging this pull request may close these issues.

None yet

3 participants