inverted control between nipst builder and atx builder #806
Conversation
antonlerner
force-pushed
the
poet_challange
branch
from
e21b163
to
be75a5e
Compare
…into poet_challange
Verified with @barakshani
| if err != nil { | ||
| return err | ||
| } | ||
| //todo: should we do something about it? wait for something? |
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
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 | |||
activation/atxdb.go
Outdated
| } | ||
| prevAtxIds, err := db.GetNodeAtxIds(atx.NodeId) | ||
| if err == nil && len(prevAtxIds) > 0 { | ||
| prevAtx, err := db.GetAtx(prevAtxIds[len(prevAtxIds)]) |
There was a problem hiding this comment.
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).
activation/atxdb.go
Outdated
| if err == nil && len(prevAtxIds) > 0 { | ||
| prevAtx, err := db.GetAtx(prevAtxIds[len(prevAtxIds)]) | ||
| if err == nil { | ||
| if prevAtx.Sequence == atx.Sequence { |
There was a problem hiding this comment.
We're only checking the last ATX - does db.GetNodeAtxIds return a sorted list?
There was a problem hiding this comment.
We don't check for a lower sequence number (e.g. 1 -> 2 -> 3 -> 2 would pass validation).
There was a problem hiding this comment.
We don't validate the PrevATXId field at all.
There was a problem hiding this comment.
returns a list of received atx's sorted by the time they were received
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
More importantly - you're not checking for the opposite case, where someone doesn't link to a previous ATX, but one exists.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
An ATX in the database should always be valid, no?
There was a problem hiding this comment.
after our discussion right now, I think we should keep invalid ATXs as well
No description provided.