Farmer voting #485
Farmer voting #485
Conversation
…extend solution ranges with voting properties
…ntime side, no equivocation check
… signing/signature to reward signing/signature with context that is distinct from Substrate's default
…it compatible with Polkadot.js api and generally decrease stack size requirements
…andom errors in tests and wasn't correct in case those types didn't match (like typically the case in tests)
…transaction pool validation of the vote
…quivocation yet), send all rewards of equivocated farmer into storage escrow
…validate errors more specifically, add slot validation test
…ty of solutions doesn't downgrade due to more votes found (especially with big solution range initially)
nazar-pc
requested review from
i1i1,
liuchengxu,
rg3l3dr and
vedhavyas
as code owners
|
@liuchengxu we had another CI error, might be the same as before, I can't recall: https://github.com/subspace/subspace/runs/6555767954 |
|
Last 3 commits should not have been merged here, but 🤷♂️ |
…istory size to u128 numbers, avoid potential overflows in total space pledged calculation
|
Yeah, the CI failure you linked indeed looks like the same issue we have met before. Now I'm more inclined to believe it's Windows-specific as all the failures so far happened on Windows, and it also seems to be indeterministic, don't know how to reproduce it, making it harder to dig into it. Haven't checked the commits during the last Substrate upgrade, not sure if it can be really helpful but will have a look today. |
| @@ -107,6 +107,14 @@ struct Salts { | |||
| next: Option<Salt>, | |||
| } | |||
|
|
|||
| struct AbortOnDrop<T>(JoinHandle<T>); | |||
There was a problem hiding this comment.
There is a good wrapper in the rust-analyzer which is called Defer in go like manner. It has only constructor - a FnOnce closure which will be called on drop. Should we try to use smth like this?
There was a problem hiding this comment.
Not sure what you mean by that
There was a problem hiding this comment.
I mean that it would remove those class of struct overall. You can just call:
let _defer = Defer::new(|| handle.abort());It would remove unnecessary boilerplate with implementing drop every time.
There was a problem hiding this comment.
If we had that use case more often I think it'd be fine, but we have just one-off here. If you notice another use case for this, feel free to create a more general purpose utility.
…t allows slot number to be the same in the block as in the vote
| let current_block_number = frame_system::Pallet::<T>::current_block_number(); | ||
|
|
||
| if current_block_number <= One::one() || height <= One::one() { | ||
| debug!( |
There was a problem hiding this comment.
Hmm, why not error!? and the other ones.
There was a problem hiding this comment.
It is a potentially user-provided transaction, also transactions can be late for inclusion even if they were valid initially. No need to print errors in node's logs if someone submits invalid transaction.
| slot_info.solution_range, | ||
| slot_info.salt, | ||
| ) { | ||
| // Try to first find a block authoring solution, then if not found try to find a vote |
There was a problem hiding this comment.
How heavy is a call to find_by_range()? I think you could save almost everyone (except winner and voters) one call if you first check for voting_solution_range since block authoring solution_range is contained within it.
Right now the winner only does 1 call and everyone else 2. If you first check voting range, the winner and voters will do 2 calls and everyone else 1.
There was a problem hiding this comment.
If we just check for voting_solution_range we'll potentially find solutions that are only sufficient for voting, but not block authoring, hence block authoring frequency will decrease even if we have corresponding commitments.
The call itself isn't very heavy, but it is a database access, so it isn't free for sure.
I think we can optimize it a bit though, will submit a PR later.
There was a problem hiding this comment.
I didn't mean only check for voting, I meant first check for voting, and if yes, then check also for authoring. So everyone checks for voting, and then voters and a potential winner check for authoring too, who gets the closest to target.
Brace yourself, this is a big one!
Unfortunately a few refactorings were necessary along the way and it was painful to move them out, so they are part of this PR.
The primary goal of this PR is to introduce initial concept of voting. Farmers can solve with a reduced difficulty in additional to regular difficulty to have a chance of creating a vote on top of the parent block. Vote currently is just a transaction, there is no incentive (yet) for farmers to include it, but we'll change that later. For such vote farmer will also get a reward. Votes are also subject to equivocation just like block producers, though all of the logic is contained within runtime.
Votes on top of block N can only be included in block N+1 or N+2 (this is because delay between N and N+1 might be too small for vote to actually propagate over the network, which would result in many votes lost).
Voting is disabled (as useless) when solution range adjustment isn't active.
Since there are new ways to equivocate it is possible that block author itself equivocates, in which case they don't get block reward or fees, all that instead goes to storage escrow for future farmers.
While implementing voting signatures I decided to finally create a separate reward signing context for Sr25519 crypto that is separate from Substrate's, just like we have a separate context for solutions.
Along the way when working on tests I noticed that the way reward address was implemented was incorrect and it shouldn't have treated public key of the farmer and reward address as the same type. Those are distinct and in fact different in tests (there we use
u64as account ID instead), so that had to change, which involved a lot of changes throughout the codebase on all levels.Some other tweaks like avoiding floats in solution range adjustment or making
PiececontainVec<u8>instead of[u8; 4096](such that Polkadot.js API doesn't complain about arrays over 256 bytes and doesn't break explorer) were done as well.I recommend to go through individual commits to see what changes were done and look at refactoring, but for actual logic and test checks final diff would be the most helpful IMO.