-
Notifications
You must be signed in to change notification settings - Fork 581
Milestone 1 and 2 for Offline Election Prediction Tool #1288
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
Conversation
Fix link to application
|
On a different note, there's a mistype in milestones 2 and 3 in the proposal:
|
|
Hi @michalisFr |
|
Thanks for following up @Ipsa11. Basically yes. In more details, what I mean is the following: In Milestone 2, Deliverable 1 says:
The title is correct, but the description is the same as Deliverable 1 of Milestone 1: "Core Election Engine" At the same time, in Milestone 3 you say:
Here the title ("Core Election Engine") is incorrect and the description corresponds to the above deliverable of Milestone 2. At the same time, based on the RFP, the 3rd milestone is about the API functionality which isn't mentioned. So basically the deliverable in Milestone 2 should read:
and for Milestone 3 something like:
|
|
Thank you @michalisFr for the clarification — you’re absolutely right. Could you please guide me on the next steps to update the PR, since it has already been merged? I want to make sure the corrections are made properly. |
|
@semuelle Can you please help? ☝️ |
|
Hey @Ipsa11. You can simply pull the latest master branch, change the affected lines in your original application document and then create a new PR. We will review it asap. Should be quick for such a small change. |
|
Hi @semuelle @michalisFr, |
|
It looks like you did not use a recent fork. It's 40 commits behind the current master branch. I recommend you check out the repo anew and apply the changes there. |
|
Hii @semuelle I have made the changes. Please have a look once. |
|
Hey @semuelle just checking in have you had a chance to go through Milestone 1 for the Offline Election Prediction Tool? Please share any updates or feedback when you get a moment. Thanks! |
|
Hey @Ipsa11, sorry for the delay. Let me check with @michalisFr. |
|
Thanks @semuelle Waiting for your reply once you’ve had a chance to check with @michalisFr. |
|
Hey @Ipsa11. I have left some review comments. Can you please address them? In the meantime, I'll try to run the tool and see if it works as expected. But I'd also like for a core dev to review the code before approving. I've pinged @sigurpol but he is OOO until Mon and he might be busy next week with the AHM. |
|
Hey @michalisFr thanks for the update. Could you please share the link where you’ve added the review comments so I can check and address them? |
I've added them inline to the file. They are also visible above in the conversation. Can you see them? |
|
Hey @michalisFr Thanks for letting me know. I’m not able to see the comments on my end — could you please double-check if they were submitted or visible in the file? Attaching the ss for the same.
|
| | Number | Deliverable | Link | Notes | | ||
| | ------------- | ------------- | ------------- |------------- | | ||
| | **0a.** | License |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/LICENSE |Apache 2.0 / GPLv3 / MIT / Unlicense. | | ||
| | **0b.** | Documentation |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/README.md |Inline code documentation and a tutorial explaining how to simulate an election with default or custom inputs via CLI. | |
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.
- Missing instructions on the format of the input JSON and examples of its usage
- Inconsistent examples, e.g.
--use-cached-datawithout--cache-diror--uriinstead of--chain-uri - Instructions for Docker
desired-validatorsrefers to the size of the active set? It's not clear- Is there a reason all examples are for 19 validators?
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 @michalisFr,
- The missing instructions for the input JSON format have been added.
- All inconsistent examples have been corrected.
- Docker usage instructions are now included as well.
- Yes, the desired-validators parameter refers to the size of the active set.
- There’s no fixed limit of 19 validators — you can specify any number. If not provided, the tool will automatically fetch the active validator count from the chain.
| | **0a.** | License |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/LICENSE |Apache 2.0 / GPLv3 / MIT / Unlicense. | | ||
| | **0b.** | Documentation |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/README.md |Inline code documentation and a tutorial explaining how to simulate an election with default or custom inputs via CLI. | | ||
| | **0c.** | Testing and Testing Guide |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/tests/integration_tests.rs | Unit tests for the election logic; guide on how to run and interpret the results. | | ||
| | **0d.** | Docker |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/README.md | Dockerfile to build and run the CLI simulator locally. | |
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.
This links to the README instead of the Dockerfile
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 @michalisFr
The link to Docker file has been updated.The link is as follows
https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/Dockerfile.README.md
| | **0b.** | Documentation |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/README.md |Inline code documentation and a tutorial explaining how to simulate an election with default or custom inputs via CLI. | | ||
| | **0c.** | Testing and Testing Guide |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/tests/integration_tests.rs | Unit tests for the election logic; guide on how to run and interpret the results. | | ||
| | **0d.** | Docker |https://github.com/antiers-solutions/polkadot-staking-miner/blob/feat/offline-election-prediction-tool/offline-election-prediction-staking/README.md | Dockerfile to build and run the CLI simulator locally. | | ||
| | 1. | Core Election Engine | https://github.com/antiers-solutions/polkadot-staking-miner/tree/feat/offline-election-prediction-tool/offline-election-prediction-staking| Updated version of the existing election script with support for accurate simulation of on-chain validator election logic using Phragmén and other supported algorithms. | |
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 Could you please provide a 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.
Sure, I will review it, but after Polkadot AHM so realistically not before the end of this week if it's OK for you.
@Ipsa11 I am assuming we want to merge https://github.com/paritytech/polkadot-staking-miner/antiers-solutions:polkadot-staking-miner:feat/offline-election-prediction-tool branch into main, is it correct ? If it's the case, can we have a PR against main branch so it's easier to review for me as well?
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
Yes, we want it to be merged into the main branch. The PR for the same is [https://github.com/paritytech/polkadot-staking-miner/pull/1189]
|
Hey @Ipsa11. My sincere apologies! I hadn't submitted the comments 🤦♂️ You should be able to see them now |
|
I took an initial look at https://github.com/antiers-solutions/polkadot-staking-miner and have some general preliminary questions and comments after briefly reviewing the changes.
All of this could theoretically have reused utilities from the main miner. The current design seems instead of having the prediction tools completely independent from the main miner and let the miner import offline-election-prediction-staking as sort of plugin / standalone library and offline-election-prediction-staking redefines whatever it needs.
I have added a comment here to explain how I would approach the problem. It's just my 2c and the way I envisioned it, but I am more than open to discuss radically different approaches |
|
Thank you for the comment @sigurpol! I'll revert on what approach might be best, but I want to echo your approach on how the prediction command should work that you posted in here, regardless if it's an extension of the miner or a standalone tool. I also echo your second point above. @Ipsa11 I'd like to provide some feedback on the functionality of the tool itself. I ran it earlier today on AH outside the election window and with no parameters and I'm attaching the results. Here are some "cosmetic" changes that would be useful:
as previously mentioned and discussed here. Besides these, I have a few questions:
However, the most important thing I'd like to note is that the results don't make sense I'm afraid. The tool predicts that validators will make to the active set with ~600k DOT, which is clearly wrong, while others will be in the active set with 3+ MDOT, which is also extremely unlikely. Finally, what would be useful is if there where two outputs: a
I acknowledge that this is the first I'm mentioning this request 😅 |
|
Thank You @michalisFr and @sigurpol for the review and suggestions! We’re implementing the recommended changes and will update the codebase shortly. |
|
Hey @michalisFr @sigurpol, |
|
@Ipsa11 ok now it works for Polkadot, I am able to correctly predict when the snapshot is fetched and when it is built just at the end of the I am not able to predict for Kusama. When the snapshot is fetched, predictions are off outside the snapshot window the prediction file is empty |
|
@Ipsa11 I run some additional tests, predictions for Polkadot are OK For Kusama, when a snapshot is available, predictions are slightly off, see for block When the snapshot is not availble the built one is totally different from the fetched one (tested for block |
|
Hey @filippoweb3 |
|
@Ipsa11 I tested the tool and I am able to predict correctly for both Kusama and Polkadot within and outside the snapshot window. Regarding the custom data, do I need to add all nominators and candidates there, or is there a way to remove specific accounts, add new ones, or edit nominations for specific accounts? (as requested in the Milestones) I tried to use the example you have in the README, and I get the following error The aim is that the custom file provides the data to:
The current system "sees" only the custom file, but ideally we should:
Afaik, you did not provide an API for the prediction feature, correct? |
|
Hey @filippoweb3 To clarify scope and status:
We have implemented all the requested custom data functionality . The overrides option allows adding candidates that may not exist on-chain, removing specific candidates from the election, adding or overriding voters with custom stake amounts regardless of on-chain bonded amounts, and removing specific voters from the election. Now, the system first fetches or builds the snapshot and then overrides it with provided custom data as expected. Could you please confirm whether the current custom data support meets the Milestone 2 expectations? Regarding the API, you are correct — it is part of Milestone 3, and we will push that code in the meanwhile. Looking forward to your feedback. |
|
Hi @Ipsa11. First of all thank you for the work you've done so far on this tool! At this point and after our testing, I consider the core election engine part of the first milestone delivered. However, the custom input either with existing accounts (milestone 1) or with non-existent or not bonded accounts (milestone 2) is not delivered I'm afraid. As you mention in your comment the custom file should allow adding and removing candidates and adding and removing voters. However, the way it works now apparently, the custom file replaces the whole snapshot data, which is not the requested functionality and which also doesn't work apparently. The idea of this functionality is that the data in the custom file changes the snapshot accordingly and then the election prediction is run on this modified snapshot. Also, there is no point in assigning stake to candidates; this will be determined by the election. In the original script, the JSON file had this structure: I like you Please make the custom file to work in this way and once we've tested it we'll be happy to award these two milestones. |
|
Hey @michalisFr |
|
@Ipsa11 I tried to test with the latest pushes and
Left to do: API |
|
Thanks @Ipsa11 for the update and @filippoweb3 for testing. So, I believe at this point we can sign off on the first two milestones @semuelle! |
|
Hey @michalisFr @filippoweb3 |
For the latter I already tagged @semuelle (we posted at the same time). For merging into staking miner, let me tag @sigurpol |
|
Hey @sigurpol @michalisFr |
I'll review last version today and provide a feedback - thanks all for the great work so far |
|
@Ipsa11 I see you are still polishing the PR - let me know when you are done and it's good time for final review. Thank you |
|
@Ipsa11 , I've added my review - couple of relevant issues I would like you to investigate / address + few minor issues not so important (but low hanging fruit so why not) |
Hey @sigurpol |
|
@Ipsa11 let me know when I can test the API so that we got all Milestones covered (if you are still aiming for it) |
Re-reviewed, thx for taking care of my comments. I have one very tiny cosmetic comment left but I can merge as it is once @filippoweb3 confirms results are OK |
|
Hey @filippoweb3 @sigurpol |
|
@Ipsa11 it all checks, the changes did not affect how the prediction works. Regarding the API: The goal is to make election simulations and snapshot retrieval programmatically accessible for downstream tooling, dashboards, and research workflows, without requiring direct CLI or runtime integration. Proposed API overviewThe API would run as a standalone REST server backed by a configurable RPC endpoint. Server startupDefault:
Custom address:
REST API endpointsPOST /simulateSimulate an election using a snapshot and configurable parameters. Query parameters
Request body Where
Success response (200 OK): Basically "active validators" and "nominators" are the equivalent of the GET /snapshotRetrieve the election snapshot used for simulations. Query parameters
Success response (200 OK): |
|
@Ipsa11 I have merged the changes for Milestone 1 and 2 into the miner repo main branch. Thanks for all the good work. |
|
@sigurpol @michalisFr @semuelle Could you please initiate the payment process for these two milestones when convenient? Also let me know if any input is required from my side. |
semuelle
left a comment
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.
|
🪙 Please fill out the invoice form in order to initiate the payment process. Please make sure that you follow the instructions and requirements as laid out in the form as well as our Terms & Conditions. Thank you! |
|
Hey @semuelle, Could you please confirm if the payment can be processed to this address? |
The PR only contains one file. I see it titled as "Milestone 1 and 2", but the file in it clearly states |




Milestone Delivery Checklist
Link to the application pull request: w3f/Grants-Program#2623