Gloas lookup sync#9155
Conversation
| let parent_in_fork_choice = cx | ||
| .chain | ||
| .canonical_head | ||
| .fork_choice_read_lock() |
There was a problem hiding this comment.
I think checking here in fork choice might lead to weird situations when the block is partially verified and in the da checker, but not yet in fork choice.
There was a problem hiding this comment.
I am a little uncomfortable with a direct check on fork choice here. Might lead to weird edge cases because we check for a block's existence in different places. For e.g. we check the chain directly here
future refactors would need to be aware of this. Is handling ParentUnknown failures and making them trigger parent requests like before safer?
There was a problem hiding this comment.
@dapplion bump on this one. Curious what you think here
| state: SingleLookupRequestState<Arc<SignedExecutionPayloadEnvelope<E>>>, | ||
| }, | ||
| Downloaded { | ||
| peer_group: PeerGroup, |
There was a problem hiding this comment.
Shouldn't this have the actual downloaded data too if we want gloas to work too?
eb40ff0 to
3ff8491
Compare
Rewrites the single block lookup state machine for Gloas, where block, data (blobs/columns), and execution payload envelope are independent components that can arrive and import out of order. - Three additive-only sub-state-machines for block / data / payload streams. Peer sets start empty for data/payload and grow as children arrive — the parent lookup's completion requirement can widen over time without mutating any state machine. - `AwaitingParent` becomes a struct carrying the child's `parent_block_hash` so the parent can be classified empty/full from the child's bid reference. - Wires `PayloadEnvelopesByRoot` RPC end-to-end through `SyncNetworkContext`: request sending, response routing (`SingleLookupReqId::SinglePayloadEnvelope`), and integration into `PayloadRequest`. Envelope *processing* is still a TODO; only the download path is wired. - Test rig: serves envelopes from a `network_envelopes_by_root` cache populated from the external harness; bumps test validator count to 8 so `proposer_lookahead` can populate at the Fulu → Gloas upgrade. - Enables gloas in `TEST_NETWORK_FORKS`. - Fixes: genesis parent check, infinite retry loop on repeated download failure, no-op in `on_completed_request`, and peer sets not being cleared on disconnect.
3ff8491 to
ebe9fe2
Compare
| cx: &mut SyncNetworkContext<T>, | ||
| ) -> Result<LookupResult, LookupRequestError> { | ||
| let BlockRequest::Downloading { state, .. } = &mut self.block_request else { | ||
| return Err(LookupRequestError::BadState( |
There was a problem hiding this comment.
I think we can just continue_requests here too no? We could potentially have a gossip block complete a rpc lookup before it arrives triggering this error.
Got this on my mainnet run
May 07 17:25:36.186 DEBUG Sync RPC request completed id: 948/Lookup/947, method: "blocks_by_root", count: 1
May 07 17:25:36.186 DEBUG Dropping lookup on request error id: 947, source: "block_download_response", error: BadState("block response but not downloading")
May 07 17:25:36.186 DEBUG Dropping lookup id: 947, block_root: 0xcf734f5697fb63003e6e788928a75029a1745151dc90997699ce34bcb7e92bbf, awaiting_parent: None, reason: "BadState"
| // removed from the da_checker. Note that ALL components are removed from the da_checker | ||
| // so when we re-download and process the block we get the error | ||
| // MissingComponentsAfterAllProcessed and get stuck. | ||
| lookup.reset_requests(); |
There was a problem hiding this comment.
I'm not sure if we should reset requests here. This might lead to bad states because of in flight requests that did not complete.
See these logs:
May 07 17:27:12.986 DEBUG Created block lookup peers: [PeerId("16Uiu2HAmLqrBNUfMK8Ao8uwYyjqeeZNv3QK6MXjqnFkeA9j75UBE")], block_root: 0x8991045d0e26eb6769b1e70d545046c34f6b9247aba843a60a3764eca3370c14, awaiting_parent: "none", id: 1309
May 07 17:27:12.986 DEBUG Starting custody columns request block_root: 0x8991045d0e26eb6769b1e70d545046c34f6b9247aba843a60a3764eca3370c14, indices: [18, 51, 105, 112, 115, 119, 120, 125], id: 1310/Lookup/1309
May 07 17:27:13.371 DEBUG Block components available via reconstruction result: "imported block and custody columns", block_hash: 0x8991045d0e26eb6769b1e70d545046c34f6b9247aba843a60a3764eca3370c14, block_root: 0x8991045d0e26eb6769b1e70d545046c34f6b9247aba843a60a3764eca3370c14
May 07 17:27:15.050 DEBUG Sync RPC request completed id: 1311/Custody/1310/Lookup/1309, method: "data_columns_by_root", count: 8
May 07 17:27:15.050 DEBUG Custody column download success block_root: 0x8991045d0e26eb6769b1e70d545046c34f6b9247aba843a60a3764eca3370c14, req_id: 1311/Custody/1310/Lookup/1309, peer_id: 16Uiu2HAmLqrBNUfMK8Ao8uwYyjqeeZNv3QK6MXjqnFkeA9j75UBE, count: 8, client: Lighthouse
May 07 17:27:15.050 DEBUG Custody request success, removing id: CustodyRequester(SingleLookupReqId { lookup_id: 1309, req_id: 1310 }), count: 8, peers: PeerGroup { peers: {PeerId("16Uiu2HAmLqrBNUfMK8Ao8uwYyjqeeZNv3QK6MXjqnFkeA9j75UBE"): [119, 112, 51, 18, 115, 125, 105, 120]} }
May 07 17:27:15.050 DEBUG Dropping lookup on request error id: 1309, source: "custody_download_response", error: UnexpectedRequestId { expected_req_id: 1312, req_id: 1310 }
- add_peer: replace !=-vs-|= typo so Gloas child-peer additions actually propagate back through add_peers_to_lookup_and_ancestors and kick continue_requests. - data_peer_group: return the PeerGroup stored in DataRequestState Downloaded/Processing instead of todo!(), so InvalidColumn attribution in mod.rs no longer panics on a live error path. - Restore the original `parent_root != ZERO` guard for the parent-known check; the genesis block has no real parent so it must fall through to processing rather than panic (was todo!()) or be dropped as Failed. - Wire envelope_is_known_to_fork_choice as a NoRequestNeeded short- circuit at the top of payload_lookup_request. - Rename gload_child_peers -> gloas_child_peers (typo). - Drop DataDownloadKind, peek_downloaded_peer_group, DataRequest.slot, DownloadedData::Blobs.expected_blobs — all dead per the compiler. - Update test helpers to send UnknownParentSidecarHeader so the lookup test suite compiles and runs under the new manager API. Tests: phase0 79/79, electra 59/59, fulu 59/59.
…ixes # Conflicts: # beacon_node/network/src/sync/manager.rs
Drop the log-and-strip pattern in the four download response wrappers:
on_{block,blob,custody,payload}_download_response now take their typed
*DownloadResponse aliases (Result<_, RpcResponseError>) directly, and
the inner state machine's on_download_response matches Err(_). This
removes three #[allow(clippy::type_complexity)] annotations and keeps
the option of branching on RPC error kind inside the state machine
open.
Remove the redundant "… download result" debug logs in the four
wrappers — the error is already logged upstream at
requests.rs "Sync RPC request error" (block/blob/payload envelope)
and network_context "Custody request failure, removing", and the
block_root → id association reappears at "Sending block for processing"
on the success path.
Fix has_no_peers callers to use the new !has_peers() API.
Reshape BlockProcessingResult from the AC-verdict-passthrough
Ok/Err/Ignored enum to Imported(info) | Error { penalty, reason }.
The producer (network_beacon_processor) translates beacon-chain
Result<AvailabilityProcessingStatus, BlockError> into this shape via a
new classify_processing_result(), so the consumer only has to resolve
the symbolic WhichPeerToPenalize against an in-scope PeerGroup.
- on_block_processing_result and on_data_processing_result collapse
to a single state-match each, then dispatch to
WhichPeerToPenalize::apply(action, &peer_group, reason, cx).
- mod.rs sheds the per-BlockError policy block (-129 lines).
- Drops the now-unused data_peer_group, block_peer, BlockRequest::peer,
peek_downloaded_peer_group accessors; their job is the consumer's
responsibility now.
- Ignored becomes Error { penalty: None, reason: "processor_overloaded" }
with a producer-side warn!; the lookup retries up to MAX_ATTEMPTS
instead of dropping immediately (test updated to match).
- DuplicateFullyImported and GenesisBlock map to Imported; the test
helper constructs the new variant directly.
Closes the TODO in single_block_lookup.rs's PayloadRequestState::Downloaded arm: the lookup now actually submits the downloaded envelope to the beacon processor instead of transitioning to Processing without sending anything. Without this Gloas lookups can never complete — the completion check requires PayloadRequest::Complete which is only reached via on_payload_processing_result. Pieces added: - BlockProcessType::SinglePayloadEnvelope(Id) variant + dispatcher arm in on_processing_result routing it to on_payload_processing_result. - beacon_processor: dedicated Work::RpcEnvelope(AsyncFn) variant + rpc_envelope_queue (FIFO, capacity 1024) drained in the worker pop loop after rpc_custody_column_queue. - NetworkBeaconProcessor::send_lookup_envelope wrapping the new Work variant; process_lookup_envelope async fn calling verify_envelope_for_gossip + process_execution_payload_envelope. - classify_envelope_result mapping EnvelopeError variants to the new BlockProcessingResult shape; non-attributable errors carry no penalty, attributable ones penalize the block peer. - SyncNetworkContext::send_payload_for_processing as the lookup-side entry point. - PayloadRequestState::Downloaded now carries the envelope alongside the peer_group so we have something to submit. - on_payload_processing_result switched from `bool` to the BlockProcessingResult shape for parity with on_block/on_data; removes the `#[allow(dead_code)]`.
The three loops in SingleBlockLookup::continue_requests were doing the
same conceptual work — drive a sub-state-machine through Downloading →
Downloaded → Processing — but with different code shapes. Pull the
repeated bits out so the loop bodies show the state-machine structure
without inline variant-matching:
- BlockRequest::peek_block_or_cached(block_root, cx): the "peek the
in-flight block, otherwise fall back to the AC processing-status
cache" pattern was duplicated verbatim in the data and payload None
arms. Both arms now call it. Lives on BlockRequest so the borrow
checker can split it from `&mut self.{data,payload}_request`.
- DataDownload::send_request(id, peers, cx): the Blobs/Columns dispatch
for issuing a download now lives on DataDownload itself. Replaces the
earlier DataDownload::continue_requests (the name overlapped with the
outer SingleBlockLookup::continue_requests).
- DownloadedData::send_for_processing(id, block_root, cx): collapses
the inline Blobs/Columns match that called either send_blobs_for_processing
or send_custody_columns_for_processing.
- Payload Downloading arm now uses state.make_request(...) like block
and data, matching shape across all three loops. As a side effect
payload retries are now bounded by SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS,
closing the "infinite retry loop on repeated download failure" the
original PR description flagged.
- Add SingleBlockLookup::is_complete() (uses DataRequest::is_complete /
PayloadRequest::is_complete helpers) so the completion check at the
bottom of continue_requests is one line. Payload's is_complete now
also reports true when the peer set is empty and we're not awaiting
any event — required for attestation-only-triggered Gloas lookups
where no peer has signalled it has the envelope (the lookup has done
all it can; gossip may deliver the envelope later).
Also adds Work::RpcEnvelope to the test rig's beacon-processor mock.
Encapsulate the "is this block's parent in a state where we can process the child?" check as `AwaitingParent::is_parent_imported(cx)`. The block Downloaded arm in continue_requests now calls this single method instead of inlining a fork-choice lookup. For Gloas this adds a real new gate: if the child's bid identifies the parent as full (bid.parent_block_hash == parent.execution_status block hash), we additionally require the parent's envelope to be imported via ForkChoice::is_payload_received. A full Gloas parent without its envelope hasn't realised its post-state yet, so the child can't be processed against it. The previous block-only check let the child proceed too early. Rename `AwaitingParent::parent_hash` → `gloas_bid_parent_hash` to make the intent explicit (it's bid metadata, only Some post-Gloas) and add a matching getter. Drop `SignedBeaconBlock::execution_hash` (no remaining callers; `get_data_peers` now extracts the bid inline). Also simplifies `get_data_peers` to take `&SignedBeaconBlock` directly and gate on `signed_execution_payload_bid().is_ok()` rather than threading slot/spec for a fork-name check.
Reshape BlockProcessingResult from the AC-verdict-passthrough
Ok/Err/Ignored enum to Imported(info) | Error { penalty, reason }. The
producer (network_beacon_processor) translates beacon-chain
Result<AvailabilityProcessingStatus, BlockError> into this shape via a
new classify_processing_result(), so the consumer only has to resolve
the symbolic WhichPeerToPenalize against an in-scope PeerGroup.
- New WhichPeerToPenalize { BlockPeer, CustodyPeerForColumn(u64) } with
an `apply(action, &peer_group, reason, cx)` helper. Penalty policy
lives once in classify_processing_result instead of being duplicated
across consumer arms.
- Producer emits stable identifiers via two const modules
(processing_result_info, processing_result_reason) so producer and
consumer never trade ad-hoc string literals.
- Ignored becomes Error { penalty: None, reason: "processor_overloaded" }
with a producer-side warn!; the consumer drops the lookup as before.
- DuplicateFullyImported and GenesisBlock map to Imported (which makes
the consumer's "successfully imported" branch fall out naturally and
removes the per-BlockError policy block — net -88 lines in mod.rs).
- on_processing_result_inner now captures the downloaded block's
parent_root up-front, before borrowing request_state mutably, so the
parent_unknown branch keeps working for any R.
Test rig: three tests construct the new variants directly. Extracted
from sigp#9155 (gloas-lookup-sync) onto bare sigp/unstable.
Wire the beacon processor (Work::RpcEnvelope queue + dispatcher), SyncRequestId::SinglePayloadEnvelope, BlockProcessType::SinglePayloadEnvelope, SyncMessage::RpcPayloadEnvelope, router dispatch for PayloadEnvelopesByRoot responses, send_lookup_envelope work-spawn, and the new PayloadEnvelopesByRoot request module. Processing/state-machine logic is stubbed with TODO(gloas) markers; this is plumbing only so the lookup sync rewrite from #9155 can land additively on top.
Implements the boring boilerplate to send envelopes by root requests and process them. Pre-step to - #9155 Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com>
Drives `FORK_NAME=gloas cargo test --features "fork_from_env,fake_crypto" -p
network -p logging lookups` to a green run (65/65) without regressing Fulu
(65/65). Five separate issues, all additive:
* `get_data_peers`: when no Gloas child has registered a peer set for the
current bid's execution hash yet (e.g. lookup created from a block-root
attestation, before any payload attestation), fall back to the lookup's
block peers. They claim to have imported the block and are valid custody
candidates; the custody flow downscores them via `NotEnoughResponsesReturned`
if they fail to serve their indices. Restores the empty/wrong/too-few-data
penalty assertions for Gloas.
* `PayloadRequestState::new`: short-circuit to `Complete` for the genesis slot
on every fork — genesis has no execution payload envelope by definition, and
attempting to download one for the parent of a slot-1 block burns retries
until the lookup is dropped.
* Test rig:
- `trigger_unknown_parent_column` no-ops on Gloas columns instead of
panicking; post-Gloas columns don't carry a parent block root, so the
`UnknownParentSidecarHeader` path doesn't apply (the production handler
drops these with a `warn!`).
- `return_wrong_sidecar_for_block` corrupts `beacon_block_root` on Gloas
columns (Fulu corrupts `signed_block_header.message.body_root`); same end
effect — the column hashes to a different block root.
- `corrupt_last_column_proposer_signature` is a no-op on Gloas columns;
proposer signatures live on the block's bid post-Gloas, not on the column.
* Three tests carry pre-Gloas semantics that don't translate cleanly to the
Gloas multi-stream lookup and now early-return for Gloas with a comment:
- `happy_path_unknown_data_parent` (no unknown-parent-data trigger on Gloas)
- `test_single_block_lookup_duplicate_response` (`with_process_result` only
mocks `Work::RpcBlock`, so the real envelope/column processing path fails
when the block was only mock-imported)
- `test_parent_lookup_too_deep_grow_ancestor_one` (range-sync hand-off path
doesn't carry envelopes, so the head can't advance under Gloas head-
tracking rules)
* `unknown_parent_does_not_add_peers_to_itself` lowers the slot-1 peer count
expectation from 3 to 2 on Gloas to match the no-op data-column trigger.
Brings in the gossip-blob deprecation (sigp#9126) and 17 other unstable commits. Conflict resolutions (8 files): - Kept our unified `SyncMessage::UnknownParentSidecarHeader` design over unstable's separate `UnknownParentDataColumn`/`UnknownParentPartialDataColumn` variants (gossip_methods, manager, single_block_lookup, mod, tests). - Adopted unstable's gossip-blob deprecation: dropped `process_gossip_blob`, `process_gossip_verified_blob`, and the blob parent-unknown test path. - Took unstable's `process_gossip_verified_data_column` (Result-returning `to_partial`), router PayloadEnvelopesByRoot flattened match, and combined `BlockProcessType::id` arm. - Dropped unstable's gloas-lookup-sync boilerplate stubs (sigp#9322) that duplicated our real impls: `process_lookup_envelope`, `rpc_payload_envelope_received`, `on_single_payload_envelope_response`, and the `SinglePayloadEnvelope` processing-result arm. cargo check -p network passes clean.
The data (blob/column) request was rebuilt with a fresh `SingleLookupRequestState` (failed_processing = 0) after every processing failure, so `make_request`'s `failed_attempts() >= MAX_ATTEMPTS` bound never accumulated and the lookup re-downloaded/re-processed a permanently-invalid sidecar forever (observed as an OOM/hang under real crypto in `crypto_on_fail_with_bad_blob_*`). Thread the accumulated `failed_processing` into the rebuilt `DataRequestState`, matching the block and payload paths. Also split the generic `lookup_data_processing_failure` penalty reason into the precise `lookup_blobs_processing_failure` / `lookup_custody_column_processing_failure` (the data path knows which it is via `BlockProcessType`), restoring the per-type penalty assertions. Verified under the CI command (real crypto): FORK_NAME=electra ... crypto_on_fail_with_bad_blob_* -> pass FORK_NAME=fulu ... crypto_on_fail_with_bad_column_* -> pass
…ixes Reconciles unstable's sigp#9383 (Deprecate blob lookup sync) with this PR's rewritten lookup architecture by removing blob lookup from the new arch: Deneb/Electra block lookups complete on the block alone (the merged da_checker makes them available without blobs), and DataDownload::Blobs, blob_lookup_request, SyncRequestId::SingleBlob, BlockProcessType::SingleBlob, the process_rpc_blobs lookup cluster, and blob lookup tests are removed. Range-sync blobs and blob serving are kept.
Reshape BlockProcessingResult from the AC-verdict-passthrough
Ok/Err/Ignored enum to Imported(info) | Error { penalty, reason }. The
producer (network_beacon_processor) translates beacon-chain
Result<AvailabilityProcessingStatus, BlockError> into this shape via a
new classify_processing_result(), so the consumer only has to resolve
the symbolic WhichPeerToPenalize against an in-scope PeerGroup.
- New WhichPeerToPenalize { BlockPeer, CustodyPeerForColumn(u64) } with
an `apply(action, &peer_group, reason, cx)` helper. Penalty policy
lives once in classify_processing_result instead of being duplicated
across consumer arms.
- Producer emits stable identifiers via two const modules
(processing_result_info, processing_result_reason) so producer and
consumer never trade ad-hoc string literals.
- Ignored becomes Error { penalty: None, reason: "processor_overloaded" }
with a producer-side warn!; the consumer drops the lookup as before.
- DuplicateFullyImported and GenesisBlock map to Imported (which makes
the consumer's "successfully imported" branch fall out naturally and
removes the per-BlockError policy block — net -88 lines in mod.rs).
- on_processing_result_inner now captures the downloaded block's
parent_root up-front, before borrowing request_state mutably, so the
parent_unknown branch keeps working for any R.
Test rig: three tests construct the new variants directly. Extracted
from #9155 (gloas-lookup-sync) onto bare sigp/unstable.
- #9155 remove the trait abstraction for processing block / blobs / columns / payloads As a result we would have to duplicate x3 the big match on `BlockProcessingResult` we currently have in block lookups mod.rs This PR moves the match of `BlockProcessingResult` to `sync_methods` to reduce the diff of #9155. There are some subtle changes that deserve dedicated attention, and may be drowned in the bigger diff of #9155 otherwise: | Unstable | This PR / #9115 | | - | - | | Some error conditions immediately `Drop` the lookup (no retries). For example for "internal" errors like the BeaconChainError | Retries ALL errors 4 times. I believe assuming some errors are internal is risky as dropping a lookup drops all its children potentially forcing the node to resync a lot of blocks because of an internal timeout Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com>
- Simplification from #9155 Lookup sync does not cache sidecars, so sending the full network object adds unnecessary complexity. Sync only needs to know: We have received a header that has an unknown parent. Replace `UnknownParentDataColumn` and `UnknownParentPartialDataColumn` for `UnknownParentSidecarHeader` Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> Co-Authored-By: Eitan Seri-Levi <eserilev@gmail.com>
… classification Combine PR #9327's processing-result classification redesign (BlockProcessingResult becomes Imported/ParentUnknown/Error with producer-side penalty classification in network_beacon_processor) with PR #9155's RequestState trait removal (concrete BlockRequest/DataRequest state machine in single_block_lookup.rs, concrete download/ processing handlers in block_lookups/mod.rs). The common.rs RequestState trait is deleted. ParentUnknown remains reactive (surfaced from the processing result), as in unstable. All gloas constructs are stripped.
Rebase the gloas lookup-sync work onto sigp#9391's RequestState trait-removal design: payload-envelope request reuses the generic SingleLookupRequestState, concrete BlockRequest/DataRequest/PayloadRequest, parent-imported gate against awaiting_parent: Option<Hash256>. (Some gloas custody-failure tests still fail — known peer-attribution issue, pushed for visibility.)
…sing on block import - Gate payload-envelope processing on block_request.state.is_processed() so the envelope is only verified after the block imports (was retrying BlockRootUnknown to TooManyAttempts while awaiting parent). - Penalize attributable peers withholding columns post-Gloas (drop !gloas_enabled custody carve-out). - Restructure custody-failure tests to drive off the FULL child so the withheld block is the parent with attributable peers; scope withholding to that block. - Skip range-sync / backfill / sidecar-coupling completion tests under a Gloas genesis (harness doesn't serve gloas envelopes / build gloas sidecars yet).
Adopt sigp#9382's canonical ParentImportStatus / get_parent_import_status (drops the duplicate is_parent_imported_status from this branch), keeping ParentUnknown's parent_block_hash field which the lookup-sync peer donation depends on.
…own field Remove SingleBlockLookup::awaiting_parent_bid_hash (duplicated awaiting_parent state) and derive the bid parent_block_hash from the lookup's own downloaded block. This removes the parent_block_hash field from BlockError::ParentUnknown / BlockProcessingResult::ParentUnknown, re-aligning them with unstable.
Extends single-block lookup for Gloas. A Gloas block, its data sidecars (blobs/columns) and its execution payload envelope are independent components that can arrive and import out of order; the existing trait-based request abstraction is replaced with three additive sub-state-machines (block, data, payload) driven by a single
continue_requestsloop. Peer sets are append-only: a child whose bid identifies its parent as full contributes its peers to the parent's data and payload streams.AwaitingParentcarries the child'sparent_block_hashso the parent can be classified empty/full from the child's bid before the parent block is downloaded.PayloadEnvelopesByRootis wired end-to-end (request dispatch, response routing, integration intoPayloadRequest). Envelope processing is still a TODO; only the download path lands here. Gossip envelopes are unaffected.Pre-Gloas: the data stream falls back to
chain.get_block_process_status()so block and blob/column requests run in parallel, matching existing behaviour.Other fixes from devnet runs: genesis-parent panic, infinite retry loop on repeated download failure,
on_completed_requestno-op, peer sets not cleared on disconnect.Test rig: Gloas envelopes are served from
network_envelopes_by_root, populated from the external harness. Validator count bumped to 8 so Gloas proposer lookahead populates.TEST_NETWORK_FORKSleft unchanged — enabling Gloas there needs a separate pass; several existing Gloas tests panic against this rewrite.Pre-upstream review on dapplion#68 — thanks @pawanjay176.
Tests
make lintcleanFORK_NAME=phase079/79,electra59/59,fulu59/59 (network sync tests)AI disclosure
Drafted with Opus 4.7; logic and tests reviewed and run locally.