-
Notifications
You must be signed in to change notification settings - Fork 20
Feat/offline election prediction tool #1189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/offline election prediction tool #1189
Conversation
User @karanvir12, please sign the CLA here.User @pushkar1262, please sign the CLA here. |
| } | ||
|
|
||
| /// Convert Plancks to tokens (divide by 10^decimals) and format with token symbol | ||
| pub fn planck_to_token(planck: u128, decimals: u8, symbol: &str) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen this crate? it can likely simplify this code a bit for you: https://github.com/paritytech/ss58-registry
|
Hey @kianenigma |
|
Hi @Ipsa11 , can you please rebase vs latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left quite a few comments, some cosmetic and some more substantial.
Most critical ones:
- remove duplicate election execution and call mine_solution only once
- remove duplication. Imho, The prediction command should reuse:
- fetch_missing_snapshots() for snapshot data
- mine_solution() for election computation
- Snapshot
- All existing type infrastructure
monitoralready relies on
It might be beneficial to create a single data fetching layer that both monitor and predict can reuse. Something that 1. try to fetch snapshot 2.optional fallback: fetch from staking pallet and convert to snapshot format
I would like to have predict_command something like this (very high level)
pub async fn predict_cmd(...) -> Result<(), Error> {
// this get_election_data() is the same method to fetch data that monitor will use too.
// For predict, we will enable fallback in case snapshot is not available
let (targets, voters) = get_election_data(...).await?;
// same mine_solution that monitor relies on, SDK does the whole work
let solution = mine_solution(targets, voters, ...).await?;
// Extract prediction from solution.solution_pages
let predictions = build_predictions_from_solution(&solution, ...);
write_output(predictions)?;
Ok(())What do you think?
scripts/generate_changelog.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this file?
README.md
Outdated
| ```json | ||
| { | ||
| "metadata": { | ||
| "ss58Prefix": 42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to specify ss58Prefix? Isn't it redundant since we specify the chain via --uri ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ipsa11 can you reply to the open comments in this file from previous round of review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sigurpol
We have removed this ss58Prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ipsa11 it's still referenced in multiple places in tests/predict.rs e.g. when you create a temp custom file and still in this README...
Hey @sigurpol, |
Hi @Ipsa11 , I believe the An example in the SDK where this is done is here. I believe the miner could just use // after calling mine_solution()
for solution_page in paged_raw_solution.solution_pages {
let supports = feasibility_check_page_inner_with_snapshot::<MinerConfig>(
solution_page,
&snapshot_voters,
&snapshot_targets,
desired_targets,
)?;
// `supports` now contains full validator support with AccountIds and stakes |
…b.com/antiers-solutions/polkadot-staking-miner into feat/offline-election-prediction-tool
|
Looks good (to be properly tested by @michalisFr still 😅 ). @Ipsa11 can we please do a final round of cleanup and address some small easy pending comments to reduce the scope of the PR:
So we can have just changes specific to the predict command, thanks. |
|
Hey @sigurpol |
…minators with their allocated stake
src/commands/predict.rs
Outdated
| .map_err(|e| { | ||
| Error::Other(format!("Failed to fetch Desired Targets from chain: {e}")) | ||
| })? | ||
| .expect("Error in fetching desired validators from chain") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would return an error here instead of panic here replacing expect with ok_or_else() or similar
src/client.rs
Outdated
|
|
||
| let backend: ChainHeadBackend<Config> = | ||
| ChainHeadBackendBuilder::default().build_with_background_driver(reconnecting_rpc); | ||
| let backend = LegacyBackend::builder().build(reconnecting_rpc.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[IMPORTANT] This is a breaking change vs current monitor command. For monitor cmd - which is expect to run 24/7 we want and need to use the new ChainHeadBackend API and not the legacy one. I would suggest to revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sigurpol
If we use ChainHeadBackend instead of LegacyBackend, we are unable to access older blocks, which effectively renders the block-number option meaningless. As a result, we can no longer run election predictions on blocks that have snapshots.
We would appreciate your guidance on how to address this issue. For reference, below is the error we encounter when attempting to access older blocks using ChainHeadBackend. Based on our testing, it only allows access to approximately the most recent 15 blocks.
Error:
Subxt(Rpc(ClientError(User(UserError { code: -32801, message: "Invalid block hash", data: None }))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have experienced something similar while playing with a PR to add support for Smoldot (temporarily parked, went for RPC pool support as stop gap solution until I have time to address it better). Unless @jsdw has better idea here, I would suggest to use two path: let monitor command rely on ChainHeadBackend as today so no change for the main monitor cmd we want to run 24/7 to submit solutions. And let the predict command use the LegacyBackend. Does it make sense for you?
src/dynamic/election_data.rs
Outdated
| // validator has only self-vote if: | ||
| // 1. They are a validator (in active_set) | ||
| // 2. Their only target is themselves | ||
| // NOTE: Reverted to your original logic as requested, assuming you want strictly this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is NOTE a leftover? There is a misalignment between what comment says and what code does.
The comment says AND, the code does OR, right?
Suggestion:
- remove NOTE
- check what SDK does, look at get_npos_voters Logic (in staking-async)
- align comment and code
I believe the current logic is not aligned with what SDK does IIRC - meaning that if eg Alice is a validator and nominates Bob and Charlie, then it should count as valid nominator. Maybe targets.len() == 1 && targets[0] == *nominator would be a more correct check. Can you double-check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sigurpol
Yes, you are correct the Note shouldn't be there. But let us explain that the condition active_set.contains(nominator) || (targets.len() == 1 && targets[0] == *nominator)is for filtering the validators with only self stake after the mining of the solution, so that the nominators_results.json do not contain any validators with self stakes. Please have a look at the comment: [https://github.com/w3f/Grant-Milestone-Delivery/pull/1288#issuecomment-3586979252](w3f/Grant-Milestone-Delivery#1288 (comment))
we believe the current implementation focuses only on not including any validators in the nominators' result so both:
targets.len() == 1 && targets[0] == *nominator and
active_set.contains(nominator) || (targets.len() == 1 && targets[0] == *nominator)
seem to be equivalent. If there is any other query we're all ears. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the explanation so basically you want validators in one file, nominators in another file. If this is intent then the logic is fine and name could be better maybe (E.g. from validators_with_only_self_vote to validators_to_exclude_from_nominator_output or something like this. But that's just a cosmetic suggestion
src/commands/predict.rs
Outdated
| let validators_output = output_dir.join("validators_prediction.json"); | ||
| let nominators_output = output_dir.join("nominators_prediction.json"); | ||
| // Save validators prediction | ||
| write_data_to_json_file(&validators_prediction, validators_output.to_str().unwrap()).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably no need to panic here, can we return an error instead?
src/commands/predict.rs
Outdated
| ); | ||
|
|
||
| // Save nominators prediction | ||
| write_data_to_json_file(&nominators_prediction, nominators_output.to_str().unwrap()).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably no need to panic here, can we return an error instead?
This PR introduces the Offline Election Prediction Tool, which allows users to simulate and predict staking election results without relying on a live network connection.
The tool supports custom parameters such as election algorithm, snapshot block, and chain state, and produces detailed outputs showing validator–nominator allocations and corresponding stake amounts. It defaults to the on-chain Phragmén algorithm and can use the latest chain state if no snapshot block is provided.