A set of fixes and features for wit\2 #2472
Conversation
…oposal is the best one
…osed in the previous epoch
…lock was part of wit\2
…ake transaction fee
…to block overview
|
|
||
| let _ = stakes.add_reward( | ||
| miner_pkh, | ||
| Wit::from(50_000_000_000) + Wit::from(transaction_fees), |
There was a problem hiding this comment.
Afaik, 50 WIT as block reward is not yet decided. Perhaps this proposal of reducing block time and reward should be part a dedicated PR, not mixed up with a potential fix or improvement in the mining elegibility algorithm.
There was a problem hiding this comment.
Of course, I fully agree, but I had to put something there as a place holder.
| return Ok(IneligibilityReason::InsufficientPower.into()); | ||
| let mut rank = self.rank(Capability::Mining, epoch); | ||
| let (_, max_power) = rank.next().unwrap_or_default(); | ||
| let (_, threshold) = rank.nth(MINING_REPLICATION_FACTOR - 2).unwrap_or_default(); |
There was a problem hiding this comment.
Shouldn't it be rank.nth(MINING_REPLICATION_FACTOR -1) instead?
There was a problem hiding this comment.
No, because rank.next() consumes element 0 of the iterator and you want the element up to MINING_REPLICATION_FACTOR - 1. If rf is 4, line 167 consumes element 0 and nth(2) returns and consumes up to the nth element starting from 0.
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In case of no valid block candidate to consolidate for previous epoch, the top MINING_REPLICATION_FACTOR eligible validators should be slashed, whereas with this code only those with at least 1 / MINING_REPLICATION_FACTOR of the highest power would actually get slashed.
There was a problem hiding this comment.
Besides, I'd rather recommend to reset coin age in proportion to each one's rank position:
- First: reset coin age to zero
- Second: subtract half the current age
- Third: subtract one third of current age
- And so on (sequential prime divisors).
This way we would avoid/minimize the chances of a colluding set of big stakers to end up being within the same eligible set on periodically base.
There was a problem hiding this comment.
In case of no valid block candidate to consolidate for previous epoch, the top MINING_REPLICATION_FACTOR eligible validators should be slashed, whereas with this code only those with at least 1 / MINING_REPLICATION_FACTOR of the highest power would actually get slashed.
Yes, my initial implementation actually slashed all validator up to rf validators. The problem with that is that you can actually be slashing validators which were not eligible to propose a block which seems rather unfair.
For example, imagine validator 1 and 2 have a power of 100k and validators 3 and 4 have a power of 10k. The former two are guaranteed to propose a block, but for the latter two it's a matter of whether their VRF is smaller than a target hash. Hence the choice in above algorithm to not slash validators 3 and 4. I'm up for changing that though.
There was a problem hiding this comment.
Besides, I'd rather recommend to reset coin age in proportion to each one's rank position:
- First: reset coin age to zero
- Second: subtract half the current age
- Third: subtract one third of current age
- And so on
Assume validator 1 and 2 have the same power, do we rest both to zero? I'm not sure how deterministic the rank function is with respect to multiple distinct nodes since it seems to be sorted by power only.
This way we would avoid/minimize the chances of a colluding set of big stakers to end up being within the same eligible set on periodically base.
I understand the point and actually agree that this might be a good defense, but I do wonder if it's worse to have a single epoch where multiple colluding attackers are executing an attack (and hence we only have a single epoch with a block missing) or several epochs where those attackers separately try to run a griefing attack (and we might have several epochs with missing blocks if network connectivity is not perfect or there is also censoring taking place).
There was a problem hiding this comment.
For example, imagine validator 1 and 2 have a power of 100k and validators 3 and 4 have a power of 10k. The former two are guaranteed to propose a block+, but for the latter two it's a matter of whether their VRF is smaller than a target hash. Hence the choice in above algorithm to not slash validators 3 and 4. I'm up for changing that though.
According to the simulation source code that I believe it's being replicated, with a replication factor of 4, validators 1 to 4 would indeed be eligible and expected to broadcast a valid block proposal. The VRF is just used for the other nodes to agree upon which to ultimately accept as best proposal, meaning that although validators 3 and 4 have less chances to win, they do still have some (even if higher ranked validators actually broadcast valid proposals), and further more, they are obliged to broadcast a block candidate whatsoever. Therefore, in case of no blocks candidates in previous epoch, the 4 eligible validators on that epoch should all be slashed, no matter if their absolute mining power is "low" compared to the others.
There was a problem hiding this comment.
Assume validator 1 and 2 have the same power, do we rest both to zero? I'm not sure how deterministic the rank function is with respect to multiple distinct nodes since it seems to be sorted by power only.
Nope, I do believe that eligible validators coincidentally having same power should be slashed in different proportions, instead of resetting all of them to the same value. The easiest and most convenient way to implement that would be to rely on the implementation of the rank function, as long as it is deterministic (i.e. non dependent on underlying OS, crate version, or thinks alike), as I believe it must be.
There was a problem hiding this comment.
I understand the point and actually agree that this might be a good defense, but I do wonder if it's worse to have a single epoch where multiple colluding attackers are executing an attack (and hence we only have a single epoch with a block missing) or several epochs where those attackers separately try to run a griefing attack (and we might have several epochs with missing blocks if network connectivity is not perfect or there is also censoring taking place).
Considering that we're planning a replication factor as low as 4, I think it would be best to reduce the chances of multiple colluding attackers within same epoch. In fact, with this goal in mind, I'd rather suggest to consider a higher replication factor, like 8 or even 16, as long as we were certain that p2p gossiping doesn't grow to an unmanageable number of messages for the average node specs.
There was a problem hiding this comment.
According to the simulation source code that I believe it's being replicated, with a replication factor of 4, validators 1 to 4 would indeed be eligible and expected to broadcast a valid block proposal. The VRF is just used for the other nodes to agree upon which to ultimately accept as best proposal, meaning that although validators 3 and 4 have less chances to win, they do still have some (even if higher ranked validators actually broadcast valid proposals), and further more, they are obliged to broadcast a block candidate whatsoever. Therefore, in case of no blocks candidates in previous epoch, the 4 eligible validators on that epoch should all be slashed, no matter if their absolute mining power is "low" compared to the others.
That is certainly not the case for the simulation and not how it is implemented in this PR. If a validator has more than rf less power, this line combined with a node's VRF can result in them not proposing a block. It's not just that they have less chances to win, they will not even propose and broadcast a block. Whether a block wins or not ultimately comes down to the selector used in the simulation, but in this PR power wins, then VRF, then block hash.
The easiest and most convenient way to implement that would be to rely on the implementation of the rank function, as long as it is deterministic (i.e. non dependent on underlying OS, crate version, or thinks alike), as I believe it must be.
I am simply not sure the current implementation of rank is deterministic. I don't know if a BTreeMap in Rust maintains the order of insertion when iterated over or not and because we are only sorting on a single metric which can be the same for different validators, I imagine it could lead to non-determinism. Of course, that should be easily fixable by changing the line to something like this: sorted_by_key(|(stake_key, power)| (stake_key.validator.clone(), *power))
In fact, with this goal in mind, I'd rather suggest to consider a higher replication factor, like 8 or even 16, as long as we were certain that p2p gossiping doesn't grow to an unmanageable number of messages for the average node specs.
I guess it's fine to increase the rf for the simple reason that the p2p traffic should reduce more or less exponentially. Any node will only forward a block if he considers it as a better block and hence most nodes will never see rf blocks. 8 feels fine to me, 16 feels a bit over the top.
The first four commits are part of #2469.
The latter four commits are all new self-contained patches that introduce a fix or feature, but they build upon each other, so I kept them in the same branch.