feat: forester API server #2163
Open
+3,529
−768
Conversation
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
sergeytimoshin
requested review from
SwenSchaeferjohann and
ananas-block
as code owners
Contributor
📝 WalkthroughWalkthroughAdds a Warp-based HTTP API server and static dashboard; implements comprehensive forester status and V2 queue parsing; introduces group-authority filtering and new config fields; renames many CLI env keys; adjusts batching/queue-fetch signatures, metrics, RPC trait, and owner propagation across tree handling. Changes
Sequence Diagram(s)sequenceDiagram
participant B as Browser / Client
participant API as Forester API Server
participant S as Forester Status Logic
participant RPC as LightClient / Solana RPC
participant MET as Prometheus Registry
B->>API: GET /status
activate API
API->>S: assemble_status(rpc_url, filter_by_group_authority)
activate S
S->>RPC: fetch registry PDAs & tree accounts
RPC-->>S: PDAs, TreeAccounts (with owner)
S->>RPC: batch fetch merkle trees, queues, balances
RPC-->>S: account data
S->>MET: read Prometheus metrics
MET-->>S: metric families
S-->>API: ForesterStatus JSON
API-->>B: 200 OK {status, trees, metrics}
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)forester/**/*.rs📄 CodeRabbit inference engine (forester/README.md)
Files:
🧠 Learnings (79)📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-11-24T18:02:15.670ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2026-01-14T00:06:11.902ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2026-01-11T01:05:19.378ZApplied to files:
📚 Learning: 2026-01-10T19:26:21.240ZApplied to files:
📚 Learning: 2026-01-14T00:07:21.323ZApplied to files:
📚 Learning: 2025-11-24T17:59:13.714ZApplied to files:
📚 Learning: 2025-11-24T17:59:03.485ZApplied to files:
📚 Learning: 2025-11-24T17:57:14.561ZApplied to files:
📚 Learning: 2025-11-24T17:57:02.558ZApplied to files:
📚 Learning: 2025-11-24T17:58:35.079ZApplied to files:
📚 Learning: 2025-11-24T17:58:10.392ZApplied to files:
📚 Learning: 2025-11-24T17:58:35.079ZApplied to files:
📚 Learning: 2025-11-24T17:59:54.233ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:57:39.230ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:57:39.230ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:57:24.369ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:57:14.561ZApplied to files:
📚 Learning: 2025-11-24T17:58:10.392ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:58:35.079ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:58:10.392ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T18:02:15.670ZApplied to files:
📚 Learning: 2025-12-19T12:31:15.409ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2026-01-12T14:27:24.122ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:58:35.079ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2026-01-10T19:26:10.578ZApplied to files:
📚 Learning: 2026-01-14T00:06:11.902ZApplied to files:
📚 Learning: 2025-11-24T17:58:50.237ZApplied to files:
📚 Learning: 2026-01-14T00:05:32.198ZApplied to files:
📚 Learning: 2025-11-24T17:59:03.485ZApplied to files:
📚 Learning: 2025-11-24T18:02:15.670ZApplied to files:
📚 Learning: 2026-01-10T19:26:10.578ZApplied to files:
📚 Learning: 2025-11-24T18:02:15.670ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:57:39.230ZApplied to files:
📚 Learning: 2025-11-24T17:57:53.312ZApplied to files:
📚 Learning: 2025-11-24T17:58:35.079ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:20.711ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:56:00.229ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
📚 Learning: 2025-11-24T17:56:50.011ZApplied to files:
🧬 Code graph analysis (1)forester/src/main.rs (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (16)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sergeytimoshin
force-pushed
the
sergey/forester-status-api
branch
from
f623171 to
b328f49
Compare
![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: 14
🤖 Fix all issues with AI agents
In @forester/src/api_server.rs:
- Around line 96-98: The code is ignoring the Result from encoder.encode(),
which can leave buffer empty and return an empty MetricsResponse silently;
change the call to check the Result of encoder.encode(&metric_families, &mut
buffer), and on Err log the error (including the error message) and return an
appropriate error response (e.g., an HTTP 500 MetricsResponse or propagate the
error) instead of continuing to create text from an empty buffer; update the
logic around buffer/text/MetricsResponse to handle the failure path cleanly.
- Around line 49-74: The status route currently uses tokio::task::spawn_blocking
to call get_forester_status_blocking, causing redundant runtime creation; change
the handler to call get_forester_status_async(&rpc_url) directly from the async
closure (remove spawn_blocking and get_forester_status_blocking usage),
propagate and handle its Result the same way you did before (Ok(status) -> json
status, Err(e) -> log and return ErrorResponse), and make
get_forester_status_async public so it can be called from this module.
In @forester/src/forester_status.rs:
- Line 94: The function get_forester_status_async is currently private but
should be made public so external callers (like the API server) can await it
directly and avoid creating nested runtimes; change its signature to pub async
fn get_forester_status_async(rpc_url: &str) -> crate::Result<ForesterStatus> and
update any module exports if necessary (e.g., pub(crate) vs pub) so
api_server.rs can call it directly and remove the redundant runtime::block_on or
runtime creation there.
- Around line 266-268: The loop over tree_statuses currently does let
queue_pubkey: Pubkey = status.queue.parse().unwrap_or_default(), which silently
substitutes Pubkey::default() on parse failures; change this to explicitly
handle parse errors (e.g., match or if let Err) so you log an error that
includes the invalid status.queue string and the parse error and then skip
processing that status (continue) instead of computing schedules with a zero
pubkey; update the block that creates schedule: Vec<Option<usize>> to run only
after a successful parse (using the parsed queue_pubkey variable).
- Line 586: The match arm handling TreeType::Unknown currently returns (0.0, 0,
0, None, None) silently; update that arm to emit a warning before returning (use
the project's logging facility, e.g., log::warn! or tracing::warn!) and include
identifying context such as the tree id/uuid or name and the tree_type
(TreeType::Unknown) so operators can see when an unexpected type is encountered,
then return the same tuple as before.
- Around line 338-363: The code currently ignores errors from forester_result,
epoch_result, and config_result which can hide RPC/deserialization failures;
update the handling so each result is matched (or check if let Err(e)) and when
Err occurs call the logger (e.g., process_logger.warn/error or similar)
including the error details and context (e.g., "failed fetching forester
accounts", "failed fetching epoch accounts", "failed fetching protocol config
accounts"), while keeping the existing success path that deserializes with
ForesterEpochPda::try_deserialize_unchecked,
EpochPda::try_deserialize_unchecked, and
ProtocolConfigPda::try_deserialize_unchecked and pushes PDAs into
forester_epoch_pdas, epoch_pdas, and protocol_config_pdas respectively.
In @forester/src/main.rs:
- Around line 113-114: The spawn_api_server call currently ignores the Result
from warp::serve(routes).run(addr).await so bind failures are silent; update
spawn_api_server to await the server run result and handle Err by logging the
address and error (e.g., check if let Err(e) =
warp::serve(routes).run(addr).await { error!(...) }), and consider propagating
failures to the caller instead of swallowing them (e.g., make spawn_api_server
return a JoinHandle<Result<()>> or send an error over a channel) so the main
pipeline can react if the API server fails to bind.
In @forester/src/queue_helpers.rs:
- Around line 430-479: get_state_v2_output_queue_info and
print_state_v2_output_queue_info duplicate the loop over
queue.batch_metadata.batches; extract that shared parsing into a helper (e.g.,
parse_batches or build_batch_info) that takes a reference to
BatchedQueueAccount::batch_metadata (or the batch slice) and returns the derived
values needed by both functions (Vec<BatchInfo>,
total_unprocessed/pending_batches, zkp_batch_size,
output_items_in_current_zkp_batch, etc.); replace the existing loop in
get_state_v2_output_queue_info with a call to that helper and update
print_state_v2_output_queue_info to call the same helper so both functions reuse
the parsing logic and return/build a V2QueueInfo or use the helper outputs to
construct it.
- Around line 197-205: The log in queue_helpers.rs that currently uses
tracing::info! to emit per-fetch stats (referencing queue_pubkey, total_items,
total_pending, start_index, end_index, filtered_queue.len()) should be demoted
to tracing::debug! to avoid flooding production logs; replace
tracing::info!(...) with tracing::debug!(...) keeping the same formatted message
and arguments so the detailed stats remain available at debug level.
In @forester/src/tree_data_sync.rs:
- Around line 319-325: The function get_registered_program_pda duplicates the
implementation found in the registry SDK
(account_compression_cpi::sdk::get_registered_program_pda); remove the local
implementation from tree_data_sync.rs and import/use the single implementation
from the registry SDK (e.g., add a use/import for
account_compression_cpi::sdk::get_registered_program_pda or call it via the
registry SDK module) so there is a single source of truth for
get_registered_program_pda.
In @forester/static/dashboard.html:
- Around line 745-748: The showError function injects untrusted message into
innerHTML causing XSS; change it to set the container's text content instead of
HTML. In the showError function (targeting the element with id
'errorContainer'), replace the innerHTML assignment with setting textContent (or
creating a text node) so the literal "Error: " prefix plus the message are
rendered as plain text rather than parsed as HTML. Ensure any styling wrapper
remains via existing CSS class on the container rather than inserting HTML
markup.
- Around line 770-785: Replace the hardcoded 0.46 magic number with a single
named constant (e.g., SLOT_SECONDS or SECONDS_PER_SLOT) defined near the top of
this file and reuse it in formatSlotsToTime and the other two places referenced
(lines ~1020 and ~1196) so all slot-to-time calculations are driven by one
authoritative value; update formatSlotsToTime to compute totalSeconds using that
constant (Math.floor(slots * SLOT_SECONDS)) and ensure the constant is
exported/accessible to any other functions in this file that perform similar
conversions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In @forester/src/api_server.rs:
- Around line 106-133: The current line parsing uses rsplit_once(' ') which is
brittle; instead trim each line, skip comments/empty lines, get the last
whitespace-separated token (e.g., using split_whitespace().last()) as the
value_str and parse it with .parse().ok() (skip the line on parse failure) and
keep metric_part = the rest joined or obtained via splitting off the last token
so extract_label still receives a trimmed metric string; update the block around
text.lines(), replace rsplit_once usage, remove unwrap_or(0.0) in favor of
handling parse errors, and ensure extract_label, transactions_processed,
transaction_rate, last_run_timestamp, forester_balances, and queue_lengths
behave with the trimmed/robust metric_part.
- Around line 58-71: The error branches currently return
Ok(warp::reply::json(&error_response)) which yields HTTP 200; change both
branches (the ones matching Ok(Err(e)) and Err(e)) to return
Ok(warp::reply::with_status(warp::reply::json(&error_response),
StatusCode::INTERNAL_SERVER_ERROR)) (or another appropriate StatusCode) so the
response body and HTTP status both indicate failure, and add/import StatusCode
from warp::http if not already available.
- Around line 91-143: In get_metrics_json, don't ignore the Result from
encoder.encode(&metric_families, &mut buffer); instead handle the Result: if
encoding returns Err, log the error (e.g., via log::error or tracing::error)
with context like "failed to encode Prometheus metrics" and either propagate an
error response or set a clear failure indicator in the returned MetricsResponse
(so callers/HTTP handler can surface it); update the call site around
encoder.encode in get_metrics_json to match this error handling and ensure
MetricsResponse construction reflects the failure path.
In @forester/src/cli.rs:
- Around line 20-87: StartArgs now binds fields like rpc_url, push_gateway_url,
payer, derivation, etc. to generic env names (RPC_URL, PUSH_GATEWAY_URL, PAYER,
DERIVATION_PUBKEY), which breaks existing FORESTER_* env-based deployments;
update the README, Dockerfile, Kubernetes manifests, and CI/deployment scripts
to use the new env names OR add backward-compatible support in StartArgs
(cli.rs) to accept the old FORESTER_RPC_URL, FORESTER_PUSH_GATEWAY_URL,
FORESTER_PAYER, FORESTER_DERIVATION_PUBKEY (and other FORESTER_* variants) as
fallbacks for rpc_url, push_gateway_url, payer, derivation, prover_* and
photon_* fields so existing deployments continue working.
In @forester/src/epoch_manager.rs:
- Around line 2818-2848: Inside the Ok(mut fetched_trees) branch (the match
fetch_result block) where you compute group_authority (from
config.general_config.group_authority or fetch_protocol_group_authority) and
then filter fetched_trees by owner, add an explicit warn log when there were
trees before filtering (before_count > 0) but none remain after filtering
(fetched_trees.len() == 0); reference the variables fetched_trees, before_count
and group_authority in the message so it’s clear which authority produced the
empty result and aids debugging of misconfigured group authorities or mismatched
tree owners.
In @forester/src/forester_status.rs:
- Around line 323-369: fetch_registry_accounts_filtered currently ignores errors
from the three tokio::join! results (forester_result, epoch_result,
config_result) and silently proceeds; update the Err branches to log a warning
with context and the error (e.g., "fetch_registry_accounts_filtered: forester
RPC error: {}", error) for each of the three cases before continuing so
operators can see RPC/deserialization failures while still allowing the function
to return empty vectors if desired; reference the function name
fetch_registry_accounts_filtered and the result variables forester_result,
epoch_result, config_result when implementing the warnings.
- Around line 462-587: In parse_tree_status, the TreeType::Unknown arm silently
returns zeros; add a tracing::warn! before returning to surface mis-typed trees
(include tree.pubkey and tree.tree_type in the message) so operators see a
warning when TreeType::Unknown is encountered; update the TreeType::Unknown
branch in parse_tree_status to emit the warn and then return the same (0.0, 0,
0, None, None).
- Around line 265-287: The loop currently uses
status.queue.parse().unwrap_or_default(), which silently turns parse failures
into Pubkey::default() and corrupts schedules; replace that with explicit error
handling: parse the pubkey with status.queue.parse::<Pubkey>() and handle Err by
either returning an error from this function or by skipping this tree (e.g., log
the bad status.queue value and continue the for status in &mut tree_statuses
loop), then use the successfully parsed queue_pubkey in the existing call to
ForesterEpochPda::get_eligible_forester_index; ensure any logging includes the
offending status.queue and sufficient context (references: status.queue,
ForesterEpochPda::get_eligible_forester_index, active_epoch_forester_pdas,
authority_to_index, tree_statuses).
- Around line 88-93: Make get_forester_status_async public so async callers can
use it directly and avoid spawning nested runtimes; change the visibility of the
existing get_forester_status_async function to pub, leave
get_forester_status_blocking as a thin legacy wrapper that creates a tokio
Runtime and calls get_forester_status_async, and consider annotating the
blocking wrapper as deprecated in comments to guide users toward the async API.
Ensure any module exports or uses reference the newly public
get_forester_status_async symbol so async contexts call it directly while
synchronous callers continue to use get_forester_status_blocking.
In @forester/src/main.rs:
- Around line 113-115: spawn_api_server currently "fires and forgets", so
failures (e.g., bind errors) are lost while run_pipeline proceeds; update
spawn_api_server to return a JoinHandle<Result<(), anyhow::Error>> (or similar)
and capture that handle where it's called (the call site using rpc_url_for_api
and args.api_server_port) so you can await or spawn a monitor task that logs the
Result; alternatively, ensure the thread created inside spawn_api_server logs
any startup errors (including bind failures) to the process logger before
exiting and document in comments that API server failures are non-fatal if you
choose not to propagate the error.
In @forester/src/queue_helpers.rs:
- Around line 82-86: The code that computes output_items_in_current_zkp_batch is
comparing b.state to the string "Fill", which is fragile; change the predicate
to compare against the enum variant (e.g., BatchState::Fill) or use
matches!(b.state, BatchState::Fill) so it uses the actual BatchState type like
the input-queue code does; update the find closure on batch_infos (the one
producing output_items_in_current_zkp_batch and referencing
items_in_current_zkp_batch) to use that enum comparison.
- Around line 197-205: The log currently uses tracing::info! inside the queue
fetch path and emits per-fetch stats (queue_pubkey, total_items, total_pending,
start_index, end_index, filtered_queue.len()), which can flood logs; change the
call from tracing::info! to tracing::debug! (or tracing::trace! if you prefer
even quieter output) so these detailed stats are emitted only at debug-level,
and optionally wrap it with a tracing::debug_enabled() check if computing the
message is expensive.
- Around line 415-480: The batch-iteration logic is duplicated across
get_state_v2_output_queue_info, print_state_v2_output_queue_info, and
parse_state_v2_queue_info; extract that loop into a shared helper (e.g.,
parse_v2_output_batches) that accepts a reference to
BatchedQueueAccount::batch_metadata (or the BatchedQueueAccount) and returns the
computed Vec<BatchInfo>, total_unprocessed, zkp_batch_size, and
output_items_in_current_zkp_batch; inside the helper reuse the same calls used
now (DEFAULT_ZKP_BATCH_SIZE, batch.get_num_inserted_zkps(),
batch.get_current_zkp_batch_index(), batch.get_num_inserted_zkp_batch(),
batch.get_state()) to compute batch_infos and totals, then replace the
duplicated loops in get_state_v2_output_queue_info,
print_state_v2_output_queue_info, and parse_state_v2_queue_info to call this
helper and construct their respective V2QueueInfo or print output.
In @forester/static/dashboard.html:
- Around line 1059-1067: The template interpolation renders f.authority directly
into the HTML (inside the active_epoch_foresters map that builds each
.forester-item), which risks unescaped content; update the rendering to escape
f.authority before insertion (e.g., add/ reuse an escapeHtml utility and call it
when injecting f.authority) or switch to constructing the span via DOM and
setting its textContent for the authority value; ensure the change is applied
where active_epoch_foresters is mapped (the block using getForesterColor,
getBalanceClass, and formatBalance) so only the escaped authority string is
output.
- Around line 745-748: The showError function writes unsanitized message into
innerHTML causing XSS; update it to avoid innerHTML by selecting the
'errorContainer' element and assigning its textContent (or create and append a
text node) with a safe string like "Error: " + message, ensuring you check the
element exists before setting to prevent null refs; do not use innerHTML or
string interpolation for user-controlled error.message.
- Around line 770-785: The code uses the literal slot duration (0.46 / 460) in
multiple places which is fragile; extract a single named constant (e.g.,
SLOT_DURATION_SECONDS = 0.46 and SLOT_DURATION_MS = 460) and replace all literal
uses: inside formatSlotsToTime use Math.floor(slots * SLOT_DURATION_SECONDS) to
compute total seconds, replace any occurrences like slots_until_next_light_slot
* 0.46 with slots_until_next_light_slot * SLOT_DURATION_SECONDS, and replace
slotsRemaining * 460 with slotsRemaining * SLOT_DURATION_MS (preserve any
existing rounding/Math.max logic such as adding the 500ms buffer). Ensure the
new constants are defined near the top of the script so all functions
(formatSlotsToTime and the code handling slots_until_next_light_slot and
slotsRemaining) reference the same values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
forester/src/processor/v2/tx_sender.rs (2)
269-339: Unused counter_address_append_countis tracked but never logged.You're incrementing
_address_append_counton line 309, but it's not included in theinstr_typeformatting (lines 331-339) likeappend_countandnullify_count. Either include it in the log message for completeness or remove the tracking entirely.♻️ Suggested fix to include address append count in logging
let instr_type = if append_count > 0 && nullify_count > 0 { format!("Append+Nullify({}+{})", append_count, nullify_count) } else if append_count > 0 { "Append".to_string() } else if nullify_count > 0 { "Nullify".to_string() } else { - "AddressAppend".to_string() + format!("AddressAppend({})", _address_append_count) };Or alternatively, remove the unused counter:
- let mut _address_append_count = 0usize; ... - _address_append_count += 1;
501-502: Minor:_proof_msis captured but unused.The proof duration is already accumulated into
self.proof_timingsat lines 446-455, so capturing it again here serves no purpose. Consider removing this tracking or using it if there's a future plan for per-batch proof timing metrics.♻️ Suggested cleanup
let round_trip = std::mem::replace(&mut self.pending_batch_round_trip_ms, 0); - let _proof_ms = std::mem::replace(&mut self.pending_batch_proof_ms, 0); + self.pending_batch_proof_ms = 0; let earliest = self.pending_batch_earliest_submit.take();forester/src/processor/v1/helpers.rs (3)
226-252: Potential silent data loss if proof count mismatches.The zip at line 226 (and line 255 for state items) will silently truncate if
address_items.len() != address_proofs.len(). While you log a warning when counts mismatch (lines 129-134), the code proceeds to usezipwhich silently drops extra items from the longer iterator.If a mismatch represents a bug or data integrity issue, consider either:
- Returning an error instead of proceeding
- Using
zip_eqfrom itertools which panics on length mismatch- Explicitly asserting lengths match before zipping
🔧 Proposed fix: Fail on mismatch instead of silent truncation
+ if address_items.len() != address_proofs.len() { + return Err(anyhow::anyhow!( + "Address proof count mismatch: requested={}, received={}", + address_items.len(), + address_proofs.len() + )); + } + for (item, proof) in address_items.iter().zip(address_proofs.into_iter()) {Apply similarly before the state items loop.
373-375: Likely incorrect hostname check — "mainnet" is not a valid host.The condition
url.host_str() != Some("mainnet")will almost always be true because"mainnet"isn't a valid hostname (it would typically be something like"mainnet.helius-rpc.com"or similar). This means the function returns the hardcoded10_000for nearly all URLs, including actual mainnet endpoints.Did you intend to check if the URL contains "mainnet" in the hostname? Or check against a specific RPC provider's domain?
🐛 Possible fix: Check if hostname contains "mainnet"
- if url.host_str() != Some("mainnet") { + if !url.host_str().map_or(false, |h| h.contains("mainnet")) { return Ok(10_000); }Or if you want to specifically target Helius:
- if url.host_str() != Some("mainnet") { + let is_helius_mainnet = url.host_str() + .map_or(false, |h| h.contains("mainnet") && h.contains("helius")); + if !is_helius_mainnet { return Ok(10_000); }
402-408: Consider reusingreqwest::Clientinstead of creating one per call.
reqwest::Client::new()creates a new connection pool. TheClientis designed to be reused across requests — it handles connection pooling, keep-alives, and other optimizations internally. Creating a fresh client per call negates these benefits.Consider either:
- Accepting a
&reqwest::Clientparameter- Using a
lazy_staticoronce_cellsingleton- Caching the client at a higher level
♻️ Proposed refactor: Accept client as parameter
pub async fn request_priority_fee_estimate( + client: &reqwest::Client, url: &Url, account_keys: Vec<Pubkey>, ) -> crate::Result<u64> { // ... - let client = reqwest::Client::new(); let response = client .post(url.clone())Callers would then manage a single shared
Clientinstance.forester/src/processor/v1/send_transaction.rs (1)
287-300: Return Timeout (not Cancelled) when the global deadline is hit.The early check returns
Cancelledeven whenInstant::now() >= timeout_deadline, which makes post-analysis harder and can hide systemic latency issues. Split the branch and returnTransactionSendResult::Timeouton deadline.forester/src/epoch_manager.rs (3)
770-875: Critical: processing_epochs flag can get stuck true forever (reset guard is created too late).Any early
return Ok(())/return Err(...)before Line 869 will skip the guard and leaveprocessing_flag=true, permanently preventing that epoch from being processed again.Move the reset guard to immediately after the successful
compare_exchange, so all exits reset the flag.Proposed fix
if processing_flag .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_err() { debug!("Epoch {} is already being processed, skipping", epoch); return Ok(()); } - + // Always reset the processing flag on every exit path. + let _reset_guard = scopeguard::guard((), |_| { + processing_flag.store(false, Ordering::SeqCst); + }); let phases = get_epoch_phases(&self.protocol_config, epoch); // Attempt to recover registration info debug!("Recovering registration info for epoch {}", epoch); let mut registration_info = match self.recover_registration_info(epoch).await { Ok(info) => info, Err(e) => { // ... Err(ForesterError::Registration( RegistrationError::RegistrationPhaseEnded { .. }, )) => { // ... return Ok(()); } // ... } }; // ... - // Ensure we reset the processing flag when we're done - let _reset_guard = scopeguard::guard((), |_| { - processing_flag.store(false, Ordering::SeqCst); - }); - info!("Exiting process_epoch"); Ok(())
285-370: Critical: monitor_handle is never aborted; shutdown detaches it and leaks a live epoch monitor task.Dropping a Tokio
JoinHandledoes not cancel the task; it detaches it. Your scopeguard aborts 3 tasks but notmonitor_handle, so on shutdown (or ifrun()is dropped) the monitor continues running in the background.Consider:
- passing a shutdown receiver into
EpochManager::runand havingmonitor_epochsselect!on it, and- aborting
monitor_handlein the same shutdown/guard path as the other tasks.
1585-1593: Fix V2 eligibility seed—must use queue pubkey per protocol rules, not merkle_tree.The call at line 1587 passes
&tree_accounts.merkle_treetocheck_forester_eligibility, but the protocol requires the queue pubkey. This breaks the eligibility calculation:get_eligible_forester_indexhashes the seed with epoch and current slot to derive a deterministic forester index. Using merkle_tree instead of queue produces incorrect eligibility and mismatches the/statusendpoint schedule (which correctly uses queue). All occurrences inforester_status.rsuse&queue_pubkey; V1 processing uses&tree_accounts.queue. The method parameter is even namedqueue_pubkey, making this semantic mismatch obvious.Change
&tree_accounts.merkle_treeto&tree_accounts.queueand verify all eligibility calculations across the codebase use the queue pubkey consistently.
🤖 Fix all issues with AI agents
In @forester/src/api_server.rs:
- Around line 30-78: spawn_api_server currently spawns a detached thread and can
hang forever; change it to return a JoinHandle (or accept a shutdown receiver)
and hook warp::serve(...).run(addr) into a graceful shutdown future (e.g.
with_graceful_shutdown) so the server can be stopped; additionally wrap calls to
get_forester_status in tokio::time::timeout inside status_route (use
tokio::time::timeout(Duration::from_secs(...),
get_forester_status(&rpc_url)).await and handle the Elapsed error by returning a
timeout ErrorResponse) and ensure the thread runtime is shut down when the
shutdown signal or returned JoinHandle is used.
- Around line 80-143: The current get_metrics_json implementation manually
parses the Prometheus text output (using encoder.encode + rsplit_once and
extract_label), which is fragile; change it to iterate REGISTRY.gather() and
process each prometheus::proto::MetricFamily and its Metric messages directly to
extract names, label pairs, and values (handle Counter, Gauge, Untyped,
Histogram/Summary appropriately), building the MetricsResponse from those
structured fields, properly handling NaN/Inf and exemplar data and returning
encoder errors instead of ignoring them, and remove or repurpose extract_label
since labels should come from metric.get_label() rather than string slicing.
- Around line 34-36: The code currently binds the HTTP API server to 0.0.0.0
using SocketAddr::from(([0, 0, 0, 0], port)) and logs it with info!("Starting
HTTP API server on {}", addr); change the default to bind to localhost
(127.0.0.1) or require/inspect an explicit CLI/config flag before binding to
0.0.0.0; specifically update the creation of addr (replace [0,0,0,0] with
[127,0,0,1]) and/or add a boolean flag (e.g., allow_public_bind) that, when
false, forces localhost and when true allows the current behavior and logs a
clear warning before starting the server, ensuring sensitive endpoints like
/status and /metrics/json are not exposed by default.
- Around line 37-41: The current dashboard_route closure constructs a Response
via Response::builder().body(DASHBOARD_HTML) which yields a Result and not a
Warp Reply; replace the closure to return a proper warp reply—prefer using
warp::reply::html(DASHBOARD_HTML) (or
warp::reply::with_header(warp::reply::html(...), "content-type", "text/html;
charset=utf-8") if you need to set headers explicitly) instead of the
Response::builder path so dashboard_route produces a valid impl Reply.
- Around line 49-63: The /status route currently returns a 200 OK with an error
JSON on get_forester_status failures; change the error branch in status_route to
return a non-200 HTTP status (e.g. StatusCode::INTERNAL_SERVER_ERROR or another
appropriate code) by wrapping the JSON reply with warp::reply::with_status
instead of plain warp::reply::json, and ensure StatusCode is imported
(warp::http::StatusCode). Keep the Ok::<_, warp::Rejection>(...) return but
replace the error path to
Ok(warp::reply::with_status(warp::reply::json(&error_response),
StatusCode::INTERNAL_SERVER_ERROR)) so clients and monitors see the proper HTTP
failure code while preserving the ErrorResponse body.
In @forester/src/epoch_manager.rs:
- Around line 367-371: The file has formatting issues—run rustfmt (e.g., cargo
fmt) on epoch_manager.rs to fix whitespace/formatting around the block that ends
with Err(anyhow!("Epoch monitor channel closed - forester cannot function
without it")) and the other misformatted region around the code at the 768-780
range; reformat the file, commit the changes, and ensure CI passes.
In @forester/src/forester_status.rs:
- Around line 317-363: fetch_registry_accounts_filtered is swallowing RPC errors
by using tokio::join! and then ignoring Err cases (forester_result,
epoch_result, config_result), causing empty vectors on failure; change to
propagate RPC errors by using tokio::try_join! (or otherwise return Err when any
rpc.get_program_accounts_with_discriminator call fails) so the function returns
an error via its crate::Result instead of silently returning empty data—update
the handling around forester_result/epoch_result/config_result (or remove the if
let Ok branches) to use the propagated results and let the ? operator bubble
errors up to callers.
- Around line 201-213: The code currently applies protocol group-authority
filtering implicitly whenever fetch_protocol_group_authority() succeeds; make
this behavior explicit by adding a configurable toggle/parameter and guarding
the logic with it (e.g., add a bool flag like filter_by_group to the status
handler or read from config/Options); only call fetch_protocol_group_authority()
and run trees.retain(...) when that flag is true, and update the debug/warn logs
to explicitly state whether filtering was enabled or disabled; refer to the
existing symbols fetch_protocol_group_authority, trees.retain, and the
surrounding status handler in forester_status.rs to implement the conditional
gating and logging.
- Around line 259-292: The schedule generation is seeding eligibility with the
queue pubkey (status.queue) but V2 uses the tree merkle key; change the seed to
parse and use the tree merkle pubkey (e.g., parse status.merkle_tree or the
equivalent merkle tree field on TreeStatus) instead of status.queue when calling
ForesterEpochPda::get_eligible_forester_index and building
schedule/assigned_forester so V2 trees use the correct seed; update the local
variable name (queue_pubkey -> tree_pubkey) and any related uses (the call site
that computes forester_idx and the assigned_forester lookup) accordingly.
In @forester/src/metrics.rs:
- Around line 211-228: Call register_metrics() at the start of both
update_indexer_response_time and update_indexer_proof_count to guarantee metrics
are registered before any .with_label_values/.set calls; this is cheap if
register_metrics uses a std::sync::Once or similar, so simply invoking
register_metrics() as the first statement in those functions ensures safe,
idempotent registration and prevents silent no-ops or panics when metrics are
updated before explicit registration.
In @forester/src/processor/v1/helpers.rs:
- Around line 107-152: The code currently clones each chunk with batch.to_vec()
before calling rpc.indexer()?.get_multiple_new_address_proofs which may be
avoidable—if get_multiple_new_address_proofs can accept a slice change the call
site to pass batch (a &[ [u8;32] ]) instead of allocating via batch.to_vec();
otherwise leave the Vec but document/justify the allocation. Also make the
per-batch RPC call more resilient by wrapping the
get_multiple_new_address_proofs call in a retry loop (e.g., const MAX_RETRIES,
loop with attempt counter, on Err log a warn with batch_idx and attempt, await
tokio::time::sleep with exponential or linear backoff, break on Ok, and only
return Err after retries are exhausted) so transient network errors don’t abort
the whole addresses.chunks/ADDRESS_PROOF_BATCH_SIZE processing.
In @forester/src/processor/v2/processor.rs:
- Around line 29-31: Replace the hard-coded constant MAX_BATCHES_PER_TREE with a
configurable value read at startup (e.g., via an env var like
MAX_BATCHES_PER_TREE or via the existing ProcessorConfig/Config struct) so
operators can adjust without code changes; implement parsing with a sensible
default (20) and bounds-checking, wire the resulting usize into any places that
currently reference MAX_BATCHES_PER_TREE (e.g., the processor's batching logic
and tests), and log the effective configured value at startup so changes are
visible in logs.
In @forester/src/processor/v2/tx_sender.rs:
- Around line 410-421: The tokio::select! block uses the biased; modifier which
intentionally polls the 1-second sleep branch before proof_rx.recv() to force
periodic eligibility re-checks; add a brief inline comment above the select!
(referencing the tokio::select! with biased;, the Duration::from_secs(1) sleep
branch and proof_rx.recv() branch) explaining that biased ensures the timer is
checked first to avoid getting stuck waiting on proofs near epoch boundaries and
to trigger periodic re-evaluation of sender eligibility.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
forester/src/processor/v1/send_transaction.rs (1)
287-290: Timeout vs cancellation conflated: returnTimeoutwhen deadline is exceeded.Right now the pre-send guard returns
Cancelledeven when the deadline passed; you even left a “// Or Timeout” hint. This makes downstream logs/metrics ambiguous and leavesTransactionSendResult::Timeoutunderused.Proposed fix
- if cancel_signal_clone.load(Ordering::SeqCst) || Instant::now() >= timeout_deadline { - return TransactionSendResult::Cancelled; // Or Timeout - } + if cancel_signal_clone.load(Ordering::SeqCst) { + return TransactionSendResult::Cancelled; + } + if Instant::now() >= timeout_deadline { + return TransactionSendResult::Timeout; + }Also applies to: 349-355
forester/src/epoch_manager.rs (2)
770-875: Critical:processing_flagis not reset on early returns/errors; epochs can get stuck “processing”.You only create the scopeguard reset near the end (Line 869+). But there are multiple
return Ok(())/return Err(...)paths earlier (e.g., registration phase ended at Line 831, or any error bubble-up), leaving the flagtrueforever for that epoch.-->Proposed fix
if processing_flag .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_err() { // Another task is already processing this epoch debug!("Epoch {} is already being processed, skipping", epoch); return Ok(()); } -~ + // Always reset, regardless of how we exit this function. + let _reset_guard = scopeguard::guard((), |_| { + processing_flag.store(false, Ordering::SeqCst); + }); let phases = get_epoch_phases(&self.protocol_config, epoch); // Attempt to recover registration info debug!("Recovering registration info for epoch {}", epoch); let mut registration_info = match self.recover_registration_info(epoch).await { Ok(info) => info, Err(e) => { ... } }; ... - // Ensure we reset the processing flag when we're done - let _reset_guard = scopeguard::guard((), |_| { - processing_flag.store(false, Ordering::SeqCst); - }); - info!("Exiting process_epoch"); Ok(())
1585-1593: Usequeuepubkey for V2 eligibility checking to match schedule generation.At line 1589, the V2 code passes
&tree_accounts.merkle_treetocheck_forester_eligibility, but the function expectsqueue_pubkey(line 1642) which it uses as input toForesterEpochPda::get_eligible_forester_indexfor deterministic forester selection (lines 1665–1670).For StateV2 trees with separate output queues, this breaks scheduling semantics: the forester determined by hashing
merkle_treewon't match the schedule generated usingqueue. AddressV2 trees (with integrated queues viaPubkey::default()) may accidentally align, but StateV2 will mismatch.Use
&tree_accounts.queueconsistently for both V1 and V2, or branch by tree type if the pubkeys differ semantically.forester/src/processor/v2/tx_sender.rs (1)
391-441: Critical: early termination drops already-received proofs (buffer/pending_batch) and caches under a stale root.When eligibility ends, you call
save_proofs_to_cache(&mut proof_rx, ...)but you do not:
- flush
self.pending_batch(proofs already ordered and ready to send), nor- drain
self.bufferentries already inserted, nor- update
self.last_seen_rootin the outer task (sender task mutates its ownsender_last_root).Result: you can discard a large amount of completed proof work and also poison reuse because the cache is “warmed” with an old root.
At minimum: before returning, drain
pending_batch+ any buffer entries to cache, and keeplast_seen_rootupdated as you pop entries / append to pending batch.-->
Also applies to: 512-518, 533-579
🤖 Fix all issues with AI agents
In @forester/src/api_server.rs:
- Around line 49-63: The /status handler currently always returns 200 on error;
update the Err(e) branch in the status_route closure (where get_forester_status
is awaited and ErrorResponse is built) to return a JSON body with an appropriate
HTTP status using warp::reply::with_status and warp::http::StatusCode instead of
a plain JSON reply. Map errors from get_forester_status to 502 Bad Gateway when
they indicate upstream RPC/indexer failures (otherwise use 500 Internal Server
Error), and return Ok::<_,
warp::Rejection>(warp::reply::with_status(warp::reply::json(&error_response),
chosen_status)). Ensure you import or qualify StatusCode and keep the success
branch returning the existing 200 JSON reply.
- Around line 80-143: The current get_metrics_json parses Prometheus text output
with rsplit_once(' ') which is brittle; instead iterate REGISTRY.gather()
MetricFamily objects directly: for each MetricFamily inspect family.get_name()
(e.g., "forester_transactions_processed_total", "forester_transaction_rate",
"forester_last_run_timestamp", "forester_sol_balance", "queue_length"), then
iterate family.get_metric() items and read the numeric value from the
appropriate metric type (get_counter().get_value() or get_gauge().get_value() /
get_untyped if used), extract labels by scanning metric.get_label() for
label.name == "epoch" / "pubkey" / "tree_pubkey" to get the key, and populate
transactions_processed, transaction_rate, last_run_timestamp, forester_balances,
and queue_lengths accordingly; remove the brittle string parsing (and/or replace
extract_label with a small helper that reads label values from
metric.get_label()). Ensure you handle multiple metrics per family and default
missing values safely.
In @forester/src/epoch_manager.rs:
- Around line 312-324: The scopeguard currently aborts current_previous_handle,
new_tree_handle, and balance_check_handle but omits monitor_handle, leaving the
epoch monitor task running on shutdown; update the scopeguard tuples and
closures (the one at the shown diff and the similar guards around the earlier
285-289 and later 326-371 blocks) to include monitor_handle and call .abort() on
it alongside the other handles so the monitor task is properly aborted on
drop/early exit.
- Around line 2896-2924: The code currently treats
config.general_config.group_authority == None as "try protocol default and
filter" which is a behavioral change; make this explicit by adding a new boolean
config flag (e.g., use_protocol_group_authority) in the same config struct and
check that flag before calling fetch_protocol_group_authority, and log at
info/warn level when the protocol default is being used (reference
config.general_config.group_authority, use_protocol_group_authority, and
fetch_protocol_group_authority). Ensure the filtering branch that retains trees
by group_authority (reference fetched_trees.retain and variable group_authority)
emits a clear metric/telemetry counter for "filtered_out_trees" reporting how
many trees were removed (before_count - fetched_trees.len()) and include an info
log that it was applied due to the explicit flag.
In @forester/src/forester_status.rs:
- Around line 456-497: Replace the magic numbers and add safety docs in
parse_tree_status for TreeType::StateV1: use the existing
STATE_MERKLE_TREE_HEIGHT constant instead of let height = 26u64; add a brief
comment above the unsafe parse_hash_set_from_bytes::<QueueAccount>(&acc.data)
call explaining why the unsafe read is required for V1 legacy format and why
.ok() error handling is acceptable; extract the literal used with
.saturating_sub(3) on IndexedMerkleTree::next_index() into a named constant
(e.g., INDEXED_MERKLE_TREE_RESERVED_SLOTS) with a comment describing the
reserved sentinel elements and mirror the same constant/comment used in
rollover/operations.rs to keep behavior consistent.
In @forester/src/processor/v1/helpers.rs:
- Around line 97-173: The code currently discards the returned proof.address and
trusts the indexer to preserve ordering, which can mis-assign proofs when
ordering differs or items are dropped; update the handling around
get_multiple_new_address_proofs and the subsequent zip of address_items with
address_proofs to preserve and validate the response address field for each
returned proof: when receiving response.value.items, keep the returned address
alongside its proof, compare each returned address to the requested hash for
that slot (or use the returned address to re-map proofs to the original
requested addresses), and if any proof.address is missing or does not match the
requested address either reorder/match accordingly or return an error instead of
merely warning; apply the same validation/fail-fast behavior to the state-proof
path as well (the code that zips address_items with address_proofs and the
analogous state proof section).
In @forester/src/processor/v1/send_transaction.rs:
- Around line 189-201: The fetch call for queue_item_data currently logs an
error then maps any fetch failure to ForesterError::General; harmonize with the
new helper by (1) replacing the pre-mapping error! call with warn! (or removing
it) so we don't duplicate the helper's warning when it returns Ok(Vec::new()),
and (2) ensure only true failures produce ForesterError::General from the
fetch_queue_item_data call (keep the map_err but lower the log level to warn! or
omit logging here since fetch_queue_item_data already warns). Target the block
around pool.get_connection(), fetch_queue_item_data, queue_item_data and update
the error! -> warn! (or remove) and keep ForesterError::General mapping only for
actual errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @forester/src/queue_helpers.rs:
- Around line 47-83: The parsing logic in parse_batch_metadata is duplicated in
parse_state_v2_queue_info; refactor by having parse_state_v2_queue_info call
parse_batch_metadata (or extract the common loop into a new helper used by both)
so BatchInfo and ParsedBatchData are produced once; ensure you pass the same
&[light_batched_merkle_tree::batch::Batch] input (or convert the queue batches
to that shape), preserve zkp_batch_size and items_in_current_zkp_batch
semantics, and update usage sites to consume the returned ParsedBatchData
instead of repeating the loop.
- Around line 128-133: The code is fragile because it matches b.state as the
string "Fill"; instead compare the enum/state directly or use a stable
indicator: update the lookup that sets output_items_in_current_zkp_batch to
identify the Fill batch by inspecting the real batch state enum (e.g., call
output_queue.batch_metadata.batches.get(index).map(|b| b.get_state() ==
BatchState::Fill) when iterating batch_infos) or add a boolean on BatchInfo
(e.g., is_fill) and use that instead of string comparison; change references to
b.state in the logic to use the enum comparison or the new boolean flag and keep
mapping to items_in_current_zkp_batch as before.
In @forester/src/tree_data_sync.rs:
- Around line 318-341: In fetch_protocol_group_authority: run cargo fmt to fix
the multiline formatting around get_registered_program_pda, and add an explicit
Anchor discriminator check before calling RegisteredProgram::deserialize on
account.data[8..] (validate the first 8 bytes match
RegisteredProgram::discriminator or expected value) to avoid deserializing the
wrong account; keep the existing error handling and log the group_authority_pda
after successful validation/deserialization.
In @forester/static/dashboard.html:
- Around line 1192-1216: The refresh logic in scheduleNextRefresh can trigger
rapid polling when slots_until_next_light_slot is very small; add a minimum
interval guard and a request-in-flight guard to prevent overlapping fetches:
introduce a boolean flag (e.g., isRefreshing) used inside fetchStatus to return
early if a fetch is already running, and enforce a configurable minRefreshMs
(e.g., 1000–2000 ms) when computing refreshIn inside scheduleNextRefresh so the
timeout is never below that; ensure you clear/reset isRefreshing in fetchStatus
finally block and keep the existing capped refreshTimeout behavior.
- Around line 1063-1073: The template directly interpolates user-controlled
forester authority strings (f.authority) into innerHTML, risking XSS; add a
sanitizer helper (e.g., escapeHtml) and use it wherever user data is injected in
the template (replace ${f.authority} with ${escapeHtml(f.authority)}) while
leaving other helpers (getForesterColor, getBalanceClass, formatBalance, and the
active_epoch_foresters mapping) unchanged; ensure escapeHtml returns a safely
escaped string for insertion into the template and use it consistently for any
other interpolated fields.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forester/tests/priority_fee_test.rs (1)
196-208: Test expects panic but implementation may not panic.Line 203 uses
std::panic::catch_unwindexpectingget_capped_priority_feeto panic whenmax_fee_lamports < min_fee_lamports. However, looking at the implementation inhelpers.rs, line 476 clampsmax_fee_lamportsto be at leastmin_fee_lamports:let max_fee_lamports = cap_config.max_fee_lamports.max(cap_config.min_fee_lamports);This means the function won't panic—it will clamp and continue. The test assertion at line 204-207 will fail because
result.is_err()will be false.🐛 Fix the test expectation
let cap_config = CapConfig { rec_fee_microlamports_per_cu: 10_000, min_fee_lamports: 10_000, max_fee_lamports: 0, compute_unit_limit: 1_000_000, }; - println!("expecting panic"); - let result = std::panic::catch_unwind(|| get_capped_priority_fee(cap_config)); - assert!( - result.is_err(), - "Expected panic for max fee less than min fee" - ); + // Implementation clamps max_fee to min_fee when max < min, so no panic + let result = get_capped_priority_fee(cap_config); + // After clamping, max_fee_lamports becomes 10_000, and the fee should be clamped to min + assert_eq!(result, 10_000, "Should clamp to min when max < min");
🤖 Fix all issues with AI agents
In @forester/src/cli.rs:
- Around line 20-236: The CLI arg env names are missing the FORESTER_ prefix
causing env vars to be ignored; update the env attributes for all affected
fields in the CLI struct (e.g., rpc_url, ws_rpc_url, indexer_url, prover_url,
prover_append_url, prover_update_url, prover_address_append_url, prover_api_key,
payer, derivation, photon_api_key, photon_grpc_url, indexer_batch_size,
legacy_ixs_per_tx, processor_mode, queue_polling_mode, tree_ids,
lookup_table_address, and any other fields listed) to use the documented
FORESTER_ prefix (e.g., env = "FORESTER_RPC_URL", env = "FORESTER_PAYER", env =
"FORESTER_INDEXER_URL", etc.) so the application reads the environment variables
matching .env.example/README.
- Around line 264-274: The change made rpc_url in the StatusArgs struct to a
required String which breaks callers; revert rpc_url to Option<String> (i.e.,
the original optional type) in forester/src/cli.rs and keep the #[arg(..., env =
"RPC_URL", value_name = "RPC_URL")] attribute so the env var remains supported;
then update any call sites that previously assumed a String (references to
StatusArgs::rpc_url) to handle the Option (e.g., propagate the Option, provide a
clear error where required, or supply a default) so runtime behavior is
unchanged for existing scripts.
In @forester/src/main.rs:
- Around line 113-118: The API server handle returned from spawn_api_server
(ApiServerHandle) is created but not used, so the API server never receives the
graceful shutdown signal; modify the shutdown flow so spawn_shutdown_handler (or
the Ctrl+C handler that sends to service_sender) also invokes the API server
shutdown path — e.g., accept or capture the ApiServerHandle returned from
spawn_api_server and call its shutdown() method or send on its shutdown_tx when
the shutdown signal is triggered, ensuring the API server is signaled to stop
before/alongside the main service shutdown.
In @forester/src/metrics.rs:
- Around line 211-232: Both update_indexer_response_time and
update_indexer_proof_count call register_metrics() on every invocation which
adds unnecessary overhead; remove the register_metrics() calls from these
functions (i.e., delete the register_metrics() line in
update_indexer_response_time and the one in update_indexer_proof_count) and
ensure register_metrics() is invoked once during startup/initialization
(document this requirement near the module or in the init path) so metrics are
registered before any calls to update_indexer_response_time or
update_indexer_proof_count.
In @forester/src/processor/v1/helpers.rs:
- Around line 209-258: The state proofs path currently calls
rpc.indexer().get_multiple_compressed_account_proofs once (inside the
state_proofs block) without retries; add the same retry/backoff logic used for
address proofs to this call so transient network errors are retried before
returning Err. Wrap
rpc.indexer()?.get_multiple_compressed_account_proofs(states, None).await in the
existing retry mechanism (same max attempts, backoff, and logging), ensure
update_indexer_response_time and update_indexer_proof_count are still called on
success, emit per-attempt warnings on failure, and only return Err after retries
are exhausted to keep behavior consistent with the address proofs code path.
In @forester/src/processor/v2/processor.rs:
- Around line 29-31: The comment is incorrect: MAX_BATCHES_PER_TREE is a
hardcoded optimization constant, not stored in BatchContext or configurable via
CLI; update the doc comment above const MAX_BATCHES_PER_TREE to state it is a
compile-time limit (value 4) used to bound per-tree queue size and processing
concurrency (keeps queue_size = zkp_batch_size * MAX_BATCHES_PER_TREE and limits
memory/throughput), and include the rationale that the value was chosen to limit
resource usage and was tuned empirically. If you instead want runtime
configurability, add a max_batches_per_tree field to BatchContext, add a CLI
flag to populate it, replace uses of the const with that field (and remove the
const or provide a default), and propagate the value into all places that
currently reference MAX_BATCHES_PER_TREE (functions/methods that compute
queue_size, fetch batches, and warmup logic).
In @forester/src/tree_data_sync.rs:
- Around line 319-344: In fetch_protocol_group_authority, validate the account
discriminator before deserializing RegisteredProgram: after obtaining account
(variable account) call check_discriminator::<RegisteredProgram>(&account.data)?
and only then call RegisteredProgram::deserialize(&mut &account.data[8..]); this
mirrors other account handlers
(StateMerkleTreeAccount/AddressMerkleTreeAccount/BatchedMerkleTreeAccount) and
prevents deserializing the wrong account type.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
forester/src/epoch_manager.rs (5)
770-875: Critical:process_epochcan deadlock itself by never resettingprocessing_flagon early returns/errors.Any
return/?before the guard at Line 868 leaves the epoch’s flag stucktrue, so all future attempts will always “already being processed, skipping”. This is a release-blocker.Proposed fix (reset guard must be created immediately after acquiring the flag)
@@ async fn process_epoch(&self, epoch: u64) -> Result<()> { @@ if processing_flag .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_err() { @@ return Ok(()); } -~ + // Always reset, even on early returns/errors. + let _reset_guard = scopeguard::guard(processing_flag.clone(), |flag| { + flag.store(false, Ordering::SeqCst); + }); @@ - // Ensure we reset the processing flag when we're done - let _reset_guard = scopeguard::guard((), |_| { - processing_flag.store(false, Ordering::SeqCst); - }); - info!("Exiting process_epoch"); Ok(()) }
496-654: Make epoch-channel send failure handling consistent (fatal) to avoid noisy looping when the receiver is gone.You correctly
return Errfor the “current epoch send” failure (Line 548-554), but later send failures (e.g., Line 612-618 / 629-637) justbreak, which will keep the monitor alive and repeatedly attempt sends to a closed channel.Proposed fix (treat send failure as terminal everywhere)
@@ - if let Err(e) = tx.send(target_epoch).await { + if let Err(e) = tx.send(target_epoch).await { error!( "Failed to send epoch {} for processing: {:?}", target_epoch, e ); - break; + return Err(anyhow!("Epoch channel closed: {}", e)); } @@ - if let Err(e) = tx.send(target_epoch).await { + if let Err(e) = tx.send(target_epoch).await { error!( "Failed to send epoch {} for processing: {:?}", target_epoch, e ); - break; + return Err(anyhow!("Epoch channel closed: {}", e)); }
281-375: Good:tokio::select!monitors both epoch feed and monitor death, but the shutdown story is still partial.
- Nice improvement: if the monitor dies, you fail fast + optionally page (Line 347-368).
- Remaining gap: spawned
process_epochtasks are untracked; on shutdown / fatal error you abort background tasks, but not those per-epoch processors.Consider keeping a
JoinSetfor per-epoch tasks (or a cancellation token) so “monitor died” and “shutdown” actually stop active epoch processing promptly.
1585-1686: Minor: V2 eligibility now usesmerkle_treeas the seed, but the code/logging still calls itqueue_pubkey.Since
check_forester_eligibilityis now passed&tree_accounts.merkle_treefor V2 (Line 1589-1590), the parameter name and log text (“process tree {}”, printingqueue_pubkey) are misleading.Rename
queue_pubkey→seed_pubkey(and update log phrasing) to prevent future “wrong seed” regressions.
1970-2032:max_batches_per_treewiring looks correct, but guard against pathological values.
max_batches_per_treeflows into the V2 processor and affects allocation/work per tick. Since this is configuration-driven, consider clamping/validating at config-load time (e.g.,>= 1and a sane upper bound) so a bad env var can’t explode work or overflowzkp_batch_size * max_batches_per_tree.forester/src/processor/v2/common.rs (1)
77-125: LGTM:max_batches_per_treeis correctly added and propagated viaClone.Only follow-up: ensure configuration enforces
max_batches_per_tree >= 1(and a reasonable upper bound) so callers don’t accidentally create “no-op” processors or huge batch windows.forester/src/processor/v2/processor.rs (1)
495-522: Major:prewarm_proofsshould bail out whenmax_batchesclamps to 0.With
max_batches_per_tree = 0,max_batchesbecomes 0 and you still spawn workers + callfetch_queue_data(..., 0, ...). Add the same guard you already have inprewarm_from_indexer.Proposed fix
@@ pub async fn prewarm_proofs( &mut self, cache: Arc<SharedProofCache>, queue_size: u64, ) -> crate::Result<ProcessingResult> { @@ - let max_batches = - ((queue_size / self.zkp_batch_size) as usize).min(self.context.max_batches_per_tree); + let max_batches = + ((queue_size / self.zkp_batch_size) as usize).min(self.context.max_batches_per_tree); + if max_batches == 0 { + return Ok(ProcessingResult::default()); + }forester/src/main.rs (1)
90-123: Verifyapi_server_handle.shutdown()is non-blocking; otherwise move it off the async runtime.Shutdown is invoked from the tokio task created by
spawn_shutdown_handler(Line 28-53). Ifshutdown()joins the API thread, it can block a runtime worker thread during shutdown.If
shutdown()might block, wrap it intokio::task::spawn_blockingor makeshutdown()only signal (and let the thread join elsewhere).
🤖 Fix all issues with AI agents
In @forester/.env.example:
- Around line 23-24: Add a clear placeholder comment above the PAYER and
DERIVATION_PUBKEY entries to indicate they are example-only and must be replaced
with real keys (e.g., "# EXAMPLE ONLY - replace with your actual keys"), and
consider replacing the long sequential byte arrays for PAYER and
DERIVATION_PUBKEY with obviously fake placeholder values (or a short explanatory
token) so readers cannot mistake them for real credentials; update references to
the PAYER and DERIVATION_PUBKEY variables accordingly in the file.
In @forester/src/cli.rs:
- Around line 107-113: The help text for max_batches_per_tree advertises a 1-20
range but no validation exists; in ForesterConfig::new_for_start after parsing
args, validate args.max_batches_per_tree is between 1 and 20 and return a
ConfigError::InvalidArguments (mentioning field "max_batches_per_tree" and the
invalid value) if it's out of range; place the check immediately after args are
available so invalid inputs fail fast and surface a clear error.
In @forester/src/epoch_manager.rs:
- Around line 2897-2925: After filtering by group authority (the match that sets
group_authority and the fetched_trees.retain(|tree| tree.owner ==
group_authority) call), add a check for fetched_trees.is_empty() and emit a
warn! that includes the group_authority value and the RPC endpoint (obtain the
endpoint from the existing rpc client used in fetch_protocol_group_authority).
Reference the same symbols: group_authority, fetched_trees,
fetched_trees.retain, and rpc (used in fetch_protocol_group_authority) so
operators can see a clear warning when the filter yields zero trees.
In @forester/src/forester_status.rs:
- Around line 504-654: The unsafe calls to parse_hash_set_from_bytes in
parse_tree_status (for TreeType::StateV1 and TreeType::AddressV1) need a clear
safety boundary: before calling unsafe
parse_hash_set_from_bytes::<QueueAccount>(&acc.data) validate the account looks
like a QueueAccount (check the 8-byte discriminator at acc.data[0..8], confirm
owner/discriminator/length or attempt QueueAccount::deserialize(&mut
&acc.data[8..]) and bail if it fails) and only then call the unsafe parser (or
replace the unsafe call entirely with a safe parser that returns Result and use
that), then map the resulting hash-set to count cells with
sequence_number.is_none() as before.
- Around line 413-470: fetch_tree_statuses_batched currently builds
pubkeys/pubkey_map and calls rpc.get_multiple_accounts for all keys at once
which can exceed Solana's 100-account limit; change it to dedupe any duplicate
Pubkeys if possible and split pubkeys into chunks of at most 100, call
get_multiple_accounts per chunk, collect/concatenate the returned account
vectors preserving original pubkey_map ordering, then populate tree_accounts and
call parse_tree_status as before. Also update fetch_forester_balances to collect
forester pubkeys into a HashSet (to dedupe active and registration lists) before
building the batched requests, then chunk the unique pubkeys into <=100 slices
and merge RPC results. Ensure references to pubkey_map, tree_accounts,
fetch_tree_statuses_batched, and fetch_forester_balances are used so existing
parsing/assignment logic can consume the merged account results unchanged.
In @forester/src/main.rs:
- Around line 74-89: The spawned task that consumes work_report_receiver
currently calls debug!("Work Report: {:?}", report) for every message which can
create high log volume; modify the task (the tokio::spawn async move block that
reads from work_report_receiver) to reduce noise by either lowering the level to
trace! or add simple sampling/throttling (e.g., keep a counter or timestamp and
only log every Nth report or once per interval) while preserving the existing
debug content and using the same report variable for formatting.
In @forester/src/processor/v1/helpers.rs:
- Around line 24-26: The constants ADDRESS_PROOF_BATCH_SIZE,
ADDRESS_PROOF_MAX_RETRIES, and ADDRESS_PROOF_RETRY_BASE_DELAY_MS are used for
both address and state proof logic but named as if only for address proofs;
rename them to PROOF_BATCH_SIZE, PROOF_MAX_RETRIES, and
PROOF_RETRY_BASE_DELAY_MS and update all references (e.g., any usages in
functions handling address or state proof fetching around the state proof code
paths) to the new names so the intent is clear and no usages are left pointing
to the old identifiers.
In @forester/src/processor/v1/send_transaction.rs:
- Line 26: WORK_ITEM_BATCH_SIZE is hardcoded which prevents tuning; make it
configurable by adding a field (e.g., work_item_batch_size: usize) to the
existing SendBatchedTransactionsConfig or TransactionConfig, replace the const
WORK_ITEM_BATCH_SIZE usage with reading that config value in the code paths that
previously referenced WORK_ITEM_BATCH_SIZE, provide a sensible default (100)
when constructing the config, and update any initialization/constructor sites
and tests to pass or mock the new config field.
In @forester/src/processor/v2/processor.rs:
- Around line 114-117: The multiplication for queue_size in Processor::process
can overflow because max_batches_per_tree is config-driven; replace the plain
multiply with a saturating multiply using
zkp_batch_size.saturating_mul(self.context.max_batches_per_tree as u64) to clamp
the result and prevent overflow when computing queue_size in the process method.
In @forester/static/dashboard.html:
- Around line 275-299: Replace the clickable <span> filter pills that use the
.tree-type-pill class with semantic <button> elements (preserve classes and
existing onclick handlers) or, if you must keep spans, add role="button",
tabindex="0", keydown handlers for Enter/Space, and an appropriate ARIA state
like aria-pressed to mirror the .active class; also ensure focus styling remains
visible. For the schedule grid cells that convey forester assignment, add
accessible labels by copying/enhancing the existing title attribute into an
aria-label (or aria-describedby) that includes the forester index/description so
screen readers and keyboard users get the same information as color alone.
- Line 1174: The template renders tree.merkle_tree directly, creating an XSS
risk; update the cell to call the existing escapeHtml helper (same as used
elsewhere) when outputting tree.merkle_tree so the value is HTML-escaped before
insertion; locate the table cell that currently uses ${tree.merkle_tree} and
change it to use escapeHtml(tree.merkle_tree) (or the template's equivalent
escaping helper) to ensure consistent defense-in-depth.
- Around line 1073-1099: The template injects raw forester values (f.authority)
into the HTML which can lead to XSS; add a small HTML-escape helper (e.g.,
escapeHtml(str) that replaces & < > " ' with entities and returns '' for
null/undefined) and call it wherever f.authority is used (both inside the
active_epoch_foresters map and registration_epoch_foresters map) so the template
uses ${escapeHtml(f.authority)} instead of ${f.authority}; ensure you
import/define escapeHtml in the same scope as
getForesterColor/getBalanceClass/formatBalance and handle non-string values
safely.
Comment on lines
+23
to
+24
| export PAYER='[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64]' | ||
| export DERIVATION_PUBKEY='[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32]' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sensitive credentials in example file - consider clarifying these are placeholders.
The PAYER and DERIVATION_PUBKEY examples contain sequential byte arrays that might be mistaken for real credentials. Consider adding a comment like # EXAMPLE ONLY - replace with your actual keys to make it clear these are placeholders.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 23-23: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 24-24: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 24-24: [UnorderedKey] The DERIVATION_PUBKEY key should go before the PAYER key
(UnorderedKey)
🤖 Prompt for AI Agents
In @forester/.env.example around lines 23 - 24, Add a clear placeholder comment
above the PAYER and DERIVATION_PUBKEY entries to indicate they are example-only
and must be replaced with real keys (e.g., "# EXAMPLE ONLY - replace with your
actual keys"), and consider replacing the long sequential byte arrays for PAYER
and DERIVATION_PUBKEY with obviously fake placeholder values (or a short
explanatory token) so readers cannot mistake them for real credentials; update
references to the PAYER and DERIVATION_PUBKEY variables accordingly in the file.
Comment on lines
107
to
+113
| #[arg( | ||
| long, | ||
| env = "FORESTER_TX_CACHE_TTL_SECONDS", | ||
| env = "MAX_BATCHES_PER_TREE", | ||
| default_value = "4", | ||
| help = "Maximum batches to process per tree per iteration (1-20, default: 4)" | ||
| )] | ||
| pub max_batches_per_tree: usize, |
Contributor
There was a problem hiding this comment.
max_batches_per_tree range not enforced.
The help text states "1-20, default: 4" but there's no validation to enforce this range. Values outside this range could cause unexpected behavior.
🔧 Add range validation in config.rs
In ForesterConfig::new_for_start, add validation after parsing:
if args.max_batches_per_tree < 1 || args.max_batches_per_tree > 20 {
return Err(ConfigError::InvalidArguments {
field: "max_batches_per_tree",
invalid_values: vec![format!(
"must be between 1 and 20, got {}",
args.max_batches_per_tree
)],
}.into());
}🤖 Prompt for AI Agents
In @forester/src/cli.rs around lines 107 - 113, The help text for
max_batches_per_tree advertises a 1-20 range but no validation exists; in
ForesterConfig::new_for_start after parsing args, validate
args.max_batches_per_tree is between 1 and 20 and return a
ConfigError::InvalidArguments (mentioning field "max_batches_per_tree" and the
invalid value) if it's out of range; place the check immediately after args are
available so invalid inputs fail fast and surface a clear error.
Comment on lines
+2897
to
+2925
| let group_authority = match config.general_config.group_authority { | ||
| Some(ga) => Some(ga), | ||
| None => { | ||
| match fetch_protocol_group_authority(&*rpc).await { | ||
| Ok(ga) => { | ||
| info!("Using protocol default group authority: {}", ga); | ||
| Some(ga) | ||
| } | ||
| Err(e) => { | ||
| warn!( | ||
| "Failed to fetch protocol group authority, processing all trees: {:?}", | ||
| e | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| if let Some(group_authority) = group_authority { | ||
| let before_count = fetched_trees.len(); | ||
| fetched_trees.retain(|tree| tree.owner == group_authority); | ||
| info!( | ||
| "Filtered trees by group authority {}: {} -> {} trees", | ||
| group_authority, | ||
| before_count, | ||
| fetched_trees.len() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Group-authority filtering needs a “filtered to zero” signal (otherwise it looks like the forester is healthy but idle).
If retain(|tree| tree.owner == group_authority) removes everything, it’s likely configuration/cluster mismatch. Suggest logging warn! when result is empty and include the authority + RPC URL, so operators don’t silently run a no-op forester.
🤖 Prompt for AI Agents
In @forester/src/epoch_manager.rs around lines 2897 - 2925, After filtering by
group authority (the match that sets group_authority and the
fetched_trees.retain(|tree| tree.owner == group_authority) call), add a check
for fetched_trees.is_empty() and emit a warn! that includes the group_authority
value and the RPC endpoint (obtain the endpoint from the existing rpc client
used in fetch_protocol_group_authority). Reference the same symbols:
group_authority, fetched_trees, fetched_trees.retain, and rpc (used in
fetch_protocol_group_authority) so operators can see a clear warning when the
filter yields zero trees.
Comment on lines
+275
to
+299
| .tree-type-pill { | ||
| font-size: 11px; | ||
| padding: 2px 8px; | ||
| border-radius: 10px; | ||
| background: #f5f5f7; | ||
| color: #86868b; | ||
| cursor: pointer; | ||
| transition: all 0.2s; | ||
| border: 1px solid transparent; | ||
| } | ||
|
|
||
| .tree-type-pill:hover { | ||
| background: #e5e5e7; | ||
| } | ||
|
|
||
| .tree-type-pill.active { | ||
| background: #007aff; | ||
| color: white; | ||
| } | ||
|
|
||
| .tree-type-pill.clear-filter { | ||
| background: transparent; | ||
| color: #007aff; | ||
| border: 1px solid #007aff; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Accessibility improvement opportunity for filter pills.
The filter pills are <span> elements with onclick handlers. For keyboard accessibility, consider using <button> elements or adding role="button" and tabindex="0" with keydown handlers for Enter/Space.
Similarly, the schedule grid colors convey forester assignment information. Users who can't distinguish colors might benefit from tooltips showing the forester index (which you partially have with the title attribute on line 876).
🤖 Prompt for AI Agents
In @forester/static/dashboard.html around lines 275 - 299, Replace the clickable
<span> filter pills that use the .tree-type-pill class with semantic <button>
elements (preserve classes and existing onclick handlers) or, if you must keep
spans, add role="button", tabindex="0", keydown handlers for Enter/Space, and an
appropriate ARIA state like aria-pressed to mirror the .active class; also
ensure focus styling remains visible. For the schedule grid cells that convey
forester assignment, add accessible labels by copying/enhancing the existing
title attribute into an aria-label (or aria-describedby) that includes the
forester index/description so screen readers and keyboard users get the same
information as color alone.
Comment on lines
+1073
to
+1099
| ${activeCount > 0 ? ` | ||
| <div class="forester-list"> | ||
| ${data.active_epoch_foresters.map((f, idx) => ` | ||
| <div class="forester-item"> | ||
| <span class="forester-address"> | ||
| <span class="forester-color-dot" style="background: ${getForesterColor(idx)};"></span> | ||
| ${f.authority} | ||
| </span> | ||
| <span class="forester-balance ${getBalanceClass(f.balance_sol)}">${formatBalance(f.balance_sol)}</span> | ||
| </div> | ||
| `).join('')} | ||
| </div> | ||
| ` : '<div class="empty-state">No foresters</div>'} | ||
| </div> | ||
| <div class="forester-column"> | ||
| <h4>Registration Epoch (${regCount})</h4> | ||
| ${regCount > 0 ? ` | ||
| <div class="forester-list"> | ||
| ${data.registration_epoch_foresters.map(f => ` | ||
| <div class="forester-item"> | ||
| <span class="forester-address">${f.authority}</span> | ||
| <span class="forester-balance ${getBalanceClass(f.balance_sol)}">${formatBalance(f.balance_sol)}</span> | ||
| </div> | ||
| `).join('')} | ||
| </div> | ||
| ` : '<div class="empty-state">No foresters</div>'} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Potential XSS vulnerability when rendering forester data.
The f.authority values are injected directly into HTML via template literals without escaping. If a malicious value somehow makes it into the API response (e.g., an authority string containing <script> tags), it would execute in the browser context.
Consider adding a simple escape function:
function escapeHtml(str) {
if (!str) return '';
return str.replace(/[&<>"']/g, c => ({
'&': '&', '<': '<', '>': '>', '"': '"', "'": '''
}[c]));
}Then use it: ${escapeHtml(f.authority)}
🤖 Prompt for AI Agents
In @forester/static/dashboard.html around lines 1073 - 1099, The template
injects raw forester values (f.authority) into the HTML which can lead to XSS;
add a small HTML-escape helper (e.g., escapeHtml(str) that replaces & < > " '
with entities and returns '' for null/undefined) and call it wherever
f.authority is used (both inside the active_epoch_foresters map and
registration_epoch_foresters map) so the template uses
${escapeHtml(f.authority)} instead of ${f.authority}; ensure you import/define
escapeHtml in the same scope as getForesterColor/getBalanceClass/formatBalance
and handle non-string values safely.
| <span class="type-badge ${getTypeBadgeClass(tree.tree_type)}">${tree.tree_type}</span> | ||
| ${isRolledOver ? '<span class="rolled-over-badge">INACTIVE</span>' : ''} | ||
| </td> | ||
| <td class="mono">${tree.merkle_tree}</td> |
Contributor
There was a problem hiding this comment.
Same XSS concern applies to tree addresses.
The tree.merkle_tree value is rendered directly into HTML. Apply the same escapeHtml function here for defense in depth.
🤖 Prompt for AI Agents
In @forester/static/dashboard.html at line 1174, The template renders
tree.merkle_tree directly, creating an XSS risk; update the cell to call the
existing escapeHtml helper (same as used elsewhere) when outputting
tree.merkle_tree so the value is HTML-escaped before insertion; locate the table
cell that currently uses ${tree.merkle_tree} and change it to use
escapeHtml(tree.merkle_tree) (or the template's equivalent escaping helper) to
ensure consistent defense-in-depth.
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 (6)
forester/tests/e2e_test.rs (2)
608-647: Consider using.expect()with descriptive messages for consistency.The account fetches in this function use
unwrap_or_elsewith detailed panic messages (e.g., lines 600-606), but the merkle tree fetches use bare.unwrap(). When tests fail, having context-specific error messages significantly speeds up debugging.♻️ Suggested improvement
let merkle_tree = get_concurrent_merkle_tree::<StateMerkleTreeAccount, LightClient, Poseidon, 26>( rpc, *merkle_tree_pubkey, ) .await - .unwrap(); + .expect("Failed to fetch StateV1 concurrent merkle tree");let merkle_tree = get_indexed_merkle_tree::< AddressMerkleTreeAccount, LightClient, Poseidon, usize, 26, 16, >(rpc, *merkle_tree_pubkey) .await - .unwrap(); + .expect("Failed to fetch AddressV1 indexed merkle tree");
701-727: Same consistency improvement applies here.Similar to
get_initial_merkle_tree_state, using.expect()with descriptive messages would provide better debugging context when these tree fetches fail.♻️ Suggested improvement
TreeType::StateV1 => { let merkle_tree = get_concurrent_merkle_tree::<StateMerkleTreeAccount, LightClient, Poseidon, 26>( rpc, *merkle_tree_pubkey, ) .await - .unwrap(); + .expect("Failed to fetch StateV1 tree for root verification");TreeType::AddressV1 => { let merkle_tree = get_indexed_merkle_tree::< AddressMerkleTreeAccount, LightClient, Poseidon, usize, 26, 16, >(rpc, *merkle_tree_pubkey) .await - .unwrap(); + .expect("Failed to fetch AddressV1 tree for root verification");forester/src/rollover/operations.rs (4)
125-146: Inconsistent error handling between V1 and V2 paths.V2 paths (StateV2, AddressV2) return raw
anyhow::Errorviaok_or_else(|| anyhow::anyhow!(...)), while V1 paths now explicitly wrap inForesterError::Other. This inconsistency can lead to confusion when handling errors upstream—callers may receive different error variants for semantically similar failures.Consider unifying the approach: either all paths should explicitly construct
ForesterError::Other, or theFrom<anyhow::Error>impl should be relied upon consistently.Also applies to: 148-171
396-406: Unresolved TODO and error context loss.Two items here:
Line 401: The
// TODO: check if this is correctcomment suggests uncertainty about whetherold_cpi_context_pubkeyis the right value. This should either be verified and the TODO removed, or tracked as a follow-up issue.Lines 405-406: Same error chain loss via
"{}"formatting as discussed above.Would you like me to open an issue to track verifying the
cpi_contextfield usage?
71-74: Hardcoded tree height and threshold calculation.A few observations:
Magic number: Height
26is hardcoded here and again at line 110. If this value ever needs to change, you'd have to hunt for all occurrences. Consider extracting to a constant likeconst STATE_V1_TREE_HEIGHT: u32 = 26;.Threshold calculation: The expression
(1 << height) * rollover_threshold / 100could overflow ifrollover_thresholdis large, since the multiplication happens before division. In practice,rollover_thresholdis likely a percentage (0-100), so this is probably safe, but worth being aware of.
113-116: Undocumented magic number insaturating_sub(3).The call
merkle_tree.next_index().saturating_sub(3)at line 116 subtracts 3 without explanation. Based on the learnings, address trees are initialized with some reserved entries, but a brief inline comment would help future readers understand why exactly 3 is subtracted here.// Address trees have 3 reserved initial entries (indexed 0, 1, 2) let next_index = merkle_tree.next_index().saturating_sub(3);
🤖 Fix all issues with AI agents
In @forester/src/rollover/operations.rs:
- Around line 68-69: The map_err closure currently discards the error chain by
formatting with "{}"; instead preserve the original error when mapping to
ForesterError::Other by converting the incoming error into an anyhow/Error or
boxed error rather than formatting it—update the closure in the
.await.map_err(|e| …) call to use the original error (e.into() or
into::<anyhow::Error>()) so ForesterError::Other receives the full error chain
rather than a formatted string.
- Around line 339-340: The map_err in get_address_bundle_config is converting
the error to a formatted string (anyhow::anyhow!("{}", e)) which discards the
original error chain; change the closure to preserve the original error as the
source, e.g. convert the incoming error into an anyhow::Error (or into
ForesterError) without stringifying it — for example replace .map_err(|e|
ForesterError::Other(anyhow::anyhow!("{}", e))) with something that preserves
the error chain like .map_err(|e| ForesterError::Other(e.into())) or by using
anyhow::Error::from(e) so the original context is retained.
- Around line 108-109: The map_err is currently formatting the error with "{}"
which discards its diagnostic chain; update the closure that produces
ForesterError::Other to preserve the original error as the source (e.g., convert
the incoming error into an anyhow::Error and call .context("failed to fetch
indexed merkle tree") or otherwise attach context rather than formatting), so
the original error chain from the indexed merkle tree fetch is retained; locate
the map_err on the awaited fetch in operations.rs and replace the
formatting-based creation (map_err(|e|
ForesterError::Other(anyhow::anyhow!("{}", e)))) with a construction that wraps
the original error (From/into + .context or similar) to keep the source.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
forester-utils/src/account_zero_copy.rsis excluded by none and included by noneforester-utils/src/address_merkle_tree_config.rsis excluded by none and included by noneforester-utils/src/registry.rsis excluded by none and included by none
📒 Files selected for processing (2)
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
🧰 Additional context used
📓 Path-based instructions (3)
forester/**/*.rs
📄 CodeRabbit inference engine (forester/README.md)
forester/**/*.rs: When running tests locally, the test will automatically spawn a local validator and start a local prover service using predefined URLs (localhost:8899 for RPC, localhost:8784 for indexer)
Automatically skip minting tokens, executing transactions, and root verification for disabled test types
Files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: When testing account state, use borsh deserialization with a single assert_eq against an expected reference account
Use type-safe assertions with actual struct fields instead of magic byte offsets when testing account data
Test assertions should deserialize account data using borsh before validation
Extract runtime-specific values from deserialized accounts before building expected reference state
Files:
forester/tests/e2e_test.rs
forester/**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run forester end-to-end tests with 'TEST_MODE=local cargo test --package forester e2e_test -- --nocapture'
Files:
forester/tests/e2e_test.rs
🧠 Learnings (46)
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/rollover_{state,address}_tree.rs : Trees must support rollover when reaching capacity (2^height leaves) by creating a new tree and marking the old tree as rolled over, preserving old tree roots
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/rollover_{state,address}_tree.rs : Rollover can only be performed once the rollover threshold is met (default: 95% full)
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Calculate rollover_fee based on rollover_threshold, height, and merkle_tree_rent amount in `src/initialize_address_tree.rs`. Set access_metadata with owner, program_owner, and forester (if provided). Set rollover_metadata with index, rollover_fee, rollover_threshold, network_fee, and close_threshold.
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-12-06T00:49:21.983Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-12-06T00:49:21.983Z
Learning: Applies to program-libs/compressible/src/error.rs : Support ProgramError conversions for Anchor, Pinocchio, and Solana frameworks in error implementation
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Return error `MerkleTreeMetadataError::InvalidRolloverThreshold` when rollover threshold value is invalid (must be percentage)
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2026-01-10T19:26:21.240Z
Learnt from: CR
Repo: Lightprotocol/program-examples PR: 0
File: basic-operations/CLAUDE.md:0-0
Timestamp: 2026-01-10T19:26:21.240Z
Learning: Applies to basic-operations/**/{anchor,native}/**/src/**/*.rs : Validate that `address_tree_pubkey.to_bytes() == ADDRESS_TREE_V2` before processing account operations
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/*.rs : Error handling must use BatchedMerkleTreeError enum from errors.rs with u32 error codes in the 14301-14312 range
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Charge rollover fee when creating output compressed accounts (insertion into output queue), not on tree operations
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-12-07T03:17:42.217Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-12-07T03:17:42.217Z
Learning: Applies to program-libs/account-checks/**/error.rs : Implement automatic error conversions from AccountError to ProgramError for both solana-program and pinocchio SDKs
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-12-07T03:17:42.217Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-12-07T03:17:42.217Z
Learning: Applies to program-libs/account-checks/{**/error.rs,**/account_info/pinocchio.rs} : Map Pinocchio ProgramError with standard codes 1-11 and handle BorrowError conversions for safe data access
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Set tree_capacity = 2^height in output queue metadata
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:57:39.230Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/QUEUE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:39.230Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/queue.rs : Check `tree_capacity` (2^height) against new insertions to prevent overflow in `BatchedQueueAccount`
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Compute and store hashed pubkeys by hashing and truncating to 31 bytes for bn254 field compatibility in both queue and tree accounts
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Define access control parameters: program_owner (Option<Pubkey>), forester (Option<Pubkey>) for non-Light foresters, and owner (Pubkey passed as function parameter)
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Initialize root history as a cyclic buffer with capacity = root_history_capacity, with first entry = zero bytes for tree height
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:57:39.230Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/QUEUE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:39.230Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/queue.rs : Pre-hash merkle tree pubkeys to 31 bytes (248 bits) with 1 byte padding for bn254 field size compatibility in Poseidon hashing within ZK circuits
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Tree height must be fixed at 40 for address trees in `src/initialize_address_tree.rs`. Validate that height equals 40.
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Define tree configuration parameters in `InitStateTreeAccountsInstructionData` struct: height (u32, default 32), index (u64), and root_history_capacity (u32, default 200)
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : Verify tree capacity constraint before tree update: `tree.next_index + zkp_batch_size <= tree_capacity`; error with `TreeIsFull` (14310) if violated
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:57:24.369Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INSERT_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:57:24.369Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/queue.rs : Before inserting into the output queue, validate that the tree is not full by checking that next_index < tree_capacity
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Use `check_account_balance_is_rent_exempt` function from `light-account-checks` to verify rent exemption for both queue_account and merkle_tree_account during initialization
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Associate output queue with merkle_tree_account pubkey in queue metadata and calculate rollover_fee based on rollover_threshold, height, and total rent (merkle_tree_rent + additional_bytes_rent + queue_rent)
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : Verify queue and tree association by matching pubkeys; error with `MerkleTreeAndQueueNotAssociated` (14001) if they don't match
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Verify merkle_tree_account balance is rent-exempt in `src/initialize_address_tree.rs`. Use `check_account_balance_is_rent_exempt` from `light-account-checks` library. Return error `AccountError::AccountNotRentExempt` (error code: 12011) if account balance is insufficient for rent exemption at calculated size.
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/merkle_tree.rs : State tree nullifier insertion must use the BatchedMerkleTreeAccount::insert_nullifier_into_queue method
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Return error `AccountError::AccountNotRentExempt` (error code 12011) when account balance is insufficient for rent exemption at calculated size
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:57:14.561Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INSERT_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:57:14.561Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In `BatchedMerkleTreeAccount::insert_nullifier_into_queue`, verify tree type is `TreeType::StateV2` before allowing nullifier insertion; reject address trees
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Associate state tree with queue_account pubkey in tree metadata and set rollover_fee to 0 (charged on queue insertion, not tree operations)
Applied to files:
forester/src/rollover/operations.rsforester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:57:14.561Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INSERT_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:57:14.561Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In `BatchedMerkleTreeAccount::insert_nullifier_into_queue`, perform non-inclusion check to ensure compressed_account_hash does not exist in any bloom filter to prevent double-spending
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:58:10.392Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:10.392Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In src/merkle_tree.rs, implement BatchedMerkleTreeAccount::update_tree_from_input_queue method for batch nullify operations on StateV2 trees. The method must verify ZKP proving correctness of: old_root + queue nullifiers → new_root, update tree root, increment tree sequence_number, increment tree nullifier_next_index by zkp_batch_size, mark ZKP batch as inserted, transition batch state to Inserted when all ZKP batches complete, and zero out bloom filter when current batch is 50% inserted.
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Initialize a state Merkle tree with a `TreeType::StateV2` (4) tree type, storing tree roots, root history, and integrated input queue (bloom filters + hash chains for nullifiers) in the `BatchedMerkleTreeAccount` with discriminator b`BatchMta` [66, 97, 116, 99, 104, 77, 116, 97]
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : BatchedMerkleTreeAccount must be of type `TreeType::StateV2` when updating tree from output queue
Applied to files:
forester/src/rollover/operations.rs
📚 Learning: 2026-01-05T19:54:29.872Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T19:54:29.872Z
Learning: Applies to forester/**/*test*.rs : Run forester end-to-end tests with 'TEST_MODE=local cargo test --package forester e2e_test -- --nocapture'
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : After successful ZKP verification, increment tree `sequence_number` by 1 (tracks number of tree updates) and append `new_root` to `root_history` cyclic buffer
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : After successful ZKP verification, increment tree `next_index` by `zkp_batch_size` (represents leaf index for next insertion)
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:57:02.558Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INSERT_ADDRESS_QUEUE.md:0-0
Timestamp: 2025-11-24T17:57:02.558Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : Always increment `queue_next_index` by 1 after successfully inserting an address. This value serves as a sequence number for indexers to track address insertion order.
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:10.392Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:10.392Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : When all ZKP batches for a batch are inserted, set batch sequence_number to tree_sequence_number + root_history_capacity and store root_index to identify the root that must not exist when bloom filter is zeroed.
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Set Merkle tree next_index to 0 (starts empty) and sequence_number to 0 (increments with each tree update)
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:57:14.561Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INSERT_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:57:14.561Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In `BatchedMerkleTreeAccount::insert_nullifier_into_queue`, increment `nullifier_next_index` after each insertion to provide sequence number for indexers
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Initialize sequence_number to 0 in BatchedMerkleTreeAccount in `src/initialize_address_tree.rs`. Increments with each tree update.
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : When all ZKP batches of a batch are inserted, set batch `sequence_number = tree_sequence_number + root_history_capacity` (threshold when root at root_index is overwritten) and set `root_index` to identify root that must not exist when bloom filter is zeroed
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : Transition batch state to `Inserted` when all ZKP batches are complete and increment `pending_batch_index` to switch to other batch
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Initialize next_index to 1 (not 0) in BatchedMerkleTreeAccount in `src/initialize_address_tree.rs`. Index 0 contains pre-initialized sentinel element.
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Initialize address tree with integrated address queue in `src/initialize_address_tree.rs`. Creates one `BatchedMerkleTreeAccount` with account layout defined in `src/merkle_tree.rs`, metadata in `src/merkle_tree_metadata.rs`, tree type `TreeType::AddressV2` (5), initial root `ADDRESS_TREE_INIT_ROOT_40`, starts at next_index 1, and discriminator `BatchMta` (8 bytes).
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:57:53.312Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/TREE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:53.312Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/*.rs : Address trees must initialize with `ADDRESS_TREE_INIT_ROOT_40` (hardcoded for height 40) and start with next_index = 1 (pre-initialized with one element at index 0)
Applied to files:
forester/tests/e2e_test.rs
📚 Learning: 2025-11-24T17:58:10.392Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_INPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:10.392Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In src/merkle_tree.rs, implement BatchedMerkleTreeAccount::update_tree_from_address_queue method for batch address append operations on AddressV2 trees. The method must verify ZKP proving correctness of: old_root + queue addresses → new_root using indexed Merkle tree insertion, update tree root, increment tree sequence_number, increment tree next_index by zkp_batch_size, mark ZKP batch as inserted, transition batch state to Inserted when all ZKP batches complete, and zero out bloom filter when current batch is 50% inserted.
Applied to files:
forester/tests/e2e_test.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (python)
- GitHub Check: programs (compressed-token-and-e2e, ["cargo test -p light-compressed-token", "cargo-test-sbf -p c...
- GitHub Check: Test program-libs-slow
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: Test program-libs-fast
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-ctoke...
- GitHub Check: lint
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (3)
forester/tests/e2e_test.rs (1)
244-244: LGTM!The
group_authority: Nonefield correctly initializes the new filtering capability for tests, allowing them to run without group authority restrictions. This aligns well with the PR's objective of adding group authority filtering support.forester/src/rollover/operations.rs (2)
46-181: Overall structure ofget_tree_fullnessis sound.The function correctly handles all
TreeTypevariants, computes fullness metrics consistently, and returns appropriate defaults forTreeType::Unknown. The logic for capacity/threshold/fullness calculations is correct.
458-525: Rent exemption helpers look good.Both
get_rent_exemption_for_state_merkle_tree_and_queueandget_rent_exemption_for_address_merkle_tree_and_queueproperly useanyhow::Contextfor error wrapping, which is the preferred pattern—errors retain context while remaining ergonomic. The size calculations use the correct config fields.
…oints; add dashboard UI - Added `spawn_api_server` to serve HTTP endpoints for health checks, system status, and metrics. - Added static `dashboard.html` for a web-based status UI.
…ndling and logging
…rove dashboard script for better slot timing
- Improved batch parsing and queue processes with consolidated metadata parsing. - Upgraded metrics tracking by replacing `GaugeVec` with `HistogramVec` and enhancing proof counting. - Config updates: simplified environment variable names for clarity and consistency. - Enhanced dashboard with refresh guards and adaptive intervals for better responsiveness.
sergeytimoshin
force-pushed
the
sergey/forester-status-api
branch
from
41fc19c to
8486bd0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
forester/src/epoch_manager.rs (2)
285-368: Fix PagerDuty field ordering in epoch-monitor death alert (summary/severity swapped).
send_pagerduty_alert(routing_key, summary, severity, source)is called here with"critical"in the summary slot and the human-readable message in the severity slot, unlike the other call site (Line 938+). This will degrade alert quality (and may violate PagerDuty’s expected severity values).Proposed fix
- if let Some(pagerduty_key) = &self.config.external_services.pagerduty_routing_key { - let _ = send_pagerduty_alert( - pagerduty_key, - "critical", - &format!("Forester epoch monitor died unexpectedly on {}", self.config.payer_keypair.pubkey()), - "epoch_monitor_dead", - ).await; - } + if let Some(pagerduty_key) = &self.config.external_services.pagerduty_routing_key { + if let Err(e) = send_pagerduty_alert( + pagerduty_key, + &format!( + "Forester epoch monitor died unexpectedly on {}", + self.config.payer_keypair.pubkey() + ), + "critical", + "epoch_monitor_dead", + ) + .await + { + error!("Failed to send PagerDuty alert: {:?}", e); + } + }
770-875: Resetprocessing_flagon all exit paths (it currently gets stuck true on early returns/errors).
processing_flagis set totrue(Line 781-788), but the reset guard is only created near the end (Line 868-871). Any earlierreturn Ok(())(e.g., registration-phase-ended handling at Line 831) or propagatedErrwill leave the flagtrue, permanently skipping future processing for that epoch.Proposed fix
if processing_flag .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_err() { // Another task is already processing this epoch debug!("Epoch {} is already being processed, skipping", epoch); return Ok(()); } -~ + // Always clear the flag on any exit path (Ok/Err/early-return). + let _processing_flag_guard = scopeguard::guard(processing_flag.clone(), |flag| { + flag.store(false, Ordering::SeqCst); + }); let phases = get_epoch_phases(&self.protocol_config, epoch); @@ - // Ensure we reset the processing flag when we're done - let _reset_guard = scopeguard::guard((), |_| { - processing_flag.store(false, Ordering::SeqCst); - }); - info!("Exiting process_epoch"); Ok(())forester/src/queue_helpers.rs (2)
192-252: Use a single source of truth forzkp_batch_sizein StateV2 output queue printing.
This function readszkp_batch_sizefrommetadata.batch_metadata.zkp_batch_size(Line 200) but also parses per-batch sizes viaparse_batch_metadata(Line 202). If these can diverge, the “items_completed/items_pending” math becomes inconsistent. Prefer usingparsed.zkp_batch_size(or assert equality).
146-190: Use the canonical queue deserialization API and eliminate the double iteration.The offset calculation pattern is established throughout the codebase (including
programs/account-compression/src/state/queue.rs), but forester should call the canonicalqueue_from_bytes_copy()function from the library rather than duplicating the offset logic. This centralizes any future layout changes.Additionally, the code builds
all_itemsjust to iterate twice—once to count and once to filter. Computetotal_items,total_pending, andfiltered_queuein a single pass overqueue.iter().forester/src/forester_status.rs (1)
978-993: CLI path usestree.queueas eligibility seed for all trees, including V2.The new API path (
get_forester_status_with_options) correctly uses the merkle tree pubkey for V2 trees. However, this CLI function (print_current_forester_assignments) still uses&tree.queue(line 981) for the eligibility calculation regardless of tree type. This causes inconsistent forester assignments between API and CLI for V2 trees.Suggested fix
for tree in trees { + // V2 trees use merkle tree pubkey as seed; V1 trees use queue + let seed_pubkey = match tree.tree_type { + TreeType::StateV2 | TreeType::AddressV2 => &tree.merkle_tree, + _ => &tree.queue, + }; let eligible_forester_slot_index = match ForesterEpochPda::get_eligible_forester_index( current_light_slot_index, - &tree.queue, + seed_pubkey, total_epoch_weight, current_active_epoch, ) {
🤖 Fix all issues with AI agents
In `@forester/src/api_server.rs`:
- Around line 63-181: The warp server uses a non-existent chaining pattern; in
spawn_api_server replace the .bind(...).await.graceful(...).run().await sequence
with warp::serve(routes).bind_with_graceful_shutdown(addr, async { let _ =
shutdown_rx.await; info!("API server received shutdown signal"); }) which
returns (addr, server_fut); log the addr, await server_fut, then log shutdown
completion. Ensure you move/clone shutdown_rx into the async shutdown closure
and remove the old graceful/run calls.
In `@forester/src/cli.rs`:
- Around line 94-98: The env var name for the Arg on the legacy_ixs_per_tx field
is misspelled as LEGACY_XS_PER_TX; update the attribute on the legacy_ixs_per_tx
field (the #[arg(...)] for legacy_ixs_per_tx) to use the correct env key
LEGACY_IXS_PER_TX so the environment variable matches the field name and
operators can set it reliably.
In `@forester/src/config.rs`:
- Line 100: Add a brief doc comment above the GeneralConfig struct field
group_authority explaining its purpose and behavior: state that it is an
optional Pubkey used to filter which trees are processed (when set, only trees
owned by that authority are processed), and clarify any semantics (e.g., whether
absence means process all trees). Place the comment immediately above the
declaration of group_authority in GeneralConfig to match the style of other
documented fields.
In `@forester/src/main.rs`:
- Around line 80-88: The verbose manual Option initialization for
rpc_rate_limiter and send_tx_limiter should be replaced by mapping the optional
config values directly to RateLimiter instances: use
config.external_services.rpc_rate_limit.map(|rate| RateLimiter::new(rate)) and
similarly for config.external_services.send_tx_rate_limit to assign to
rpc_rate_limiter and send_tx_limiter respectively; ensure the target variables
remain of the same Option type and remove the current if let blocks that set
them to Some(...).
In `@forester/src/processor/v1/helpers.rs`:
- Line 297: The code uses proofs.unwrap_or_default() even though the loop's
control flow guarantees proofs is Some; replace proofs.unwrap_or_default() with
proofs.unwrap() or proofs.expect("proofs set by successful loop") to document
the invariant and avoid masking logic errors—update the call site where proofs
is consumed (the variable proofs in the surrounding function) to use
unwrap/expect with a short message.
In `@forester/src/queue_helpers.rs`:
- Around line 43-81: In parse_batch_metadata, stop overwriting zkp_batch_size
each loop; instead capture the initial zkp_batch_size (from
DEFAULT_ZKP_BATCH_SIZE or the first batch) and then compare each
batch.zkp_batch_size against that value, and if any differ either assert/return
an error or emit a clear log/warn (e.g., using assert_eq! or log::warn!) before
continuing; keep the stored zkp_batch_size consistent when constructing
ParsedBatchData and reference parse_batch_metadata, zkp_batch_size,
batch.zkp_batch_size, and ParsedBatchData in your change.
- Around line 13-121: The BatchInfo.batch_state is stored as a raw u64 causing
fragile enum mapping; change BatchInfo.batch_state to use the BatchState enum
(define BatchState with Serialize/Deserialize, #[repr(u64)] and
#[serde(rename_all = "snake_case")] as suggested) and update
parse_batch_metadata to assign batch.get_state() directly (or convert to the
enum type) instead of calling .into() to a u64; also update V2QueueInfo where it
references batches to expect BatchInfo with batch_state: BatchState so serde
JSON contains explicit state names rather than numeric codes.
In `@forester/src/rollover/operations.rs`:
- Around line 405-406: The current .map_err(|e|
ForesterError::Other(anyhow::anyhow!("{}", e))) call in the
get_state_bundle_config call discards the error chain; replace that mapping so
the original error context is preserved (either use .context(...) from anyhow to
attach contextual message while keeping causes, or convert the error into
ForesterError without formatting, e.g., into() or wrapping the original error
into ForesterError::Other), updating the map_err on the get_state_bundle_config
invocation in operations.rs to use .context(...) or .map_err(|e|
ForesterError::Other(e.into())) so backtraces/causes are retained.
♻️ Duplicate comments (24)
forester/src/rollover/operations.rs (3)
68-69: Error chain information is lost with"{}"formatting.This concern was raised in a previous review. The
Displayformatting discards the error chain, making production debugging significantly harder. Consider using"{:?}"or converting the error directly with.into()if the error type supports it.
108-109: Same error chain loss pattern for indexed merkle tree fetch.Already flagged in a previous review. Apply the same fix as suggested for line 69.
339-340: Error context loss inget_address_bundle_config.Already flagged in a previous review. The error chain from network errors or account failures will be reduced to a flat string.
forester/src/processor/v1/send_transaction.rs (1)
26-26: Hardcoded batch size limits tunability.This constant was flagged in prior reviews. For different network conditions, RPC latencies, or queue depths, a fixed value of 100 may not be optimal. Consider wiring this through
SendBatchedTransactionsConfigso operators can tune without code changes.forester/src/processor/v1/helpers.rs (3)
24-26: Constants named for address proofs but also govern state-proof retries.These constants are used for both address proofs (line 117) and state proofs (lines 219-229), yet the
ADDRESS_PROOF_prefix suggests they're address-specific. Consider renaming toPROOF_BATCH_SIZE,PROOF_MAX_RETRIES, andPROOF_RETRY_BASE_DELAY_MSfor clarity.
153-160: Proof-to-address ordering validation is still missing—this is a correctness risk.The code warns when
proofs_received != batch_sizebut continues execution. If the indexer returns proofs out-of-order or drops items,response.value.itemsgets appended without validation that each proof corresponds to its intended address. Theaddressfield from the Photon API response is discarded during conversion inphoton_indexer.rs, eliminating the ability to verify correct mapping.When these proofs are later zipped with
address_items(line 302), a mismatch or reordering would silently pair wrong proofs with wrong addresses.Consider either:
- Preserving and validating the returned
addressfield against the requested hash- Erroring on count mismatch instead of warning
- Documenting and enforcing the indexer's ordering guarantee in code
302-302: Consider adding a length assertion before zipping to fail fast on mismatch.Both zip operations (lines 302 and 331) will silently drop items if the proof vectors are shorter than the item vectors. Given that you already log warnings on count mismatches (lines 153-158, 253-258), you could make this fail-fast:
💡 Optional: Add length assertions
// Before line 302: if address_items.len() != address_proofs.len() { return Err(anyhow::anyhow!( "Address proof count mismatch: items={}, proofs={}", address_items.len(), address_proofs.len() )); } // Before line 331: if state_items.len() != state_proofs.len() { return Err(anyhow::anyhow!( "State proof count mismatch: items={}, proofs={}", state_items.len(), state_proofs.len() )); }Also applies to: 331-331
forester/src/metrics.rs (1)
214-235: Consider removing redundantregister_metrics()calls and adding debug log for parity.Both update functions call
register_metrics()on every invocation. While theOnceguard makes this safe, it adds a small overhead on each call. Additionally,update_indexer_response_timehas a debug log butupdate_indexer_proof_countdoes not.🔧 Suggested improvements
pub fn update_indexer_response_time(operation: &str, tree_type: &str, duration_secs: f64) { - // Ensure metrics are registered before updating (idempotent via Once) - register_metrics(); INDEXER_RESPONSE_TIME .with_label_values(&[operation, tree_type]) .observe(duration_secs); debug!( "Indexer {} for {} took {:.3}s", operation, tree_type, duration_secs ); } pub fn update_indexer_proof_count(tree_type: &str, requested: u64, received: u64) { - // Ensure metrics are registered before updating (idempotent via Once) - register_metrics(); INDEXER_PROOF_COUNT .with_label_values(&[tree_type, "requested"]) .inc_by(requested); INDEXER_PROOF_COUNT .with_label_values(&[tree_type, "received"]) .inc_by(received); + debug!( + "Indexer proof count for {}: requested={}, received={}", + tree_type, requested, received + ); }Ensure
register_metrics()is called once during startup initialization.forester/.env.example (2)
1-56: Documentation and coding guidelines conflict with implementation.The coding guidelines state: "Configure environment variables using the
FORESTER_prefix for all Forester options", but the CLI code binds to unprefixed environment variable names (RPC_URL,WS_RPC_URL, etc.). This.env.examplecorrectly matches the CLI implementation, but the guidelines and README documentation need to be updated for consistency.Please update either:
- The coding guidelines and README to reflect the actual unprefixed naming convention, or
- The CLI code to use the
FORESTER_prefix as documentedThis prevents user confusion when they follow documentation that doesn't match the actual behavior.
22-24: Add placeholder comments for credential examples.The
PAYERandDERIVATION_PUBKEYexamples contain sequential byte arrays that might be mistaken for real credentials.📝 Suggested improvement
# Auth +# EXAMPLE ONLY - replace with your actual keys export PAYER='[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64]' export DERIVATION_PUBKEY='[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32]'forester/src/processor/v2/processor.rs (1)
115-116: Use saturating multiplication to prevent overflow.While config-driven values are typically reasonable, this multiplication could theoretically overflow if
max_batches_per_treeis set very high. Use saturating arithmetic for defensive programming:- let queue_size = self.zkp_batch_size * self.context.max_batches_per_tree as u64; + let queue_size = self.zkp_batch_size.saturating_mul(self.context.max_batches_per_tree as u64);forester/src/main.rs (1)
90-95: API server startup errors are now logged but not propagated.Looking at the
spawn_api_serverimplementation inapi_server.rs(lines 62-180), the server useswarp::serve(...).bind(addr).awaitwhich will panic on bind failure rather than returning an error gracefully. If the port is already in use, the spawned thread will panic and terminate silently.The shutdown integration (lines 109, 120) is now properly wired—good fix from the previous review. However, bind failures still won't be communicated back to the main thread.
Consider either:
- Having
spawn_api_serverattempt a preliminary port check or return aResult- Adding a startup confirmation channel so main knows the server is actually running
- At minimum, document that API server failures are non-fatal
#!/bin/bash # Check how warp::serve().bind() behaves on port conflicts rg -n "warp::serve" forester/src/api_server.rs -A 10forester/src/tree_data_sync.rs (1)
319-344: Missing discriminator check before deserializing RegisteredProgram.Every other account deserialization in this file validates the discriminator first:
- Line 238:
check_discriminator::<StateMerkleTreeAccount>(&account.data)?- Line 248:
check_discriminator::<AddressMerkleTreeAccount>(&account.data)?- Lines 258, 277:
check_discriminator::<BatchedMerkleTreeAccount>(&account.data)But
fetch_protocol_group_authorityskips straight to deserializing at line 335. If the PDA somehow contains unexpected data, you'll get a confusing deserialization error instead of a clear discriminator mismatch.🛡️ Proposed fix
let account = rpc .get_account(registered_program_pda) .await? .ok_or_else(|| { anyhow::anyhow!( "RegisteredProgram PDA not found for light_registry at {}", registered_program_pda ) })?; + check_discriminator::<RegisteredProgram>(&account.data)?; + let registered_program = RegisteredProgram::deserialize(&mut &account.data[8..]) .map_err(|e| anyhow::anyhow!("Failed to deserialize RegisteredProgram: {}", e))?;forester/static/dashboard.html (2)
1073-1099: Forester authority values rendered without escaping—low risk but consider defense-in-depth.The
f.authorityvalues (lines 1079, 1093) are interpolated directly into HTML via template literals. Since these are Solana pubkeys (base58-encoded, alphanumeric only), they're inherently safe from XSS. However, for defense-in-depth, especially if the API response could ever be manipulated:♻️ Optional: Add escapeHtml utility
function escapeHtml(str) { if (!str) return ''; const div = document.createElement('div'); div.textContent = str; return div.innerHTML; } // Then use: ${escapeHtml(f.authority)}This is a low-priority nitpick since pubkeys can't contain
<or>characters.
275-299: Accessibility: Filter pills could use semantic buttons.The tree type filter pills are
<span>elements withonclickhandlers. For keyboard accessibility, consider using<button>elements instead, or at minimum addingtabindex="0"androle="button"with Enter/Space key handlers.Current users relying on keyboard navigation won't be able to interact with these filters.
forester/src/epoch_manager.rs (1)
2897-2925: Add a warn when group-authority filtering drops all trees (silent no-op).
This is the same concern already raised before: ifbefore_count > 0andretain(|tree| tree.owner == group_authority)yields0, the service will run “healthy but idle” and it’s hard to diagnose.forester/src/cli.rs (2)
18-87: Env var prefix change vs documentedFORESTER_convention—confirm compatibility/docs.
StartArgs now readsRPC_URL,PAYER, etc., notFORESTER_RPC_URL,FORESTER_PAYER, etc. If this is intentional, make sure the documented.envworkflow and deployments were migrated (or add fallback env aliases). Based on learnings, Forester env vars are expected to use theFORESTER_prefix.
107-113: Enforcemax_batches_per_treerange at the CLI boundary (1–20).
The help text claims1-20but there’s no validation here, so invalid values can leak into runtime. Consider using clap’svalue_parserrange (or validating in config build).forester/src/forester_status.rs (6)
1100-1106: Schedule function also uses queue pubkey for V2 tree eligibility.Same inconsistency as
print_current_forester_assignments. The function signature accepts separatetreeandqueueparams but doesn't receivetree_type, so it can't differentiate V2 trees. Consider addingtree_type: TreeTypeto the signature.
534-542: Unsafe queue parsing needs a safety comment or upfront validation.The
unsafe { parse_hash_set_from_bytes::<QueueAccount>(&acc.data) }block interprets raw bytes as aQueueAccount. If the account data doesn't match the expected layout (wrong discriminator, insufficient size), this can invoke undefined behavior before the.ok()has a chance to catch an error.Either add a brief safety comment explaining the invariants (e.g., "caller ensures account is a valid QueueAccount from the expected program"), or perform a quick discriminator check before the unsafe call:
Option 1: Safety comment
let queue_len = queue_account.and_then(|acc| { + // SAFETY: Account was fetched from the queue pubkey associated with this tree. + // parse_hash_set_from_bytes validates internal structure; .ok() handles parse failures. unsafe { parse_hash_set_from_bytes::<QueueAccount>(&acc.data) } .ok() .map(|hs| {Option 2: Upfront discriminator check
let queue_len = queue_account.and_then(|acc| { + // Validate discriminator before unsafe parse + if acc.data.len() < 8 || &acc.data[..8] != QueueAccount::DISCRIMINATOR { + return None; + } unsafe { parse_hash_set_from_bytes::<QueueAccount>(&acc.data) } .ok()
140-147: Integer division truncates hours to 0 for most practical slot counts.The formula
slots * 460 / 1000 / 3600evaluates left-to-right with integer math. For the result to be ≥1 hour, you needslots ≥ 7827(since7826 * 460 / 1000 / 3600 = 0). If typical phase lengths are smaller, clients always seehours_until_next_epoch: 0.Consider either:
- Computing in floating-point:
(slots as f64 * 0.46) / 3600.0- Returning minutes instead:
slots * 460 / 1000 / 60- Reordering to avoid early truncation:
slots * 460 / (1000 * 3600)Suggested fix (floating-point)
- let hours_until_next_epoch = - active_phase_length.saturating_sub(active_epoch_progress) * 460 / 1000 / 3600; + let slots_remaining = active_phase_length.saturating_sub(active_epoch_progress); + let hours_until_next_epoch = (slots_remaining as f64 * 0.46 / 3600.0) as u64; let slots_until_next_registration = protocol_config_pda .config .registration_phase_length .saturating_sub(active_epoch_progress); - let hours_until_next_registration = slots_until_next_registration * 460 / 1000 / 3600; + let hours_until_next_registration = (slots_until_next_registration as f64 * 0.46 / 3600.0) as u64;
335-341: Use bounds-checked access foractive_epoch_forestersarray.Line 339 directly indexes
active_epoch_foresters[*forester_idx]. Whileforester_idxoriginates fromauthority_to_index(built from the same collection), using.get()provides defensive safety against any data inconsistency.Suggested fix
// Set current assigned forester if let Some(current_idx) = current_light_slot { if let Some(Some(forester_idx)) = schedule.get(current_idx as usize) { - status.assigned_forester = - Some(active_epoch_foresters[*forester_idx].authority.clone()); + if let Some(forester) = active_epoch_foresters.get(*forester_idx) { + status.assigned_forester = Some(forester.authority.clone()); + } } }
162-169: Deduplicate forester pubkeys before batch-fetching balances.A forester registered in both the active and registration epochs appears twice in
all_forester_pubkeys, wasting RPC capacity (especially given the 100-account limit). Using aHashSetbefore converting toVecensures unique entries.Suggested fix
+ use std::collections::HashSet; + // Fetch all forester balances in one batch call using Rpc trait - let all_forester_pubkeys: Vec<Pubkey> = active_forester_authorities + let all_forester_pubkeys: Vec<Pubkey> = active_forester_authorities .iter() .chain(registration_forester_authorities.iter()) .cloned() + .collect::<HashSet<_>>() + .into_iter() .collect();
472-502: Balance fetch also needs chunking for large forester sets.Same 100-account RPC limit applies here. While forester counts are typically smaller than tree counts, the function should still chunk requests defensively.
Suggested approach
+ const RPC_BATCH_LIMIT: usize = 100; + if pubkeys.is_empty() { return balances; } - match rpc.get_multiple_accounts(pubkeys).await { - Ok(accounts) => { - for (i, account_opt) in accounts.iter().enumerate() { + for (chunk_start, chunk) in pubkeys.chunks(RPC_BATCH_LIMIT).enumerate().map(|(i, c)| (i * RPC_BATCH_LIMIT, c)) { + match rpc.get_multiple_accounts(chunk).await { + Ok(accounts) => { + for (i, account_opt) in accounts.iter().enumerate() { + let global_idx = chunk_start + i; + if let Some(pubkey) = pubkeys.get(global_idx) { + let balance = account_opt + .as_ref() + .map(|acc| acc.lamports as f64 / 1_000_000_000.0); + balances.insert(*pubkey, balance); + } + } + } + Err(e) => { + tracing::warn!("Failed to fetch forester balances for chunk: {:?}", e); + for pubkey in chunk { + balances.insert(*pubkey, None); + } + } + } + }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forester/src/epoch_manager.rs (1)
1587-1595: Document V2 eligibility design: clarify why merkle tree replaces queue for eligibility calculation.The V2 implementation intentionally passes
tree_accounts.merkle_treeinstead oftree_accounts.queuetocheck_forester_eligibility. While this design appears sound—bothStateV2andAddressV2route throughprocess_light_slot_v2—the parameter namequeue_pubkeyis now semantically misleading for V2 usage.For
AddressV2trees, this makes sense sinceassociated_queueisPubkey::default(). ForStateV2trees, however, this represents a behavioral shift from V1's queue-based eligibility to tree-based eligibility. This design choice lacks explanation.Recommended improvements:
- Add an inline comment above the
check_forester_eligibilitycall inprocess_light_slot_v2explaining why V2 uses the merkle tree pubkey instead of the queue for eligibility calculations- Consider renaming the parameter to
identifier_pubkeyorseed_pubkeyto better reflect its dual use (queue in V1, tree in V2)
🤖 Fix all issues with AI agents
In `@forester/src/epoch_manager.rs`:
- Around line 359-366: The call to send_pagerduty_alert is passing parameters in
the wrong order: send_pagerduty_alert(routing_key, summary, severity, source) —
update the call in epoch_monitor (where pagerduty_key /
self.config.external_services.pagerduty_routing_key is used) to pass the
human-readable summary (the format!("Forester epoch monitor died unexpectedly on
{}", self.config.payer_keypair.pubkey())) as the second argument, "critical" as
the third (severity), and "epoch_monitor_dead" as the fourth (source) so
severity and summary are not swapped.
♻️ Duplicate comments (1)
forester/src/epoch_manager.rs (1)
2900-2929: Group authority filtering logic is solid; consider the edge case warning from past review.The filtering implementation correctly:
- Prioritizes config override over protocol default
- Falls back gracefully if protocol fetch fails
- Logs before/after counts for visibility
However, the past review comment suggested adding a warning when
before_count > 0but the filtered result is empty — this helps diagnose misconfigured group authorities. The current logging shows counts but doesn't explicitly warn on the zero-result edge case:if let Some(group_authority) = group_authority { let before_count = fetched_trees.len(); fetched_trees.retain(|tree| tree.owner == group_authority); + if before_count > 0 && fetched_trees.is_empty() { + warn!( + "Group authority {} matched zero trees out of {}. Check if the authority is correct for this environment.", + group_authority, before_count + ); + } info!( "Filtered trees by group authority {}: {} -> {} trees",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
forester/src/epoch_manager.rs
🧰 Additional context used
📓 Path-based instructions (1)
forester/**/*.rs
📄 CodeRabbit inference engine (forester/README.md)
forester/**/*.rs: When running tests locally, the test will automatically spawn a local validator and start a local prover service using predefined URLs (localhost:8899 for RPC, localhost:8784 for indexer)
Automatically skip minting tokens, executing transactions, and root verification for disabled test types
Files:
forester/src/epoch_manager.rs
🧠 Learnings (40)
📚 Learning: 2025-11-24T18:02:15.670Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:02:15.670Z
Learning: When forester PDA is present, `check_forester()` must validate epoch registration and eligibility, track work, and require network fee; when absent, validate that authority matches the tree's designated forester (for private trees)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Define access control parameters: program_owner (Option<Pubkey>), forester (Option<Pubkey>) for non-Light foresters, and owner (Pubkey passed as function parameter)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/rollover_{state,address}_tree.rs : Trees must support rollover when reaching capacity (2^height leaves) by creating a new tree and marking the old tree as rolled over, preserving old tree roots
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-11T01:05:19.378Z
Learnt from: CR
Repo: Lightprotocol/distributor PR: 0
File: programs/merkle-distributor/CLAUDE.md:0-0
Timestamp: 2026-01-11T01:05:19.378Z
Learning: Applies to programs/merkle-distributor/**/error.rs : Define error codes as an `ErrorCode` enum in error.rs with proper variants
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-12T14:27:24.122Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/compressible/docs/SOLANA_RENT.md:0-0
Timestamp: 2026-01-12T14:27:24.122Z
Learning: Applies to program-libs/compressible/docs/**/*.rs : Use integer-based epoch accounting (432,000 slots per epoch, approximately 2.5 days) instead of floating-point years for rent calculations in Light Protocol
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/merkle_tree.rs : Tree updates from input queue must use update_tree_from_input_queue function for state trees and update_tree_from_address_queue for address trees with ZKP verification
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree_metadata.rs : Define `BatchedMerkleTreeMetadata` structure in `src/merkle_tree_metadata.rs`. Include tree_type, associated_queue, rollover_fee calculation, access_metadata (owner, program_owner, forester), rollover_metadata (index, rollover_fee, rollover_threshold, network_fee, close_threshold, additional_bytes), hashed_pubkey, next_index, and sequence_number fields.
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : BatchedMerkleTreeAccount must be of type `TreeType::StateV2` when updating tree from output queue
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Associate state tree with queue_account pubkey in tree metadata and set rollover_fee to 0 (charged on queue insertion, not tree operations)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : After successful ZKP verification, increment tree `sequence_number` by 1 (tracks number of tree updates) and append `new_root` to `root_history` cyclic buffer
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : Verify queue and tree association by matching pubkeys; error with `MerkleTreeAndQueueNotAssociated` (14001) if they don't match
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Associate output queue with merkle_tree_account pubkey in queue metadata and calculate rollover_fee based on rollover_threshold, height, and total rent (merkle_tree_rent + additional_bytes_rent + queue_rent)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/merkle_tree.rs : Tree updates from output queue must use BatchedMerkleTreeAccount::update_tree_from_output_queue_account method with ZKP verification
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-10T19:26:10.578Z
Learnt from: CR
Repo: Lightprotocol/program-examples PR: 0
File: airdrop-implementations/simple-claim/CLAUDE.md:0-0
Timestamp: 2026-01-10T19:26:10.578Z
Learning: Applies to airdrop-implementations/simple-claim/program/src/processor.rs : Validate that the derived PDA matches the provided airdrop_pda account in the Claim instruction
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-14T00:06:11.902Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2026-01-14T00:06:11.902Z
Learning: Applies to program-libs/compressible/src/config.rs : Provide PDA derivation methods (`derive_pda`, `derive_v1_config_pda`) for CompressibleConfig account location
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:58:50.237Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/account-checks/docs/ACCOUNT_CHECKS.md:0-0
Timestamp: 2025-11-24T17:58:50.237Z
Learning: Applies to program-libs/account-checks/docs/program-libs/account-checks/src/checks.rs : Implement `check_pda_seeds` function to derive PDA using `find_program_address` and verify it matches account key, returning `InvalidSeeds` (20010) error on mismatch
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-14T00:05:32.198Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: sdk-tests/sdk-light-token-test/README.md:0-0
Timestamp: 2026-01-14T00:05:32.198Z
Learning: Applies to sdk-tests/sdk-light-token-test/**/*.rs : Derive PDAs using `Pubkey::find_program_address()` before using them as authority/owner in `invoke_signed()` calls
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:59:03.485Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/account-checks/docs/ACCOUNT_INFO_TRAIT.md:0-0
Timestamp: 2025-11-24T17:59:03.485Z
Learning: Applies to program-libs/account-checks/docs/program-libs/account-checks/src/account_info/pinocchio.rs : Pinocchio AccountInfo implementations should use native on-chain implementations for PDA functions when available, with fallback to solana_pubkey off-chain
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T18:02:15.670Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:02:15.670Z
Learning: Wrapper instruction Account context must include: optional `registered_forester_pda` (mutable), `authority` signer, `cpi_authority` PDA with seeds, `registered_program_pda`, `account_compression_program`, `log_wrapper` for events, and `target_account` (mutable)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2026-01-10T19:26:10.578Z
Learnt from: CR
Repo: Lightprotocol/program-examples PR: 0
File: airdrop-implementations/simple-claim/CLAUDE.md:0-0
Timestamp: 2026-01-10T19:26:10.578Z
Learning: Applies to airdrop-implementations/simple-claim/program/src/processor.rs : Airdrop PDA must be derived using seeds: [claimant.to_bytes(), mint.to_bytes(), unlock_slot.to_le_bytes(), bump] via Pubkey::create_program_address
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T18:02:15.670Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:02:15.670Z
Learning: Call `check_forester()` to validate authority and track work performed, passing account metadata, authority key, target account key, forester PDA, and work units
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Charge rollover fee when creating output compressed accounts (insertion into output queue), not on tree operations
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Set tree_type to `TreeType::AddressV2` (5) and associated_queue to Pubkey::default() for address trees in `src/initialize_address_tree.rs`. Address trees have no separate output queue - the address queue is integrated into the tree account.
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Compute and store hashed pubkeys by hashing and truncating to 31 bytes for bn254 field compatibility in both queue and tree accounts
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:57:39.230Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/QUEUE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:39.230Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/queue.rs : Validate account ownership by Light account compression program using `check_owner` from `light-account-checks` when deserializing `BatchedQueueAccount` with `output_from_account_info`
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : `InitAddressTreeAccountsInstructionData` struct in `src/initialize_address_tree.rs` must include: height (u32, default 40), index (u64), root_history_capacity (u32, default 200), input_queue_batch_size (u64, default 15,000), input_queue_zkp_batch_size (u64, default 250), bloom_filter_capacity (u64, default batch_size * 8), bloom_filter_num_iters (u64, default 3 for test/10 for production), program_owner (Option<Pubkey>), forester (Option<Pubkey>), rollover_threshold (Option<u64>, default 95%), network_fee (Option<u64>, default 10,000 lamports), and close_threshold (Option<u64>).
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Calculate rollover_fee based on rollover_threshold, height, and merkle_tree_rent amount in `src/initialize_address_tree.rs`. Set access_metadata with owner, program_owner, and forester (if provided). Set rollover_metadata with index, rollover_fee, rollover_threshold, network_fee, and close_threshold.
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Use `check_account_balance_is_rent_exempt` function from `light-account-checks` to verify rent exemption for both queue_account and merkle_tree_account during initialization
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:57:53.312Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/TREE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:53.312Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/*.rs : Account validation for state trees must check: (1) account ownership by Light account compression program, (2) account discriminator is `BatchMta`, and (3) tree type is `STATE_MERKLE_TREE_TYPE_V2` (4)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T18:02:15.670Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:02:15.670Z
Learning: Applies to programs/registry/src/lib.rs : In wrapper instruction handler in `lib.rs`, load account metadata, determine work units, call `check_forester()`, then call the processing function
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:58:35.079Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/UPDATE_FROM_OUTPUT_QUEUE.md:0-0
Timestamp: 2025-11-24T17:58:35.079Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/merkle_tree.rs : In `src/merkle_tree.rs`, implement `BatchedMerkleTreeAccount::update_tree_from_output_queue_account` method that batch appends values from output queue to state Merkle tree with ZKP verification, verifying proof of old_root + queue_values → new_root
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:57:39.230Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/QUEUE_ACCOUNT.md:0-0
Timestamp: 2025-11-24T17:57:39.230Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/queue.rs : Check `tree_capacity` (2^height) against new insertions to prevent overflow in `BatchedQueueAccount`
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Define bloom filter configuration for input queue: bloom_filter_capacity (u64, default batch_size * 8) and bloom_filter_num_iters (u64, default 3 for test and 10 for production)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Define batch size parameters in `InitStateTreeAccountsInstructionData` struct: input_queue_batch_size (u64, default 15,000), output_queue_batch_size (u64, default 15,000), input_queue_zkp_batch_size (u64, default 500), and output_queue_zkp_batch_size (u64, default 500)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:20.711Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_ADDRESS_TREE.md:0-0
Timestamp: 2025-11-24T17:56:20.711Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_address_tree.rs : Initialize address tree with integrated address queue in `src/initialize_address_tree.rs`. Creates one `BatchedMerkleTreeAccount` with account layout defined in `src/merkle_tree.rs`, metadata in `src/merkle_tree_metadata.rs`, tree type `TreeType::AddressV2` (5), initial root `ADDRESS_TREE_INIT_ROOT_40`, starts at next_index 1, and discriminator `BatchMta` (8 bytes).
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/merkle_tree.rs : Address tree address insertion must use the BatchedMerkleTreeAccount::insert_address_into_queue method
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:00.229Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:00.229Z
Learning: Applies to program-libs/batched-merkle-tree/docs/**/initialize_address_tree.rs : Address tree initialization must create a single Solana account: BatchedMerkleTreeAccount with integrated input queue, initialized with a single leaf: H(0, HIGHEST_ADDRESS_PLUS_ONE)
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Set output queue account discriminator to b`queueacc` (8 bytes) and initialize queue metadata with QueueType::OutputStateV2
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Initialize an output queue account as `BatchedQueueAccount` with `QueueType::OutputStateV2`, temporary storage for compressed account hashes before tree insertion with immediate spending capability via proof-by-index, using discriminator b`queueacc` [113, 117, 101, 117, 101, 97, 99, 99]
Applied to files:
forester/src/epoch_manager.rs
📚 Learning: 2025-11-24T17:56:50.011Z
Learnt from: CR
Repo: Lightprotocol/light-protocol PR: 0
File: program-libs/batched-merkle-tree/docs/INITIALIZE_STATE_TREE.md:0-0
Timestamp: 2025-11-24T17:56:50.011Z
Learning: Applies to program-libs/batched-merkle-tree/docs/src/initialize_state_tree.rs : Initialize a state Merkle tree with a `TreeType::StateV2` (4) tree type, storing tree roots, root history, and integrated input queue (bloom filters + hash chains for nullifiers) in the `BatchedMerkleTreeAccount` with discriminator b`BatchMta` [66, 97, 116, 99, 104, 77, 116, 97]
Applied to files:
forester/src/epoch_manager.rs
🧬 Code graph analysis (1)
forester/src/epoch_manager.rs (5)
forester/src/tree_data_sync.rs (2)
fetch_protocol_group_authority(319-344)fetch_trees(27-44)forester/src/config.rs (5)
clone(457-474)default(104-119)default(169-176)default(180-185)default(189-202)forester/src/processor/v2/common.rs (1)
clone(103-124)forester/src/pagerduty.rs (1)
send_pagerduty_alert(20-54)forester/src/metrics.rs (1)
push_metrics(237-268)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test program-libs-slow
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo test -p light-compressed-token", "cargo-test-sbf -p c...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: stateless-js-v2
- GitHub Check: Forester e2e test
🔇 Additional comments (11)
forester/src/epoch_manager.rs (11)
75-76: LGTM!The import addition aligns with the new group authority filtering capability introduced in
run_service. Good to see it grouped with the relatedfetch_treesimport.
312-324: Monitor handle abort placement is functional but less robust than scopeguard.The past review suggested including
monitor_handlein the scopeguard for cleanup safety. The current approach works becauseabort()is called immediately after the loop exits, but if any panicking code were added between lines 370-373, the monitor task would leak.Since the current code path has no operations between loop exit and abort, this is acceptable. Just be mindful if future changes add logic in that gap.
Also applies to: 372-374
499-536: Well-designed exponential backoff with appropriate logging thresholds.This is a solid improvement to the epoch monitor's resilience:
- Exponential backoff capped at 60 seconds prevents hammering RPC during outages
- Recovery logging helps operators know when issues resolve
- Graduated logging (warn → error every 10th) balances observability against log noise
Nice teaching moment: the
2u64.pow(consecutive_failures.min(6))pattern elegantly caps the exponent while keeping the backoff calculation simple.
548-553: Good error propagation — ensures forester doesn't silently lose epoch processing.Returning an error when the channel closes (instead of just logging) ensures the monitor exits cleanly and triggers the shutdown handling in
run(). This prevents the forester from operating in a degraded state where it appears healthy but isn't processing epochs.
685-690: Good scoping of RPC connection to prevent resource holding across await points.Wrapping the RPC fetch in a block ensures the connection is returned to the pool before the subsequent async operations. This is a thoughtful improvement for connection pool efficiency.
770-779: Excellent inline documentation of a subtle DashMap + async interaction.The comment at lines 772-774 explains a non-obvious Rust pattern: without the immediate
.clone(), theRefMutguard fromDashMap::entrywould be held across async operations, potentially blocking other epochs on the same shard. This kind of documentation prevents future contributors from "optimizing" away the clone and introducing a deadlock.
1502-1504: Good error handling for metric push — warn level is appropriate.Metrics are valuable for observability but not critical to forester operation. Using
warn!ensures operators see failures without triggering alert fatigue. This addresses the PR objective for handling metric push failures.
1216-1222: Reasonable use of default owner for compression placeholder tree.The compression tree is a synthetic entry used for scheduling purposes (TreeType::Unknown). Using
Pubkey::default()for owner is appropriate here since this isn't a real tree with on-chain ownership. Real trees get their owner from the fetch operation.
3173-3174: Tests properly updated with new struct fields.The test configurations correctly initialize the new
group_authorityfield toNoneandownerfields toDefault::default(). This ensures tests compile and run without exercising the new filtering logic, which is appropriate for these unit tests focused on other functionality.
1291-1301: Clean task coordination pattern.Using
join_allto wait on tree processing tasks and logging the count provides good observability. The match pattern handles all three cases (success, error, panic) appropriately.
2034-2035: No action needed. Themax_batches_per_treefield is already properly defined in theBatchContextstruct (line 99 of common.rs) and correctly handled in the Clone implementation (line 122). The field assignment at line 2034 of epoch_manager.rs is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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.
dashboard.htmlfor a web-based status UISummary by CodeRabbit
New Features
Performance
Metrics
Chores
✏️ Tip: You can customize this high-level summary in your review settings.