Conversation
kianenigma
reviewed
May 28, 2025
kianenigma
reviewed
May 28, 2025
sigurpol
changed the title
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
Ank4n
reviewed
May 29, 2025
Ank4n
reviewed
May 29, 2025
| .and_then(|(final_score, winner_count)| { | ||
| let desired_targets = crate::Snapshot::<T>::desired_targets().unwrap(); | ||
| let desired_targets = | ||
| crate::Snapshot::<T>::desired_targets().defensive_unwrap_or(u32::MAX); |
Contributor
There was a problem hiding this comment.
This code path is a bit messy because:
• DataProvider::desired_targets() returns a Result, but it’s infallible.
• Staking::ValidatorCount is a ValueQuery and defaults to 0 if not set.
• From this pallet’s pov, DesiredTargets might be unset, so we default to u32::MAX, whereas staking defaults to 0.
Practically this is of no significance — this code path should never be hit. But it’s a bit amusing and maybe a sign that our interfaces could use some cleanup.
Contributor
Author
There was a problem hiding this comment.
agree - maybe something to address next week when we do a complete review / cleanup of the staking code?
Contributor
There was a problem hiding this comment.
Yeah I think a good follow-up would be to actually create_targets_snapshot and create_voters_snapshot to not return a (pointless) Result<_>, and handle everything internally.
If any of them fail, it should handle triggering a defensive panic + emit Unexpected event.
Ank4n
approved these changes
May 29, 2025
Replace unwrap() with defensive_unwrap_or_default() in the on_initialize path to gracefully handle runtime misconfigurations where voter snapshot conversion fails, preventing potential panics during block initialization.
Replace unwrap() with defensive_unwrap_or(u32::MAX) to ensure solution fails verification gracefully when desired_targets is unavailable rather than panicking.
The commit adds new events to track when snapshot creation fails. This helps with monitoring and debugging failures in target/voter snapshot operations.
sigurpol
force-pushed
the
sigurpol/epmb_no_unwrap_on_init
branch
from
e4dea84 to
b6d96c4
Compare
kianenigma
reviewed
May 30, 2025
kianenigma
reviewed
May 30, 2025
kianenigma
approved these changes
May 30, 2025
Snapshot creation functions now emit failure events and trigger a defensive panic instead of returning errors. Call sites updated accordingly.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ordian
added a commit
that referenced
this pull request
Jun 4, 2025
* master: omni-node: fix `benchmark pallet` to work with `--runtime` (#8594) Handle and suppress "New unknown `FromSwarm` libp2p event" warning (#8731) Implement detailed logging for XCM failures (#8724) [pallet-revive] contract's nonce starts at 1 (#8734) sync/fix: Clear gap sync on known imported blocks (#8445) [PoP] Add personhood tracking pallets (#8164) client/net: Use litep2p as the default network backend (#8461) Unflake `returns_status_for_pruned_blocks` (#8709) [AHM] Report the weights of epmb pallet to expose kusama and polkadot weights (#8704) Remove all XCM dependencies from `pallet-revive` (#8584) Docker master image tag fix (#8711) Record ed as part of the storage deposit (#8718) [pallet-revive] update dry-run logic (#8662) feat: add collator peer ID to ParachainInherentData (#8708) Nest errors in pallet-xcm (#7730) pallet-assets ERC20 precompile (#8554) Broker: Introduce min price + adjust renewals to lower market. (#8630) [AHM] Staking async fixes for XCM and election planning (#8422) Staking (EPMB): Add defensive error handling to voter snapshot creation and solution verification (#8687)
pgherveou
pushed a commit
that referenced
this pull request
Jun 11, 2025
…on and solution verification (#8687) - Refactor snapshot creation to emit events and triggers defensive panic on failure - Replace unwrap() with defensive_unwrap_or(u32::MAX) to ensure solution fails verification gracefully when desired_targets is unavailable rather than panicking. - Add error events for failed target and voter snapshots Close #8685.
alvicsam
pushed a commit
that referenced
this pull request
Oct 17, 2025
…on and solution verification (#8687) - Refactor snapshot creation to emit events and triggers defensive panic on failure - Replace unwrap() with defensive_unwrap_or(u32::MAX) to ensure solution fails verification gracefully when desired_targets is unavailable rather than panicking. - Add error events for failed target and voter snapshots Close #8685.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #8685.