Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/chunk_ingress_service/chunk_data_writer.rs`:
- Line 146: The calls that currently discard the Result from write_batch (the
lines using let _ = self.write_batch(...)) should surface failures instead of
swallowing them; update those call sites to either propagate the Result
(returning or mapping Err(QueueError::WriteFailed) up the call chain from the
caller) or at minimum inspect the Result and log errors with error! including
context (e.g., batch_size and any identifying metadata) so failures are
observable; locate the failing call sites by the pattern let _ =
self.write_batch(...) and modify them to handle the Result rather than ignoring
it.

In `@crates/actors/src/chunk_migration_service.rs`:
- Around line 502-507: The manual span guard created by service_span and its
enter() (_guard) is being held across an await in the tokio::select! loop
(symbols: service_span, _guard, tracing::info_span!("chunk_migration_service")),
which violates tracing async semantics; remove the manual span and guard and
instead annotate the function with #[tracing::instrument(name =
"chunk_migration_service", level = "trace", skip_all, err)] (or adjust the
existing #[instrument] on the enclosing function) so the span is applied per
await-aware invocation and do not call service_span.enter() around the loop or
across await points.

In `@crates/database/src/data_ledger.rs`:
- Around line 81-82: The tracing span for get_expired_slot_indexes currently
uses skip_all and declares fields(epoch_height) which creates an empty field;
update the attribute on pub fn get_expired_slot_indexes(&self, epoch_height:
u64) to explicitly record the argument value by changing the span fields to
fields(epoch_height = %epoch_height) so the epoch_height is captured in the
trace.

In `@crates/p2p/src/chain_sync.rs`:
- Around line 347-360: The running-flag reset is only done on normal completion
and will remain true if the spawned task panics; create a small RAII guard type
(e.g., WhitelistRunningGuard) whose Drop impl sets
stake_and_pledge_whitelist_running_flag.store(false, Ordering::Relaxed) and
instantiate it at the start of the async block that calls
handler.pull_and_process_stake_and_pledge_whitelist().await so the flag is
cleared even if the task panics; you can then remove or keep the explicit
store(false) call at the end, but ensure the guard variable lives for the whole
task scope so Drop runs on unwind.

---

Outside diff comments:
In `@crates/actors/src/data_sync_service.rs`:
- Around line 621-632: The code creates a tracing span via service_span =
tracing::info_span!("data_sync_service") and holds a guard across the
tokio::select! loop (entering the span before await points), which should be
removed; delete the manual span and the _guard = service_span.enter() usage
around the select, and instead annotate the async function (data_sync_service)
with a function-level tracing attribute that sets the span name (e.g.,
#[tracing::instrument(name = "data_sync_service", level = "trace", skip_all)]);
leave the rest of the loop (interval, peer_events_rx, tokio::select!) untouched
so tracing uses the function-level instrumented span.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/data_sync_service.rs`:
- Line 619: The tracing span on the data_sync_service start function is missing
error capture; update the tracing::instrument attribute on the start function
(the one annotated with name = "data_sync_service_start") to include err so
errors returned from start (eyre::Result<()>) are recorded in the span—match the
pattern used in storage_module_service_start and chunk_migration_service_start
by adding err to the attribute.

---

Outside diff comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 1140-1148: Remove the spurious blank line between the
#[tracing::instrument(..., err)] attribute and the function declaration so the
attribute attaches directly to pub fn poa_is_valid; i.e., edit the block
containing the tracing::instrument attribute (which references fields like
miner_address, poa.partition_chunk_offset, poa.partition_hash,
config.entropy_packing_iterations, config.chunk_size) to eliminate the empty
line before the pub fn poa_is_valid declaration and then run cargo fmt --all to
ensure formatting is stable.

In `@crates/actors/src/data_sync_service.rs`:
- Around line 213-221: The else branch inside the optimize_peer_concurrency
function contains only a dead, commented-out debug! block (referencing
peer_addr, current_max, utilization_ratio, health_score); remove that commented
code and the empty else branch entirely (or collapse the if/else into a single
if) so the function contains no noisy commented-out debug lines.

In `@crates/actors/src/mempool_service.rs`:
- Around line 3174-3178: The span currently hides the channel_name because
#[tracing::instrument(..., skip_all)] suppresses all args; change the attribute
on handle_broadcast_recv to include the channel as a span field and only skip
non-serializable args: replace skip_all with skip(result) (or skip the specific
non-serializable param) and add fields(channel = %channel_name) so the span
carries a channel field while still skipping the receive result in the span.

In `@crates/database/src/data_ledger.rs`:
- Around line 131-140: Add a tracing span to the public mutation entry-point
expire_old_slots so callers are attributed in traces; create a span (or add
#[tracing::instrument] on pub fn expire_old_slots) that records epoch_height
(and optionally the number of expired slots) and enter it before calling
get_expired_slot_indexes and mutating self.slots, so the trace tree has no
unattributed gap when this method is invoked directly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/chunk_ingress_service.rs`:
- Around line 188-193: Validate and bound the configured concurrent-task value
before constructing the semaphore and setting max_concurrent_tasks: ensure the
configured max_concurrent_chunk_ingress_tasks is at least 1 and fits into u32
(or cap it to u32::MAX or return an error), compute a single bounded value
(e.g., bounded_permits) and use that for both Semaphore::new(bounded_permits)
and u32::try_from(bounded_permits) so you don't create a 0-permit semaphore or
call expect on an out-of-range conversion. Update the same pattern in the second
occurrence (the block around the other message_handler_semaphore /
max_concurrent_tasks assignment).

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/chunk_ingress_service.rs`:
- Line 210: Replace the inline tokio::runtime::Handle::current() call passed to
.start(...) with the same explicit handle-cloning pattern used in
mempool_service: clone the existing runtime_handle into a local name (e.g.,
handle_for_inner) before spawning the task and pass that cloned handle to the
chunk ingress service's start(...) call so the code uses runtime_handle.clone()
-> handle_for_inner and .start(handle_for_inner) instead of
.start(tokio::runtime::Handle::current()).

In `@crates/actors/src/mempool_service.rs`:
- Around line 3148-3181: The shutdown drain currently uses one 10s timeout for
both acquiring all permits and draining the queue (process_remaining), which
lets slow in-flight handlers block the drain; split the phases so acquire_many
on message_handler_semaphore / self.inner.max_concurrent_tasks is awaited under
its own timeout (or allowed to complete without the 10s limit) and then run the
loop that calls self.msg_rx.try_recv() +
wait_with_progress(self.inner.handle_message(...)) under a separate
tokio::time::timeout(Duration::from_secs(10)); update logging to distinguish
failures when acquiring permits vs when draining the queue and ensure you still
call .instrument(span) and handle Err results from each phase (referencing
process_remaining, acquire_many, message_handler_semaphore,
max_concurrent_tasks, msg_rx, try_recv, wait_with_progress, and
tokio::time::timeout).

---

Outside diff comments:
In `@crates/actors/src/mempool_service.rs`:
- Line 3041: The spawned task in mempool_service uses
.instrument(tracing::Span::current()), which propagates the caller's span and
can leave the service without a stable root span; replace this with creating and
using a named span like tracing::info_span!("mempool_service") and attach it to
the spawned future (e.g., create let span =
tracing::info_span!("mempool_service");
tokio::spawn(my_future.instrument(span))); update the .instrument(...) call in
the mempool_service spawn site to use that named span so traces are consistently
rooted and filterable (mirror chunk_ingress_service's approach).

---

Duplicate comments:
In `@crates/actors/src/chunk_ingress_service.rs`:
- Around line 158-201: The clamp of
mempool_config.max_concurrent_chunk_ingress_tasks to range 1..=u32::MAX and the
subsequent use of u32::try_from when initializing max_concurrent_tasks resolves
the prior issues; ensure the clamp logic (raw_max_concurrent.clamp(1,
MAX_PERMITS)) is kept, the warning log for adjusted values remains, and the
u32::try_from call on max_concurrent_chunk_ingress_tasks in
ChunkIngressServiceInner initialization is preserved so it cannot panic.
- Around line 307-331: The current tokio::time::timeout wraps the entire
process_remaining closure so acquire_many(self.inner.max_concurrent_tasks) and
the drain loop share the same 10s budget; split this into two phases: first
await acquire_many with its own short timeout (so we don't block waiting for
in-flight handlers indefinitely), then run the try_recv drain loop inside a
separate timeout (or with its own bounded wait) to ensure queued messages get
time to be processed; update the logic around the process_remaining closure,
acquire_many, msg_rx.try_recv, wait_with_progress and handle_message to use two
distinct timeouts and preserve the existing logs/behavior on Ok/Err.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/mempool_service.rs`:
- Line 3211: The traced span on handle_broadcast_recv at
#[tracing::instrument(level = "trace", skip_all)] is low-value noise; either
remove the attribute entirely or change it to a less-chatty level (e.g., level =
"debug") and add meaningful fields to the span if you need context (for example
request ids or peer identifiers) so the outer span provides useful data; update
the attribute on the handle_broadcast_recv function accordingly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/mempool_service.rs`:
- Around line 3188-3213: The shutdown currently persists the mempool regardless
of whether acquire_many succeeded, risking inconsistent snapshots if handlers
timed out; modify the logic so persist_mempool_to_disk()
(self.inner.persist_mempool_to_disk()) is only called when the acquire_many
branch succeeded (the Ok(Ok(p)) case / when _all_permits is Some), e.g., move
the tokio::time::timeout(... self.inner.persist_mempool_to_disk() ...) call into
the successful branch or add an explicit check that _all_permits.is_some()
before attempting the 10s timeout persist; keep the existing timeout and error
handling for the persist call unchanged.

---

Outside diff comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 1734-1741: The code currently logs and returns the entire vdf_info
via "{:?}" in the error! and ValidationError::SeedDataInvalid call; replace that
with a compact diagnostic composed only of the minimal seed fields needed for
triage (for example extract vdf_info.seed and vdf_info.seed_height or equivalent
minimal identifiers), build a small string like "expected: X, got: seed=<...>,
height=<...>" and use that string in both error! and the
ValidationError::SeedDataInvalid message instead of printing the full vdf_info;
update the code around expected_seed_data, vdf_info, and the
ValidationError::SeedDataInvalid construction to reference those extracted
fields.
- Around line 1727-1728: The TODO in block_validation.rs notes a
consensus-critical missing difficulty validation but has no tracking; create a
formal issue in the repo (include acceptance criteria and tests for
seed/difficulty validation) and then update the TODO comment in
block_validation.rs to include the issue tracker reference (e.g.
"CONSENSUS-TODO: see issue `#123`" or URL) and a short actionable note pointing to
the code area to be fixed; also add the issue ID to any consensus-related
tracking document (e.g. issues.md or CHANGELOG) so the gap is discoverable and
not lost.

In `@crates/actors/src/chunk_ingress_service.rs`:
- Around line 314-347: The shutdown currently proceeds to drain queued messages
and flush the chunk writer even if acquiring the shutdown permits failed; change
the logic so that the Phase 2 drain (loop using self.msg_rx.try_recv(),
wait_with_progress(self.inner.handle_message(...)), and the final
self.inner.chunk_data_writer.flush().await) only run when the permit acquisition
succeeded (i.e., when the match produced Some(p) for the acquire_fut result).
Use the existing _all_permits result (rename to all_permits or check .is_some())
to gate running the drain_fut and flush, and log/skipping the drain+flush path
when permits were not acquired to avoid racing with in-flight handlers.

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/actors/src/mempool_service.rs`:
- Line 3226: The #[tracing::instrument(level = "trace", skip_all)] attribute on
the hot utility function (the function annotated with
#[tracing::instrument(level = "trace", skip_all)] that runs on every reorg
broadcast tick) should be removed to avoid cheap, low-value spans; either delete
the attribute entirely or replace it with a lighter-weight/meaningful span
(e.g., remove skip_all and add specific fields like reorg_id or tick_count) so
that only useful context is recorded and you don't instrument this hot path
unnecessarily.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/actors/src/chunk_ingress_service.rs`:
- Around line 340-344: The timeout branch of the tokio::time::timeout match
abandons remaining queued messages, causing callers to receive generic
closed-channel errors; update the Err(_) arm (the timeout path around drain_fut)
to explicitly notify queued request/response items with a typed
overload/shutdown error by reusing the existing timeout notifier helper (use the
same notifier used elsewhere in this module to signal timeouts) or by iterating
the remaining items in the chunk ingress queue and sending a proper error via
their responder channels; reference the drain_fut match and the request/response
enum variants in chunk_ingress_service.rs and ensure each pending message uses
the existing notifier API to deliver the explicit error before returning.

In `@crates/actors/src/mempool_service.rs`:
- Around line 3159-3188: The loop currently pops a message from msg_rx
(traced.into_parts()) before attempting semaphore.acquire_owned() with timeout,
so when acquire times out the popped message is dropped and remaining messages
aren't drained; fix by acquiring the permit (using
inner.message_handler_semaphore.acquire_owned() with the same timeout) before
removing a message from msg_rx, then after successfully obtaining the permit
call msg_rx.try_recv() (or try_recv in a small loop) to get the next message and
spawn the task that calls inner.handle_message wrapped by wait_with_progress;
keep the tracing span creation and use of runtime_handle.spawn, and handle Err
from acquire_owned (semaphore closed) and timeout without discarding messages so
no queued messages are lost during shutdown drain.

---

Outside diff comments:
In `@crates/database/src/database.rs`:
- Around line 267-271: The tracing span for cache_chunk_verified should include
the chunk's data_root for better traceability; update the
#[tracing::instrument(...)] on the cache_chunk_verified function to add a span
field that records the chunk's data_root (i.e., capture chunk.data_root or
UnpackedChunk's data_root property) while keeping the level="trace" and skip_all
behavior, mirroring how insert_block_header includes block_hash in its span
fields.

---

Duplicate comments:
In `@crates/actors/src/chunk_ingress_service.rs`:
- Around line 314-326: The shutdown currently proceeds to call
chunk_data_writer.flush() even if acquire_many (the acquire_fut assigned to
_all_permits) timed out or the semaphore closed, which risks missing in-flight
enqueues; change the shutdown logic so it only performs the final drain/flush
when acquire_many succeeded (i.e., _all_permits is Some(p)). Specifically,
inspect the match result from the tokio::time::timeout block (the variable
created from acquire_fut / acquire_many) and if it returned Ok(Ok(p)) proceed
with handing p to chunk_data_writer.flush(); otherwise log and skip or retry
waiting rather than flushing immediately. Ensure references to
acquire_many/acquire_fut/_all_permits and chunk_data_writer.flush() are used so
the guard is clearly enforced.

In `@crates/p2p/src/chain_sync.rs`:
- Around line 357-370: The spawned task must always clear
stake_and_pledge_whitelist_running_flag even if it panics; wrap the task body
with a panic-safe cleanup (e.g., create a small ResetFlagGuard that stores false
in its Drop impl, or call std::panic::catch_unwind around the async body) and
instantiate it at the start of the async closure that calls
handler.pull_and_process_stake_and_pledge_whitelist().await so the flag is set
to false in all exit paths; keep the
tracing::info_span!("stake_and_pledge_whitelist_update") instrumentation around
the spawned task.

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/actors/src/mempool_service.rs`:
- Around line 3166-3197: The shutdown inline fallback in the NoPermits branch
bypasses inner.message_handler_semaphore and can exceed max_concurrent_tasks;
modify the NoPermits handling so it still respects the semaphore: instead of
invoking inner.handle_message(msg) directly, re-acquire a permit (e.g., await
inner.message_handler_semaphore.acquire_owned().await) before calling
handle_message, and then run the handler under the acquired permit (using the
same wait_with_progress, task_info and span logic) so both the inline path and
spawned tasks honor the semaphore bound and the popped message is still
processed during shutdown.