From ff49b4110ba044dd16e333bc8089e0d81b58129f 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 31f4e820e..ed8e39bf8 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 47e2db697..bfe940cf7 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 ffe4946ca..4dcc0b9c5 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)