Conversation
shamil-gadelshin
left a comment
shamil-gadelshin
left a comment
There was a problem hiding this comment.
The changes introduce a lot of calculations, side-effects, and code branches. Can we have tests for the code (we have a mock runtime)?
|
|
||
| // Entropy injection interval must be bigger than injection delay or else we may end up in a | ||
| // situation where we'll need to do more than one injection at the same slot | ||
| const_assert!(POT_ENTROPY_INJECTION_INTERVAL as u64 > POT_ENTROPY_INJECTION_DELAY); |
There was a problem hiding this comment.
Can it be changed? If yes - we need to add a check POT_ENTROPY_INJECTION_INTERVAL > 0 because we have a division in the code.
There was a problem hiding this comment.
Theoretically yes, but I don't think we'll need to change them, they are supposed to be constants.
vedhavyas
left a comment
vedhavyas
left a comment
There was a problem hiding this comment.
Make sense. I think testing these would be easier once we have sp-lightclient is updated
nazar-pc
left a comment
nazar-pc
left a comment
There was a problem hiding this comment.
The changes introduce a lot of calculations, side-effects, and code branches. Can we have tests for the code (we have a mock runtime)?
We should, but there is no good infrastructure for it right now. Also tests in sp-lightclient and pallet-subspace are disabled when pot feature is enabled because they don't even compile. When fixed pallet-subspace will likely be updated to cover many of the cases. But I definitely agree that many automated tests are necessary here.
|
|
||
| // Entropy injection interval must be bigger than injection delay or else we may end up in a | ||
| // situation where we'll need to do more than one injection at the same slot | ||
| const_assert!(POT_ENTROPY_INJECTION_INTERVAL as u64 > POT_ENTROPY_INJECTION_DELAY); |
There was a problem hiding this comment.
Theoretically yes, but I don't think we'll need to change them, they are supposed to be constants.
nazar-pc
force-pushed
the
pot-entropy-injection
branch
from
e4a60e9 to
5443c10
Compare
nazar-pc
force-pushed
the
pot-entropy-injection
branch
from
5443c10 to
b000948
Compare
nazar-pc
dismissed stale reviews from dariolina, shamil-gadelshin, and vedhavyas
The base branch was changed.
4 participants
Most of the complex logic went into #1929 with verification, this is just a straightforward implementation of the entropy injection.
I have tested various edge-cases locally and added some static assertions in the runtime. If you modify those values for whatever reason, you still need to make sure things compile or else you'll get into broken invariants and inevitably get errors sooner or later.
Let me know if something doesn't make sense or you have questions about anything.
Next step is to implement voting (right now it is broken) and then fix tests (
sp-lightclientwill also need to be adjusted). I also have some concerns and local patches for gossip, also verifiction cache is susceptible to gossiping attacks, so I will have to adjust that as well and have a discussion about how to implement it securely.Major things lacking from eventual implementation:
Code contributor checklist: