Skip to content

fix(sync): pull Malachite suffix-gap fix - #283

Open
huklaa wants to merge 1 commit into
circlefin:mainfrom
huklaa:fix-214-malachite-suffix-gap-clean
Open

fix(sync): pull Malachite suffix-gap fix#283
huklaa wants to merge 1 commit into
circlefin:mainfrom
huklaa:fix-214-malachite-suffix-gap-clean

Conversation

@huklaa

@huklaa huklaa commented Aug 26, 2026

Copy link
Copy Markdown

Fixes #214.
This updates the Malachite revision to include the upstream suffix-gap fix that addresses the remaining follower-sync deadlock path.
PR #227 only moved Arc to an earlier Malachite revision and explicitly did not include this suffix-gap fix, so this is a separate follow-up scope.
The updated Malachite revision preserves the requested suffix when a missing prefix is re-requested, preventing the follower from losing the suffix and getting stuck in a sync loop.
Validation:
cargo build -p arc-node-consensus --locked
targeted regression coverage is included upstream in Malachite commit 378b0e5

@osr21

osr21 commented Aug 26, 2026

Copy link
Copy Markdown

Verified the scope of this bump against upstream before anything else, since consensus-dep pins deserve suspicion. The core claims all check out:

One thing I'd fix before merge:

The Cargo.lock diff contains churn the pin change doesn't require. Malachite's own manifests are untouched between the two revs (the upstream compare shows no Cargo.toml/Cargo.lock changes), so the only necessary lock edits are the arc-malachitebft-* source lines. But this lock also moves unrelated registry crates: windows-sys hops (0.48.0/0.59.0 → 0.60.2/0.61.2), itertools downgrades from 0.12.1 to 0.10.5 inside four prost-build variants, and a syn 2.0.117 → 1.0.109 flip. That pattern says the lock was regenerated more broadly than cargo needed for a git-pin bump. For a consensus repo the lock delta on a dep bump should be exactly auditable — and downgrades in particular can silently reintroduce fixed behavior in build-time deps. Suggest resetting Cargo.lock to main's copy, editing only the rev fields, and letting a plain cargo build refresh just the git pins; the diff should then collapse to the ~19 malachitebft entries.

Two smaller notes:

  • Validation is compile-only. cargo build --locked proves resolution, not behavior — worth running the sync-adjacent test suites here, and ideally a quake follower scenario shaped like the Follower sync deadlock: failed multi-height fetch skips a height, consensus wedges permanently (v0.7.3) #214 wedge (multi-height fetch failure with a prefix-only peer), since that's the exact path this changes and the upstream test exercises Malachite's handler in isolation, not Arc's integration of it.
  • Cosmetic: the new pins use a 7-char rev (378b0e5) where the repo convention is 8 (8ee5d998, 5cd137fb). Harmless to cargo, mildly annoying to grep.

Also a practical heads-up given #282 is in flight: #214 currently has no assignee, and under the incoming PR gate this PR's author would need to be assigned to it. A maintainer assigning @huklaa to #214 would both reflect reality and keep this PR from tripping the new policy if it's reopened later.

Scope, target, and upstream test all verified — with a minimal lockfile this is the right way to close #214.

@huklaa
huklaa force-pushed the fix-214-malachite-suffix-gap-clean branch from a7ec533 to f07f99e Compare August 26, 2026 19:55
@melekes

melekes commented Aug 27, 2026

Copy link
Copy Markdown

Hi @huklaa, thanks for putting this together and for the clear write-up linking back to #214 and #227 — the suffix-gap context is really helpful.

We're going to close this for now. The malachite dependency is scheduled to receive a broader update as part of our upcoming v0.8 release, which will bring in the upstream suffix-gap fix (and several related changes) in one coordinated bump.

Thanks again for the contribution!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follower sync deadlock: failed multi-height fetch skips a height, consensus wedges permanently (v0.7.3)

3 participants