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

inverted control between nipst builder and atx builder #806

Merged
merged 20 commits into from Apr 15, 2019

Conversation

Projects
None yet
2 participants
@antonlerner
Copy link
Member

commented Apr 10, 2019

No description provided.

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

@antonlerner antonlerner force-pushed the poet_challange branch from e21b163 to be75a5e Apr 14, 2019

noamnelke added some commits Apr 14, 2019

remove todo
Verified with @barakshani
if err != nil {
return err
}
//todo: should we do something about it? wait for something?

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

Is this todo in the right place? Seems like it should be here 👇?
daea588#diff-1ba4cf64f19ae66633cb8b7ca7158fabR103

@@ -94,6 +96,50 @@ func (m *ActivationDb) CalcActiveSetFromView(a *types.ActivationTx) (uint32, err

}

const NIPST_LAYERTIME = 1000

func (db *ActivationDb) ValidateAtx(atx *types.ActivationTx) error {

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

There are inconsistent receiver names in this module. These methods are different than the rest:

func (m *ActivationDb) ProcessBlockATXs(blk *types.Block) {
func (m *ActivationDb) CalcActiveSetFromView(a *types.ActivationTx) (uint32, error) {
@@ -94,6 +96,50 @@ func (m *ActivationDb) CalcActiveSetFromView(a *types.ActivationTx) (uint32, err

}

const NIPST_LAYERTIME = 1000

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

Use CamelCase

@antonlerner antonlerner marked this pull request as ready for review Apr 14, 2019

}
prevAtxIds, err := db.GetNodeAtxIds(atx.NodeId)
if err == nil && len(prevAtxIds) > 0 {
prevAtx, err := db.GetAtx(prevAtxIds[len(prevAtxIds)])

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

prevAtxIds[len(prevAtxIds)] will always panic.

If you have a 3 element slice (len()==3) it has elements 0, 1 and 2, but never 3 (or its length would be 4).

if err == nil && len(prevAtxIds) > 0 {
prevAtx, err := db.GetAtx(prevAtxIds[len(prevAtxIds)])
if err == nil {
if prevAtx.Sequence == atx.Sequence {

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

We're only checking the last ATX - does db.GetNodeAtxIds return a sorted list?

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

We don't check for a lower sequence number (e.g. 1 -> 2 -> 3 -> 2 would pass validation).

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

We don't validate the PrevATXId field at all.

This comment has been minimized.

Copy link
@antonlerner

antonlerner Apr 14, 2019

Author Member

returns a list of received atx's sorted by the time they were received

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

The way I think this should be implemented is by storing the nextATX on the prev ATX (doubly linked list).

Then, to validate, you go to the linked prevATX and validate that its nextATX is blank and that the sequence numbers between those two are correct (current is greater than previous).

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

Of course, we'd also need to verify that if no prevATX was linked - then no other ATXs exist for that miner ID.

}
}
} else {
if prevAtxIds == nil && atx.PrevATXId != types.EmptyAtx {

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

Replace prevAtxIds == nil with len(prevAtxIds) == 0?

This would better convey the intention and also be safer (what If someone makes db.GetNodeAtxIds return an empty slice instead of nil when nothing is found?).

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

More importantly - you're not checking for the opposite case, where someone doesn't link to a previous ATX, but one exists.

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

BTW, this is the biggest thing we need to defend against, since it allows working on multiple ATXs concurrently.

if err != nil {
return fmt.Errorf("positioning atx not found")
}
if !posAtx.Valid {

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

An ATX in the database should always be valid, no?

This comment has been minimized.

Copy link
@noamnelke

noamnelke Apr 14, 2019

Contributor

after our discussion right now, I think we should keep invalid ATXs as well

@noamnelke noamnelke merged commit 1c13cc1 into develop Apr 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.