From 17b3ed9a5a0577232e63823c39062a4dca9ad043 Mon Sep 17 00:00:00 2001 From: Jorropo Date: Mon, 15 Jan 2024 13:11:39 +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 | 60 +++++++++++++------------------- gateway/blocks_backend.go | 5 +-- ipld/merkledag/merkledag_test.go | 28 +++++++++------ 3 files changed, 44 insertions(+), 49 deletions(-) diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go index e9772928c..4cac1d87f 100644 --- 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 index bb76eff66..ccda88351 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 ...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 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)