Skip to content

fix(deps): bump malachite pin to fix follower sync_height wedge (#214) - #227

Open
ygd58 wants to merge 1 commit into
circlefin:mainfrom
ygd58:fix/malachite-sync-height-wedge-214
Open

fix(deps): bump malachite pin to fix follower sync_height wedge (#214)#227
ygd58 wants to merge 1 commit into
circlefin:mainfrom
ygd58:fix/malachite-sync-height-wedge-214

Conversation

@ygd58

@ygd58 ygd58 commented Aug 6, 2026

Copy link
Copy Markdown

Summary

Bumps the pinned circlefin/malachite git rev from 8ee5d998 (2026-05-22) to 5cd137fb (current main tip) to pull in circlefin/malachite#1567, which fixes a follower-node sync deadlock reported in #214.

Root cause

A batched sync fetch that partially fails advances sync_height past the entire failed range, but the retry only re-requests the first height. The skipped height is never re-requested; consensus stalls there while sync's parallel-request slots fill with undeliverable later heights. Confirmed reproducible ~2x/day on testnet followers per the issue thread.

Fix

circlefin/malachite#1567 makes the forward set_sync_height() call conditional so it no longer clobbers a rewind from a partial-range failure. It merged into malachite main on 2026-06-22 and is currently the tip of main — no newer unrelated commits are pulled in by this bump.

circlefin/malachite#1543 (reject non-contiguous sync responses), also mentioned in the issue thread, was already included in the current pin, so no separate action needed for it.

Testing

Cargo.lock regenerated against the new rev via cargo build -p arc-node-consensus.

Fixes #214

…irclefin#214)

Follower nodes running v0.7.3 can deadlock permanently: a batched sync
fetch that partially fails advances sync_height past the whole failed
range, but the retry only re-requests the first height. The stranded
height is never re-requested; consensus stalls at that height while
sync's 5 parallel slots fill with later heights that can never be
delivered.

Root cause is in crates/sync/src/handle.rs upstream: the unconditional
forward set_sync_height() at the end of
send_and_track_request_to_peer() clobbers the rewind performed by
re_request_values_from_peer_except() on a partial-range failure.

Already fixed upstream in circlefin/malachite#1567 (merged 2026-06-22,
"fix(sync): preserve rewound sync_height after concurrent
re-request"). The related circlefin/malachite#1543
(reject non-contiguous sync responses) was already covered by the
current pin.

This bumps the malachitebft-* git rev from 8ee5d998 (2026-05-22,
predates #1567) to 5cd137fb, the current tip of circlefin/malachite
main, which includes the fix.

Fixes circlefin#214

@osr21 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified every claim in the PR body independently before reviewing — all check out:

  • circlefin/malachite compare 8ee5d998...5cd137fb: exactly 2 commits, and the second is the merge commit of malachite#1567 itself; the only other content is a CI-only chore (GitHub Actions Node 24 bump, #1564) with zero runtime code. This is about as tight as a git-pin bump gets — no unrelated behavior rides along.
  • malachite#1567 merged 2026-06-22 and is the current main tip, as stated. Its mechanism matches the wedge in #214 precisely: the unconditional set_sync_height(range.end + 1) in send_and_track_request_to_peer clobbering a concurrent rewind from the peer-exhaustion path is exactly the "skipped height below sync_height, never re-requested" state we captured in follower logs. The upstream fix also lands with a regression test that reconstructs the two-pending-range interleaving, so the fix is pinned by a test, not just a code change.
  • The malachite#1543 claim is correct too: it merged 2026-04-08, well before the old pin's 2026-05-22 commit, so it's already in the current build — good that the PR body pre-empts that question rather than leaving reviewers to chase it.
  • All 13 Cargo.toml rev entries and all 19 Cargo.lock sources move to the same rev — no split-pin risk where some malachite crates resolve to the old commit.

Two operational notes for whoever merges and for operators watching #214:

  1. The fix is preventive, not curative. A follower currently wedged stays wedged until restarted — the bad state (skipped height below sync_height, request slots full of undeliverable later heights) lives in memory, not on disk, so the binary upgrade's restart clears it as a side effect. Worth one line in the release notes so operators don't wait for an in-place recovery that won't come.

  2. The ~2x/day repro rate is an asset for validation. Since #214 reproduces roughly twice daily per follower on testnet, a 48–72h soak on one follower running this build gives strong empirical confirmation cheaply — zero wedges in that window is meaningful signal, not luck. Happy to run that soak on my follower once a build is available and report back on #214 before this is tagged into a release.

The stated testing (cargo build -p arc-node-consensus to regenerate the lock) is appropriate scope for a pin bump — the behavioral test lives upstream in malachite's suite where it belongs. LGTM.

@osr21

osr21 commented Aug 8, 2026

Copy link
Copy Markdown

I built malachite at the exact commit this PR pins (5cd137fb) and traced the sync paths to check whether the bump actually closes #214. As far as I can tell it doesn't — #1567 hardened a different branch than the one that wedges. Putting the trace here so it's on record before this lands as the fix.

What #1567 changed

It added a sync_height rollback in the no-peer-available branch of re_request_values_from_peer_except (code/crates/sync/src/handle.rs:923 @ 5cd137fb):

// only reached when random_peer_with_except(...) returns None
set_sync_height(state, min(state.sync_height, *entry.range.start()));

That fires only when there is no eligible peer for the failed range.

The branch that actually wedges is the peer-available one

random_peer_with_exceptfilter_peers_by_range (state.rs:114) returns a trimmed prefix whenever no single peer covers the whole range:

// state.rs — else branch: no peer has the whole range
.map(|(peer, status)| (*peer, *range.start()..=status.tip_height))

The caller then requests only that trimmed sub-range:

// handle.rs:933  re_request_values_from_peer_except
send_and_track_request_to_peer(&co, state, metrics, peer, peer_range, entry.excluded_peers).await?;

peer_range is entry.range.start()..=peer.tip_height. The suffix peer.tip_height+1 ..= entry.range.end() is never requested, and sync_height was already advanced past entry.range.end() by the original request — so the dropped suffix sits below sync_height with no covering pending request and is never re-issued. That is exactly the 52893874 gap in the report: request 39162 covered ..873..=..874, failed, and the replacement carried only ..873.

The identical trim-and-drop exists on the partial-response path request_values_range (handle.rs:750), which is also single-shot.

Net

This pin bump looks necessary but not sufficient: it stops the no-peers-at-all stall but not the reported one (peers present, none covering the full failed range). @arjun215-eng's fork commit (378b0e5) is the right shape — it loops the remainder so the whole range is eventually covered across peers; I walked its termination argument on #214 and it holds. To actually close #214, that fix needs to land in circlefin/malachite and the pin bumped to that commit rather than 5cd137fb.

Happy to help verify a repro — the trigger is any failed multi-height fetch where the replacement peer's tip_height is below the failed range's end.

@ygd58

ygd58 commented Aug 9, 2026

Copy link
Copy Markdown
Author

Agreed — this matches what I flagged over on #214 after realizing the same thing. To be precise about scope: this PR only hardens the no-peer branch (#1567), it does not close #214 on its own. I have re-titled my mental model of this PR as "necessary but not sufficient" and left #214 open rather than closing it via this PR's description.

Given your trace confirms @arjun215-eng's suffix-loop fix is the actual fix for the wedge, I will hold off on bumping the pin further until that lands in circlefin/malachite:main, then update this PR (or open a follow-up) to point at that commit so #214 can close for real. Also asked on #214 whether external node operators can get onboarded as P2P/gossip sentries on testnet to help soak-test it once it's in — no path for that today as far as I can tell, so verification is currently limited to the unit-test level (which your and arjun215-eng's analysis already covers well).

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)

2 participants