Merge pull request #2433 from OffchainLabs/fix-das-store-sig-checking

Remove sig from DAS writer iface, sign in client
OffchainLabs · Jun 26, 2024 · 3ecd01e · 3ecd01e
2 parents 4e0d39c + ffba9cc
commit 3ecd01e
Show file tree

Hide file tree

Showing 14 changed files with 28 additions and 82 deletions.
diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go
@@ -1272,7 +1272,7 @@ func (b *BatchPoster) maybePostSequencerBatch(ctx context.Context) (bool, error)
 			batchPosterDAFailureCounter.Inc(1)
 			return false, fmt.Errorf("%w: nonce changed from %d to %d while creating batch", storage.ErrStorageRace, nonce, gotNonce)
 		}
-		sequencerMsg, err = b.dapWriter.Store(ctx, sequencerMsg, uint64(time.Now().Add(config.DASRetentionPeriod).Unix()), []byte{}, config.DisableDapFallbackStoreDataOnChain)
+		sequencerMsg, err = b.dapWriter.Store(ctx, sequencerMsg, uint64(time.Now().Add(config.DASRetentionPeriod).Unix()), config.DisableDapFallbackStoreDataOnChain)
 		if err != nil {
 			batchPosterDAFailureCounter.Inc(1)
 			return false, err

diff --git a/arbstate/daprovider/util.go b/arbstate/daprovider/util.go
@@ -30,7 +30,7 @@ type DASReader interface {
 
 type DASWriter interface {
 	// Store requests that the message be stored until timeout (UTC time in unix epoch seconds).
-	Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*DataAvailabilityCertificate, error)
+	Store(ctx context.Context, message []byte, timeout uint64) (*DataAvailabilityCertificate, error)
 	fmt.Stringer
 }
 

diff --git a/arbstate/daprovider/writer.go b/arbstate/daprovider/writer.go
@@ -17,7 +17,6 @@ type Writer interface {
 		ctx context.Context,
 		message []byte,
 		timeout uint64,
-		sig []byte,
 		disableFallbackStoreDataOnChain bool,
 	) ([]byte, error)
 }
@@ -32,8 +31,8 @@ type writerForDAS struct {
 	dasWriter DASWriter
 }
 
-func (d *writerForDAS) Store(ctx context.Context, message []byte, timeout uint64, sig []byte, disableFallbackStoreDataOnChain bool) ([]byte, error) {
-	cert, err := d.dasWriter.Store(ctx, message, timeout, []byte{})
+func (d *writerForDAS) Store(ctx context.Context, message []byte, timeout uint64, disableFallbackStoreDataOnChain bool) ([]byte, error) {
+	cert, err := d.dasWriter.Store(ctx, message, timeout)
 	if errors.Is(err, ErrBatchToDasFailed) {
 		if disableFallbackStoreDataOnChain {
 			return nil, errors.New("unable to batch to DAS and fallback storing data on chain is disabled")

diff --git a/cmd/datool/datool.go b/cmd/datool/datool.go
@@ -166,9 +166,9 @@ func startClientStore(args []string) error {
 		if err != nil {
 			return err
 		}
-		cert, err = client.Store(ctx, message, uint64(time.Now().Add(config.DASRetentionPeriod).Unix()), []byte{})
+		cert, err = client.Store(ctx, message, uint64(time.Now().Add(config.DASRetentionPeriod).Unix()))
 	} else if len(config.Message) > 0 {
-		cert, err = client.Store(ctx, []byte(config.Message), uint64(time.Now().Add(config.DASRetentionPeriod).Unix()), []byte{})
+		cert, err = client.Store(ctx, []byte(config.Message), uint64(time.Now().Add(config.DASRetentionPeriod).Unix()))
 	} else {
 		return errors.New("--message or --random-message-size must be specified")
 	}

diff --git a/das/aggregator.go b/das/aggregator.go
@@ -22,7 +22,6 @@ import (
 	"github.com/offchainlabs/nitro/blsSignatures"
 	"github.com/offchainlabs/nitro/das/dastree"
 	"github.com/offchainlabs/nitro/solgen/go/bridgegen"
-	"github.com/offchainlabs/nitro/util/contracts"
 	"github.com/offchainlabs/nitro/util/pretty"
 )
 
@@ -56,7 +55,6 @@ type Aggregator struct {
 	maxAllowedServiceStoreFailures int
 	keysetHash                     [32]byte
 	keysetBytes                    []byte
-	addrVerifier                   *contracts.AddressVerifier
 }
 
 type ServiceDetails struct {
@@ -124,11 +122,6 @@ func NewAggregatorWithSeqInboxCaller(
 		return nil, err
 	}
 
-	var addrVerifier *contracts.AddressVerifier
-	if seqInboxCaller != nil {
-		addrVerifier = contracts.NewAddressVerifier(seqInboxCaller)
-	}
-
 	return &Aggregator{
 		config:                         config.RPCAggregator,
 		services:                       services,
@@ -137,7 +130,6 @@ func NewAggregatorWithSeqInboxCaller(
 		maxAllowedServiceStoreFailures: config.RPCAggregator.AssumedHonest - 1,
 		keysetHash:                     keysetHash,
 		keysetBytes:                    keysetBytes,
-		addrVerifier:                   addrVerifier,
 	}, nil
 }
 
@@ -160,27 +152,8 @@ type storeResponse struct {
 //
 // If Store gets not enough successful responses by the time its context is canceled
 // (eg via TimeoutWrapper) then it also returns an error.
-//
-// If Sequencer Inbox contract details are provided when a das.Aggregator is
-// constructed, calls to Store(...) will try to verify the passed-in data's signature
-// is from the batch poster. If the contract details are not provided, then the
-// signature is not checked, which is useful for testing.
-func (a *Aggregator) Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error) {
-	log.Trace("das.Aggregator.Store", "message", pretty.FirstFewBytes(message), "timeout", time.Unix(int64(timeout), 0), "sig", pretty.FirstFewBytes(sig))
-	if a.addrVerifier != nil {
-		actualSigner, err := DasRecoverSigner(message, sig, timeout)
-		if err != nil {
-			return nil, err
-		}
-		isBatchPosterOrSequencer, err := a.addrVerifier.IsBatchPosterOrSequencer(ctx, actualSigner)
-		if err != nil {
-			return nil, err
-		}
-		if !isBatchPosterOrSequencer {
-			return nil, errors.New("store request not properly signed")
-		}
-	}
-
+func (a *Aggregator) Store(ctx context.Context, message []byte, timeout uint64) (*daprovider.DataAvailabilityCertificate, error) {
+	log.Trace("das.Aggregator.Store", "message", pretty.FirstFewBytes(message), "timeout", time.Unix(int64(timeout), 0))
 	responses := make(chan storeResponse, len(a.services))
 
 	expectedHash := dastree.Hash(message)
@@ -195,7 +168,7 @@ func (a *Aggregator) Store(ctx context.Context, message []byte, timeout uint64,
 				metrics.GetOrRegisterCounter(metricBase+"/error/all/total", nil).Inc(1)
 			}
 
-			cert, err := d.service.Store(storeCtx, message, timeout, sig)
+			cert, err := d.service.Store(storeCtx, message, timeout)
 			if err != nil {
 				incFailureMetric()
 				if errors.Is(err, context.DeadlineExceeded) {

diff --git a/das/aggregator_test.go b/das/aggregator_test.go
@@ -54,7 +54,7 @@ func TestDAS_BasicAggregationLocal(t *testing.T) {
 	Require(t, err)
 
 	rawMsg := []byte("It's time for you to see the fnords.")
-	cert, err := aggregator.Store(ctx, rawMsg, 0, []byte{})
+	cert, err := aggregator.Store(ctx, rawMsg, 0)
 	Require(t, err, "Error storing message")
 
 	for _, storageService := range storageServices {
@@ -123,17 +123,17 @@ type WrapStore struct {
 	DataAvailabilityServiceWriter
 }
 
-func (w *WrapStore) Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error) {
+func (w *WrapStore) Store(ctx context.Context, message []byte, timeout uint64) (*daprovider.DataAvailabilityCertificate, error) {
 	switch w.injector.shouldFail() {
 	case success:
-		return w.DataAvailabilityServiceWriter.Store(ctx, message, timeout, sig)
+		return w.DataAvailabilityServiceWriter.Store(ctx, message, timeout)
 	case immediateError:
 		return nil, errors.New("expected Store failure")
 	case tooSlow:
 		<-ctx.Done()
 		return nil, ctx.Err()
 	case dataCorruption:
-		cert, err := w.DataAvailabilityServiceWriter.Store(ctx, message, timeout, sig)
+		cert, err := w.DataAvailabilityServiceWriter.Store(ctx, message, timeout)
 		if err != nil {
 			return nil, err
 		}
@@ -214,7 +214,7 @@ func testConfigurableStorageFailures(t *testing.T, shouldFailAggregation bool) {
 	Require(t, err)
 
 	rawMsg := []byte("It's time for you to see the fnords.")
-	cert, err := aggregator.Store(ctx, rawMsg, 0, []byte{})
+	cert, err := aggregator.Store(ctx, rawMsg, 0)
 	if !shouldFailAggregation {
 		Require(t, err, "Error storing message")
 	} else {

diff --git a/das/das.go b/das/das.go
@@ -20,7 +20,7 @@ import (
 
 type DataAvailabilityServiceWriter interface {
 	// Store requests that the message be stored until timeout (UTC time in unix epoch seconds).
-	Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error)
+	Store(ctx context.Context, message []byte, timeout uint64) (*daprovider.DataAvailabilityCertificate, error)
 	fmt.Stringer
 }
 

diff --git a/das/dasRpcClient.go b/das/dasRpcClient.go
@@ -57,7 +57,7 @@ func NewDASRPCClient(target string, signer signature.DataSignerFunc, maxStoreChu
 	}, nil
 }
 
-func (c *DASRPCClient) Store(ctx context.Context, message []byte, timeout uint64, _ []byte) (*daprovider.DataAvailabilityCertificate, error) {
+func (c *DASRPCClient) Store(ctx context.Context, message []byte, timeout uint64) (*daprovider.DataAvailabilityCertificate, error) {
 	timestamp := uint64(time.Now().Unix())
 	nChunks := uint64(len(message)) / c.chunkSize
 	lastChunkSize := uint64(len(message)) % c.chunkSize

diff --git a/das/dasRpcServer.go b/das/dasRpcServer.go
@@ -125,7 +125,7 @@ func (s *DASRPCServer) Store(ctx context.Context, message hexutil.Bytes, timeout
 		return nil, err
 	}
 
-	cert, err := s.daWriter.Store(ctx, message, uint64(timeout), nil)
+	cert, err := s.daWriter.Store(ctx, message, uint64(timeout))
 	if err != nil {
 		return nil, err
 	}
@@ -325,7 +325,7 @@ func (s *DASRPCServer) CommitChunkedStore(ctx context.Context, batchId hexutil.U
 		return nil, err
 	}
 
-	cert, err := s.daWriter.Store(ctx, message, timeout, nil)
+	cert, err := s.daWriter.Store(ctx, message, timeout)
 	success := false
 	defer func() {
 		if success {

diff --git a/das/das_test.go b/das/das_test.go
@@ -56,7 +56,7 @@ func testDASStoreRetrieveMultipleInstances(t *testing.T, storageType string) {
 
 	timeout := uint64(time.Now().Add(time.Hour * 24).Unix())
 	messageSaved := []byte("hello world")
-	cert, err := daWriter.Store(firstCtx, messageSaved, timeout, []byte{})
+	cert, err := daWriter.Store(firstCtx, messageSaved, timeout)
 	Require(t, err, "Error storing message")
 	if cert.Timeout != timeout {
 		Fail(t, fmt.Sprintf("Expected timeout of %d in cert, was %d", timeout, cert.Timeout))
@@ -145,7 +145,7 @@ func testDASMissingMessage(t *testing.T, storageType string) {
 
 	messageSaved := []byte("hello world")
 	timeout := uint64(time.Now().Add(time.Hour * 24).Unix())
-	cert, err := daWriter.Store(ctx, messageSaved, timeout, []byte{})
+	cert, err := daWriter.Store(ctx, messageSaved, timeout)
 	Require(t, err, "Error storing message")
 	if cert.Timeout != timeout {
 		Fail(t, fmt.Sprintf("Expected timeout of %d in cert, was %d", timeout, cert.Timeout))

diff --git a/das/extra_signature_checker_test.go b/das/extra_signature_checker_test.go
@@ -5,25 +5,19 @@ package das
 
 import (
 	"bytes"
-	"context"
 	"encoding/hex"
 	"errors"
 	"io/ioutil"
 	"testing"
 
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/crypto"
 
 	"github.com/offchainlabs/nitro/arbstate/daprovider"
 	"github.com/offchainlabs/nitro/util/signature"
 )
 
-type StubSignatureCheckDAS struct {
-	keyDir string
-}
-
-func (s *StubSignatureCheckDAS) Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error) {
-	pubkeyEncoded, err := ioutil.ReadFile(s.keyDir + "/ecdsa.pub")
+func checkSig(keyDir string, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error) {
+	pubkeyEncoded, err := ioutil.ReadFile(keyDir + "/ecdsa.pub")
 	if err != nil {
 		return nil, err
 	}
@@ -39,22 +33,6 @@ func (s *StubSignatureCheckDAS) Store(ctx context.Context, message []byte, timeo
 	return nil, nil
 }
 
-func (s *StubSignatureCheckDAS) ExpirationPolicy(ctx context.Context) (daprovider.ExpirationPolicy, error) {
-	return daprovider.KeepForever, nil
-}
-
-func (s *StubSignatureCheckDAS) GetByHash(ctx context.Context, hash common.Hash) ([]byte, error) {
-	return []byte{}, nil
-}
-
-func (s *StubSignatureCheckDAS) HealthCheck(ctx context.Context) error {
-	return nil
-}
-
-func (s *StubSignatureCheckDAS) String() string {
-	return "StubSignatureCheckDAS"
-}
-
 func TestExtraSignatureCheck(t *testing.T) {
 	keyDir := t.TempDir()
 	err := GenerateAndStoreECDSAKeys(keyDir)
@@ -64,13 +42,11 @@ func TestExtraSignatureCheck(t *testing.T) {
 	Require(t, err)
 	signer := signature.DataSignerFromPrivateKey(privateKey)
 
-	var da DataAvailabilityServiceWriter = &StubSignatureCheckDAS{keyDir}
-
 	msg := []byte("Hello world")
 	timeout := uint64(1234)
 	sig, err := applyDasSigner(signer, msg, timeout)
 	Require(t, err)
-	_, err = da.Store(context.Background(), msg, timeout, sig)
+	_, err = checkSig(keyDir, msg, timeout, sig)
 	Require(t, err)
 }
 

diff --git a/das/panic_wrapper.go b/das/panic_wrapper.go
@@ -26,8 +26,8 @@ func (w *WriterPanicWrapper) String() string {
 	return fmt.Sprintf("WriterPanicWrapper{%v}", w.DataAvailabilityServiceWriter)
 }
 
-func (w *WriterPanicWrapper) Store(ctx context.Context, message []byte, timeout uint64, sig []byte) (*daprovider.DataAvailabilityCertificate, error) {
-	cert, err := w.DataAvailabilityServiceWriter.Store(ctx, message, timeout, sig)
+func (w *WriterPanicWrapper) Store(ctx context.Context, message []byte, timeout uint64) (*daprovider.DataAvailabilityCertificate, error) {
+	cert, err := w.DataAvailabilityServiceWriter.Store(ctx, message, timeout)
 	if err != nil {
 		panic(fmt.Sprintf("panic wrapper Store: %v", err))
 	}

diff --git a/das/rpc_test.go b/das/rpc_test.go
@@ -96,7 +96,7 @@ func testRpcImpl(t *testing.T, size, times int, concurrent bool) {
 	runStore := func() {
 		defer wg.Done()
 		msg := testhelpers.RandomizeSlice(make([]byte, size))
-		cert, err := rpcAgg.Store(ctx, msg, 0, nil)
+		cert, err := rpcAgg.Store(ctx, msg, 0)
 		testhelpers.RequireImpl(t, err)
 
 		retrievedMessage, err := storageService.GetByHash(ctx, cert.DataHash)

diff --git a/das/sign_after_store_das_writer.go b/das/sign_after_store_das_writer.go
@@ -104,10 +104,8 @@ func NewSignAfterStoreDASWriter(ctx context.Context, config DataAvailabilityConf
 	}, nil
 }
 
-func (d *SignAfterStoreDASWriter) Store(
-	ctx context.Context, message []byte, timeout uint64, sig []byte,
-) (c *daprovider.DataAvailabilityCertificate, err error) {
-	log.Trace("das.SignAfterStoreDASWriter.Store", "message", pretty.FirstFewBytes(message), "timeout", time.Unix(int64(timeout), 0), "sig", pretty.FirstFewBytes(sig), "this", d)
+func (d *SignAfterStoreDASWriter) Store(ctx context.Context, message []byte, timeout uint64) (c *daprovider.DataAvailabilityCertificate, err error) {
+	log.Trace("das.SignAfterStoreDASWriter.Store", "message", pretty.FirstFewBytes(message), "timeout", time.Unix(int64(timeout), 0), "this", d)
 	c = &daprovider.DataAvailabilityCertificate{
 		Timeout:     timeout,
 		DataHash:    dastree.Hash(message),