feat: only set nodes as active after state sync #319
Conversation
| NodeStatus::Syncing | ||
| // TODO: (Find out in review) Is my own ID also part of this list? |
There was a problem hiding this comment.
I saw in other places that participants can include the own ID. Do we need to worry about this here? We wouldn't want to trigger a sync with ourselves... @ChaoticTempest
There was a problem hiding this comment.
yep, the mesh connections contains us, so we need to add a check against participant == me here
There was a problem hiding this comment.
Interesting, so in the pool we also maintain a NodeConnection to self? Shouldn't we filter that when we spawn the tasks?
There was a problem hiding this comment.
so right now, Mesh doesn't have a notion of me: Participant, so unable to filter out that connection right now. We could try adding it in via the NodeStateWatcher but likely won't behave well with initial KeyGen so I avoided adding it for now. Sync at least has the me info, so we can filter it there
There was a problem hiding this comment.
Okay, I think I have this covered now with a check in the sync task, right inside the spawned task of broadcast_sync. It's a bit ugly but in turn we keep to things simple in this PR, I guess.
See discussion here: sig-net#319 (comment)
Yes, good point. I noticed that too when I wrote this PR. I think it should be easy to add them after this is merged. But anyway, as long as they don't respond back with a list of what they still have from our owned presignatures / tripples, this isn't actually providing much value. The main thing we can prevent by waiting for sync to finish is weird race conditions in cases where sync removes data in just the wrong moment. |
|
I see this warning many times while running integrations tests:
from here: mpc/chain-signatures/node/src/protocol/message/mod.rs Lines 961 to 966 in 0e22123 Should I worry about that? @ChaoticTempest |
|
shouldn't be too big of a concern. Those messages should only pop up during keygen. It's because some nodes haven't started their keygen yet, and one node has fully reached the point where it has started. The message is a bit of a misnomer because keygen uses all participants regardless if they're active or not |
|
Hm but there are some test failures. Not sure if I find time before next week to look into them. |
|
I'll take a look and try to fix it |
|
ahh, it's because of keygen again. Since now we have |
683575b to
440b34e
Compare
|
Thanks @ChaoticTempest for finding this! I changed it back to the old behaviour but one test still fails |
|
that one is a bit flaky sometime. Let me rerun the CI to see |
|
looks like this change affects resharing when there's latency. Still looking into why, but going from resharing to running makes the test fail where we don't have enough stable participants to generate a signature |
|
I think the issue with this is that now we have a race condition where some nodes observe stable to be different than other nodes meaning some nodes are synced faster than others. And then when they have their own version of So I don't think we can add this in until we have consensus with Init messages then |
|
@ChaoticTempest that is what I was talking about at our last meeting. Only proposer should decide who is stable and can participate in the protocol. All other protocol participants should blindly participate (or accept protocol start message) without checking of proposer is a stable connection. |
|
talked with @volovyks offline to clarify, but what he was proposing is that we deterministically choose proposer on the list of all participants instead of just The issue with the queue of proposers is that it creates a way for a participant to eventually be proposer and look ahead of time which retry count where they are a proposer. This might open up an attack where a rogue participant can hijack and be the one to publish a signature, so I don't think the queue works well here. Also some more context on the issue I observed, we immediately try to do generation on the next list of stable participants resharing, but since sync is happening in the background the list of |
|
Thanks @ChaoticTempest !
It believe this race condition is not new, it just now gets caught by the tests due to additional latency. Overall, we must be able to deal with cases where a part of the network is unstable and different nodes having a different view on that.
Sounds good to me.
@volovyks Yes, agreed. Whether the proposer is stable or not shouldn't change our decision to participate. We should primarily only check that the request is valid (indexed and final).
@ChaoticTempest I see what you mean. But the protocol is not byzantine fault tolerant at the moment and I don't think we are ready to tackle that, yet. Let's focus on something that's reliable under nice condition first. But you are right, this might become a problem in the future. We will need a byzantine-tolerant proposer election at some point.
I think we should start with:
Hopefully this makes things reliable enough that we can have different views on the network per node without it breaking down. Otherwise, we will have to search deeper. |
@ChaoticTempest Yeah but only when the proposer selects the list of participants. Everyone else must trust the list sent by the proposer and not interfere with it in any way. |
This introduces NodeState::Syncing per connection on the mesh level. The SyncTask will take peers in this state and prioritize syncing them. After it was synced, this is reported back to the mesh using a message channel. This PR tries to introduce this new state without changing too much architecture and code. I'm not super clear about it yet, how exactly the code would be refactored best. For now, this seems to be good enough to me but I am very much open to ideas.
440b34e to
f0055d2
Compare
feat: make only inactive => active be syncable
This introduces NodeState::Syncing per connection on the mesh level.
The SyncTask will take peers in this state and prioritize syncing them. After it was synced, this is reported back to the mesh using a message channel.
This PR tries to introduce this new state without changing too much architecture and code. I'm not super clear about it yet, how exactly the code would be refactored best. For now, this seems to be good enough to me but I am very much open to ideas.