Skip to content

fix: update soft block limit to 2MiB#8968

Open
Jorropo wants to merge 1 commit intoipfs:masterfrom
Jorropo:fix-big-blocks
Open

fix: update soft block limit to 2MiB#8968
Jorropo wants to merge 1 commit intoipfs:masterfrom
Jorropo:fix-big-blocks

Conversation

@Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 12, 2022

This is needed because the 1MiB limit is the chunker limit, however the
actual blocks can be wrapped in protobuf which adds a small ~10 byte
overhead.

@Jorropo Jorropo requested a review from aschmahmann May 12, 2022 17:06
@Jorropo Jorropo self-assigned this May 12, 2022
@Jorropo Jorropo enabled auto-merge (rebase) May 12, 2022 17:07
@Jorropo Jorropo disabled auto-merge May 12, 2022 17:19
@Jorropo Jorropo force-pushed the fix-big-blocks branch 3 times, most recently from e518f3a to 53d219d Compare May 24, 2022 17:19
@Jorropo Jorropo added this to the go-ipfs 0.13 milestone May 25, 2022
@Jorropo
Copy link
Contributor Author

Jorropo commented May 26, 2022

may 26 2022 discussion:
Need to add two tests:

  • ipfs block get $(ipfs add --raw-leaves=false --chunker=size-$(( 1024 * 1024 )) 1MiBFile) | ipfs block put works. (non raw leaves blocks with 1MiB chunks include a ~10 bytes protobuf wrapping making them slightly too big, thoses blocks MUST pass, this is what this PR fixes)
  • top down blackbox exchange test of 2MiB blocks.

@Jorropo
Copy link
Contributor Author

Jorropo commented May 26, 2022

Other possible solution if this drag on too much, revert 04e7e95 in 0.13.0 and move it to 0.14.0.

@BigLep BigLep modified the milestones: go-ipfs 0.13, go-ipfs 0.14 Jun 2, 2022
@lidel lidel modified the milestones: kubo 0.14, kubo 0.15 Jul 20, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I'm moving this to 0.15: this needs end-to-end regression tests so we are not surprised when something changes deep in one of libp2p deps. We want tests for two cases:

  • confirm two nodes exchange 2MiB block
  • confirm two nodes are unable to exchange 2MiB+1 byte block

@BigLep
Copy link
Contributor

BigLep commented Sep 1, 2022

What's the priority of this? What is motivating this work?

@BigLep BigLep removed the request for review from aschmahmann September 1, 2022 01:52
@Jorropo
Copy link
Contributor Author

Jorropo commented Sep 1, 2022

What is motivating this work?

  1. There is edge cases when you use 1MiB without --raw-leaves you exceed 1MiB because the protobuf wrapper.
  2. linux2ipfs use 2MiB raw leaves blocks and I don't like being annoyed when I use it.

@lidel
Copy link
Member

lidel commented Sep 1, 2022

This still needs tests described in #8968 (review)

@lidel lidel removed this from the kubo 0.15 milestone Sep 1, 2022
@Jorropo
Copy link
Contributor Author

Jorropo commented Sep 1, 2022

I know, I didn't took time to wrote them yet.

@BigLep BigLep marked this pull request as draft September 2, 2022 00:53
@lidel lidel mentioned this pull request Sep 5, 2022
17 tasks
@Jorropo Jorropo removed the need_tests label Jan 5, 2023
@Jorropo Jorropo marked this pull request as ready for review January 5, 2023 05:53
@Jorropo Jorropo requested a review from lidel January 5, 2023 05:53
@Jorropo
Copy link
Contributor Author

Jorropo commented Jan 5, 2023

confirm two nodes are unable to exchange 2MiB+1 byte block

I didnt do this because we don't really have consensus on this: ipfs/specs#331 (comment)

This is needed because the 1MiB limit is the chunker limit, however the
actual blocks can be wrapped in protobuf which adds a small ~10 byte
overhead.
@Jorropo Jorropo removed their assignment Mar 4, 2024
@gammazero gammazero added the need/triage Needs initial labeling and prioritization label Feb 3, 2026
@gammazero
Copy link
Contributor

Can we close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need/triage Needs initial labeling and prioritization

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants