F3 errors may not be round-tripping through RPC

[rvagg]

These errors are only ever used as values:

lotus/api/api_errors.go

Lines 25 to 42 in 02a8b97

	// ErrF3Disabled signals that F3 consensus process is disabled.
	ErrF3Disabled = errF3Disabled{}
	// ErrF3ParticipationTicketInvalid signals that F3ParticipationTicket cannot be decoded.
	ErrF3ParticipationTicketInvalid = errF3ParticipationTicketInvalid{}
	// ErrF3ParticipationTicketExpired signals that the current GPBFT instance as surpassed the expiry of the ticket.
	ErrF3ParticipationTicketExpired = errF3ParticipationTicketExpired{}
	// ErrF3ParticipationIssuerMismatch signals that the ticket is not issued by the current node.
	ErrF3ParticipationIssuerMismatch = errF3ParticipationIssuerMismatch{}
	// ErrF3ParticipationTooManyInstances signals that participation ticket cannot be
	// issued because it asks for too many instances.
	ErrF3ParticipationTooManyInstances = errF3ParticipationTooManyInstances{}
	// ErrF3ParticipationTicketStartBeforeExisting signals that participation ticket
	// is before the start instance of an existing lease held by the miner.
	ErrF3ParticipationTicketStartBeforeExisting = errF3ParticipationTicketStartBeforeExisting{}
	// ErrF3NotReady signals that the F3 instance isn't ready for participation yet. The caller
	// should back off and try again later.
	ErrF3NotReady = errF3NotReady{}

But go-jsonrpc needs it to match what it was registered with: https://github.com/filecoin-project/go-jsonrpc/blob/c1852724d242f0833191649eb190e51534a7a181/handler.go#L340

But we register them as pointers:

lotus/api/api_errors.go

Lines 55 to 62 in 02a8b97

	RPCErrors.Register(EF3Disabled, new(*errF3Disabled))
	RPCErrors.Register(EF3ParticipationTicketInvalid, new(*errF3ParticipationTicketInvalid))
	RPCErrors.Register(EF3ParticipationTicketExpired, new(*errF3ParticipationTicketExpired))
	RPCErrors.Register(EF3ParticipationIssuerMismatch, new(*errF3ParticipationIssuerMismatch))
	RPCErrors.Register(EF3ParticipationTooManyInstances, new(*errF3ParticipationTooManyInstances))
	RPCErrors.Register(EF3ParticipationTicketStartBeforeExisting, new(*errF3ParticipationTicketStartBeforeExisting))
	RPCErrors.Register(EF3NotReady, new(*errF3NotReady))
	}

They get looked up as the specific type using reflect: https://github.com/filecoin-project/go-jsonrpc/blob/c1852724d242f0833191649eb190e51534a7a181/handler.go#L340

I haven't actually checked this with F3 errors, maybe it's working properly and I'm missing something, but it would be good if we could see a check for an ErrF3 type in an itest where it has to do a jsonrpc roundtrip. If it's not, then they're all generic error types without being coded so the type passes through the rpc boundary as we apparently want.

The equivalents here are api.ErrActorNotFound and api.ErrOutOfGas, which are not set up as reusable constants but they are instantiated as pointers whenever we need them.

Here's a quick diff in go-jsonrpc with a similar setup that demonstrates the issue:

diff --git a/rpc_test.go b/rpc_test.go
index 709d90e..5b7df42 100644
--- a/rpc_test.go
+++ b/rpc_test.go
@@ -1158,6 +1158,14 @@ func (e ErrSomethingBad) Error() string {
        return "something bad has happened"
 }
 
+type ErrSomethingBadder struct{}
+
+func (e ErrSomethingBadder) Error() string {
+       return "something worse has happened"
+}
+
+var ErrSomethingBadderInstance = ErrSomethingBadder{}
+
 type ErrMyErr struct{ str string }
 
 var _ error = ErrSomethingBad{}
@@ -1184,6 +1192,10 @@ func (h *ErrHandler) TestP() error {
        return &ErrSomethingBad{}
 }
 
+func (h *ErrHandler) TestBadder() error {
+       return ErrSomethingBadderInstance
+}
+
 func (h *ErrHandler) TestMy(s string) error {
        return &ErrMyErr{
                str: s,
@@ -1198,12 +1210,14 @@ func TestUserError(t *testing.T) {
        const (
                EBad = iota + FirstUserCode
                EBad2
+               EBad3
                EMy
        )
 
        errs := NewErrors()
        errs.Register(EBad, new(ErrSomethingBad))
        errs.Register(EBad2, new(*ErrSomethingBad))
+       errs.Register(EBad3, new(*ErrSomethingBadder))
        errs.Register(EMy, new(*ErrMyErr))
 
        rpcServer := NewServer(WithServerErrors(errs))
@@ -1216,9 +1230,10 @@ func TestUserError(t *testing.T) {
        // setup client
 
        var client struct {
-               Test   func() error
-               TestP  func() error
-               TestMy func(s string) error
+               Test       func() error
+               TestP      func() error
+               TestBadder func() error
+               TestMy     func(s string) error
        }
        closer, err := NewMergeClient(context.Background(), "ws://"+testServ.Listener.Addr().String(), "ErrHandler", []interface{}{
                &client,
@@ -1231,6 +1246,9 @@ func TestUserError(t *testing.T) {
        e = client.TestP()
        require.True(t, xerrors.Is(e, &ErrSomethingBad{}))
 
+       e = client.TestBadder()
+       require.True(t, xerrors.Is(e, ErrSomethingBadderInstance))
+
        e = client.TestMy("some event")
        require.Error(t, e)
        require.Equal(t, "this happened: some event", e.Error())

^ change to errs.Register(EBad3, new(ErrSomethingBadder)), or var ErrSomethingBadderInstance = &ErrSomethingBadder{} and it'll pass.

@masih would you mind having a look into this?

[rvagg]

Separately @virajbhartiya and I are discussing whether we should loosen the test in go-jsonrpc to avoid a footgun, but for now it's kind of locked in as is and it even has a specific test for pointer vs value types registered as different errors.

[rvagg]

Sorry! I meant @akaladarshi and I are discussing this, working over in @virajbhartiya's go-jsonrpc fork.