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
Conversation
TestMultipleNodes is flaky and I'm going to fix it as part of a pr anyway so you can just leave it skipped |
dff909b
to
a4d14f4
Compare
//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 |
There was a problem hiding this comment.
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
Thank god for unit tests.
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.
@@ -17,7 +17,7 @@ import ( | |||
) | |||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #789
I had to skip
TestMultipleNodes
since it fails for now, need to fix and unskip before merging.Major TODOs:
ActivationDB
VRF Signer related (are we doing any of this right now?):