New validation package

chain/sync.go

@@ -3,7 +3,6 @@ package chain
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"sync"
@@ -43,6 +42,7 @@ import (
 	"github.com/filecoin-project/lotus/chain/types"
 	"github.com/filecoin-project/lotus/lib/sigs"
 	"github.com/filecoin-project/lotus/metrics"
+	"github.com/filecoin-project/lotus/validation"
 )
 
 var log = logging.Logger("chain")
@@ -442,8 +442,15 @@ func (syncer *Syncer) Sync(ctx context.Context, maybeHead *types.TipSet) error {
 	return nil
 }
 
+// FIXME: Document.
 func isPermanent(err error) bool {
-	return !errors.Is(err, ErrTemporal)
+	if errorWithCategory, ok := err.(validation.ErrorWithCategory); ok {
+		return errorWithCategory.Category() != validation.BlockFutureTimestamp
+	}
+	// FIXME: Include this in `ErrorWithCategory` if it makes sense, it could allow
+	//  us to extend the logic and traverse the hierarchy.
+
+	return false
 }
 
 func (syncer *Syncer) ValidateTipSet(ctx context.Context, fts *store.FullTipSet) error {
@@ -491,8 +498,6 @@ func (syncer *Syncer) minerIsValid(ctx context.Context, maddr address.Address, b
 	return nil
 }
 
-var ErrTemporal = errors.New("temporal error")
-
 // Should match up with 'Semantical Validation' in validation.md in the spec
 func (syncer *Syncer) ValidateBlock(ctx context.Context, b *types.FullBlock) error {
 	ctx, span := trace.StartSpan(ctx, "validateBlock")
@@ -506,6 +511,8 @@ func (syncer *Syncer) ValidateBlock(ctx context.Context, b *types.FullBlock) err
 	baseTs, err := syncer.store.LoadTipSet(types.NewTipSetKey(h.Parents...))
 	if err != nil {
 		return xerrors.Errorf("load parent tipset failed (%s): %w", h.Parents, err)
+		// FIXME: If the block is referencing unknown parents we may want to add this to the
+		//  validation package, but probably there's an early point where that is discovered.
 	}
 
 	prevBeacon, err := syncer.store.GetLatestBeaconEntry(baseTs)
@@ -517,34 +524,36 @@ func (syncer *Syncer) ValidateBlock(ctx context.Context, b *types.FullBlock) err
 
 	// fast checks first
 	if h.BlockSig == nil {
-		return xerrors.Errorf("block had nil signature")
+		return validation.NilSignature.Error()
 	}
 
-	now := uint64(time.Now().Unix())
-	if h.Timestamp > now+build.AllowableClockDrift {
-		return xerrors.Errorf("block was from the future (now=%d, blk=%d): %w", now, h.Timestamp, ErrTemporal)
+	maxTimeDrift := time.Now().Add(build.AllowableClockDrift)
+	if h.Timestamp > uint64(maxTimeDrift.Unix()) {
+		return validation.BlockFutureTimestamp.FromString(fmt.Sprintf("%s > %s", time.Unix(int64(h.Timestamp), 0).Format("%FT%T"), maxTimeDrift.Format("%FT%T")))
+		// FIXME: There surely is a simpler way to format these times.
 	}
-	if h.Timestamp > now {
+	if h.Timestamp > uint64(time.Now().Unix()) {
 		log.Warn("Got block from the future, but within threshold", h.Timestamp, time.Now().Unix())
 	}
 
 	if h.Timestamp < baseTs.MinTimestamp()+(build.BlockDelay*uint64(h.Height-baseTs.Height())) {
 		log.Warn("timestamp funtimes: ", h.Timestamp, baseTs.MinTimestamp(), h.Height, baseTs.Height())
 		diff := (baseTs.MinTimestamp() + (build.BlockDelay * uint64(h.Height-baseTs.Height()))) - h.Timestamp
 
-		return xerrors.Errorf("block was generated too soon (h.ts:%d < base.mints:%d + BLOCK_DELAY:%d * deltaH:%d; diff %d)", h.Timestamp, baseTs.MinTimestamp(), build.BlockDelay, h.Height-baseTs.Height(), diff)
+		return validation.Timestamp.FromString(fmt.Sprintf("h.ts:%d < base.mints:%d + BLOCK_DELAY:%d * deltaH:%d; diff %d",
+			h.Timestamp, baseTs.MinTimestamp(), build.BlockDelay, h.Height-baseTs.Height(), diff))
 	}
 
 	msgsCheck := async.Err(func() error {
 		if err := syncer.checkBlockMessages(ctx, b, baseTs); err != nil {
-			return xerrors.Errorf("block had invalid messages: %w", err)
+			return err
 		}
 		return nil
 	})
 
 	minerCheck := async.Err(func() error {
 		if err := syncer.minerIsValid(ctx, h.Miner, baseTs); err != nil {
-			return xerrors.Errorf("minerIsValid failed: %w", err)
+			return validation.Miner.WrapError(err)
 		}
 		return nil
 	})
@@ -804,6 +813,7 @@ func (syncer *Syncer) VerifyElectionPoStProof(ctx context.Context, h *types.Bloc
 }
 */
 
+// Returns validation errors from the `InvalidMessage` category.
 func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock, baseTs *types.TipSet) error {
 	{
 		var sigCids []cid.Cid // this is what we get for people not wanting the marshalcbor method on the cid type
@@ -815,13 +825,14 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 			pubk, err := syncer.sm.GetBlsPublicKey(ctx, m.From, baseTs)
 			if err != nil {
 				return xerrors.Errorf("failed to load bls public to validate block: %w", err)
+				// FIXME: Is there a validation error here?
 			}
 
 			pubks = append(pubks, pubk)
 		}
 
 		if err := syncer.verifyBlsAggregate(ctx, b.Header.BLSAggregate, sigCids, pubks); err != nil {
-			return xerrors.Errorf("bls aggregate signature was invalid: %w", err)
+			return validation.InvalidMessage.WrapError(err)
 		}
 	}
 
@@ -840,20 +851,22 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 
 	checkMsg := func(m *types.Message) error {
 		if m.To == address.Undef {
-			return xerrors.New("'To' address cannot be empty")
+			return validation.EmptyRecipient.Error()
 		}
 
 		if _, ok := nonces[m.From]; !ok {
 			// `GetActor` does not validate that this is an account actor.
 			act, err := st.GetActor(m.From)
 			if err != nil {
 				return xerrors.Errorf("failed to get actor: %w", err)
+				// FIXME: Some of `GetActor` errors should be in the validation package (it
+				// does many checks besides retrieving information).
 			}
 			nonces[m.From] = act.Nonce
 		}
 
 		if nonces[m.From] != m.Nonce {
-			return xerrors.Errorf("wrong nonce (exp: %d, got: %d)", nonces[m.From], m.Nonce)
+			return validation.WrongNonce.FromString(fmt.Sprintf("exp: %d, got: %d", nonces[m.From], m.Nonce))
 		}
 		nonces[m.From]++
 
@@ -864,7 +877,7 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 
 	for i, m := range b.BlsMessages {
 		if err := checkMsg(m); err != nil {
-			return xerrors.Errorf("block had invalid bls message at index %d: %w", i, err)
+			return validation.InvalidMessage.FromString(fmt.Sprintf("invalid bls message at index %d: %s", i, err))
 		}
 
 		c := cbg.CborCid(m.Cid())
@@ -874,7 +887,7 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 	var secpkCids []cbg.CBORMarshaler
 	for i, m := range b.SecpkMessages {
 		if err := checkMsg(&m.Message); err != nil {
-			return xerrors.Errorf("block had invalid secpk message at index %d: %w", i, err)
+			return validation.InvalidMessage.FromString(fmt.Sprintf("invalid secpk message at index %d: %s", i, err))
 		}
 
 		// `From` being an account actor is only validated inside the `vm.ResolveToKeyAddr` call
@@ -885,7 +898,7 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 		}
 
 		if err := sigs.Verify(&m.Signature, kaddr, m.Message.Cid().Bytes()); err != nil {
-			return xerrors.Errorf("secpk message %s has invalid signature: %w", m.Cid(), err)
+			return validation.InvalidSecpkSignature.FromString(fmt.Sprintf("%s: %s", err.Error(), m.Cid()))
 		}
 
 		c := cbg.CborCid(m.Cid())
@@ -911,7 +924,8 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
 	}
 
 	if b.Header.Messages != mrcid {
-		return fmt.Errorf("messages didnt match message root in header")
+		return validation.InvalidMessagesCID.Error()
+		// FIXME: This is more of a block check than a message check.
 	}
 
 	return nil

validation/category.go

@@ -0,0 +1,39 @@
+package validation
+
+// Errors extracted from `ValidateBlock` and `checkBlockMessages` and converted
+// into more generic "categories" that are related with each other in a hierarchical
+// fashion and allow to spawn errors from them.
+// Explicitly split into separate `var` declarations instead of having a single
+// block (to avoid `go fmt` potentially changing alignment of all errors and
+// producing harder-to-read diffs).
+// FIXME: How to better express this error hierarchy *without* reflection? (nested packages?)
+
+var InvalidBlock = NewHierarchicalErrorClass("invalid block")
+
+var NilSignature = InvalidBlock.Child("nil signature")
+
+var Timestamp = InvalidBlock.Child("invalid timestamp")
+var BlockFutureTimestamp = Timestamp.Child("ahead of current time")
+
+var BlockMinedEarly = InvalidBlock.Child("block was generated too soon")
+
+var Winner = InvalidBlock.Child("not a winner")
+var SlashedMiner = Winner.Child("slashed or invalid miner")
+var NoCandidates = Winner.Child("no candidates")
+var DuplicateCandidates = Winner.Child("duplicate epost candidates")
+
+// FIXME: Might want to include these in some EPost category.
+
+var Miner = InvalidBlock.Child("invalid miner")
+
+var InvalidMessagesCID = InvalidBlock.Child("messages CID didn't match message root in header")
+
+var InvalidMessage = InvalidBlock.Child("invalid message")
+var InvalidBlsSignature = InvalidMessage.Child("invalid bls aggregate signature")
+var InvalidSecpkSignature = InvalidMessage.Child("invalid secpk signature")
+var EmptyRecipient = InvalidMessage.Child("empty 'To' address")
+var WrongNonce = InvalidMessage.Child("wrong nonce")
+
+// FIXME: Errors from `checkBlockMessages` are too generic and should probably be extracted, is there
+//  another place where we validate them (like the message pool)? In that case we need a different
+//  root category like `InvalidMessage`.

validation/error.go

@@ -0,0 +1,95 @@
+package validation
+
+import (
+	"fmt"
+
+	"golang.org/x/xerrors"
+)
+
+// Hierarchy of error categories to programmatically preserve in the language
+// information linking errors from different contexts. Used inside
+// `ErrorWithCategory`, it complements the standard `xerrors.Errorf` wrapping
+// mechanism that preserves call stack information. This is not an error itself,
+// it explicitly does not implement the interface forcing explicit calls to
+// convert a category into a specific error instance used in validation functions.
+// The hierarchy is implemented through a pointer to a (unique) `parent` category
+// (if any). The `description` string identifies the category and will be used
+// as the error message if one is not explicitly provided (like in `FromString()`).
+// FIXME: This is general enough to be outside of this package.
+type ErrorCategory struct {
+	parent      *ErrorCategory
+	description string
+}
+
+func NewHierarchicalErrorClass(description string) *ErrorCategory {
+	return &ErrorCategory{description: description}
+}
+
+// Creates a new (child) category included in this one.
+func (c *ErrorCategory) Child(description string) *ErrorCategory {
+	return &ErrorCategory{parent: c, description: description}
+}
+
+// Returns a new error with the category description as its message.
+func (c *ErrorCategory) Error() error {
+	return &ErrorWithCategory{category: c, msg: c.description, frame: getCallerFrame()}
+}
+
+// Returns a new error from this category with the specified message.
+func (c *ErrorCategory) FromString(msg string) error {
+	return &ErrorWithCategory{category: c, msg: msg, frame: getCallerFrame()}
+}
+
+// Similar to `Error`, returns an error err.
+func (c *ErrorCategory) WrapError(wrappedError error) error {
+	err := c.Error().(ErrorWithCategory)
+	err.wrappedError = wrappedError
+	return err
+}
+
+// Error implementing the `error` interface providing category information
+// and wrapped error semantics similar to `xerrors.Errorf`, including frame
+// information (based on `xerrors.wrapError`).
+type ErrorWithCategory struct {
+	category *ErrorCategory
+
+	// Copied `xerrors.wrapError` attributes (since its a private structure)
+	wrappedError error
+	msg          string
+	frame        xerrors.Frame
+}
+
+func (e *ErrorWithCategory) Category() *ErrorCategory {
+	return e.category
+}
+
+// FIXME: Why can't it have a pointer receiver to satisfy the `error` interface?
+//  (`xerrors.wrapError` uses pointer)
+func (e ErrorWithCategory) Error() string {
+	return fmt.Sprint(e)
+}
+
+func (e *ErrorWithCategory) Format(s fmt.State, v rune) { xerrors.FormatError(e, s, v) }
+
+func (e *ErrorWithCategory) FormatError(p xerrors.Printer) (next error) {
+	p.Print(e.msg)
+	// FIXME: If we're using an external string as the error message we should
+	//  probably want to include the category description here.
+	e.frame.Format(p)
+	return e.wrappedError
+}
+
+func (e *ErrorWithCategory) Unwrap() error {
+	return e.wrappedError
+}
+
+func getCallerFrame() xerrors.Frame {
+	frame := xerrors.Frame{}
+	// if internal.EnableTrace {
+	frame = xerrors.Caller(1)
+	// }
+	// FIXME: Can't reference internal package, if the flag is necessary we can extract
+	//  it from an oracle of `xerrors` behavior (hacky).
+
+	return frame
+}