whisper: PoW requirement #15701
whisper: PoW requirement #15701
Conversation
| sendMsg(t, true, 0) | ||
| checkPropagation(t) |
There was a problem hiding this comment.
You should create two unit tests, both with a specific name. You want tests to be as specific as possible, to pinpoint the root cause quicker.
There was a problem hiding this comment.
generally yes, but this case is special. this is already the worst whisper test (in terms of duration), because initialization takes too long. if i splitted the test, it would take double to run it. and it is already quite bad.
whisper/whisperv6/whisper.go
Outdated
| w.settings.Store(minPowIdx, val) | ||
| } else { | ||
| go func() { | ||
| // // allow some time before all the peers have processed the notification |
There was a problem hiding this comment.
comment / uncomment error :)
whisper/whisperv6/whisper.go
Outdated
|
|
||
| w.notifyPeersAboutPowRequirementChange(val) | ||
|
|
||
| if testMode { |
There was a problem hiding this comment.
Why making such a big difference between the tests and the regular code? If the real function waits, the tests should too.
There was a problem hiding this comment.
real function waits for a reason, which is related to syncronization in real-life environment. in test environment it does not make much sense. but if this test will run longer than all other tests together, than it will be really painful to run the tests every time.
There was a problem hiding this comment.
I understand it might take longer to run the test, at the same time running the whole list of tests is really long because of swarm so I would not lose sleep over this. What you can also do, is configure the waiting time to be different in the case of a test environment, so the code remains the same but the wait time is shorter for tests. That would ensure that the code that is being tested is the same as that which runs in production.
I have been looking for a way to distinguish a normal build from a test build in the doc, haven't found anything so far. Anything else that the powers that be have decided you don't need?
If you manage to do that, then my recommendation in the previous comment doesn't need to happen.
whisper/whisperv6/whisper.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (w *Whisper) notifyPeersAboutPowRequirementChange(pow float64) { | ||
| w.peerMu.Lock() |
There was a problem hiding this comment.
Doing network stuff in a critical section, that's a long waiting time while holdig a mutex. Could you do something lighter, e.g. starting a goroutine to do this? Or copy the list ? How contentious do we expect that list to be, and how much of a problem is it going to be if a peer is removed/added?
whisper/whisperv6/whisper.go
Outdated
| return errors.New("invalid powRequirementCode message") | ||
| } | ||
| f := math.Float64frombits(i) | ||
| if math.IsInf(f, 0) || math.IsNaN(f) { |
There was a problem hiding this comment.
zero allowed, but not negative
There was a problem hiding this comment.
My bad, I misread what IsInf was doing.
There was a problem hiding this comment.
Actually, no I didn't. You also need to make sure that the PoW is positive
whisper/whisperv6/whisper.go
Outdated
| return false, nil // drop envelope without error | ||
| return false, nil // drop envelope without error for now | ||
|
|
||
| // after the Status message will include the PoW requirement, it should return an error here: |
There was a problem hiding this comment.
once the status message includes the PoW requirement, an error should be returned here
whisper/whisperv6/whisper.go
Outdated
| @@ -176,14 +181,41 @@ func (w *Whisper) SetMaxMessageSize(size uint32) error { | |||
| } | |||
|
|
|||
| // SetMinimumPoW sets the minimal PoW required by this node | |||
| func (w *Whisper) SetMinimumPoW(val float64) error { | |||
| func (w *Whisper) SetMinimumPoW(val float64, testMode bool) error { | |||
There was a problem hiding this comment.
It would make more sense to Mock SetMinimumPoW during your tests instead of explicitly changing the function interface to specify if this is a test or not. Many things can slip through the cracks this way.
There was a problem hiding this comment.
do you mean, i should create another function, e.g. SetMinimumPowTest?
There was a problem hiding this comment.
yes, have a Whisper as an interface that implements SetMinimumPoW. Then, in tests, implement the Whisper interface as a WhisperMock structure and implement a different SetMinimumPoW for this. However, as I'm saying below, I think this is overkill when you can simply use the same function in both cases.
|
"Premature optimization is the root of all evil" |
|
Please rebase |
|
nevermind, done. |
New Whisper-level message introduced (PoW requirement), corresponding logic added, plus some tests.
New Whisper-level message introduced (PoW requirement), corresponding logic added, plus some tests.