-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
whisper: PoW requirement #15701
whisper: PoW requirement #15701
Changes from 7 commits
fac0300
4bd592a
8b96630
1501a11
8e6fe2c
0fec3e7
c6f32a8
04353a8
f78a54c
45bfeb8
4c26af9
f3582d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
crand "crypto/rand" | ||
"crypto/sha256" | ||
"fmt" | ||
"math" | ||
"runtime" | ||
"sync" | ||
"time" | ||
|
@@ -30,6 +31,7 @@ import ( | |
"github.com/ethereum/go-ethereum/crypto" | ||
"github.com/ethereum/go-ethereum/log" | ||
"github.com/ethereum/go-ethereum/p2p" | ||
"github.com/ethereum/go-ethereum/rlp" | ||
"github.com/ethereum/go-ethereum/rpc" | ||
"github.com/syndtr/goleveldb/leveldb/errors" | ||
"golang.org/x/crypto/pbkdf2" | ||
|
@@ -74,6 +76,8 @@ type Whisper struct { | |
|
||
settings syncmap.Map // holds configuration settings that can be dynamically changed | ||
|
||
reactionAllowance int // maximum time in seconds allowed to process the whisper-related messages | ||
|
||
statsMu sync.Mutex // guard stats | ||
stats Statistics // Statistics of whisper node | ||
|
||
|
@@ -87,14 +91,15 @@ func New(cfg *Config) *Whisper { | |
} | ||
|
||
whisper := &Whisper{ | ||
privateKeys: make(map[string]*ecdsa.PrivateKey), | ||
symKeys: make(map[string][]byte), | ||
envelopes: make(map[common.Hash]*Envelope), | ||
expirations: make(map[uint32]*set.SetNonTS), | ||
peers: make(map[*Peer]struct{}), | ||
messageQueue: make(chan *Envelope, messageQueueLimit), | ||
p2pMsgQueue: make(chan *Envelope, messageQueueLimit), | ||
quit: make(chan struct{}), | ||
privateKeys: make(map[string]*ecdsa.PrivateKey), | ||
symKeys: make(map[string][]byte), | ||
envelopes: make(map[common.Hash]*Envelope), | ||
expirations: make(map[uint32]*set.SetNonTS), | ||
peers: make(map[*Peer]struct{}), | ||
messageQueue: make(chan *Envelope, messageQueueLimit), | ||
p2pMsgQueue: make(chan *Envelope, messageQueueLimit), | ||
quit: make(chan struct{}), | ||
reactionAllowance: SynchAllowance, | ||
} | ||
|
||
whisper.filters = NewFilters(whisper) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would make more sense to Mock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean, i should create another function, e.g. SetMinimumPowTest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, have a |
||
if val <= 0.0 { | ||
return fmt.Errorf("invalid PoW: %f", val) | ||
} | ||
w.settings.Store(minPowIdx, val) | ||
|
||
w.notifyPeersAboutPowRequirementChange(val) | ||
|
||
if testMode { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment / uncomment error :) |
||
time.Sleep(time.Duration(w.reactionAllowance) * time.Second) | ||
w.settings.Store(minPowIdx, val) | ||
}() | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (w *Whisper) notifyPeersAboutPowRequirementChange(pow float64) { | ||
w.peerMu.Lock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
defer w.peerMu.Unlock() | ||
for p := range w.peers { | ||
err := p.notifyAboutPowRequirementChange(pow) | ||
if err != nil { | ||
// allow one retry | ||
err = p.notifyAboutPowRequirementChange(pow) | ||
} | ||
if err != nil { | ||
fmt.Errorf("Error sending PoW notification to peer [%x]: %s", p.ID(), err) | ||
} | ||
} | ||
} | ||
|
||
// getPeer retrieves peer by ID | ||
func (w *Whisper) getPeer(peerID []byte) (*Peer, error) { | ||
w.peerMu.Lock() | ||
|
@@ -233,7 +265,7 @@ func (w *Whisper) SendP2PMessage(peerID []byte, envelope *Envelope) error { | |
|
||
// SendP2PDirect sends a peer-to-peer message to a specific peer. | ||
func (w *Whisper) SendP2PDirect(peer *Peer, envelope *Envelope) error { | ||
return p2p.Send(peer.ws, p2pCode, envelope) | ||
return p2p.Send(peer.ws, p2pMessageCode, envelope) | ||
} | ||
|
||
// NewKeyPair generates a new cryptographic identity for the client, and injects | ||
|
@@ -528,7 +560,22 @@ func (wh *Whisper) runMessageLoop(p *Peer, rw p2p.MsgReadWriter) error { | |
if cached { | ||
p.mark(&envelope) | ||
} | ||
case p2pCode: | ||
case powRequirementCode: | ||
s := rlp.NewStream(packet.Payload, uint64(packet.Size)) | ||
i, err := s.Uint() | ||
if err != nil { | ||
log.Warn("failed to decode powRequirementCode message, peer will be disconnected", "peer", p.peer.ID(), "err", err) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And negative/zero values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zero allowed, but not negative There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, I misread what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, no I didn't. You also need to make sure that the PoW is positive |
||
log.Warn("invalid value in powRequirementCode message, peer will be disconnected", "peer", p.peer.ID(), "err", err) | ||
return errors.New("invalid value in powRequirementCode message") | ||
} | ||
p.powRequirement = float64(f) | ||
case bloomFilterExCode: | ||
// to be implemented | ||
case p2pMessageCode: | ||
// peer-to-peer message, sent directly to peer bypassing PoW checks, etc. | ||
// this message is not supposed to be forwarded to other peers, and | ||
// therefore might not satisfy the PoW, expiry and other requirements. | ||
|
@@ -591,7 +638,10 @@ func (wh *Whisper) add(envelope *Envelope) (bool, error) { | |
|
||
if envelope.PoW() < wh.MinPow() { | ||
log.Debug("envelope with low PoW dropped", "PoW", envelope.PoW(), "hash", envelope.Hash().Hex()) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. once the status message includes the PoW requirement, an error should be returned here |
||
//return false, fmt.Errorf("envelope with low PoW dropped: PoW=%f, hash=[%v]", envelope.PoW(), envelope.Hash().Hex()) | ||
} | ||
|
||
hash := envelope.Hash() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.