renproject / aw

A flexible P2P networking library for upgradable distributed systems.
MIT License
38 stars 18 forks source link

Sync message always gets denied when pulling #59

Closed tok-kkk closed 3 years ago

tok-kkk commented 3 years ago

The Gossiper.didReceivePush() function will deny the content id in the filter after the context is done. https://github.com/renproject/aw/blob/27ccfc3bf6e42e58328bafc71802d139da40f6a3/peer/gossip.go#L104-L112

But the context is defer cancelled and will be cancelled after finishing sending the pull message

https://github.com/renproject/aw/blob/27ccfc3bf6e42e58328bafc71802d139da40f6a3/peer/gossip.go#L86-L87 This causes the incoming sync message will always be rejected.

rahulghangas commented 3 years ago

Fix pushed as part of PR 56

jazg commented 3 years ago

Just tested this out and it still seems to be causing problems. Is this conditional in the DidReceiveMessage method correct?

https://github.com/renproject/aw/blob/ac3835b6789ad7260e74dce6be88cf9bdedbb2db/peer/gossip.go#L70-L72

Based on the usage of Filter inside the syncer, I think it might need to be:

if !g.filter.Filter(from, msg) { ... }
rahulghangas commented 3 years ago

I think the this is right, Filter should returns true if the content should be filtered, that is, should be blocked. However, the logic in Filter itself is slightly incorrect and should something like the following

func (f *SyncFilter) Filter(from id.Signatory, msg wire.Msg) bool {
    if msg.Type != wire.MsgTypeSync {
        return true
    }

    f.expectingMu.RLock()
    defer f.expectingMu.RUnlock()

    _, ok := f.expecting[string(msg.Data)]
    if !ok {
        return true
    }
        return false
}

--EDIT

and the call in DidReceiveMessage in sync.go should be changed to

if syncer.filter.Filter(from, msg) {
    return fmt.Errorf("denied message from %v", from)
}
jazg commented 3 years ago

@rahulghangas That makes sense!

rahulghangas commented 3 years ago

@jazg I've pushed the changes with a basic test (it doesn't do anything, just shows that gossip works). Can you double-check on your end and subsequently close this issue if it's resolved

jazg commented 3 years ago

@rahulghangas Works on my end!