Skip to content
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

Add storage mining pool #4364

Merged
merged 11 commits into from May 23, 2019
Merged

Add storage mining pool #4364

merged 11 commits into from May 23, 2019

Conversation

mvines
Copy link
Member

@mvines mvines commented May 21, 2019

Storage needs a mining pool to pull rewards out of.

@mvines
Copy link
Member Author

mvines commented May 21, 2019

@sagar-solana - this is still a WIP, I'm gonna plumb up the wallet commands in this PR too. Sharing early for FYI and any feedback though

@sagar-solana
Copy link
Contributor

Yeah I think I see where you're going with this. Should be okay.
Who all owns the mining pool keypair? Where does it get deposits from?
I think we've answered those questions already but I wouldn't mind another explanation 👼

@mvines
Copy link
Member Author

mvines commented May 21, 2019

Who all owns the mining pool keypair?

Doesn't matter, they probably burn it actually. You can't move funds out of the mining pool once contributed.

Where does it get deposits from?

Manna from heaven.

@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #4364 into master will decrease coverage by 3.3%.
The diff coverage is 42.5%.

@@           Coverage Diff            @@
##           master   #4364     +/-   ##
========================================
- Coverage    79.5%   76.2%   -3.4%     
========================================
  Files         173     173             
  Lines       30391   31897   +1506     
========================================
+ Hits        24184   24306    +122     
- Misses       6207    7591   +1384

@sagar-solana
Copy link
Contributor

sagar-solana commented May 21, 2019

just fyi, proof validations will not work properly right now. I'm working on getting them fixed in time. In case you need to test something, you should be able to pass in a single CheckedProof in a proof Validation and that should work.
#4377

@sagar-solana
Copy link
Contributor

Dunno why that test has started failing suddenly. :/

@mvines
Copy link
Member Author

mvines commented May 22, 2019

Oh are you seeing it as well? I was assuming it was just this PR, and was gonna debug tomorrow

@sagar-solana
Copy link
Contributor

It's likely something else. Replicators wait 50 seconds for the leader to make 16 rooted slots. Something changed recently that isn't making that happen and that test fails as a result of that.

Copy link
Contributor

@sagar-solana sagar-solana left a comment

Choose a reason for hiding this comment

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

Minor nits. I'll defer to your judgement.

Copy link
Contributor

@rob-solana rob-solana left a comment

Choose a reason for hiding this comment

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

adding the wallet command tests shows next guy how to fake it ;)

@mvines
Copy link
Member Author

mvines commented May 22, 2019

It's likely something else. Replicators wait 50 seconds for the leader to make 16 rooted slots.
Something changed recently that isn't making that happen and that test fails as a result of that.

Oh man was stuff broken. I don't understand how some of the local cluster tests were passing before

@sagar-solana
Copy link
Contributor

Oh man was stuff broken.

Shows that something needs to be fixed in our tests. That silly "start_and_exit_with_config" or whatever is a sanity test for whether or not local_clusters with a non-default config even works. It shouldn't be the only test to see some of these bugs haha

@mvines
Copy link
Member Author

mvines commented May 22, 2019

adding the wallet command tests shows next guy how to fake it ;)

I see these commands making it into the testnet participation doc once replicators actually work. Further adding to the already bloated test_wallet_process_command() and test_wallet_parse_command() tests seems like pretty low ROI. Was there another place you were thinking of?

@mvines mvines added the CI Pull Request is ready to enter CI label May 22, 2019
@mvines mvines force-pushed the pool branch 2 times, most recently from 8a06f72 to 37340b4 Compare May 22, 2019 17:11
@mvines
Copy link
Member Author

mvines commented May 22, 2019

@mvines
Copy link
Member Author

mvines commented May 22, 2019

@sagar-solana fyi on ac3ae02, like how did this ever work? I don't think it did.

@sagar-solana
Copy link
Contributor

:O Maybe I didn't build --all when I tried it? :\

@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label May 22, 2019
@rob-solana
Copy link
Contributor

adding the wallet command tests shows next guy how to fake it ;)

I see these commands making it into the testnet participation doc once replicators actually work. Further adding to the already bloated test_wallet_process_command() and test_wallet_parse_command() tests seems like pretty low ROI. Was there another place you were thinking of?

no, that's the place

your call

@mvines mvines force-pushed the pool branch 5 times, most recently from fdddaa3 to 136a5f6 Compare May 23, 2019 20:22
@mvines mvines merged commit b37d2fd into solana-labs:master May 23, 2019
@mvines mvines deleted the pool branch May 23, 2019 21:50
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.

None yet

4 participants