Fix chain optimization (#2422)

* Optimize chain optimization

The current chain optimization is badly defined, as you need to give it a block,
but for some blocks the optimization will always fail, as the nodes will not
accept the new forward-links.

This PR re-defines the chain optimization:
- if given a skipchain-ID it optimizes the route from the genesis to the latest block
- if given a random skipblock-ID, it optimizes that block only

This will allow to crawl through all blocks and ask an optimization for every
block that is lacking forward-links.

Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>

scmgr/app.go

@@ -480,16 +480,12 @@ func scOptimize(c *cli.Context) error {
 
 	// If a genesis block is provided, we optimize the entire chain
 	if sb.Index == 0 {
-		reply, err := cl.GetUpdateChain(roster, id)
-		if err != nil {
-			return fmt.Errorf("couldn't get the latest block: %v", err)
-		}
-
-		sb = reply.Update[len(reply.Update)-1]
+		log.Infof("Optimizing chain %x", sb.SkipChainID())
+	} else {
+		log.Infof("Optimizing block with index %d in chain %x",
+			sb.Index, sb.SkipChainID())
 	}
 
-	log.Infof("Optimizing chain %x for block at index %d...", sb.SkipChainID(), sb.Index)
-
 	reply, err := cl.OptimizeProof(roster, sb.Hash)
 	if err != nil {
 		return fmt.Errorf("couldn't optimize the proof: %v", err)

skipchain/msgs.go

@@ -88,9 +88,14 @@ type StoreSkipBlockReply struct {
 	Latest   *SkipBlock
 }
 
-// OptimizeProofRequest is request to create missing forward links
+// OptimizeProofRequest is request to create missing forward links.
+// If the ID is the skipchain-ID,
+// the proofs from the genesis block to the latest block are optimized.
+// If the ID is a block in the chain,
+// the proofs from that block only are optimized.
 type OptimizeProofRequest struct {
-	ID     SkipBlockID
+	ID SkipBlockID
+	// Deprecated: will not be used in the new call to OptimizeProof
 	Roster *onet.Roster
 }
 

skipchain/skipchain.go

@@ -15,6 +15,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"math"
 	"reflect"
 	"runtime"
 	"strconv"
@@ -423,67 +424,193 @@ func sendForwardLinkRequest(ro *onet.Roster, req *ForwardSignature, reply *Forwa
 	return err
 }
 
-// OptimizeProof creates missing forward links to optimize the proof of the block
-// at the given index.
+// OptimizeProof creates missing forward links to optimize the proof either
+//   - from the genesis block to the latest block of the given skipchain-ID
+//   - for the given block
 func (s *Service) OptimizeProof(req *OptimizeProofRequest) (*OptimizeProofReply, error) {
-	pr, err := s.db.GetProofForID(req.ID)
+	sb := s.db.GetByID(req.ID)
+	if sb == nil {
+		return nil, xerrors.New("didn't find that block-id in our DB")
+	}
+	if sb.Index == 0 {
+		return s.optimizeChain(req.ID)
+	}
+	return s.optimizeBlock(sb)
+}
+
+// optimizeBlock adds missing forward-links to one block only.
+// It requests all missing forward-links,
+// but cannot guarantee that any of them will be created.
+func (s *Service) optimizeBlock(sb *SkipBlock) (*OptimizeProofReply, error) {
+	latest, err := s.db.GetLatestByID(sb.SkipChainID())
+	if err != nil {
+		return nil, xerrors.Errorf("couldn't get latest block: %v", err)
+	}
+
+	maxHeight, _ := sb.pathForIndex(latest.Index)
+	if sb.GetForwardLen() > maxHeight {
+		return nil, xerrors.New("nothing to optimize for this block")
+	}
+
+	optimized := false
+	for height := sb.GetForwardLen(); height <= maxHeight; height++ {
+		indexTo := sb.Index +
+			int(math.Pow(float64(sb.BaseHeight), float64(height)))
+		if indexTo > latest.Index {
+			break
+		}
+		pr, err := s.db.GetProofFromIndex(sb.SkipChainID(), indexTo)
+		if err != nil {
+			return nil, xerrors.Errorf("couldn't get block at index %d: %v",
+				indexTo, err)
+		}
+		to := pr[len(pr)-1]
+
+		log.Lvlf2("Adding forward-link level %d to block %d", height, sb.Index)
+		if err := s.addForwardLink(height, sb, to); err != nil {
+			log.Warnf("Couldn't add a forwardlink from %d to %d "+
+				"with height %d: %v",
+				sb.Index, to.Index, height, err)
+		} else {
+			optimized = true
+		}
+	}
+
+	if !optimized {
+		return nil, xerrors.New("couldn't optimize this block")
+	}
+
+	pr, err := s.db.GetProofForID(sb.Hash)
+	if err != nil {
+		return nil, xerrors.Errorf("couldn't get proof for latest block: %v",
+			err)
+	}
+	if err := s.optimizeSend(pr); err != nil {
+		return nil, xerrors.Errorf("couldn't propagate proof: %v", err)
+	}
+	return &OptimizeProofReply{Proof: pr}, nil
+}
+
+// addForwardLink asks the roster of the block to create the forward-links,
+// but does not propagate those links.
+func (s *Service) addForwardLink(targetHeight int, from, to *SkipBlock) error {
+	req := &ForwardSignature{
+		TargetHeight: targetHeight,
+		Previous:     from.Hash,
+		Newest:       to,
+	}
+	reply := &ForwardSignatureReply{}
+
+	log.Lvlf2("requesting missing forward-link from index %d to"+
+		" %d with height %d", from.Index, to.Index, targetHeight)
+	// The signature must be created by the roster of the block
+	err := sendForwardLinkRequest(from.Roster, req, reply)
+
+	if err != nil {
+		return xerrors.Errorf("could not create a missing forward link: %v",
+			err)
+	}
+
+	// save the new forward link
+	err = from.AddForwardLink(reply.Link, targetHeight)
+	if err != nil {
+		return xerrors.Errorf("could not store the missing forward-link"+
+			": %v", err)
+	}
+	return nil
+}
+
+// optimizeChain optimizes all forward-links from the genesis to the latest
+// block.
+func (s *Service) optimizeChain(scID SkipBlockID) (
+	*OptimizeProofReply, error) {
+	sbs, err := s.db.GetProof(scID)
 	if err != nil {
 		return nil, err
 	}
+	pr := Proof(sbs)
 
 	target := pr[len(pr)-1]
 	index := 0
-	h := 0
 	newProof := Proof{}
+	optimized := false
 
 	for _, sb := range pr[:len(pr)-1] {
 		if sb.Index < index {
 			// Skip blocks thanks to the new forward-link.
 			continue
 		}
 
-		h, index = sb.pathForIndex(target.Index)
-
-		if h > 0 && len(sb.ForwardLink) <= h {
-			to := pr.Search(index)
-			if to == nil {
-				return nil, fmt.Errorf("chain is inconsistent: block at index %d not found", index)
-			}
+		var opt bool
+		index, opt, err = s.optimizeCheck(pr, sb.Index, target.Index)
+		if err != nil {
+			return nil, xerrors.Errorf("couldn't optimize block: %v", err)
+		}
+		optimized = optimized || opt
 
-			req := &ForwardSignature{
-				TargetHeight: h,
-				Previous:     sb.Hash,
-				Newest:       to,
-			}
-			reply := &ForwardSignatureReply{}
+		// sb is already updated, as we're only using a pointer.
+		newProof = append(newProof, sb)
+	}
 
-			log.Lvlf2("requesting missing forward-link at index %d with height %d / %d", sb.Index, h, index)
-			// The signature must be created by the roster of the block
-			err := sendForwardLinkRequest(sb.Roster, req, reply)
+	newProof = append(newProof, target)
 
-			if err != nil {
-				log.Error("could not create a missing forward link:", err)
-				// reset the index to try to create lower levels
-				index = sb.Index
-			} else {
-				// save the new forward link
-				err = sb.AddForwardLink(reply.Link, h)
-				if err != nil {
-					log.Error("could not store the missing forward-link:", err)
-					index = sb.Index
-				}
-			}
+	if optimized {
+		if err := s.optimizeSend(newProof); err != nil {
+			return nil, xerrors.Errorf("couldn't propagate proof: %v", err)
 		}
+	}
 
-		newProof = append(newProof, sb)
+	return &OptimizeProofReply{newProof}, nil
+}
+
+// checks whether the current block is missing a forward-link and requests a
+// new if required.
+func (s *Service) optimizeCheck(pr Proof, current, target int) (index int,
+	optimized bool, err error) {
+	sb := pr.Search(current)
+	if sb == nil {
+		return -1, false, xerrors.New("didn't find current block")
 	}
+	var h int
+	h, index = sb.pathForIndex(target)
+	if h > 0 && len(sb.ForwardLink) <= h {
+		to := pr.Search(index)
+		if to == nil {
+			return -1, false, xerrors.Errorf(
+				"chain is inconsistent: block at index %d not found", index)
+		}
 
-	newProof = append(newProof, target)
+		err := s.addForwardLink(h, sb, to)
+		if err != nil {
+			log.Errorf("Couldn't create forward-link: %v", err)
+			index = sb.Index
+		} else {
+			optimized = true
+		}
+	}
+	return
+}
 
-	// Propagate the optimized proof to the given roster
-	err = s.startPropagation(s.propagateProof, req.Roster, &PropagateProof{newProof})
+// optimizeSend propagates the new blocks to all nodes.
+func (s *Service) optimizeSend(newProof Proof) error {
+	log.Lvl2("Done creating forwardlinks, propagating new proofs")
+	roster := onet.NewRoster(newProof[0].Roster.List)
+	if len(newProof) > 1 {
+		for i, pr := range newProof[1:] {
+			roster = roster.Concat(pr.Roster.List...)
+
+			if pr.GetForwardLen() > 0 {
+				to := pr.ForwardLink[pr.GetForwardLen()-1].To
+				log.Lvlf3("%d: Block %d / %x with fl-len %d pointing to %x",
+					i, pr.Index, pr.Hash[:], pr.GetForwardLen(), to[:])
+			}
+		}
+	}
 
-	return &OptimizeProofReply{newProof}, err
+	// Propagate the optimized proof to all nodes that were defined in any of
+	// the blocks.
+	return s.startPropagation(s.propagateProof, roster,
+		&PropagateProof{newProof})
 }
 
 // GetUpdateChain returns a slice of SkipBlocks which describe the part of the

skipchain/skipchain_test.go

@@ -764,7 +764,7 @@ func TestService_ForgedPropagationMessage(t *testing.T) {
 	// checks that the targets have to match
 	err = service.propagateProofHandler(&PropagateProof{append(p1[:2], p2[2], p1[3])})
 	require.NotNil(t, err)
-	require.Contains(t, err.Error(), "Wrong targets")
+	require.Contains(t, err.Error(), "invalid or missing forward-links")
 
 	// checks that the signature must match
 	forgedBlock := NewSkipBlock()
@@ -1627,7 +1627,8 @@ func testOptimizeProof(t *testing.T, numBlock, base, max, expected int) {
 	sk5 := srvs[4].Service(ServiceName).(*Service)
 	sk6 := srvs[5].Service(ServiceName).(*Service)
 
-	sbRoot, err := makeGenesisRosterArgs(sk1, roster, nil, VerificationStandard, base, max)
+	sbRoot, err := makeGenesisRosterArgs(sk1, ro, nil, VerificationStandard,
+		base, max)
 	require.NoError(t, err)
 
 	sb := NewSkipBlock()
@@ -1646,12 +1647,79 @@ func testOptimizeProof(t *testing.T, numBlock, base, max, expected int) {
 	log.Lvl1("Request to optimize the proof")
 
 	// Ask host 5 to optimize (not the leader)
-	opr, err := sk5.OptimizeProof(&OptimizeProofRequest{Roster: ro, ID: reply.Latest.Hash})
+	opr, err := sk5.OptimizeProof(&OptimizeProofRequest{ID: sbRoot.SkipChainID()})
 	require.NoError(t, err)
 	require.Equal(t, expected, len(opr.Proof))
 
-	// And verify the proof is propagated to the roster we asked for
-	sbs, err := sk6.db.GetProofForID(reply.Latest.Hash)
+	// If base == 1, there is nothing to optimize,
+	// so the blocks won't be sent to the other nodes.
+	if base > 1 {
+		// And verify the proof is propagated to the roster we asked for
+		sbs, err := sk6.db.GetProofForID(reply.Latest.Hash)
+		require.NoError(t, err)
+		require.Equal(t, expected, len(sbs))
+	}
+}
+
+// This tests what happens when a block in the middle of the chain is
+// optimized. One corner-case was when a block is optimized while a previous
+// block points further. Given a skipchain with base-height 2 and the
+// following blocks with [block-index, max-height, current/height]:
+//   0 4 4
+//   1 1 1
+//   2 2 1
+// Now if block #2 is optimized, the proof that will be sent includes block
+// 0 and 2, but block 0 points further than 2.
+// This needs to be handled correctly.
+func TestOptimizeMiddle(t *testing.T) {
+	const numBlock = 9
+
+	local := onet.NewLocalTest(cothority.Suite)
+	defer local.CloseAll()
+	srvs, roster, _ := local.MakeSRS(cothority.Suite, 3, skipchainSID)
+
+	sks := make([]*Service, len(srvs))
+	for i, srv := range srvs {
+		sks[i] = srv.Service(ServiceName).(*Service)
+	}
+	leader := sks[0]
+
+	blocks := make([]*SkipBlock, numBlock)
+	var err error
+	blocks[0], err = makeGenesisRosterArgs(leader, roster, nil,
+		VerificationStandard, 2, 10)
+	require.NoError(t, err)
+
+	leader.disableForwardLink = true
+
+	for i := 1; i < numBlock; i++ {
+		sb := NewSkipBlock()
+		sb.Roster = roster
+		reply, err := leader.StoreSkipBlock(
+			&StoreSkipBlock{TargetSkipChainID: blocks[0].Hash, NewBlock: sb})
+		require.NoError(t, err)
+		blocks[i] = reply.Latest
+	}
+
+	leader.disableForwardLink = false
+
+	// Optimize the genesis-block,
+	// will create an empty forward-link at index 1,
+	// plus a valid forward-link at index 2.
+	log.Lvl1("Request to optimize the proof for the chain")
+	opr, err := sks[2].OptimizeProof(&OptimizeProofRequest{ID: blocks[0].Hash})
+	require.NoError(t, err)
+	require.Equal(t, 4, opr.Proof[0].GetForwardLen())
+
+	log.Lvl1("Request to optimize the proof for block index 2")
+	opr, err = sks[2].OptimizeProof(&OptimizeProofRequest{ID: blocks[2].Hash})
+	require.NoError(t, err)
+	require.Equal(t, 3, len(opr.Proof))
+	require.Equal(t, 2, opr.Proof[2].GetForwardLen())
+
+	log.Lvl1("Request to optimize the proof for block index 4")
+	opr, err = sks[2].OptimizeProof(&OptimizeProofRequest{ID: blocks[4].Hash})
 	require.NoError(t, err)
-	require.Equal(t, expected, len(sbs))
+	require.Equal(t, 4, len(opr.Proof))
+	require.Equal(t, 3, opr.Proof[3].GetForwardLen())
 }