cr feedback

Author: timwu20

internal/client/consensus/grandpa/grandpa.go

@@ -237,13 +237,7 @@ func blockImportWithAuthoritySetHardForks[
 	chainInfo := client.Info()
 	genesisHash := chainInfo.GenesisHash
 
-	persistentData, err := loadPersistent[H, N](client, genesisHash, 0, func() (primitives.AuthorityList, error) {
-		authorities, err := genesisAuthoritySetProvider.Get()
-		if err != nil {
-			return nil, err
-		}
-		return authorities, nil
-	})
+	persistentData, err := loadPersistent[H, N](client, genesisHash, 0, genesisAuthoritySetProvider.Get)
 	if err != nil {
 		return nil, LinkHalf[H, N, Hasher, Header, E]{}, err
 	}

internal/client/consensus/grandpa/import.go

@@ -129,9 +129,7 @@ func newGrandpaBlockImport[
 		setID := hardFork.SetID
 		if setID == SetID(sharedAuthoritySet.SetID()) {
 			authoritySet, unlock := sharedAuthoritySet.inner.DataMut()
-			// authoritySet.mtx.Lock()
 			authoritySet.CurrentAuthorities = hardFork.PendingChange.NextAuthorities
-			// authoritySet.mtx.Unlock()
 			unlock()
 		}
 	}
@@ -183,7 +181,7 @@ func FindScheduledChange[
 	id := runtime.OpaqueDigestItemIDConsensus(grandpa.GrandpaEngineID)
 
 	filterLog := func(log grandpa.ConsensusLog) *grandpa.ScheduledChange[N] {
-		scheduledChange, ok := log.(grandpa.ConensusLogScheduledChange[N])
+		scheduledChange, ok := log.(grandpa.ConsensusLogScheduledChange[N])
 		if !ok {
 			return nil
 		}
@@ -405,10 +403,7 @@ func (gbi *GrandpaBlockImport[H, N, Hasher, Header, E]) makeAuthoritiesChanges(
 		// with. we use the minimum between the median and the local
 		// best finalized block.
 		bestFinalizedNumber := gbi.inner.Info().FinalizedNumber
-		canonNumber := bestFinalizedNumber
-		if medianLastFinalizedNumber < bestFinalizedNumber {
-			canonNumber = medianLastFinalizedNumber
-		}
+		canonNumber := min(medianLastFinalizedNumber, bestFinalizedNumber)
 		canonHash, err := gbi.inner.Hash(canonNumber)
 		if err != nil {
 			return nil, err
@@ -536,69 +531,66 @@ func (gbi *GrandpaBlockImport[H, N, Hasher, Header, E]) importState(
 	// Force imported state finality.
 	block.Finalized = true
 	importResult, err := gbi.inner.ImportBlock(block)
-	if err == nil {
-		switch importResult := importResult.(type) {
-		case client_common.ImportResultImported:
-			aux := client_common.ImportedAux(importResult)
-			// We've just imported a new state. We trust the sync module has verified
-			// finality proofs and that the state is correct and final.
-			// So we can read the authority list and set id from the state.
-			gbi.authoritySetHardForksMtx.Lock()
-			gbi.authoritySetHardForks = make(map[H]PendingChange[H, N])
-			gbi.authoritySetHardForksMtx.Unlock()
-			authorities, err := gbi.inner.RuntimeAPI().GrandpaAuthorities(hash)
-			if err != nil {
-				return nil, err
-			}
-			setID, err := gbi.currentSetID(hash)
-			if err != nil {
-				return nil, err
-			}
-			authoritySet, err := NewAuthoritySet[H, N](
-				authorities,
-				uint64(setID),
-				forktree.NewForkTree[H, N, PendingChange[H, N]](),
-				[]PendingChange[H, N]{},
-				AuthoritySetChanges[N]{},
-			)
-			if err != nil {
-				return nil, err
-			}
+	if err != nil {
+		return nil, err
+	}
+	switch importResult := importResult.(type) {
+	case client_common.ImportResultImported:
+		aux := client_common.ImportedAux(importResult)
+		// We've just imported a new state. We trust the sync module has verified
+		// finality proofs and that the state is correct and final.
+		// So we can read the authority list and set id from the state.
+		gbi.authoritySetHardForksMtx.Lock()
+		gbi.authoritySetHardForks = make(map[H]PendingChange[H, N])
+		gbi.authoritySetHardForksMtx.Unlock()
+		authorities, err := gbi.inner.RuntimeAPI().GrandpaAuthorities(hash)
+		if err != nil {
+			return nil, err
+		}
+		setID, err := gbi.currentSetID(hash)
+		if err != nil {
+			return nil, err
+		}
+		authoritySet, err := NewAuthoritySet[H, N](
+			authorities,
+			uint64(setID),
+			forktree.NewForkTree[H, N, PendingChange[H, N]](),
+			[]PendingChange[H, N]{},
+			AuthoritySetChanges[N]{},
+		)
+		if err != nil {
+			return nil, err
+		}
 
-			locked := gbi.authoritySet.inner.Locked()
-			*locked.MutRef() = authoritySet.Clone()
-			defer locked.Unlock()
+		locked := gbi.authoritySet.inner.Locked()
+		*locked.MutRef() = authoritySet.Clone()
+		defer locked.Unlock()
 
-			err = updateAuthoritySet(
-				locked.Data(),
-				nil,
-				func(insertions []api.KeyValue) error {
-					return gbi.inner.InsertAux(insertions, nil)
-				},
-			)
-			if err != nil {
-				return nil, err
-			}
-			newSet := newAuthoritySet[H, N]{
-				CanonNumber: number,
-				CanonHash:   hash,
-				SetID:       setID,
-				Authorities: authorities,
-			}
-			gbi.sendVoterCommands <- voterCommandChangeAuthorities[H, N](newSet)
-			return client_common.ImportResultImported(aux), nil
-		case client_common.ImportResultAlreadyInChain,
-			client_common.ImportResultKnownBad,
-			client_common.ImportResultMissingState,
-			client_common.ImportResultUnknownParent:
-			//			Ok(r) => Ok(r),
-			return importResult, nil
-		default:
-			panic("unreachable")
+		err = updateAuthoritySet(
+			locked.Data(),
+			nil,
+			func(insertions []api.KeyValue) error {
+				return gbi.inner.InsertAux(insertions, nil)
+			},
+		)
+		if err != nil {
+			return nil, err
 		}
-
-	} else {
-		return nil, err
+		newSet := newAuthoritySet[H, N]{
+			CanonNumber: number,
+			CanonHash:   hash,
+			SetID:       setID,
+			Authorities: authorities,
+		}
+		gbi.sendVoterCommands <- voterCommandChangeAuthorities[H, N](newSet)
+		return client_common.ImportResultImported(aux), nil
+	case client_common.ImportResultAlreadyInChain,
+		client_common.ImportResultKnownBad,
+		client_common.ImportResultMissingState,
+		client_common.ImportResultUnknownParent:
+		return importResult, nil
+	default:
+		panic("unreachable")
 	}
 }
 
@@ -712,7 +704,6 @@ func (gbi *GrandpaBlockImport[H, N, Hasher, Header, E]) ImportBlock(
 		importedAux.ClearJustificationRequests = true
 
 	case importAppliedChangesStandard:
-		// this is a standard change, we don't apply it yet, but we will send a
 		// we can't apply this change yet since there are other dependent changes that we
 		// need to apply first, drop any justification that might have been provided with
 		// the block to make sure we request them from `sync` which will ensure they'll be

internal/primitives/consensus/grandpa/grandpa.go

@@ -116,7 +116,7 @@ type ConsensusLog interface {
 // This should be a pure function: i.e. as long as the runtime can interpret
 // the digest type it should return the same result regardless of the current
 // state.
-type ConensusLogScheduledChange[N runtime.Number] ScheduledChange[N]
+type ConsensusLogScheduledChange[N runtime.Number] ScheduledChange[N]
 
 // Force an authority set change.
 //
@@ -152,19 +152,19 @@ type ConsensusLogResume[N runtime.Number] struct {
 	Delay N
 }
 
-func (ConensusLogScheduledChange[N]) isConsensusLog() {}
-func (ConsensusLogForcedChange[N]) isConsensusLog()   {}
-func (ConsensusLogOnDisabled) isConsensusLog()        {}
-func (ConsensusLogPause[N]) isConsensusLog()          {}
-func (ConsensusLogResume[N]) isConsensusLog()         {}
+func (ConsensusLogScheduledChange[N]) isConsensusLog() {}
+func (ConsensusLogForcedChange[N]) isConsensusLog()    {}
+func (ConsensusLogOnDisabled) isConsensusLog()         {}
+func (ConsensusLogPause[N]) isConsensusLog()           {}
+func (ConsensusLogResume[N]) isConsensusLog()          {}
 
 type ConsensusLogVDT[N runtime.Number] struct {
 	inner ConsensusLog
 }
 
 func (mvdt *ConsensusLogVDT[N]) SetValue(value any) (err error) {
 	switch value := value.(type) {
-	case ConensusLogScheduledChange[N]:
+	case ConsensusLogScheduledChange[N]:
 		mvdt.inner = value
 		return
 	case ConsensusLogForcedChange[N]:
@@ -186,7 +186,7 @@ func (mvdt *ConsensusLogVDT[N]) SetValue(value any) (err error) {
 
 func (mvdt ConsensusLogVDT[N]) IndexValue() (index uint, value any, err error) {
 	switch mvdt.inner.(type) {
-	case ConensusLogScheduledChange[N]:
+	case ConsensusLogScheduledChange[N]:
 		return 1, mvdt.inner, nil
 	case ConsensusLogForcedChange[N]:
 		return 2, mvdt.inner, nil
@@ -208,7 +208,7 @@ func (mvdt ConsensusLogVDT[N]) Value() (value any, err error) {
 func (mvdt ConsensusLogVDT[N]) ValueAt(index uint) (value any, err error) {
 	switch index {
 	case 1:
-		return ConensusLogScheduledChange[N]{}, nil
+		return ConsensusLogScheduledChange[N]{}, nil
 	case 2:
 		return ConsensusLogForcedChange[N]{}, nil
 	case 3:

internal/primitives/runtime/runtime.go

@@ -47,8 +47,7 @@ func (j Justifications) Get(engineID ConsensusEngineID) *EncodedJustification {
 	return nil
 }
 
-// Return a copy of the encoded justification for the given consensus
-// engine, if it exists.
+// IntoJustification returns the encoded justification for the given consensus engine, if it exists.
 func (j Justifications) IntoJustification(enginedID ConsensusEngineID) *EncodedJustification {
 	for _, justification := range j {
 		if justification.ConsensusEngineID == enginedID {