Skip to content

fix(mfs): preserve CidBuilder object in setNodeData(), Mkdir() and NewRoot() #1125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
davidebeatrici wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(mfs): preserve CidBuilder object in setNodeData(), Mkdir() and NewRoot() #1125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f1017b
fix(mfs): preserve CidBuilder object in setNodeData()
davidebeatrici Mar 24, 2026
8b46363
refac(mfs): get rid of RootOption in favor of MkdirOpts
davidebeatrici Apr 1, 2026
7e901c0
fix(mfs): preserve CidBuilder object in Mkdir() and NewRoot()
davidebeatrici Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ The following emojis are used to highlight certain changes:

- `bitswap/server`: incoming identity CIDs in wantlist messages are now silently ignored instead of killing the connection to the remote peer. Some IPFS implementations naively send identity CIDs, and disconnecting them for it caused unnecessary churn. [#1117](https://github.com/ipfs/boxo/pull/1117)
- `bitswap/network`: `ExtractHTTPAddress` now infers default ports for portless HTTP multiaddrs (e.g. `/dns/host/https` without `/tcp/443`). [#1123](https://github.com/ipfs/boxo/pull/1123)
- `mfs`: preserve CidBuilder object in `setNodeData()`, `Mkdir()` and `NewRoot()`. [#1125](https://github.com/ipfs/boxo/pull/1125)

### Security

Expand Down
5 changes: 1 addition & 4 deletions mfs/dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ func (d *Directory) ForEachEntry(ctx context.Context, f func(NodeListing) error)

func (d *Directory) Mkdir(name string) (*Directory, error) {
return d.MkdirWithOpts(name, MkdirOpts{
CidBuilder: d.unixfsDir.GetCidBuilder(),
MaxLinks: d.unixfsDir.GetMaxLinks(),
MaxHAMTFanout: d.unixfsDir.GetMaxHAMTFanout(),
HAMTShardingSize: d.unixfsDir.GetHAMTShardingSize(),
Expand All @@ -392,10 +393,6 @@ func (d *Directory) MkdirWithOpts(name string, opts MkdirOpts) (*Directory, erro
}
}

// hector: no idea why this option is overridden, but it must be to
// keep backwards compatibility. CidBuilder from the options is
// manually set in `Mkdir` (ops.go) though.
opts.CidBuilder = d.GetCidBuilder()
Comment on lines -395 to -398
Copy link
Copy Markdown
Author

@davidebeatrici davidebeatrici Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather:

if opts.CidBuilder == nil {
	opts.CidBuilder = d.GetCidBuilder()
}

dirobj, err := NewEmptyDirectory(d.ctx, name, d, d.dagService, d.prov, opts)
if err != nil {
return nil, err
Expand Down
8 changes: 8 additions & 0 deletions mfs/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,14 @@ func (fi *File) SetModTime(ts time.Time) error {

func (fi *File) setNodeData(data []byte) error {
nd := dag.NodeWithData(data)

// Preserve previous node's CidBuilder
if oldNode, ok := fi.node.(*dag.ProtoNode); ok {
if builder := oldNode.CidBuilder(); builder != nil {
nd.SetCidBuilder(builder)
}
}

err := fi.dagService.Add(context.TODO(), nd)
if err != nil {
return err
Expand Down
72 changes: 42 additions & 30 deletions mfs/mfs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func setupRoot(ctx context.Context, t testing.TB) (ipld.DAGService, *Root) {
rt, err := NewRoot(ctx, ds, root, func(ctx context.Context, c cid.Cid) error {
fmt.Println("PUBLISHED: ", c)
return nil
}, prov)
}, prov, MkdirOpts{})
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -1800,6 +1800,7 @@ func FuzzMkdirAndWriteConcurrently(f *testing.F) {
func TestRootOptionChunker(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use 512-byte chunks (non-default: default is 256KB = 262144 bytes).
// With default chunker, 2048 bytes would produce 1 chunk.
Expand All @@ -1812,8 +1813,10 @@ func TestRootOptionChunker(t *testing.T) {
t.Fatalf("test uses default chunk size %d; use a non-default value", chunkSize)
}

opts.Chunker = chunker.SizeSplitterGen(chunkSize)

// Create root with custom chunker (512 bytes)
root, err := NewEmptyRoot(ctx, ds, nil, nil, MkdirOpts{}, WithChunker(chunker.SizeSplitterGen(chunkSize)))
root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -1879,6 +1882,7 @@ func TestRootOptionChunker(t *testing.T) {
func TestRootOptionMaxLinks(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use MaxLinks=3 (non-default: default is ~174 from helpers.DefaultLinksPerBlock).
// With default MaxLinks, 4 files would NOT trigger HAMT sharding.
Expand All @@ -1894,9 +1898,10 @@ func TestRootOptionMaxLinks(t *testing.T) {
t.Fatal("test uses default SizeEstimationMode; use a non-default value")
}

root, err := NewEmptyRoot(ctx, ds, nil, nil, MkdirOpts{SizeEstimationMode: &sizeEstimationDisabled},
WithMaxLinks(maxLinks),
WithSizeEstimationMode(sizeEstimationDisabled))
opts.MaxLinks = maxLinks
opts.SizeEstimationMode = &sizeEstimationDisabled

root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -1973,6 +1978,7 @@ func TestRootOptionMaxLinks(t *testing.T) {
func TestRootOptionSizeEstimationMode(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use SizeEstimationDisabled (non-default: default is SizeEstimationLinks=0).
// This mode ignores size-based thresholds and only considers link count.
Expand All @@ -1986,10 +1992,11 @@ func TestRootOptionSizeEstimationMode(t *testing.T) {
// Use MaxLinks=3 (non-default: default is ~174).
const maxLinks = 3

opts.MaxLinks = maxLinks
opts.SizeEstimationMode = &mode

// Create root with non-default settings
root, err := NewEmptyRoot(ctx, ds, nil, nil, MkdirOpts{SizeEstimationMode: &mode},
WithSizeEstimationMode(mode),
WithMaxLinks(maxLinks))
root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2023,9 +2030,7 @@ func TestRootOptionSizeEstimationMode(t *testing.T) {

// Create new root from persisted node with same settings.
// This tests that settings propagate to directories loaded from DAG via cacheNode().
root2, err := NewRoot(ctx, ds, rootNode.(*dag.ProtoNode), nil, nil,
WithSizeEstimationMode(mode),
WithMaxLinks(maxLinks))
root2, err := NewRoot(ctx, ds, rootNode.(*dag.ProtoNode), nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2094,6 +2099,7 @@ func TestRootOptionSizeEstimationMode(t *testing.T) {
func TestChunkerInheritance(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use 256-byte chunks (non-default: default is 256KB = 262144 bytes).
// With default chunker, 1024 bytes would produce 1 chunk.
Expand All @@ -2107,8 +2113,10 @@ func TestChunkerInheritance(t *testing.T) {
t.Fatalf("test uses default chunk size %d; use a non-default value", chunkSize)
}

opts.Chunker = chunker.SizeSplitterGen(chunkSize)

// Create root with custom chunker
root, err := NewEmptyRoot(ctx, ds, nil, nil, MkdirOpts{}, WithChunker(chunker.SizeSplitterGen(chunkSize)))
root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2179,6 +2187,7 @@ func TestChunkerInheritance(t *testing.T) {
func TestRootOptionMaxHAMTFanout(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use custom fanout of 64 (non-default: default is 256).
const customFanout = 64
Expand All @@ -2187,11 +2196,11 @@ func TestRootOptionMaxHAMTFanout(t *testing.T) {
sizeEstimationDisabled := uio.SizeEstimationDisabled
const maxLinks = 3

root, err := NewEmptyRoot(ctx, ds, nil, nil,
MkdirOpts{SizeEstimationMode: &sizeEstimationDisabled},
WithMaxLinks(maxLinks),
WithMaxHAMTFanout(customFanout),
WithSizeEstimationMode(sizeEstimationDisabled))
opts.MaxLinks = maxLinks
opts.MaxHAMTFanout = customFanout
opts.SizeEstimationMode = &sizeEstimationDisabled

root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2256,6 +2265,7 @@ func TestRootOptionMaxHAMTFanout(t *testing.T) {
func TestRootOptionHAMTShardingSize(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use very small threshold of 100 bytes (non-default: default is 256KiB).
// This should trigger HAMT conversion with just a few small files.
Expand All @@ -2264,10 +2274,10 @@ func TestRootOptionHAMTShardingSize(t *testing.T) {
// Use SizeEstimationBlock for accurate size tracking.
sizeEstimationBlock := uio.SizeEstimationBlock

root, err := NewEmptyRoot(ctx, ds, nil, nil,
MkdirOpts{SizeEstimationMode: &sizeEstimationBlock},
WithHAMTShardingSize(customShardingSize),
WithSizeEstimationMode(sizeEstimationBlock))
opts.HAMTShardingSize = customShardingSize
opts.SizeEstimationMode = &sizeEstimationBlock

root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2338,16 +2348,17 @@ func TestRootOptionHAMTShardingSize(t *testing.T) {
func TestHAMTShardingSizeInheritance(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Use very small threshold
const customShardingSize = 150
sizeEstimationBlock := uio.SizeEstimationBlock

opts.HAMTShardingSize = customShardingSize
opts.SizeEstimationMode = &sizeEstimationBlock

// Create root with custom settings
root, err := NewEmptyRoot(ctx, ds, nil, nil,
MkdirOpts{SizeEstimationMode: &sizeEstimationBlock},
WithHAMTShardingSize(customShardingSize),
WithSizeEstimationMode(sizeEstimationBlock))
root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2378,9 +2389,7 @@ func TestHAMTShardingSizeInheritance(t *testing.T) {
}

// Create new root from persisted node with same settings
root2, err := NewRoot(ctx, ds, rootNode.(*dag.ProtoNode), nil, nil,
WithHAMTShardingSize(customShardingSize),
WithSizeEstimationMode(sizeEstimationBlock))
root2, err := NewRoot(ctx, ds, rootNode.(*dag.ProtoNode), nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2431,13 +2440,16 @@ func TestHAMTShardingSizeInheritance(t *testing.T) {
func TestMkdirParentsChunker(t *testing.T) {
ctx := t.Context()
ds := getDagserv(t)
opts := MkdirOpts{}

// Create root with 256-byte chunker (the "default" for this test).
// This is what intermediate directories would incorrectly inherit
// if the bug exists.
rootChunkSize := int64(256)
root, err := NewEmptyRoot(ctx, ds, nil, nil, MkdirOpts{},
WithChunker(chunker.SizeSplitterGen(rootChunkSize)))

opts.Chunker = chunker.SizeSplitterGen(rootChunkSize)

root, err := NewEmptyRoot(ctx, ds, nil, nil, opts)
if err != nil {
t.Fatal(err)
}
Expand Down
12 changes: 1 addition & 11 deletions mfs/ops.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error {

// opts to make the parents leave MkParents and Flush as false.
parentsOpts := MkdirOpts{
CidBuilder: opts.CidBuilder,
MaxLinks: opts.MaxLinks,
MaxHAMTFanout: opts.MaxHAMTFanout,
HAMTShardingSize: opts.HAMTShardingSize,
Expand All @@ -190,11 +191,6 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error {
if err != nil {
return err
}
// MkdirWithOps uses cur.GetCidBuilder regardless of
// the option. So we must set it manually.
if opts.CidBuilder != nil {
mkd.SetCidBuilder(opts.CidBuilder)
}
fsn = mkd
} else if err != nil {
return err
Expand All @@ -214,12 +210,6 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error {
}
}

// Again, MkdirWithOpts ignores opts.CidBuilder so must be applied
// here.
if opts.CidBuilder != nil {
final.SetCidBuilder(opts.CidBuilder)
}

if opts.Flush {
err := final.Flush()
if err != nil {
Expand Down
Loading