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.

Sign up for GitHub

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

Jump to bottom

full node UT #491

Merged
merged 16 commits into from Feb 10, 2019
Merged

full node UT #491

merged 16 commits into from Feb 10, 2019

Conversation

antonlerner
Copy link
Contributor

No description provided.

almogdepaz
almogdepaz reviewed Feb 5, 2019
View reviewed changes
mesh/mesh.go Outdated
func (m *Mesh) AddLayer(layer *Layer) error {
m.lMutex.Lock()
defer m.lMutex.Unlock()
count := LayerID(m.LocalLayer())
count := LayerID(m.LatestSeenLayer())
Copy link
Contributor

@almogdepaz almogdepaz Feb 5, 2019 •
edited

Choose a reason for hiding this comment

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

change the name to something that indicates that we already received
like latestSynced/latestReceived or something

almogdepaz
almogdepaz reviewed Feb 5, 2019
View reviewed changes
mesh/mesh.go
@@ -92,18 +130,51 @@ func (m *Mesh) AddLayer(layer *Layer) error {
m.Debug("can't add layer", layer.Index(), " missing previous layers")
return errors.New("can't add layer missing previous layers")
}

atomic.StoreUint32(&m.lastSeenLayer, uint32(layer.Index()))
m.addLayer(layer)
Copy link
Contributor

Choose a reason for hiding this comment

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

layer should be sent to verification as part of this method

almogdepaz
almogdepaz reviewed Feb 5, 2019
View reviewed changes
mesh/mesh.go
@@ -92,18 +130,51 @@ func (m *Mesh) AddLayer(layer *Layer) error {
m.Debug("can't add layer", layer.Index(), " missing previous layers")
return errors.New("can't add layer missing previous layers")
}

atomic.StoreUint32(&m.lastSeenLayer, uint32(layer.Index()))
Copy link
Contributor

Choose a reason for hiding this comment

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

this basically overwrites lastSeenLayer which could be higher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a check above that validates that this layer is exactly +1 from previous recevied

@antonlerner antonlerner changed the title [WIP] full node UT full node UT Feb 6, 2019
@antonlerner antonlerner force-pushed the real_node branch 2 times, most recently from 54beed3 to 3acc142 Compare February 6, 2019 18:08
@antonlerner
compiling version of node main
0594efb
@antonlerner
fixed sync unit test and refactored to support proper work flow with …
5dd9c1c 
…tortoise
@antonlerner
running not fully functioning nodes
ae11945
@antonlerner
running working 2 node with simulator accepting 1 trans
eb4a478 
fixed getting blocks for layer from layerDB and not from orphans

unified all componenets into spacemeshApp, refactored tests to support multiple instances and written proper teardown

added basic genesis block config

passing UT with no real node tests

after another merge

added timeouts to sync tests

after CR review

removed -v from tests

for debugging in travis
@antonlerner
proper oracle handling
326c490
@antonlerner
fixed UT of hare and sync
f340ea5
y0sher
y0sher reviewed Feb 7, 2019
View reviewed changes
app/main.go Outdated

app.NodeInitCallback <- true
app.P2P = swarm
Copy link
Contributor

Choose a reason for hiding this comment

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

this happens 3 times. in line 373 (which is probably the correct one since app.setupTestFeatures already consumes app.P2P.)
again here and again in initServices

y0sher
y0sher reviewed Feb 7, 2019
View reviewed changes
hare/broker.go Outdated
//if broker.inbox != nil { // Start has been called at least twice
// log.Error("Could not start instance")
// return StartInstanceError(errors.New("instance already started"))
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can move registration back here and uncomment above code. the only rule is that protocols should register before swarm is started.

y0sher
y0sher reviewed Feb 7, 2019
View reviewed changes
hare/config/config.go Outdated
@@ -10,5 +10,5 @@ type Config struct {
}

func DefaultConfig() Config {
return Config{800, 400, 200, time.Second * time.Duration(15)}
return Config{2, 1, 2, 500 *time.Millisecond}
Copy link
Contributor

@y0sher y0sher Feb 7, 2019 •
edited

Choose a reason for hiding this comment

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

if we really want to start testing that so we should configure this now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets talk about it with @gavraz, I'll leave it as is for the meanwhile

y0sher
y0sher reviewed Feb 10, 2019
View reviewed changes
timesync/clock.go Outdated
ch <- t.currentLayer
//log.Info("iv'e notified number : %v", t.ids[ch])
Copy link
Contributor

Choose a reason for hiding this comment

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

why comment?

y0sher
y0sher reviewed Feb 10, 2019
View reviewed changes
p2p/service/sim.go Outdated
@@ -225,6 +225,7 @@ func (sn *Node) sendMessageImpl(nodeID p2pcrypto.PublicKey, protocol string, pay
return nil
}
log.Debug("%v >> %v (%v)", sn.Node.PublicKey(), nodeID, payload)
panic("could not find " + protocol + " handler for node: " + nodeID.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor

Choose a reason for hiding this comment

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

even though it should never happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe in shutdown

y0sher
y0sher approved these changes Feb 10, 2019
View reviewed changes
Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

added some comments

p2p/service/sim.go Outdated
@@ -225,6 +225,7 @@ func (sn *Node) sendMessageImpl(nodeID p2pcrypto.PublicKey, protocol string, pay
return nil
}
log.Debug("%v >> %v (%v)", sn.Node.PublicKey(), nodeID, payload)
panic("could not find " + protocol + " handler for node: " + nodeID.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe in shutdown

oracle/oracle.go Outdated
return bo.oc.Eligible(uint32(id), bo.committeeSize, pubKey)
}

/*
type HareOracle interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what in shutdown?

mesh/mesh.go Outdated
atomic.AddInt32(&m.orphanBlockCount, -1)
for _, layermap := range m.orphanBlocks{
if _, has := layermap[b]; has {
m.Log.Info("delete block ", b, "from orphans")
Copy link
Contributor

Choose a reason for hiding this comment

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

probably shouldn't be info. (will make noise)

y0sher
y0sher reviewed Feb 10, 2019
View reviewed changes
app/main.go
panic("got error starting services : " + err.Error())
}

app.startServices()

err = app.P2P.Start()
Copy link
Contributor

@y0sher y0sher Feb 10, 2019 •
edited

Choose a reason for hiding this comment

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

could go in startServices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't, it is created from outside services and passed as parameter so i'd rather the code that does that will start

@antonlerner antonlerner merged commit 9615801 into develop Feb 10, 2019
@y0sher y0sher deleted the real_node branch June 25, 2019 07:47
@lrettig lrettig mentioned this pull request Jan 7, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y0sher y0sher y0sher approved these changes

@almogdepaz almogdepaz almogdepaz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@antonlerner @y0sher @almogdepaz