Skip to content
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

Jump to bottom

docs: reword adr-013 and describe SubtreeRootThreshold #1959

Merged
merged 7 commits into from
Jun 21, 2023
Merged

docs: reword adr-013 and describe SubtreeRootThreshold #1959

merged 7 commits into from
Jun 21, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jun 18, 2023 •
edited
Loading

Closes #1877
Closes #1607

Description

  1. Rephrased a couple of sections for clarity. If reviewers don't think it's any clearer, I can revert.
  2. Added a section on the threshold. Replaced usage of MaxSquareSize with SubtreeRootThreshold where applicable.
  3. Update links. I don't see a good way to relative link to markdown files in this repo from Go code so I removed the obsolete Github permalinks. Open to ideas if reviewers have suggestions on what to do with the links in Godoc.

@rootulp rootulp added documentation Improvements or additions to documentation ADR item is directly relevant to writing or modifying an ADR labels Jun 18, 2023
@rootulp rootulp self-assigned this Jun 18, 2023
@rootulp rootulp requested a review from nashqueue June 18, 2023 22:55
@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2023 •
edited
Loading

Codecov Report

Merging #1959 (6a8ec34) into main (c1176b1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1959   +/-   ##
=======================================
  Coverage   21.21%   21.21%           
=======================================
  Files         121      121           
  Lines       13704    13704           
=======================================
  Hits         2907     2907           
  Misses      10509    10509           
  Partials      288      288
Impacted Files Coverage Δ
pkg/shares/blob_share_commitment_rules.go 100.00% <ø> (ø)

@rootulp rootulp marked this pull request as ready for review June 18, 2023 22:55
@cmwaters
Copy link
Contributor

Renamed file and title to "Non-Interactive Default Rules for Reduced Padding 2"

Why does it need to be renamed to two? Can't we just document the changes in the changelog

@rootulp
Copy link
Collaborator Author

rootulp commented Jun 19, 2023

Why does it need to be renamed to two?

It doesn't need to be renamed but I renamed b/c I thought it was misleading that the ADR was titled "zero padding". With this proposal, there will still be padding but there is just less padding than ADR-009.

evan-forbes
evan-forbes previously approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

due to the rename, its difficult to spot the rephrases, but its probably fine

I'm not blocking on renaming, but perhaps we should keep that to a separate PR?

docs/architecture/adr-020-deterministic-square-construction.md Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor

It doesn't need to be renamed but I renamed b/c I thought it was misleading that the ADR was titled "zero padding". With this proposal, there will still be padding but there is just less padding than ADR-009.

Theoretically if you have a ridiculously large SubtreeRootThreshold there will be zero padding no matter how large the blob is.

I actually don't mind renaming it to "reduced-padding" but I don't think the -2 suffix adds anything (just my personal view)

@MSevey MSevey requested a review from a team June 20, 2023 15:02
@rootulp rootulp changed the title docs: add section to adr-013 docs: reword adr-013 and describe SubtreeRootThreshold Jun 20, 2023
@rootulp rootulp requested review from evan-forbes and cmwaters June 20, 2023 15:10
evan-forbes
evan-forbes previously approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice! thanks for updating this

nashqueue
nashqueue reviewed Jun 20, 2023
View reviewed changes
Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

Thank you! Left some comments

docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md Outdated Show resolved Hide resolved
docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md Outdated Show resolved Hide resolved
docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md Outdated
Comment on lines 83 to 85
If the threshold is bigger than `MinSquareSize(blob)` then the blob will be aligned to the index of the `MinSquareSize(blob)`. This would prevent some blob size ranges to have higher padding than they had before this change. So the real new non-interactive default rules would be:

Blobs start at an index that is equal to a multiple of the blob length divided by `MaxSquareSize` rounded up. If this index is larger than the `MinSquareSize` of the blob then the blob starts at the index of the `MinSquareSize`.
Blobs start at an index that is equal to a multiple of the blob length divided by `MaxSquareSize` rounded up. If this index is larger than the `MinSquareSize(blob)` then the blob starts at the index that is a multiple of of the `MinSquareSize(blob)`.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we reference that where we explain the new non-interactive default rules? See my comment above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Attempted to address via 4a4d336

please LMK what you think

Copy link
Member

Choose a reason for hiding this comment

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

It is good. You are introducing subtree root width as a variable name, but explaining what it means in words may also make sense as it is the first mention in the ADR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was this footnote not easy to discover? or are there other words you would use to define it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to merge b/c two approvals but happy to amend the footnote or describe it differently in a follow-up PR. Thanks for your reviews!

@MSevey MSevey requested a review from a team June 20, 2023 19:01
@rootulp rootulp requested a review from nashqueue June 20, 2023 19:02
@rootulp rootulp enabled auto-merge (squash) June 20, 2023 19:03
@rootulp rootulp merged commit 6588ba4 into celestiaorg:main Jun 21, 2023
evan-forbes pushed a commit that referenced this pull request Jul 3, 2023
@rootulp @evan-forbes
docs: reword adr-013 and describe SubtreeRootThreshold (#1959)
d53c6b4 
* docs: add section to adr-013

* update links

* revert: ADR name change

* revert: hyphen change

* improve: godoc

* Address @nashqueue feedback
evan-forbes pushed a commit that referenced this pull request Jul 3, 2023
@rootulp @evan-forbes
docs: reword adr-013 and describe SubtreeRootThreshold (#1959)
b0bbe11 
* docs: add section to adr-013

* update links

* revert: ADR name change

* revert: hyphen change

* improve: godoc

* Address @nashqueue feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmwaters cmwaters cmwaters approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

@liamsi liamsi Awaiting requested review from liamsi

@adlerjohn adlerjohn Awaiting requested review from adlerjohn

@MSevey MSevey Awaiting requested review from MSevey

@nashqueue nashqueue Awaiting requested review from nashqueue

Assignees

@rootulp rootulp

Labels
ADR item is directly relevant to writing or modifying an ADR documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
5 participants
@rootulp @codecov-commenter @cmwaters @evan-forbes @nashqueue