blockservice: remove inner fields access methods

We shouldn't expose inner implementation details of the blockservice to consumers. For example by using the blockstore for the .Has call, the gateway skipped verifcid and blocker checks. Probably fine but not we want to do.
ipfs · Jan 15, 2024 · 17b3ed9 · 17b3ed9
1 parent 5e08a77
commit 17b3ed9
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 49 deletions.
diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go
@@ -99,24 +99,6 @@ func New(bs blockstore.Blockstore, exchange exchange.Interface, opts ...Option)
 	return service
 }
 
-// Blockstore returns the blockstore behind this blockservice.
-func (s *BlockService) Blockstore() blockstore.Blockstore {
-	return s.blockstore
-}
-
-// Exchange returns the exchange behind this blockservice.
-func (s *BlockService) Exchange() exchange.Interface {
-	return s.exchange
-}
-
-func (s *BlockService) Allowlist() verifcid.Allowlist {
-	return s.allowlist
-}
-
-func (s *BlockService) Blocker() Blocker {
-	return s.blocker
-}
-
 // NewSession creates a new session that allows for
 // controlled exchange of wantlists to decrease the bandwidth overhead.
 // If the current exchange is a SessionExchange, a new exchange
@@ -257,9 +239,7 @@ func (s *BlockService) getBlock(ctx context.Context, c cid.Cid, fetchFactory fun
 		}
 	}
 
-	blockstore := s.Blockstore()
-
-	block, err := blockstore.Get(ctx, c)
+	block, err := s.blockstore.Get(ctx, c)
 	switch {
 	case err == nil:
 		return block, nil
@@ -281,12 +261,12 @@ func (s *BlockService) getBlock(ctx context.Context, c cid.Cid, fetchFactory fun
 		return nil, err
 	}
 	// also write in the blockstore for caching, inform the exchange that the block is available
-	err = blockstore.Put(ctx, blk)
+	err = s.blockstore.Put(ctx, blk)
 	if err != nil {
 		return nil, err
 	}
-	if ex := s.Exchange(); ex != nil {
-		err = ex.NotifyNewBlocks(ctx, blk)
+	if s.exchange != nil {
+		err = s.exchange.NotifyNewBlocks(ctx, blk)
 		if err != nil {
 			return nil, err
 		}
@@ -352,11 +332,9 @@ func (s *BlockService) getBlocks(ctx context.Context, ks []cid.Cid, fetchFactory
 			ks = ks2
 		}
 
-		bs := s.Blockstore()
-
 		var misses []cid.Cid
 		for _, c := range ks {
-			hit, err := bs.Get(ctx, c)
+			hit, err := s.blockstore.Get(ctx, c)
 			if err != nil {
 				misses = append(misses, c)
 				continue
@@ -379,7 +357,6 @@ func (s *BlockService) getBlocks(ctx context.Context, ks []cid.Cid, fetchFactory
 			return
 		}
 
-		ex := s.Exchange()
 		var cache [1]blocks.Block // preallocate once for all iterations
 		for {
 			var b blocks.Block
@@ -394,16 +371,16 @@ func (s *BlockService) getBlocks(ctx context.Context, ks []cid.Cid, fetchFactory
 			}
 
 			// write in the blockstore for caching
-			err = bs.Put(ctx, b)
+			err = s.blockstore.Put(ctx, b)
 			if err != nil {
 				logger.Errorf("could not write blocks from the network to the blockstore: %s", err)
 				return
 			}
 
-			if ex != nil {
+			if s.exchange != nil {
 				// inform the exchange that the blocks are available
 				cache[0] = b
-				err = ex.NotifyNewBlocks(ctx, cache[:]...)
+				err = s.exchange.NotifyNewBlocks(ctx, cache[:]...)
 				if err != nil {
 					logger.Errorf("could not tell the exchange about new blocks: %s", err)
 					return
@@ -456,14 +433,13 @@ func (s *Session) grabSession() exchange.Fetcher {
 			s.sesctx = nil // early gc
 		}()
 
-		ex := s.bs.Exchange()
-		if ex == nil {
+		if s.bs.exchange == nil {
 			return
 		}
-		s.ses = ex // always fallback to non session fetches
 
-		sesEx, ok := ex.(exchange.SessionExchange)
+		sesEx, ok := s.bs.exchange.(exchange.SessionExchange)
 		if !ok {
+			s.ses = s.bs.exchange // always fallback to non session fetches
 			return
 		}
 		s.ses = sesEx.NewSession(s.sesctx)
@@ -526,3 +502,17 @@ func grabSessionFromContext(ctx context.Context, bs *BlockService) *Session {
 
 	return ss
 }
+
+func (s *BlockService) Has(ctx context.Context, c cid.Cid) (bool, error) {
+	if err := verifcid.ValidateCid(s.allowlist, c); err != nil {
+		return false, err
+	}
+
+	if s.blocker != nil {
+		if err := s.blocker(c); err != nil {
+			return false, err
+		}
+	}
+
+	return s.blockstore.Has(ctx, c)
+}
diff --git a/gateway/blocks_backend.go b/gateway/blocks_backend.go
@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"github.com/ipfs/boxo/blockservice"
-	blockstore "github.com/ipfs/boxo/blockstore"
 	"github.com/ipfs/boxo/fetcher"
 	bsfetcher "github.com/ipfs/boxo/fetcher/impl/blockservice"
 	"github.com/ipfs/boxo/files"
@@ -51,7 +50,6 @@ import (
 
 // BlocksBackend is an [IPFSBackend] implementation based on a [blockservice.BlockService].
 type BlocksBackend struct {
-	blockStore   blockstore.Blockstore
 	blockService *blockservice.BlockService
 	dagService   format.DAGService
 	resolver     resolver.Resolver
@@ -143,7 +141,6 @@ func NewBlocksBackend(blockService *blockservice.BlockService, opts ...BlocksBac
 	}
 
 	return &BlocksBackend{
-		blockStore:   blockService.Blockstore(),
 		blockService: blockService,
 		dagService:   dagService,
 		resolver:     r,
@@ -680,7 +677,7 @@ func (bb *BlocksBackend) IsCached(ctx context.Context, p path.Path) bool {
 		return false
 	}
 
-	has, _ := bb.blockStore.Has(ctx, rp.RootCid())
+	has, _ := bb.blockService.Has(ctx, rp.RootCid())
 	return has
 }
 

diff --git a/ipld/merkledag/merkledag_test.go b/ipld/merkledag/merkledag_test.go
@@ -16,9 +16,13 @@ import (
 	"testing"
 	"time"
 
+	testinstance "github.com/ipfs/boxo/bitswap/testinstance"
+	tn "github.com/ipfs/boxo/bitswap/testnet"
 	. "github.com/ipfs/boxo/ipld/merkledag"
 	mdpb "github.com/ipfs/boxo/ipld/merkledag/pb"
 	dstest "github.com/ipfs/boxo/ipld/merkledag/test"
+	mockrouting "github.com/ipfs/boxo/routing/mock"
+	delay "github.com/ipfs/go-ipfs-delay"
 
 	bserv "github.com/ipfs/boxo/blockservice"
 	bstest "github.com/ipfs/boxo/blockservice/test"
@@ -507,10 +511,12 @@ func TestCantGet(t *testing.T) {
 }
 
 func TestFetchGraph(t *testing.T) {
-	var dservs []ipld.DAGService
-	bsis := bstest.Mocks(2)
-	for _, bsi := range bsis {
-		dservs = append(dservs, NewDAGService(bsi))
+	net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(0))
+	sg := testinstance.NewTestInstanceGenerator(net, nil, nil)
+	instances := sg.Instances(2)
+	dservs := [2]ipld.DAGService{
+		NewDAGService(bserv.New(instances[0].Blockstore(), instances[0].Exchange)),
+		NewDAGService(bserv.New(instances[1].Blockstore(), instances[1].Exchange)),
 	}
 
 	read := io.LimitReader(u.NewTimeSeededRand(), 1024*32)
@@ -522,7 +528,7 @@ func TestFetchGraph(t *testing.T) {
 	}
 
 	// create an offline dagstore and ensure all blocks were fetched
-	bs := bserv.New(bsis[1].Blockstore(), offline.Exchange(bsis[1].Blockstore()))
+	bs := bserv.New(instances[1].Blockstore(), nil)
 
 	offlineDS := NewDAGService(bs)
 
@@ -547,10 +553,12 @@ func TestFetchGraphWithDepthLimit(t *testing.T) {
 	}
 
 	testF := func(t *testing.T, tc testcase) {
-		var dservs []ipld.DAGService
-		bsis := bstest.Mocks(2)
-		for _, bsi := range bsis {
-			dservs = append(dservs, NewDAGService(bsi))
+		net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(0))
+		sg := testinstance.NewTestInstanceGenerator(net, nil, nil)
+		instances := sg.Instances(2)
+		dservs := [2]ipld.DAGService{
+			NewDAGService(bserv.New(instances[0].Blockstore(), instances[0].Exchange)),
+			NewDAGService(bserv.New(instances[1].Blockstore(), instances[1].Exchange)),
 		}
 
 		root := makeDepthTestingGraph(t, dservs[0])
@@ -561,7 +569,7 @@ func TestFetchGraphWithDepthLimit(t *testing.T) {
 		}
 
 		// create an offline dagstore and ensure all blocks were fetched
-		bs := bserv.New(bsis[1].Blockstore(), offline.Exchange(bsis[1].Blockstore()))
+		bs := bserv.New(instances[1].Blockstore(), offline.Exchange(instances[1].Blockstore()))
 
 		offlineDS := NewDAGService(bs)