Fix unique chunk hash #1727
Fix unique chunk hash #1727
Conversation
WalkthroughBump of internal version tag from v4.5.42 to v4.5.43 and synchronized dependency updates: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
common/version/version.go (1)
8-8: Version bump to v4.5.42 recorded — matches the breaking deployment planThe version change is consistent with the dependency bumps and the breaking-change label.
- Please add a short migration note in the release docs explicitly stating: “Operators must remove any pending chunks and re-propose them so they get the new unique hash format.”
- Confirm CI/CD embeds the correct tag and short commit in the Version string (via
-ldflags -X), so downstream services can gate behavior if needed.Happy to draft the release/migration snippet if you want.
common/go.mod (1)
18-18: Consistent go-ethereum version alignment recommended
go-ethereum pins currently diverge across modules:
- database/go.mod:
v1.10.14-0.20250305151038-478940e79601- common/go.mod & tests/integration-test/go.mod:
v1.10.14-0.20250625112225-a67863c65587- coordinator/go.mod & rollup/go.mod:
v1.10.14-0.20250626110859-cc9a1dd82de7- bridge-history-api/go.mod:
replace tov1.10.14-0.20250729113104-bd8f141bb3e9
(hotfix for header-hash incompatibility, per inline comment)da-codec is already aligned at
v0.1.3-0.20250826112206-b4cce5c5d178
across all modules.To reduce risk of shared-type or behavior drift, consider one of the following optional refactors:
• Unify all modules to a single go-ethereum commit (e.g. the latest hotfix
bd8f141bb3e9).
• If divergence is intentional, add module-specific comments in each go.mod explaining why (e.g. “using commit X for feature Y or hotfix Z”).
• Optionally codify any intentional pins via explicitreplacedirectives (with comments) rather than transitive version mismatches.This will improve clarity and guard against subtle cross-service inconsistencies.
tests/integration-test/go.mod (3)
8-9: Confirm the da-codec commit contains the “unique chunk hash” change referenced in upstream PR 62The pseudo-version v0.1.3-0.20250826112206-b4cce5c5d178 looks like a direct pin. For traceability during incident/bisect, consider noting the exact upstream commit SHA and PR number in the commit message or PR description.
If helpful, add a short note like: “da-codec at b4cce5c5d178 (from scroll-tech/da-codec#62) includes unique chunk hash fix.”
8-9: Double-check test fixtures and any golden hashes impacted by the new unique chunk hashGiven this PR is marked breaking and deployment steps include re-proposing pending chunks, any integration tests that assert on precomputed chunk hashes or serialized payloads may fail. Ensure fixtures are regenerated and expectations updated.
I can help generate a small helper to recompute hashes over existing fixtures and emit updated goldens—want me to open a follow-up?
9-9: go-ethereum fork pseudo-version looks fine; ensure other modules pin the same commitPinning the fork to v1.10.14-0.20250625112225-a67863c65587 in tests is OK. Just confirm common/, rollup/, coordinator/, etc., are on the exact same pseudo-version to avoid resolver picking mixed APIs.
If you want to enforce this globally, consider a top-level go.work with use and consistent replace/require directives to centralize version control for these two modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
bridge-history-api/go.mod(1 hunks)common/go.mod(2 hunks)common/version/version.go(1 hunks)coordinator/go.mod(1 hunks)rollup/go.mod(1 hunks)tests/integration-test/go.mod(1 hunks)
🔇 Additional comments (4)
bridge-history-api/go.mod (1)
13-13: da-codec bump picked up — aligns with “unique chunk hash” fix; LGTMThis keeps bridge-history-api on the same da-codec commit as the rest of the repo changes in this PR. No API changes visible here.
Please verify:
- go.sum was refreshed after the bump in this module.
- No tests/assertions rely on the old chunk-hash value format (search for “chunk” and “hash” in bridge-history-api tests to ensure expectations are still valid).
rollup/go.mod (1)
18-18: Rollup now uses the new da-codec (unique chunk hash) — check hash-dependent test vectorsLooks good. Given the breaking label, double-check any rollup-side hash comparisons or serialization round-trips are updated.
Action: run a quick repository-wide search to spot hardcoded/expected chunk hashes in rollup tests and fixtures and update them if needed (e.g., searches for “chunk.*hash”).
coordinator/go.mod (1)
12-12: Coordinator moved to the new da-codec — ensure pending/proposed queue handling is migratedChange looks correct. Coordinator is usually closest to chunk proposal flows; ensure operational runbooks incorporate “remove pending chunks and re-propose with new hash.”
Confirm:
- Any persisted “pending” items keyed by the old hash are cleared or re-keyed per the deployment notes.
- E2E/integration tests that simulate re-proposals are updated accordingly.
tests/integration-test/go.mod (1)
8-9: Housekeeping: run go mod tidy and commit go.sum changes after the bumpsDependency bumps across multiple modules often alter sums/indirects. Make sure go.sum changes are included, and no stale indirect constraints remain.
You can verify locally with:
- go mod tidy in each module
- git status to ensure all go.sum updates are committed
Thegaram
force-pushed
the
fix-unique-chunk-hash
branch
from
cc5b047 to
5873abb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1727 +/- ##
========================================
Coverage 37.47% 37.47%
========================================
Files 243 243
Lines 20500 20500
========================================
Hits 7683 7683
Misses 12009 12009
Partials 808 808
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/go.mod (1)
187-187: Consider making da-codec a direct dependency in modules that rely on its hashing semanticsRight now it’s listed as “// indirect”. If any package in this module (or other submodules) imports da-codec types/APIs directly or critically depends on its hashing behavior, make it a direct requirement in those modules to document the coupling and prevent accidental downgrades by transitive dep churn. If this module doesn’t import it at all, keeping it indirect (or letting
go mod tidydrop it) is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
bridge-history-api/go.mod(1 hunks)common/go.mod(1 hunks)common/version/version.go(1 hunks)coordinator/go.mod(1 hunks)rollup/go.mod(1 hunks)tests/integration-test/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🚧 Files skipped from review as they are similar to previous changes (4)
- rollup/go.mod
- coordinator/go.mod
- tests/integration-test/go.mod
- bridge-history-api/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (3)
common/go.mod (3)
187-187: Good: da-codec bumped to the commit with the unique-chunk-hash fixThis aligns common with the upstream change (fixing the chunk hash) and matches the PR intent. No issues in go.mod syntax; the pseudo-version looks correct.
187-187: Breaking change coordination: run tidy/vendor and prep deploy stepsGiven the breaking-change label and the new hash semantics:
- Run
go mod tidy(and update vendor if you vendor) after the bump to ensure go.sum reflects the new pseudo-version only.- Prepare deployment by removing pending chunks and re-proposing them with the new hash, as noted in the PR description. Ensure downstream services roll out atomically to avoid mixed-hash environments.
187-187: Verified: da-codec pseudo-version is consistently pinnedAll
go.modfiles referencegit.colasdn.top/scroll-tech/da-codec v0.1.3-0.20250826112206-b4cce5c5d178, and yourgo.sumonly contains that version (plus the expected/go.modchecksum entry). No older or stray pseudo-versions remain. Thanks for ensuring consistency!
3 participants
Purpose or design rationale of this PR
scroll-tech/da-codec#62
Deployment:
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit