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

implemented mesh traversal for validation of active set #780

Merged
merged 12 commits into from Apr 10, 2019

Conversation

antonlerner
Copy link
Contributor

@antonlerner antonlerner commented Apr 3, 2019

implemented mesh traversal for validation of active set state in atxs

I've refactored some files into mesh so just skim over activaiton.go and atxdb.go

new code is in mesh

@antonlerner antonlerner marked this pull request as ready for review April 4, 2019 17:09
@@ -64,7 +64,7 @@ func (vp votingPattern) Layer() mesh.LayerID {
type BlockCache interface {
GetBlock(id mesh.BlockID) (*mesh.Block, error)
LayerBlockIds(id mesh.LayerID) ([]mesh.BlockID, error)
ForBlockInView(view map[mesh.BlockID]struct{}, layer mesh.LayerID, foo func(block *mesh.BlockHeader), errHandler func(err error))
ForBlockInView(view map[mesh.BlockID]struct{}, layer mesh.LayerID, foo mesh.TraversalFunc, errHandler func(err error)) 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 prefer it stayed func(block *mesh.BlockHeader) or whatever
it dosen't have to be related to mesh

)

//todo: choose which type is VRF
type Vrf string

const AtxProtocol = "AtxGossip"

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this here?, i think mesh should be kept separated from anything related to network i.e broadcasting listening ...


type Broadcaster interface {
Broadcast(channel string, data []byte) error
}
Copy link
Contributor

Choose a reason for hiding this comment

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

like i said im not sure this is a good idea
i think it should be in a separate package
if the types are an issue maybe its time to pull them out of here as well

@@ -109,7 +109,7 @@ type routingTableImpl struct {
// Local peer ID that holds this routing table
local node.DhtID

// Operations activation channels
// Operations mesh channels
Copy link
Contributor

Choose a reason for hiding this comment

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

refactor error

@@ -64,7 +64,7 @@ func (vp votingPattern) Layer() mesh.LayerID {
type BlockCache interface {
GetBlock(id mesh.BlockID) (*mesh.Block, error)
LayerBlockIds(id mesh.LayerID) ([]mesh.BlockID, error)
ForBlockInView(view map[mesh.BlockID]struct{}, layer mesh.LayerID, foo func(block *mesh.BlockHeader), errHandler func(err error))
ForBlockInView(view map[mesh.BlockID]struct{}, layer mesh.LayerID, foo mesh.TraversalFunc, errHandler func(err error)) error
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename foo?

@@ -211,7 +211,7 @@ func globalOpinion(v vec, layerSize uint64, delta float64) vec {
}

func (ni *ninjaTortoise) updateCorrectionVectors(p votingPattern, bottomOfWindow mesh.LayerID) {
foo := func(x *mesh.BlockHeader) {
foo := func(x *mesh.BlockHeader) error {
Copy link
Member

Choose a reason for hiding this comment

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

foo and x aren't great names...

@@ -462,13 +463,14 @@ func (ni *ninjaTortoise) handleIncomingLayer(newlyr *mesh.Layer) { //i most rece
view := make(map[mesh.BlockID]struct{})
lCntr := make(map[mesh.LayerID]int)
correctionMap, effCountMap, getCrrEffCnt := ni.getCorrEffCounter()
foo := func(block *mesh.BlockHeader) {
foo := func(block *mesh.BlockHeader) error {
Copy link
Member

Choose a reason for hiding this comment

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

same here

}
}

func (b *Builder) PublishActivationTx(nipst *nipst.NIPST) error {
Copy link
Member

Choose a reason for hiding this comment

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

I would change the way this method is organized:

  • get prev atx id
  • if not found: seq = 0 and prev atx id = nil
  • else: get atx seq (passing the id into the method, so it doesn't need to find it again)

prevAtx = &EmptyAtx
}

atx := NewActivationTx(b.nodeId, seq+1, *prevAtx, l, 0, *posAtx, b.activeSet.GetActiveSetSize(l-1), b.mesh.GetLatestView(), nipst)
Copy link
Member

Choose a reason for hiding this comment

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

seq+1 means counting starts at 1 instead of 0.

If we change the organization as I suggested we can do the incrementation only if a prev atx was found.

mesh/mesh.go Outdated
@@ -51,43 +58,44 @@ type Mesh struct {
done chan struct{}
}

func NewPersistentMesh(path string, rewardConfig RewardConfig, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
func NewPersistentMesh(path string, rewardConfig Config, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
Copy link
Member

Choose a reason for hiding this comment

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

rename the argument name as well?

mesh/mesh.go Outdated
}

return ll
}

func NewMemMesh(rewardConfig RewardConfig, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
func NewMemMesh(rewardConfig Config, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed config that is not reward related so it's not really relevant at the moment

}

return ll
}

func NewMesh(db *MeshDB, rewardConfig RewardConfig, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
func NewMesh(db *MeshDB, atxDb AtxDB, rewardConfig Config, mesh MeshValidator, state StateUpdater, logger log.Log) *Mesh {
Copy link
Member

Choose a reason for hiding this comment

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

same

mesh/reward.go Outdated
SimpleTxCost *big.Int
BaseReward *big.Int
PenaltyPercent *big.Int
TxQuota uint32
RewardMaturity LayerID
// global mesh config
LayersPerEpoch LayerID
Copy link
Member

Choose a reason for hiding this comment

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

kind of weird that the type is LayerID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -109,7 +109,7 @@ type routingTableImpl struct {
// Local peer ID that holds this routing table
local node.DhtID

// Operations activation channels
// Operations mesh channels
Copy link
Member

Choose a reason for hiding this comment

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

intentional?

prevAtx = &block.EmptyAtx
seq = 0
} else {
seq = b.GetLastSequence(b.nodeId)
Copy link
Member

Choose a reason for hiding this comment

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

doesn't it make more sense to pass the atxID (and rename this GetSequence) instead of looking for the prevAtx again inside this method? Not a big deal, if you disagree.

seq = 0
} else {
seq = b.GetLastSequence(b.nodeId)
if seq > 0 && prevAtx == nil {
Copy link
Member

Choose a reason for hiding this comment

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

prevAtx can never be nil here.

Copy link
Contributor

@almogdepaz almogdepaz left a comment

Choose a reason for hiding this comment

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

i went over it and i dont think block is a good choise for the package name, i looked at go-ethereum and they named it core maybe we should go with something similar
or something more descriptive to the content of the package

@antonlerner
Copy link
Contributor Author

i went over it and i dont think block is a good choise for the package name, i looked at go-ethereum and they named it core maybe we should go with something similar
or something more descriptive to the content of the package

Core is also very generic and not descriptive, currently package block contains all data structures and types that exists in a spacemesh block.
Refactoring the name of the package can be changed always but I cant think of more appropriate name. Your'e welcome to find something more descriptive but i don't want to spend more time on naming.

@antonlerner antonlerner merged commit 32f1bd8 into develop Apr 10, 2019
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

3 participants