fix: broadcast HeadChange::Revert(Tipset) in set_heaviest_tipset#6732
Open
hanabi1224 wants to merge 11 commits intomainfrom
Open
fix: broadcast HeadChange::Revert(Tipset) in set_heaviest_tipset#6732hanabi1224 wants to merge 11 commits intomainfrom
hanabi1224 wants to merge 11 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces single-event HeadChange with batched PathChanges (reverts/applies); ChainStore preloads and persists a concrete head, computes path diffs on head updates, caches the current head, and publishes PathChanges. Downstream components updated to consume batched reverts/applies. Changes
Sequence Diagram(s)sequenceDiagram
participant ChainStore
participant Cache
participant Indexer
participant MsgPool
participant RPC
ChainStore->>Cache: preload concrete head (provider or genesis)
Note right of ChainStore: on set_heaviest_tipset(head)
ChainStore->>ChainStore: persist new head key
ChainStore->>ChainStore: chain_get_path(old_head, head) -> PathChanges{reverts, applies}
alt path computed
loop each revert in PathChanges.reverts
ChainStore->>Indexer: publish Revert (part of HeadChanges)
ChainStore->>MsgPool: publish Revert
ChainStore->>RPC: publish Revert
end
loop each apply in PathChanges.applies
ChainStore->>Indexer: publish Apply (part of HeadChanges)
ChainStore->>MsgPool: publish Apply
ChainStore->>RPC: publish Apply
end
else path error
ChainStore->>ChainStore: log warning and skip publishing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state_manager/mod.rs (1)
1213-1240:⚠️ Potential issue | 🔴 CriticalClear the candidate when a matching
Revertarrives.Line 1240 makes
Reverta no-op, butcandidate_tipsetis exactly what this loop returns once the confidence window is met. If that tipset is rolled back before then, the nextApplycan still satisfy the epoch check andwait_for_messagereturns a receipt from a non-canonical chain.Possible fix
- HeadChange::Revert(_) => {} + HeadChange::Revert(tipset) => { + if candidate_tipset + .as_ref() + .is_some_and(|candidate| candidate.key() == tipset.key()) + { + candidate_tipset = None; + candidate_receipt = None; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state_manager/mod.rs` around lines 1213 - 1240, When handling HeadChange::Revert(_) inside the head_change match (the same block that updates candidate_tipset and candidate_receipt on HeadChange::Apply), clear any stored candidate state so a reverted tipset cannot later satisfy the confidence check: set candidate_tipset = None and candidate_receipt = None (and persist any needed block_revert bookkeeping if present) so wait_for_message cannot return a receipt from a non-canonical chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain/store/chain_store.rs`:
- Around line 145-169: The code treats an empty heaviest_tipset_cache as "no
previous head" which is wrong on fresh start; before acting on None in
set_heaviest_tipset, load/seed the cache from the persisted head (use the
heaviest_tipset_key_provider to retrieve the saved tipset key and load the
Tipset from the blockstore) so the method can compute a proper chain path via
crate::rpc::chain::chain_get_path(old_head.key(), head.key()) and publish the
correct revert/apply sequence via publisher; if no persisted head exists then
fall back to publishing Apply(head) as currently implemented. Ensure you update
the logic in set_heaviest_tipset and any helper used to read the persisted key
so the cache is populated atomically before comparing old_head vs head.
In `@src/daemon/mod.rs`:
- Around line 508-513: The current loop only handles HeadChange::Apply and
ignores HeadChange::Revert, leaving delegated messages from reverted tipsets in
eth_mappings; add handling for HeadChange::Revert from receiver.recv() so
reverted tipsets are removed from the ETH mapping. Specifically, in the match/if
over HeadChange, add a branch for HeadChange::Revert(ts) that computes the
delegated messages the same way (using
chain_store.headers_delegated_messages(ts.block_headers().iter())) and then
removes those entries from storage (introduce or call a removal API such as
chain_store.remove_delegated_messages or a method that deletes from
eth_mappings), and add a debug log like "Removing delegated messages for
reverted tipset {}", ts.key(); keep the Apply branch logic unchanged.
In `@src/message_pool/msgpool/msg_pool.rs`:
- Around line 561-574: The code currently calls head_change for each HeadChange
(Apply/Revert) individually, which breaks its assumption that it receives the
whole reorg (so it can cancel reapplied messages from rmsgs before re-adding);
instead, accumulate PathChange/HeadChange entries from the loop into aggregate
reverts and applies lists (collect all ts for reverts and applies), and invoke
head_change once per head transition with those full vectors (preserving
parameters like api.as_ref(), bls_sig_cache.as_ref(), repub_trigger.clone(),
republished.as_ref(), pending.as_ref(), ¤t_ts and the aggregated reverts
and applies) so that head_change can perform its cancel-and-readd logic
correctly.
---
Outside diff comments:
In `@src/state_manager/mod.rs`:
- Around line 1213-1240: When handling HeadChange::Revert(_) inside the
head_change match (the same block that updates candidate_tipset and
candidate_receipt on HeadChange::Apply), clear any stored candidate state so a
reverted tipset cannot later satisfy the confidence check: set candidate_tipset
= None and candidate_receipt = None (and persist any needed block_revert
bookkeeping if present) so wait_for_message cannot return a receipt from a
non-canonical chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9cb67768-ab65-4760-b1e7-b977f3f0d444
📒 Files selected for processing (5)
src/chain/store/chain_store.rssrc/daemon/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/rpc/methods/chain.rssrc/state_manager/mod.rs
Member
|
no green checkmark, no review! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/chain/store/chain_store.rs (1)
25-28: Consider movingPathChange/chain_get_pathout of the RPC module.
ChainStorenow depends oncrate::rpc::chainfor its core head-change type and transition logic. That couples storage internals to the public RPC surface more tightly than necessary. A small chain-internal helper/type shared by RPC andChainStorewould keep the API aligned without inverting the module boundary.Also applies to: 53-53, 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 25 - 28, ChainStore currently imports PathChange (and uses chain_get_path) from crate::rpc::chain, coupling storage to the RPC layer; extract the PathChange type and the chain_get_path helper into a small shared module (e.g., crate::chain::path or crate::core::chain) and update references: replace imports of crate::rpc::chain::PathChange and crate::rpc::chain::chain_get_path in ChainStore (and any other consumers) to point to the new shared module, and re-export or import from the new module in crate::rpc::chain so the public RPC surface keeps the same external API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain/store/chain_store.rs`:
- Around line 127-133: The current logic silently falls back to genesis for any
error reading the persisted HEAD; change ChainStore::new so it distinguishes the
explicit “no head stored yet” case from other errors: call
heaviest_tipset_key_provider.heaviest_tipset_key().map_err(...)? and if that
returns a specific NotFound/NoSuchKey variant treat it as the "no head" case and
construct Tipset::from(&genesis_block_header), but for any other Err return
Err(anyhow::Error) with context (use anyhow::Result and .context()) so the
constructor fails fast; likewise, do not swallow errors from
chain_index.load_required_tipset(&head_tsk) — propagate them with .context()
instead of falling back to genesis. Ensure references to
heaviest_tipset_key_provider.heaviest_tipset_key(),
chain_index.load_required_tipset(&head_tsk),
Tipset::from(&genesis_block_header), HEAD_KEY and ChainStore::new are used to
locate and update the code.
- Around line 152-174: Right now you mutate state (heaviest_tipset_key via
set_heaviest_tipset_key and swap heaviest_tipset_cache) before performing
fallible work (head.key().save and crate::rpc::chain::chain_get_path), which can
leave the store advanced without persisted key or without notifying subscribers;
instead first call head.key().save(self.blockstore()) and compute the path via
crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()), returning
Err(e) if path generation fails, then—only after both save and path
succeed—perform set_heaviest_tipset_key(...) and replace the cache
(std::mem::replace on heaviest_tipset_cache) and finally publish the HeadChange
items via self.publisher.send, preserving the current logging behavior on
publish failure; ensure any error from save or chain_get_path is propagated
rather than just logged.
---
Nitpick comments:
In `@src/chain/store/chain_store.rs`:
- Around line 25-28: ChainStore currently imports PathChange (and uses
chain_get_path) from crate::rpc::chain, coupling storage to the RPC layer;
extract the PathChange type and the chain_get_path helper into a small shared
module (e.g., crate::chain::path or crate::core::chain) and update references:
replace imports of crate::rpc::chain::PathChange and
crate::rpc::chain::chain_get_path in ChainStore (and any other consumers) to
point to the new shared module, and re-export or import from the new module in
crate::rpc::chain so the public RPC surface keeps the same external API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5be28acc-02ef-4ea8-aabf-2fbffef70b71
📒 Files selected for processing (1)
src/chain/store/chain_store.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/chain/store/chain_store.rs (2)
132-140:⚠️ Potential issue | 🟠 MajorDon't mask a bad persisted head by seeding from genesis.
This still treats every
heaviest_tipset_key()failure, plus a stored head that can't be loaded, as “no head yet”. On restart that seedsheaviest_tipset_cachewith the wrong base tipset and hides store corruption, so the nextset_heaviest_tipset()can compute a bogus revert/apply path. Only the explicit “no persisted head” case should fall back to genesis; other failures should abortChainStore::new.As per coding guidelines: "Use anyhow::Result for most operations and add context with
.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 132 - 140, Don't treat all failures as "no head yet": call heaviest_tipset_key() and handle its Result explicitly (return Err with .context on Err), treat only the explicit "no persisted head" (e.g. Ok(None) or whatever variant indicates missing) as the case to seed from genesis (Tipset::from(&genesis_block_header)), and for Ok(Some(key)) call chain_index.load_tipset(&key) and return Err with context if that load fails; update the initialization in ChainStore::new to use heaviest_tipset_key(), chain_index.load_tipset(), and Tipset::from(&genesis_block_header) with those distinct error branches so only the missing-head path falls back to genesis.
158-172:⚠️ Potential issue | 🟠 MajorFinish the fallible work before advancing the head.
set_heaviest_tipset_key()and the cache swap happen beforehead.key().save()andchain_get_path(), and a path failure is only logged. If either step fails,ChainStoremoves to the new head while downstream subscribers never see the transition. Save the key and build theHeadChangesfirst, then commit the cache/provider state and publish.As per coding guidelines: "Use anyhow::Result for most operations and add context with
.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 158 - 172, Reorder and make the operations fallible-before-commit: first call head.key().save(self.blockstore()) and compute the HeadChanges via crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) (returning an anyhow::Result with .context() on failures), then only if those succeed call self.heaviest_tipset_key_provider.set_heaviest_tipset_key(...) and swap the heaviest_tipset_cache (std::mem::replace), and finally publish via self.publisher.send(...); convert the function to return anyhow::Result and add .context() to each error-producing call (head.key().save, chain_get_path, set_heaviest_tipset_key, publisher.send) so failures prevent advancing the head and provide diagnostic context.
🧹 Nitpick comments (1)
src/rpc/methods/chain.rs (1)
1557-1581: Document the new publicPathChangesAPI.
PathChangesandinto_change_vecare public now, but they're undocumented. Please document the batch ordering contract (revertsfirst, thenapplies) since other modules consume this type directly.As per coding guidelines: "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1557 - 1581, Add doc comments to the public PathChanges<T> struct and its into_change_vec method explaining their purpose and the batch ordering contract: document that PathChanges.reverts holds tipsets to be reverted and PathChanges.applies holds tipsets to be applied, and that into_change_vec preserves this ordering by emitting all Revert entries first followed by Apply entries; update comments on the struct, the reverts/applies fields, and the into_change_vec method to make this contract explicit for consumers of PathChanges and PathChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/chain.rs`:
- Around line 96-98: The spawned subscription task currently uses `break` inside
the inner `for` after `if let Err(e) = sender.send(ApiHeaders(block)) { ...
break }`, which only exits the inner loop and keeps the task subscribed after
the RPC client disconnects; change that `break` to `return` (or a labeled outer
`break`) so the entire spawned task returns and unsubscribes when `sender.send`
fails, and apply the same change to the analogous `break` in the other block
around lines handling logs (the block covering the code at the comment's second
location).
- Around line 1321-1329: The loop over subscriber.recv() currently maps each
HeadChange into individual ApiHeadChange messages and sends them one-by-one,
breaking atomicity; instead collect the entire changes.into_change_vec() into a
single Vec<ApiHeadChange> preserving order (mapping
HeadChange::Apply/HeadChange::Revert to ApiHeadChange entries) and call
sender.send(...) once per recv() to emit the whole batch; update the code around
subscriber.recv().await, the for-change mapping, and the sender.send call so
that ApiHeadChange and tipset conversion happens into a single Vec and respect
existing error/break handling when send returns Err.
---
Duplicate comments:
In `@src/chain/store/chain_store.rs`:
- Around line 132-140: Don't treat all failures as "no head yet": call
heaviest_tipset_key() and handle its Result explicitly (return Err with .context
on Err), treat only the explicit "no persisted head" (e.g. Ok(None) or whatever
variant indicates missing) as the case to seed from genesis
(Tipset::from(&genesis_block_header)), and for Ok(Some(key)) call
chain_index.load_tipset(&key) and return Err with context if that load fails;
update the initialization in ChainStore::new to use heaviest_tipset_key(),
chain_index.load_tipset(), and Tipset::from(&genesis_block_header) with those
distinct error branches so only the missing-head path falls back to genesis.
- Around line 158-172: Reorder and make the operations fallible-before-commit:
first call head.key().save(self.blockstore()) and compute the HeadChanges via
crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) (returning
an anyhow::Result with .context() on failures), then only if those succeed call
self.heaviest_tipset_key_provider.set_heaviest_tipset_key(...) and swap the
heaviest_tipset_cache (std::mem::replace), and finally publish via
self.publisher.send(...); convert the function to return anyhow::Result and add
.context() to each error-producing call (head.key().save, chain_get_path,
set_heaviest_tipset_key, publisher.send) so failures prevent advancing the head
and provide diagnostic context.
---
Nitpick comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1557-1581: Add doc comments to the public PathChanges<T> struct
and its into_change_vec method explaining their purpose and the batch ordering
contract: document that PathChanges.reverts holds tipsets to be reverted and
PathChanges.applies holds tipsets to be applied, and that into_change_vec
preserves this ordering by emitting all Revert entries first followed by Apply
entries; update comments on the struct, the reverts/applies fields, and the
into_change_vec method to make this contract explicit for consumers of
PathChanges and PathChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5990110e-701b-49d3-953d-8a6945b4e8a8
📒 Files selected for processing (7)
src/chain/store/chain_store.rssrc/daemon/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/test_provider.rssrc/rpc/methods/chain.rssrc/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/message_pool/msgpool/msg_pool.rs
- src/state_manager/mod.rs
hanabi1224
force-pushed
the
hm/head-change-revert
branch
from
179bc6a to
a758183
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/chain/store/chain_store.rs (2)
160-177:⚠️ Potential issue | 🟠 MajorFinish
chain_get_path()before committing the new head.Lines 162-174 still persist the new head and swap the cache before
chain_get_path(), then only log on failure. If path generation fails, subscribers miss the transition even thoughChainStorehas already advanced.🔁 Suggested reordering
pub fn set_heaviest_tipset(&self, head: Tipset) -> Result<(), Error> { head.key().save(self.blockstore())?; + let old_head = self.heaviest_tipset(); + let changes = crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) + .map_err(|e| Error::Other(format!("failed to get chain path changes: {e}")))?; + self.heaviest_tipset_key_provider .set_heaviest_tipset_key(head.key())?; - let old_head = std::mem::replace(&mut *self.heaviest_tipset_cache.write(), head.clone()); - - match crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) { - Ok(changes) => { - if self.publisher.send(changes).is_err() { - debug!("did not publish changes, no active receivers"); - } - } - Err(e) => { - warn!("failed to get chain path changes: {e}") - } + *self.heaviest_tipset_cache.write() = head; + + if self.publisher.send(changes).is_err() { + debug!("did not publish changes, no active receivers"); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 160 - 177, In set_heaviest_tipset, do not persist the new head or swap the heaviest_tipset_cache until chain_get_path succeeds: call crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) first, and only on Ok(changes) proceed to save head.key() to the blockstore, call heaviest_tipset_key_provider.set_heaviest_tipset_key(head.key()), replace the cache (self.heaviest_tipset_cache.write()) and then send changes via self.publisher; on Err(e) return the error (or propagate it) so the chain store state is not advanced when path computation fails.
132-142:⚠️ Potential issue | 🟠 MajorOnly use the genesis fallback when
HEAD_KEYis actually absent.Line 132 currently treats
Some(head_tsk)plus a missing tipset the same as “no stored head” and seeds genesis. That leaves the cache out of sync with the persisted head, so the next update can compute reverts/applies from genesis instead of the real previous head after restart.🐛 Proposed fix
- let head = if let Some(head_tsk) = heaviest_tipset_key_provider + let head = if let Some(head_tsk) = heaviest_tipset_key_provider .heaviest_tipset_key() .context("failed to load head tipset key")? - && let Some(head) = chain_index - .load_tipset(&head_tsk) - .context("failed to load head tipset")? { - head + chain_index + .load_required_tipset(&head_tsk) + .context("failed to load persisted head tipset")? } else { Tipset::from(&genesis_block_header) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 132 - 142, The current logic falls back to genesis whenever either HEAD_KEY is missing or loading the stored tipset fails; instead only use Tipset::from(&genesis_block_header) when heaviest_tipset_key_provider.heaviest_tipset_key() returns None. Change the code to first call heaviest_tipset_key() and match on that Option: if None, set head = Tipset::from(&genesis_block_header); if Some(head_tsk), call chain_index.load_tipset(&head_tsk) and propagate an error (with context) if loading yields None or fails (do not silently fall back to genesis). Update the branch around heaviest_tipset_key_provider, heaviest_tipset_key(), chain_index.load_tipset, Tipset::from, and HEAD_KEY accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tool/subcommands/api_cmd/generate_test_snapshot.rs`:
- Around line 225-227: The current ReadOpsTrackingStore::heaviest_tipset_key()
forwards an Option<TipsetKey> which can cause ensure_chain_head_is_tracked() to
persist a None into tracker::HEAD_KEY; change ensure_chain_head_is_tracked()
(and any call sites) to only write to tracker::HEAD_KEY when
heaviest_tipset_key() returns Some(key) and retry/wait when it returns None, and
ensure that HEAD_KEY is populated from a concrete TipsetKey before calling
export_forest_car(); also update is_chain_head_tracked() logic to detect the
actual TipsetKey presence (not just a key entry) so it continues retrying until
a real TipsetKey is available in the tracker MemoryDB.
---
Duplicate comments:
In `@src/chain/store/chain_store.rs`:
- Around line 160-177: In set_heaviest_tipset, do not persist the new head or
swap the heaviest_tipset_cache until chain_get_path succeeds: call
crate::rpc::chain::chain_get_path(self, old_head.key(), head.key()) first, and
only on Ok(changes) proceed to save head.key() to the blockstore, call
heaviest_tipset_key_provider.set_heaviest_tipset_key(head.key()), replace the
cache (self.heaviest_tipset_cache.write()) and then send changes via
self.publisher; on Err(e) return the error (or propagate it) so the chain store
state is not advanced when path computation fails.
- Around line 132-142: The current logic falls back to genesis whenever either
HEAD_KEY is missing or loading the stored tipset fails; instead only use
Tipset::from(&genesis_block_header) when
heaviest_tipset_key_provider.heaviest_tipset_key() returns None. Change the code
to first call heaviest_tipset_key() and match on that Option: if None, set head
= Tipset::from(&genesis_block_header); if Some(head_tsk), call
chain_index.load_tipset(&head_tsk) and propagate an error (with context) if
loading yields None or fails (do not silently fall back to genesis). Update the
branch around heaviest_tipset_key_provider, heaviest_tipset_key(),
chain_index.load_tipset, Tipset::from, and HEAD_KEY accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1619eb52-fd3e-41b1-a890-7ff4d4451c7c
📒 Files selected for processing (7)
src/chain/store/chain_store.rssrc/db/car/many.rssrc/db/gc/snapshot.rssrc/db/memory.rssrc/db/mod.rssrc/db/parity_db.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/rpc/methods/chain.rs (2)
1321-1330:⚠️ Potential issue | 🟠 MajorBatch all changes into a single
sender.send(...)call to preserve atomicity.The current implementation sends each
HeadChangeas a separate single-elementVec<ApiHeadChange>, breaking the atomicity that Lotus-compatibleFilecoin.ChainNotifyclients expect. Lotus'sSubHeadChangespublishes all reverts followed by all applies as a single[]*api.HeadChangeslice per head transition.Additionally,
breakat line 1328 only exits the innerforloop—usebreakon the outer loop orreturnto terminate the task when send fails.🔧 Proposed fix
while let Ok(changes) = subscriber.recv().await { - for change in changes.into_change_vec() { - let (change, tipset) = match change { - HeadChange::Apply(ts) => ("apply".into(), ts), - HeadChange::Revert(ts) => ("revert".into(), ts), - }; - if sender.send(vec![ApiHeadChange { change, tipset }]).is_err() { - break; - } - } + let api_changes: Vec<ApiHeadChange> = changes + .into_change_vec() + .into_iter() + .map(|change| match change { + HeadChange::Apply(ts) => ApiHeadChange { + change: "apply".into(), + tipset: ts, + }, + HeadChange::Revert(ts) => ApiHeadChange { + change: "revert".into(), + tipset: ts, + }, + }) + .collect(); + + if sender.send(api_changes).is_err() { + break; + } }Based on learnings: In Forest's
chain_notifyfunction,Filecoin.ChainNotifymust send the full revert+apply batch for a single head transition as oneVec<ApiHeadChange>persender.send(...)call, matching LotusSubHeadChanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1321 - 1330, The loop currently sends each HeadChange individually and only breaks the inner loop on send failure; modify the logic where subscriber.recv().await yields changes so you collect all change.into_change_vec() entries into a single Vec<ApiHeadChange> (preserving order: all Revert then Apply as produced), then call sender.send(that_vec) once per recv to preserve atomicity (referencing subscriber.recv(), HeadChange::Apply, HeadChange::Revert, and ApiHeadChange), and on send error terminate the task immediately (use return or break out of the outer loop instead of breaking the inner for loop).
133-138:⚠️ Potential issue | 🟠 MajorUse
returninstead ofbreakto terminate the spawned task.The
breakat line 137 only exits the innerforloop, leaving the spawned task subscribed and processing future head changes even after the RPC client has disconnected. This is inconsistent with thenew_headsfunction (line 98) which correctly usesreturn.🔧 Proposed fix
if !logs.is_empty() && let Err(e) = sender.send(logs) { tracing::error!("Failed to send logs for tipset {}: {}", ts.key(), e); - break; + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 133 - 138, The spawned task currently uses break inside the loop handling head-change logs (the block containing sender.send(logs) and tracing::error!("Failed to send logs for tipset {}: {}", ts.key(), e)), which only exits the inner loop and keeps the task subscribed; replace that break with return so the task returns immediately (matching the behavior in new_heads) to terminate the task and stop processing further head changes when sending fails. Ensure you update the code path where Err(e) from sender.send(logs) is handled to use return rather than break.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1321-1330: The loop currently sends each HeadChange individually
and only breaks the inner loop on send failure; modify the logic where
subscriber.recv().await yields changes so you collect all
change.into_change_vec() entries into a single Vec<ApiHeadChange> (preserving
order: all Revert then Apply as produced), then call sender.send(that_vec) once
per recv to preserve atomicity (referencing subscriber.recv(),
HeadChange::Apply, HeadChange::Revert, and ApiHeadChange), and on send error
terminate the task immediately (use return or break out of the outer loop
instead of breaking the inner for loop).
- Around line 133-138: The spawned task currently uses break inside the loop
handling head-change logs (the block containing sender.send(logs) and
tracing::error!("Failed to send logs for tipset {}: {}", ts.key(), e)), which
only exits the inner loop and keeps the task subscribed; replace that break with
return so the task returns immediately (matching the behavior in new_heads) to
terminate the task and stop processing further head changes when sending fails.
Ensure you update the code path where Err(e) from sender.send(logs) is handled
to use return rather than break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 699e88bc-39c3-4d7d-b4f0-5392463f189f
📒 Files selected for processing (1)
src/rpc/methods/chain.rs
hanabi1224
force-pushed
the
hm/head-change-revert
branch
from
6215089 to
c6b0c0c
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
2 participants
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 of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6729
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Refactor
Bug Fixes