From 8de936c97867ecd48b85494088c67b4b331bbffd Mon Sep 17 00:00:00 2001 From: Jorropo Date: Mon, 15 Jan 2024 13:37:50 +0100 Subject: [PATCH] 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. --- blockservice/blockservice.go | 37 +++++++++++--------------------- gateway/blocks_backend.go | 5 +---- ipld/merkledag/merkledag_test.go | 28 +++++++++++++++--------- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go index 31f4e820ec..ed8e39bf8e 100644 --- a/blockservice/blockservice.go +++ b/blockservice/blockservice.go @@ -47,11 +47,8 @@ type BlockService interface { io.Closer BlockGetter - // Blockstore returns a reference to the underlying blockstore - Blockstore() blockstore.Blockstore - - // Exchange returns a reference to the underlying exchange (usually bitswap) - Exchange() exchange.Interface + // Has indicates if a block is present locally. + Has(ctx context.Context, c cid.Cid) (bool, error) // AddBlock puts a given block to the underlying datastore AddBlock(ctx context.Context, o blocks.Block) error @@ -132,21 +129,6 @@ func New(bs blockstore.Blockstore, exchange exchange.Interface, opts ...Option) func NewWriteThrough(bs blockstore.Blockstore, exchange exchange.Interface) BlockService { return New(bs, exchange, WriteThrough()) } - -// 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) NewSession(ctx context.Context) BlockGetter { ses := s.grabSessionFromContext(ctx) if ses != nil { @@ -447,14 +429,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) @@ -499,3 +480,11 @@ func (s *blockService) grabSessionFromContext(ctx context.Context) *session { return sss } + +func (s *blockService) Has(ctx context.Context, c cid.Cid) (bool, error) { + if err := verifcid.ValidateCid(s.allowlist, c); err != nil { + return false, err + } + + return s.blockstore.Has(ctx, c) +} diff --git a/gateway/blocks_backend.go b/gateway/blocks_backend.go index 47e2db697a..bfe940cf7d 100644 --- 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 ...BlocksBack } 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 index ffe4946ca2..4dcc0b9c56 100644 --- 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)