Kauri module implementation #89

hanish520 · 2023-02-21T00:24:25Z

kauri implementation first draft

codecov-commenter · 2023-02-21T00:31:54Z

Codecov Report

Merging #89 (e7a5cfc) into master (5196107) will increase coverage by 1.46%.
The diff coverage is 81.40%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   62.61%   64.07%   +1.46%     
==========================================
  Files          73       75       +2     
  Lines        7185     7525     +340     
==========================================
+ Hits         4499     4822     +323     
- Misses       2382     2398      +16     
- Partials      304      305       +1

Flag	Coverage Δ
unittests	`64.07% <81.40%> (+1.46%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/hotstuff/main.go	`0.00% <ø> (ø)`
cmd/plot/main.go	`0.00% <ø> (ø)`
consensus/byzantine/byzantine.go	`1.78% <0.00%> (-0.14%)`	⬇️
hotstuff.go	`100.00% <ø> (ø)`
internal/cli/root.go	`24.19% <ø> (ø)`
internal/orchestration/worker.go	`65.42% <ø> (+0.25%)`	⬆️
internal/proto/hotstuffpb/convert.go	`98.08% <ø> (ø)`
leaderrotation/carousel.go	`3.77% <ø> (ø)`
metrics/types/event.go	`100.00% <ø> (ø)`
modules/modules.go	`100.00% <ø> (ø)`
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

meling · 2023-03-14T04:04:08Z

backend/server.go

+			impl.srv.logger.Infof("Failed to get client ID: %v", err)
+			return
+		}
+		proposal.Block.Proposer = uint32(id)


Given that proposal isn't used later in this method, will this have an effect?
Or is this one still necessary: proposeMsg := hotstuffpb.ProposalFromProto(proposal)?

It could perhaps be helpful to add a comment to explain why Kauri can use the ID from proposal.Block.Proposer while non-Kauri gets the ID from context.

meling · 2023-03-14T04:10:27Z

consensus/consensus.go

-
-	cs.configuration.Propose(proposal)
+	if cs.kauri == nil {
+		cs.configuration.Propose(proposal)


Would it be useful to have a comment explaining that Kauri won't propose here, but elsewhere?

meling · 2023-03-14T04:13:14Z

internal/proto/kauripb/kauri.proto

+  uint32 ID = 1;
+  hotstuffpb.QuorumSignature Signature = 2;
+  uint64 View = 3;
+}


meling · 2023-03-14T04:14:05Z

kauri/kauri.go

@@ -0,0 +1,320 @@
+// Package kauri contains the implementation of the kauri protocol


... the Kauri protocol.

meling · 2023-03-14T04:15:15Z

kauri/kauri.go

+	modules.RegisterModule("kauri", New)
+}
+
+// TreeType determines the type of tree to be built, for future extension


... to be built (for future extension).

Is this supposed to be exported?

meling · 2023-03-14T04:16:58Z

kauri/kauri.go

+// TreeType determines the type of tree to be built, for future extension
+type TreeType int
+
+// NoFaultTree is a tree without any faulty replicas in the tree, which is currently supported.


These comments should be moved inside the const block above each of the constants so that the docs are generated correctly.

meling · 2023-03-14T04:22:14Z

kauri/kauri.go

+	}
+	k.tree = CreateTree(k.configuration.Len(), k.opts.ID())
+
+	// pIDs := make(map[hotstuff.ID]int)


Still need this commented code?

meling · 2023-03-14T04:42:17Z

kauri/tree.go

+}
+
+// CreateTree Creates the tree configuration, currently only fault free tree configuration is supported.
+func CreateTree(configurationLength int, myID hotstuff.ID) TreeConfiguration {


This is the interface anti-pattern in Go that I mentioned a while back. Generally, we avoid returning an interface type and defining interfaces before they are needed.

Suggest to rename this function to newFaultFreeTree and return *faultFreeTree instead of the interface. If you later add a new tree type, you can create another construction function that returns the other tree type.

meling · 2023-03-14T04:42:55Z

kauri/tree.go

+}
+
+// FaultFreeTree implements a fault free tree configuration.
+type FaultFreeTree struct {


Consider whether or not all these types and methods and such needs to be exported... I haven't checked all methods etc, but looks like most of methods and types are internal to the kauri package and should not be exported unless you think there is a need for managing trees outside kauri.

meling · 2023-03-14T04:44:49Z

kauri/tree.go

+		return nil
+	}
+	temp := configurationLength
+	temp = temp - 1 //root


temp := configurationLength - 1 // root

Avoids Line 36.

meling · 2023-03-14T05:00:36Z

kauri/tree.go

+	temp := configurationLength
+	temp = temp - 1 //root
+	height := 1
+	for i := 1; temp > 0; i++ {


Maybe this could be converted to a helper function:

func height(maxChild float64, treeNodes int) int { remainingNodes := treeNodes - 1 // root height := 1 for i := 1; remainingNodes > 0; i++ { remainingNodes -= int(math.Pow(maxChild, float64(i))) height++ } return height }

And you could add this test:

func TestTreeHeight(t *testing.T) { const MaxChild = 2 tests := []struct { configurationLength int expectedHeight int }{ {1, 1}, {2, 2}, {3, 2}, {4, 3}, {5, 3}, {6, 3}, {7, 3}, {8, 4}, {9, 4}, {10, 4}, {11, 4}, {12, 4}, {13, 4}, {14, 4}, {15, 4}, {16, 5}, {17, 5}, } for _, test := range tests { actualHeight := height(MaxChild, test.configurationLength) if actualHeight != test.expectedHeight { t.Errorf("height(%d) = %d, expected %d", test.configurationLength, actualHeight, test.expectedHeight) } } }

meling · 2023-03-14T05:03:14Z

kauri/tree.go

+	posToIDMapping      map[int]hotstuff.ID
+}
+
+// CreateTree Creates the tree configuration, currently only fault free tree configuration is supported.


Renaming this function to newFaultFreeTree this doc comment can be replaced with:
newFaultFreeTree creates a fault free tree configuration.

There is no need to say that it is the only supported tree type.

meling · 2023-03-14T05:11:06Z

kauri/tree.go

+	return t.GetChildrenOfNode(t.ID)
+}
+
+func (t *FaultFreeTree) isWithInIndex(position int) bool {


Is there a better name for this? Could this be simply contains(index int) bool?

meling · 2023-03-14T05:16:52Z

kauri/tree.go

+	}
+}
+
+// GetParent fetches the ID of the parent, if root, returns itself.


In Go we typically don't use Get for getters... You can simply call this Parent().

Also, the doc comment should maybe say something like:

Parent returns the tree node's parent ID and true. Otherwise, if the tree node is the root, it returns its own ID and false.

meling · 2023-03-14T05:39:13Z

kauri/kauri.go

+	seed := k.opts.SharedRandomSeed() + int64(binary.LittleEndian.Uint64(hash[:]))
+	//Shuffle the list of IDs using the shared random seed + the first 8 bytes of the hash.
+	rnd := rand.New(rand.NewSource(seed))
+	rnd.Shuffle(len(ids), reflect.Swapper(ids))


Should avoid reflect here. I think something like this will work:

func(i, j int) { ids[i], ids[j] = ids[j], ids[i] })

meling · 2023-03-14T05:52:24Z

kauri/kauri.go

+func (k *Kauri) SendContributionToParent() {
+	parent, ok := k.tree.GetParent()
+	if ok {
+		node, isPresent := k.nodes[parent]


Consider moving the nodes map to the tree and let tree.Parent() return the parent node directly instead of the hotstuff.ID.

meling · 2023-03-14T06:00:43Z

kauri/kauri.go

+	return true
+}
+
+func (k *Kauri) randomizeIDS(hash hotstuff.Hash, leaderID hotstuff.ID) map[hotstuff.ID]int {


Suggest to rename this to shuffleTreeNodes and move it into the tree. And instead of passing in the hash you could move the seed calculation out from this method... so that the signature becomes:

func (t *faultFreeTree) shuffleTreeNodes(seed int64, leaderID hotstuff.ID)

This way you can update the internals of the tree directly avoiding the passing around of maps from this method to InitializeWithPIDs (which could be removed; its functionality can be done in the constructor and in the above method.)

meling · 2023-03-14T06:20:54Z

kauri/kauri.go

+
+func (k *Kauri) initializeConfiguration() {
+	kauriCfg := kauripb.ConfigurationFromRaw(k.configuration.GetRawConfiguration(), nil)
+	for _, n := range kauriCfg.Nodes() {


Seems better to move this into the tree constructor like this:

func newFaultFreeTree(cfg kauripb.Configuration, myID hotstuff.ID) *FaultFreeTree { nodes := make(map[hotstuff.ID]*kauripb.Node) idMappings := make(map[hotstuff.ID]int) posToIDMapping := make(map[int]hotstuff.ID) for i, n := range cfg.Nodes() { nodes[hotstuff.ID(n.ID())] = n idMappings[hotstuff.ID(i+1)] = i posToIDMapping[i] = hotstuff.ID(i + 1) } // some code removed for brevity return &FaultFreeTree{ ID: myID, ConfigurationLength: len(nodes), height: height, nodes: nodes, idToPosMapping: idMappings, posToIDMapping: posToIDMapping, } }

Note: I don't think you actually need all three maps, but I didn't check.
Also, it seems the hotstuff.ID generation in the above code could be using only a single one like this:

id := hotstuff.ID(n.ID()) nodes[id] = n idMappings[id] = i posToIDMapping[i] = id

This said, I think a more idiomatic design would be to pass the TreeConfiguration as an argument to the Karui module. But I don't know how to do that with the module system.

meling · 2023-03-14T06:21:38Z

kauri/kauri.go

+	// 	pIDs[id] = index
+	// 	index++
+	// }
+	idMappings := make(map[hotstuff.ID]int)


I believe this code is redundant with the suggested constructor code above.

meling · 2023-03-14T06:24:03Z

kauri/kauri.go

+	k.blockHash = pc.BlockHash()
+	k.currentView = p.Block.View()
+	k.aggregatedContribution = pc.Signature()
+	ids := k.randomizeIDS(k.blockHash, k.leaderRotation.GetLeader(k.currentView))


This can be moved to the tree and merged with InitializeWithPIDs() as outlined in the comment below.

meling · 2023-03-14T06:25:40Z

kauri/tree.go

+// TreeConfiguration is an abstraction for a tree communication model.
+type TreeConfiguration interface {
+	InitializeWithPIDs(ids map[hotstuff.ID]int)
+	GetHeight() int


We avoid Get in Go.

meling · 2023-03-14T06:28:51Z

kauri/tree.go

+
+// FaultFreeTree implements a fault free tree configuration.
+type FaultFreeTree struct {
+	ID                  hotstuff.ID


If I understand correctly this ID field is the current/local tree node. If so, perhaps we should use a more clear name.

Also, maybe it shouldn't be exported.

meling · 2023-03-14T06:42:25Z

kauri/tree.go

+	if !ok {
+		return parent, false
+	}
+	parentPos := t.idToPosMapping[parent]


parentPos := t.idToPosMapping[parent] - 1

would avoid the two parentPos-1 on Lines 84 and 87.

meling · 2023-03-14T06:45:09Z

replica/replica.go

@@ -109,6 +109,7 @@ func New(conf Config, builder modules.Builder) (replica *Replica) {
 		srv.clientSrv.cmdCache,
 		srv.clientSrv.cmdCache,
 	)
+	//builder.Options().SetShouldVerifyVotesSync()


Should this be commented as is, or enabled or removed?

meling

I didn't comment on all methods in FaultFreeTree and Kauri, but several of the methods are non-trivial to review. That is, there should be tests for these methods that operate with different indexes and boundaries that are easy to get wrong.

Kauri module implementation

7ef9f23

fixed lint and removed some unnecessary changes.

e7a5cfc

meling reviewed Mar 14, 2023

View reviewed changes

internal/proto/kauripb/kauri.proto

uint32 ID = 1;

hotstuffpb.QuorumSignature Signature = 2;

uint64 View = 3;

}

Copy link

Member

meling Mar 14, 2023

Choose a reason for hiding this comment

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

newline

meling reviewed Mar 14, 2023

View reviewed changes

kauri/kauri.go

@@ -0,0 +1,320 @@

// Package kauri contains the implementation of the kauri protocol

Copy link

Member

meling Mar 14, 2023

Choose a reason for hiding this comment

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

... the Kauri protocol.

meling reviewed Mar 14, 2023

View reviewed changes

meling requested changes Mar 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kauri module implementation #89

Kauri module implementation #89

hanish520 commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023 •

edited

Loading

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023 •

edited

Loading

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling Mar 14, 2023

meling left a comment

		@@ -0,0 +1,320 @@
		// Package kauri contains the implementation of the kauri protocol

Kauri module implementation #89

Are you sure you want to change the base?

Kauri module implementation #89

Conversation

hanish520 commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meling Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meling left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 21, 2023 •

edited

Loading

meling Mar 14, 2023 •

edited

Loading