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

ATX Integration #820

Merged
merged 65 commits into from
Apr 30, 2019
Merged

ATX Integration #820

merged 65 commits into from
Apr 30, 2019

Conversation

noamnelke
Copy link
Member

No description provided.

@noamnelke noamnelke marked this pull request as ready for review April 16, 2019 07:59
cmd/node/node.go Outdated
log.Error("poet server not found")
}

dbStorepath := "/tmp/"
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 a config for that BaseConfig.DataDir
you can set the default instead of hardcoding

seq := uint64(0)
if err == nil {
atx, err := b.db.GetAtx(*prevAtx)
func (b *Builder) Start() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you start a builder multiple times ? if not add a lock or a comment

@@ -16,12 +14,14 @@ const AtxProtocol = "AtxGossip"
var activesetCache = NewActivesetCache(1000)

type ActiveSetProvider interface {
GetActiveSetSize(l types.LayerID) uint32
ActiveSetIds(l types.EpochId) uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

the method name seems like it returns the actual ids

if b.nipst == nil {
b.log.Info("starting build atx in epoch %v", epoch)
if b.prevATX == nil {
prevAtxId, err := b.GetPrevAtxId(b.nodeId)
Copy link
Contributor

Choose a reason for hiding this comment

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

In GetPrevAtxId implementation you're returning db.GetNodeAtxIds returned array last item, but it's not clear/documented that it is applying/enforcing that ordering.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are correct, we are now assuming no adversary behaviour and will fix this once we merge this code

b.log.Info("starting build atx in epoch %v", epoch)
if b.prevATX == nil {
prevAtxId, err := b.GetPrevAtxId(b.nodeId)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should handle both "no prev ATX found" cases equally. At the moment the first one isn't logged.
Perhaps have just one method, GetPrevAtx, which will combine the two calls, so the builder flow will be simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is ok for a node not to have a previous atx, however is is not ok for a node to have and atx id and not have the actual atx in the db

}
seq := uint64(0)
atxId := *types.EmptyAtxId
if b.prevATX != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to handle this condition in the if b.prevATX == nil { block, when you know that you just fetched it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't shink this simplifies the flow, the default behaviour is set first, and then when we have a previous atx we override the default with it.

}
}
seq := uint64(0)
atxId := *types.EmptyAtxId
Copy link
Contributor

Choose a reason for hiding this comment

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

atxId -> prevAtxId ?

atxId := *types.EmptyAtxId
if b.prevATX != nil {
seq = b.prevATX.Sequence + 1
//check if this node hasn't published an activation already
Copy link
Contributor

Choose a reason for hiding this comment

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

Line space before this one.

return
case layer := <-b.timer:
if b.working {
break
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

to not allow multiple ATXs to be created. we try to build an atx whenever a layer begind, if we are in the process of doint so we sleep until next layer

nipst/nipst.go Outdated
func (n *NIPST) ValidateNipstChallenge(expectedChallenge *common.Hash) bool {
ret := bytes.Equal(expectedChallenge[:], n.NipstChallenge[:])
if !ret {
log.Warning("expectedChallenge: %x, n.nipstChallenge: %x", expectedChallenge, n.NipstChallenge)
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to log this by the method caller.

@@ -121,10 +122,17 @@ func (n *NIPST) load() {
}

func (n *NIPST) Valid() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use nipstValidator now, remove this.

}
db.log.Info("NIPST challenge: %v, OK nipst %v", hash.ShortString(), atx.NIPSTChallenge.String())
//todo: add validation of nipst
if err = db.nipstValidator.Validate(atx.Nipst, *atx.Nipst.NipstChallenge); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks to me that the expected challenge should be atx.NIPSTChallenge and not atx.Nipst.NipstChallenge. The later is already contained in the nipst, so there's nothing to validate.

Copy link
Contributor

Choose a reason for hiding this comment

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

both checks are performed

Copy link
Contributor

Choose a reason for hiding this comment

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

Still not clear to me why to split it to 2 different validations, and why in the first one you provide atx.Nipst.NipstChallenge as the expected challenge, while it's already inside of atx.Nipst.

if err = db.nipstValidator.Validate(atx.Nipst, *atx.Nipst.NipstChallenge); err != nil {
return fmt.Errorf("NIPST not valid: %v", err)
}
if !atx.Nipst.ValidateNipstChallenge(hash) {
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 validation necessary, after nipstValidator will be implemented?

Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above

atx := types.NewActivationTxWithChallenge(*b.challenge, activeIds, b.mesh.GetLatestView(), b.nipst, true)
activeSetSize, err := b.db.CalcActiveSetFromView(atx)
if epoch != 0 && (activeSetSize == 0 || activeSetSize != atx.ActiveSetSize) {
b.log.Panic("empty active set size found! len(view): %d, view: %v", len(atx.View), atx.View)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to crash the node here?

Copy link
Contributor

Choose a reason for hiding this comment

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

currently, yes

// NewRPCPoetHarnessClient returns a new instance of RPCPoetClient
// which utilizes a local self-contained poet server instance
// in order to exercise functionality.
func NewRPCPoetHarnessClient() (*nipst.RPCPoetClient, error) {
Copy link
Contributor

@moshababo moshababo Apr 28, 2019

Choose a reason for hiding this comment

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

Instead of cloning this, you can remove it from poet_test.go to poet.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be removed anyways shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there weren't more usages i'd say no, because it's supposed to be used in tests only.

log.Error("error while cleaning up tmp dir: %v", err)
}
if matches, err := filepath.Glob("*.bin"); err != nil {
log.Error("error while finding PoET bin files: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The poet should clean up after himself. See spacemeshos/poet#35.

If you want to keep this in the meanwhile, add a comment.

@@ -0,0 +1,34 @@
syntax = "proto3";
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 no hare.proto anymore (replaced with xdr).
Please make sure you don't push this file

if bo.proofsEpoch != epochNumber {
err := bo.calcEligibilityProofs(epochNumber)
if err != nil {
return types.AtxId{}, nil, err
bo.log.Error("failed to calculate eligibility proofs: %v", err)
return types.AtxId{common.HexToHash("EEEEEEEE")}, nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Any other places you use "EEEEE..." ?
Either way, consider using a const

Copy link
Contributor

Choose a reason for hiding this comment

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

for debugging purposes only, it is supposed to be empty

log.Error("no activations found for node id \"%v\"", nodeID.Key)
return types.AtxId{}, errors.New("no activations found")
bo.log.Error("no activations found for node id \"%v\"", bo.nodeID.Key)
return types.AtxId{common.HexToHash("22222222")}, errors.New("no activations found")
Copy link
Contributor

Choose a reason for hiding this comment

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

"2222222"... ?!
Same as before

log.Error("getting node ATX IDs failed: %v", err)
return types.AtxId{}, err
bo.log.Info("did not find ATX IDs for node: %v, error: %v", bo.nodeID.Key[:5], err)
return types.AtxId{common.HexToHash("11111111")}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

?!

@@ -25,10 +29,12 @@ type MinerBlockOracle struct {
proofsEpoch types.EpochId
eligibilityProofs map[types.LayerID][]types.BlockEligibilityProof
atxID types.AtxId
log log.Log
}

func NewMinerBlockOracle(committeeSize int32, layersPerEpoch uint16, activationDb ActivationDb,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply call it BlockOracle?

Copy link
Member Author

Choose a reason for hiding this comment

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

name is taken - should be refactored when the other one is no longer used.

func (v *Validator) Validate(nipst *NIPST, expectedChallenge common.Hash) error {
if !bytes.Equal(nipst.poetChallenge[:], expectedChallenge[:]) {
if !bytes.Equal(nipst.NipstChallenge[:], expectedChallenge[:]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not introduce an Equals method for the challenge type?

Copy link
Contributor

Choose a reason for hiding this comment

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

only used once

log.Warning("PoET proof invalid: %v", err)
return fmt.Errorf("PoET proof invalid: %v", err)
}

if !bytes.Equal(nipst.postChallenge[:], nipst.postProof.Challenge) {
if !bytes.Equal(nipst.PostChallenge[:], nipst.PostProof.Challenge) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above for all comparison of the bytes of the challenge

return false, err
}
activeSetSize = atx.ActiveSetSize
if epochNumber.IsGenesis() && activeSetSize < GenesisActiveSetSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

epochNumber.IsGenesis() is already checked in the outer. Apparently we will never pass this if (are you missing a test case?)

Copy link
Contributor

Choose a reason for hiding this comment

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

@noamnelke please address this

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, this if can just be removed, I think it was added when we tried to catch a weird case and wasn't cleaned up.

common/bytes.go Outdated
@@ -77,6 +77,10 @@ func (h Hash) String() string {
return h.Hex()
}

func (h Hash) ShortString() string {
return h.Hex()[2:7]
Copy link
Contributor

Choose a reason for hiding this comment

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

Check bounderies

return 0
}
return common.BytesToUint64(val)
return common.BytesToUint32(val)
}

func (db *ActivationDb) addAtxToEpoch(epochId types.EpochId, atx types.AtxId) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd consider adding comments that addAtxToEpoch and addAtxToNodeId should only be used inside a method that invokes db.Lock(), the way it's implemented now its not that obvious

@@ -25,15 +27,15 @@ func BlockIdsAsBytes(ids []BlockID) ([]byte, error) {
func BytesToBlockIds(blockIds []byte) ([]BlockID, error) {
var ids []BlockID
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use the same naming convention for all conversion methods,
i think it should be ViewToBytes and not ViewAsBytes

@antonlerner antonlerner merged commit 5f457fe into develop Apr 30, 2019
@y0sher y0sher deleted the integrate_atx_flow branch June 25, 2019 07:48
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

6 participants