Skip to content

Conversation

@cameron1024
Copy link
Contributor

@cameron1024 cameron1024 commented Jul 6, 2022

This PR replaces the proposer_rewards.py script with an equivalent written in Rust.

Some details are fuzzy, in particular types. The python script had type annotations, but there were a few type errors so not sure how to resolve things.

It passes tests in vt-testing locally, this is the PR for vit-testing: input-output-hk/vit-testing#292

The diff is huge because of a gitignore mistake, I'll fix that

@cameron1024 cameron1024 force-pushed the rewards-script-rewrite-it-in-rust branch from 0e4d4bd to 3d908c5 Compare July 6, 2022 14:07
@zeegomo
Copy link
Contributor

zeegomo commented Jul 7, 2022

FYI, you need to solve conflicts for the CI to run

@cameron1024 cameron1024 changed the title Rewards script rust - draft Rewards script rust Jul 8, 2022
pub struct HttpResponse<T: for<'a> Deserialize<'a>> {
_marker: PhantomData<T>,
inner: Response,
body: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about this change, didn't want reqwest types in our api?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of expanding the testing for the proposers script, I've added a MockClient for testing the code paths that grab data from vit-ss, and it felt weird to have reqwest types in the public API. Also, last time I checked, it was either impossible or just annoying to construct a fake Reponse from outside reqwest, but I didn't look into that much

.collect()
}

fn calculate_total_stake_from_block0(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use VoterHIR instead of block0 like we do with other tools

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what that would look like, is there somewhere I can look for an example of this? Doing a grep for VoterHir didn't help much :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of commitee keys + block0 accept as input a Vec<VoterHIR>, which is essentially the output from our snapshot tool. Then it should be something like

hirs.iter().map(|v| v.voting_power).sum()

because we don't punt commitee keys in the snapshot (or rather, all voters in the snapshot should be eligible to vote / earn rewards)

@minikin minikin requested a review from zeegomo August 9, 2022 11:41
Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an empty cli/rewards/proposers/util.rs file

.collect()
}

fn calculate_total_stake_from_block0(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of commitee keys + block0 accept as input a Vec<VoterHIR>, which is essentially the output from our snapshot tool. Then it should be something like

hirs.iter().map(|v| v.voting_power).sum()

because we don't punt commitee keys in the snapshot (or rather, all voters in the snapshot should be eligible to vote / earn rewards)

@cameron1024
Copy link
Contributor Author

So does that mean that we should be reading some serialized VoterHir file? Other parts of the script use block0 as well, do those need changing as well? By the sounds of things, the VoterHir is derived from block0, so having both formats exist at the same time sounds like an opportunity for inconsistency bugs?

@cameron1024 cameron1024 requested a review from a team August 18, 2022 15:39
@@ -0,0 +1 @@
/nix/store/079q0fpi94z0x8a4rj3bzhjl8lrxm5xy-source No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should flake-inputs/* be committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this whole dir should be ignored 😅

Copy link

@cong-or cong-or left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM and well tested. I dont have much context on proposer logic etc.. So cant comment on that stuff.

@cameron1024 cameron1024 changed the base branch from main to full-rewards-script September 29, 2022 13:56
Base automatically changed from full-rewards-script to main October 3, 2022 00:57
@cameron1024
Copy link
Contributor Author

Some PR mixups has meant that this was addressed in the other PR

@cameron1024 cameron1024 closed this Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants