feat(indexer): race committee members on a substate read - #2526
Open
sdbondi wants to merge 1 commit into
Open
Conversation
sdbondi
force-pushed
the
feat/indexer-race-committee-reads
branch
from
September 4, 2026 12:40
ff9b423 to
c48c30b
Compare
sdbondi
commented
Sep 4, 2026
sdbondi
left a comment
Member
Author
There was a problem hiding this comment.
Reviewed at c48c30b. No blocking findings.
Verified sound
race_committeedraws from theshuffled().map(...)iterator lazily, sorequest_substate_from_vnis only entered for members that get a slot;FuturesUnordered::extend(take(width))plus onepushper settled response bounds in-flight towidth.no_more_than_the_window_is_in_flightpins this.- Dropping in-flight requests on settle is safe:
RpcMultiPool::get_or_connectdials without holding the pool lock, and the only write in the verify path (trusted_root_store.record) is best-effort. Nothing here is awith_write_txon a deadpool thread, so the cooperative-teardown trap from the follow-mode work does not apply. - Description claim checked: the old loop tested
num > fbefore incrementing, so a mid-loop settle needed the(f + 2)thDoesNotExistwhile the post-loop check neededf + 1. The tally settles on the(f + 1)th in both places.nonexistence_needs_more_than_f_agreeing_memberscovers it (size 4,f = 1, two responses). - Precedence is unchanged: proven
Up/Downon the spot,f + 1DoesNotExiston the spot, unproven fallback (highest version) then last error, then unverifiedDoesNotExistonly when every member answered without settling and none errored. Empty committee is still rejected before the tally is built. .boxed()on aSendfuture to settle rust-lang/rust#102211 matches the workaround used elsewhere;SubstateRequirementRefisCopy, so passing it both into the map closure and asdescribeis fine.- Cargo.lock delta is only the two new
tari_indexer_libdeps (futures, devtokio). No unrelated files.
Non-blocking
- crates/indexer_lib/src/committee_read.rs:104
concludelogs "Could not get substate for shard {describe}" butdescribeis a substate requirement, not a shard. Carried over from the old code; worth fixing while it is being moved. - A dial dropped mid-connect keeps going inside the networking layer but its session never reaches the pool, so the next read to that member redials. Costs one extra dial per dropped in-flight member; fine for width 3, just noting it is not free.
Nit
- crates/indexer_lib/src/committee_read.rs:22
MemberResponseisResult<(SubstateResult, bool), _>; a namedverifiedfield would read better at the observe site, but the tuple mirrorsget_substate_from_vnso leaving it is reasonable.
CI is still pending; will follow up once it is green.
Member
Author
|
Fixed the "for shard" wording in the |
sdbondi
force-pushed
the
feat/indexer-race-committee-reads
branch
from
September 4, 2026 12:43
c48c30b to
660344d
Compare
sdbondi
commented
Sep 4, 2026
sdbondi
left a comment
Member
Author
There was a problem hiding this comment.
Re-checked at 660344d: the only delta from c48c30b is the "for shard" log wording in CommitteeReadTally::conclude, which was the non-blocking note. CI is green on this head (clippy, all three test shards, integration test). Nothing outstanding from my side; approve is unavailable on my own PR so another maintainer needs to sign off.
A single-substate read asked committee members one at a time, so whenever the shuffled first pick was unreachable the read waited out the full connect timeout before trying the next. On a small committee that is a 1/n chance per read of a multi-second stall. Reads now keep up to three members in flight and settle on the first response that decides the read. The decision rules are unchanged: a proven Up/Down (or any Up/Down while proofs are not required) answers on the spot, an unproven one is held as a fallback while the rest of the committee is tried for a proof, and DoesNotExist is believed once more than f members agree, taking precedence over errors from members that could not be reached. Requests still in flight when the read settles are dropped. The tally and the race driver are split out into committee_read so the arrival-order cases can be exercised without an epoch manager.
sdbondi
force-pushed
the
feat/indexer-race-committee-reads
branch
from
September 7, 2026 05:09
660344d to
39f033f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A single-substate read asked committee members one at a time, so whenever the shuffled first pick was unreachable the read waited out the full
CONNECT_TIMEOUT(10s) before trying the next. On a small committee that is a 1/n chance per read of a multi-second stall.Reads now keep up to
READ_RACE_WIDTH = 3members in flight and settle on the first response that decides the read. Requests still in flight when the read settles are dropped.The decision rules are unchanged and are now in one place (
CommitteeReadTally):Up/Down(or anyUp/Downwhile proofs are not required) answers on the spotUp/Downis held as a fallback (highest version wins) while the rest of the committee is tried for a proofDoesNotExistis believed once more thanfmembers agree, and takes precedence over errors from unreachable membersOne behavioural nit the rewrite removes: the old inline
DoesNotExistcheck testednum > fbefore incrementing, so it neededf + 2agreeing responses on the way through the loop but onlyf + 1at the end. Both now settle onf + 1, which is what the comment always said.Tests
committee_read::testscover arrival-order cases against a stub committee: an unresponsive member does not delay a proven answer (fails with width 1), at mostwidthrequests in flight, a failed member frees its slot, unproven held until a proof arrives, highest unproven version served when nobody can prove,f + 1agreement for nonexistence, single nonexistence outvoted by a proven version, and agreement outranking errors.Phase 3 item 1 from the indexer cache plan.