fix: save stamps for all upload types regardless of any errors (#4327)

pkg/api/api.go

@@ -772,7 +772,7 @@ type putterOptions struct {
 type putterSessionWrapper struct {
 	storer.PutterSession
 	stamper postage.Stamper
-	save    func(bool) error
+	save    func() error
 }
 
 func (p *putterSessionWrapper) Put(ctx context.Context, chunk swarm.Chunk) error {
@@ -788,14 +788,14 @@ func (p *putterSessionWrapper) Done(ref swarm.Address) error {
 	if err != nil {
 		return err
 	}
-	return p.save(true)
+	return p.save()
 }
 
 func (p *putterSessionWrapper) Cleanup() error {
-	return errors.Join(p.PutterSession.Cleanup(), p.save(false))
+	return errors.Join(p.PutterSession.Cleanup(), p.save())
 }
 
-func (s *Service) getStamper(batchID []byte) (postage.Stamper, func(bool) error, error) {
+func (s *Service) getStamper(batchID []byte) (postage.Stamper, func() error, error) {
 	exists, err := s.batchStore.Exists(batchID)
 	if err != nil {
 		return nil, nil, fmt.Errorf("batch exists: %w", err)

pkg/api/postage.go

@@ -276,7 +276,7 @@ func (s *Service) postageGetStampBucketsHandler(w http.ResponseWriter, r *http.R
 	}
 	hexBatchID := hex.EncodeToString(paths.BatchID)
 
-	issuer, save, err := s.post.GetStampIssuer(paths.BatchID)
+	issuer, _, err := s.post.GetStampIssuer(paths.BatchID)
 	if err != nil {
 		logger.Debug("get stamp issuer: get issuer failed", "batch_id", hexBatchID, "error", err)
 		logger.Error(nil, "get stamp issuer: get issuer failed")
@@ -303,13 +303,6 @@ func (s *Service) postageGetStampBucketsHandler(w http.ResponseWriter, r *http.R
 		resp.Buckets[i] = bucketData{BucketID: uint32(i), Collisions: v}
 	}
 
-	if err := save(false); err != nil {
-		logger.Debug("get stamp issuer: save issuer failed", "batch_id", hexBatchID, "error", err)
-		logger.Error(nil, "get stamp issuer: save issuer failed")
-		jsonhttp.InternalServerError(w, "save issuer failed")
-		return
-	}
-
 	jsonhttp.OK(w, resp)
 }
 

pkg/api/pss.go

@@ -98,7 +98,6 @@ func (s *Service) pssPostHandler(w http.ResponseWriter, r *http.Request) {
 
 	err = s.pss.Send(r.Context(), topic, payload, stamper, queries.Recipient, targets)
 	if err != nil {
-		err = errors.Join(err, save(false))
 		logger.Debug("send payload failed", "topic", paths.Topic, "error", err)
 		logger.Error(nil, "send payload failed")
 		switch {
@@ -110,8 +109,7 @@ func (s *Service) pssPostHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = save(true)
-	if err != nil {
+	if err = save(); err != nil {
 		logger.Debug("save stamp failed", "error", err)
 		logger.Error(nil, "save stamp failed")
 		jsonhttp.InternalServerError(w, "pss send failed")

pkg/api/stewardship.go

@@ -70,20 +70,20 @@ func (s *Service) stewardshipPutHandler(w http.ResponseWriter, r *http.Request)
 
 	err = s.steward.Reupload(r.Context(), paths.Address, stamper)
 	if err != nil {
-		err = errors.Join(err, save(false))
+		err = errors.Join(err, save())
 		logger.Debug("re-upload failed", "chunk_address", paths.Address, "error", err)
 		logger.Error(nil, "re-upload failed")
 		jsonhttp.InternalServerError(w, "re-upload failed")
 		return
 	}
 
-	err = save(true)
-	if err != nil {
+	if err = save(); err != nil {
 		logger.Debug("unable to save stamper data", "batchID", batchID, "error", err)
 		logger.Error(nil, "unable to save stamper data")
 		jsonhttp.InternalServerError(w, "unable to save stamper data")
 		return
 	}
+
 	jsonhttp.OK(w, nil)
 }
 

pkg/postage/mock/service.go

@@ -85,9 +85,9 @@ func (m *mockPostage) StampIssuers() []*postage.StampIssuer {
 	return issuers
 }
 
-func (m *mockPostage) GetStampIssuer(id []byte) (*postage.StampIssuer, func(bool) error, error) {
+func (m *mockPostage) GetStampIssuer(id []byte) (*postage.StampIssuer, func() error, error) {
 	if m.acceptAll {
-		return postage.NewStampIssuer("test fallback", "test identity", id, big.NewInt(3), 24, 6, 1000, true), func(_ bool) error { return nil }, nil
+		return postage.NewStampIssuer("test fallback", "test identity", id, big.NewInt(3), 24, 6, 1000, false), func() error { return nil }, nil
 	}
 
 	m.issuerLock.Lock()
@@ -98,7 +98,7 @@ func (m *mockPostage) GetStampIssuer(id []byte) (*postage.StampIssuer, func(bool
 		return nil, nil, postage.ErrNotFound
 	}
 
-	return i, func(_ bool) error {
+	return i, func() error {
 		return nil
 	}, nil
 }

pkg/postage/service.go

@@ -32,7 +32,7 @@ var (
 type Service interface {
 	Add(*StampIssuer) error
 	StampIssuers() []*StampIssuer
-	GetStampIssuer([]byte) (*StampIssuer, func(bool) error, error)
+	GetStampIssuer([]byte) (*StampIssuer, func() error, error)
 	IssuerUsable(*StampIssuer) bool
 	BatchEventListener
 	BatchExpiryHandler
@@ -147,7 +147,7 @@ func (ps *service) IssuerUsable(st *StampIssuer) bool {
 }
 
 // GetStampIssuer finds a stamp issuer by batch ID.
-func (ps *service) GetStampIssuer(batchID []byte) (*StampIssuer, func(bool) error, error) {
+func (ps *service) GetStampIssuer(batchID []byte) (*StampIssuer, func() error, error) {
 	ps.lock.Lock()
 	defer ps.lock.Unlock()
 
@@ -156,11 +156,8 @@ func (ps *service) GetStampIssuer(batchID []byte) (*StampIssuer, func(bool) erro
 			if !ps.IssuerUsable(st) {
 				return nil, nil, ErrNotUsable
 			}
-			return st, func(update bool) error {
-				if update {
-					return ps.save(st)
-				}
-				return nil
+			return st, func() error {
+				return ps.save(st)
 			}, nil
 		}
 	}

pkg/postage/service_test.go

@@ -7,14 +7,14 @@ package postage_test
 import (
 	crand "crypto/rand"
 	"errors"
+	"io"
+	"math/big"
+	"testing"
+
 	"github.com/ethersphere/bee/pkg/postage"
 	pstoremock "github.com/ethersphere/bee/pkg/postage/batchstore/mock"
 	postagetesting "github.com/ethersphere/bee/pkg/postage/testing"
 	"github.com/ethersphere/bee/pkg/storage/inmemstore"
-	"github.com/google/go-cmp/cmp"
-	"io"
-	"math/big"
-	"testing"
 )
 
 // TestSaveLoad tests the idempotence of saving and loading the postage.Service
@@ -116,7 +116,7 @@ func TestGetStampIssuer(t *testing.T) {
 			if err != nil {
 				t.Fatalf("expected no error, got %v", err)
 			}
-			_ = save(true)
+			_ = save()
 			if st.Label() != string(id) {
 				t.Fatalf("wrong issuer returned")
 			}
@@ -163,7 +163,7 @@ func TestGetStampIssuer(t *testing.T) {
 		if st.Label() != "recovered" {
 			t.Fatal("wrong issuer returned")
 		}
-		err = sv(true)
+		err = sv()
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -177,7 +177,7 @@ func TestGetStampIssuer(t *testing.T) {
 		if err != nil {
 			t.Fatalf("expected no error, got %v", err)
 		}
-		_ = save(true)
+		_ = save()
 		if stampIssuer.Amount().Cmp(big.NewInt(13)) != 0 {
 			t.Fatalf("expected amount %d got %d", 13, stampIssuer.Amount().Int64())
 		}
@@ -191,40 +191,12 @@ func TestGetStampIssuer(t *testing.T) {
 		if err != nil {
 			t.Fatalf("expected no error, got %v", err)
 		}
-		_ = save(true)
+		_ = save()
 		if stampIssuer.Amount().Cmp(big.NewInt(3)) != 0 {
 			t.Fatalf("expected amount %d got %d", 3, stampIssuer.Amount().Int64())
 		}
 		if stampIssuer.Depth() != 17 {
 			t.Fatalf("expected depth %d got %d", 17, stampIssuer.Depth())
 		}
 	})
-	t.Run("save without update", func(t *testing.T) {
-		is, save, err := ps.GetStampIssuer(ids[1])
-		if err != nil {
-			t.Fatal(err)
-		}
-		data := is.Buckets()
-		modified := make([]uint32, len(data))
-		copy(modified, data)
-		for k, b := range modified {
-			b++
-			modified[k] = b
-		}
-
-		err = save(false)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		is, _, err = ps.GetStampIssuer(ids[1])
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		if !cmp.Equal(is.Buckets(), data) {
-			t.Fatal("expected buckets to be unchanged")
-		}
-
-	})
 }

pkg/postage/stamper.go

@@ -15,7 +15,8 @@ import (
 
 var (
 	// ErrBucketFull is the error when a collision bucket is full.
-	ErrBucketFull = errors.New("bucket full")
+	ErrBucketFull              = errors.New("bucket full")
+	ErrOverwriteImmutableIndex = errors.New("immutable batch old index overwrite due to previous faulty save")
 )
 
 // Stamper can issue stamps from the given address of chunk.
@@ -45,11 +46,13 @@ func (st *stamper) Stamp(addr swarm.Address) (*Stamp, error) {
 	}
 	switch err := st.store.Get(item); {
 	case err == nil:
-		// check if this index is in the past, it could happen that we encountered
+		// The index should be in the past. It could happen that we encountered
 		// some error after assigning this index and did not save the issuer data. In
 		// this case we should assign a new index and update it.
 		if st.issuer.assigned(item.BatchIndex) {
 			break
+		} else if st.issuer.ImmutableFlag() {
+			return nil, ErrOverwriteImmutableIndex
 		}
 		fallthrough
 	case errors.Is(err, storage.ErrNotFound):

pkg/postage/stamper_test.go

@@ -107,7 +107,7 @@ func TestStamperStamping(t *testing.T) {
 	})
 
 	t.Run("incorrect old index", func(t *testing.T) {
-		st := newTestStampIssuer(t, 1000)
+		st := newTestStampIssuerMutability(t, 1000, false)
 		chunkAddr := swarm.RandAddress(t)
 		bIdx := postage.ToBucket(st.BucketDepth(), chunkAddr)
 		index := postage.IndexToBytes(bIdx, 100)
@@ -129,6 +129,23 @@ func TestStamperStamping(t *testing.T) {
 		}
 	})
 
+	t.Run("incorrect old index immutable", func(t *testing.T) {
+		st := newTestStampIssuerMutability(t, 1000, true)
+		chunkAddr := swarm.RandAddress(t)
+		bIdx := postage.ToBucket(st.BucketDepth(), chunkAddr)
+		index := postage.IndexToBytes(bIdx, 100)
+		testItem := postage.NewStampItem().
+			WithBatchID(st.ID()).
+			WithChunkAddress(chunkAddr).
+			WithBatchIndex(index)
+		testSt := &testStore{Store: inmemstore.New(), stampItem: testItem}
+		stamper := postage.NewStamper(testSt, st, signer)
+		_, err := stamper.Stamp(chunkAddr)
+		if !errors.Is(err, postage.ErrOverwriteImmutableIndex) {
+			t.Fatalf("got err %v, wanted %v", err, postage.ErrOverwriteImmutableIndex)
+		}
+	})
+
 	// tests return with ErrOwnerMismatch
 	t.Run("owner mismatch", func(t *testing.T) {
 		owner[0] ^= 0xff // bitflip the owner first byte, this case must come last!

pkg/postage/stampissuer_test.go

@@ -47,6 +47,11 @@ func TestStampIssuerMarshalling(t *testing.T) {
 }
 
 func newTestStampIssuer(t *testing.T, block uint64) *postage.StampIssuer {
+	t.Helper()
+	return newTestStampIssuerMutability(t, block, true)
+}
+
+func newTestStampIssuerMutability(t *testing.T, block uint64, immutable bool) *postage.StampIssuer {
 	t.Helper()
 	id := make([]byte, 32)
 	_, err := io.ReadFull(crand.Reader, id)
@@ -61,7 +66,7 @@ func newTestStampIssuer(t *testing.T, block uint64) *postage.StampIssuer {
 		16,
 		8,
 		block,
-		true,
+		immutable,
 	)
 }