narwhal icon indicating copy to clipboard operation
narwhal copied to clipboard

Make network interface info optional

Open arun-koshy opened this issue 2 years ago • 4 comments

Fixes Issue#539

arun-koshy avatar Jul 27 '22 09:07 arun-koshy

  1. I would expect a change to the .proto, to make it explicit to callers they do not have to provide this information, is that possible?

Proto3 fields are optional by default (i.e. generated code for existing PrimaryAddresses proto message). They recently added back the optional keyword but its used for field presence for basic types. I think convention is to not put the explicit optional keyword in the proto, but I don't mind adding it or a comment if thats best for our clients.

Is there a reasonable cutoff point where we can push an Option futher into the backend (ideally until the Committee) ?

Refactored the backend with optional primary addresses.

arun-koshy avatar Jul 29 '22 00:07 arun-koshy

So I do think we want to make things optional, but I think we actually want to make the worker addresses optional over the primary's. We should always know concretely who the validators are and at least 1 address for them but we should be completely removing all the worker information out of the committee structure.

bmwill avatar Aug 01 '22 15:08 bmwill

Ok I did read the linked issue and I see the intent....maybe we instead want to completely remove all network info from the committee structure?

bmwill avatar Aug 01 '22 15:08 bmwill

How do we fix this? The simplest way I can think of is to modify the ReliableNetwork trait (at least), and modify the broadcast function there so that we wait, before the actual sending, until we have at least (2f+1) addresses in the Committee.

@huitseeker this is what I plan on doing. First thing I thought about doing was passing the Committee into the ReliableNetwork broadcast method instead of a list of addresses so that we could calculate if the appropriate total stake has been reached but realized that this would have to be special cased for both the WorkerNetwork and the PrimaryNetwork. So I think it makes sense to remove the default trait impl for reliable broadcast and then implement the trait in both the worker & primary case separately but with a shared trait interface that looks like this

async fn broadcast(
        &mut self,
        committee: Committee,
        worker_cache: SharedWorkerCache,
        message: &Self::Message,
    ) -> Vec<CancelOnDropHandler<MessageResult>>

The difference would be the worker impl would retrieve the worker addresses and the primary impl would retrieve the primary addresses but they would both retry the same way until we have atleast (2f+1) addresses.

The worker broadcast seems like it will work fine however the issue I am seeing for the primary broadcast is that without a SharedCommittee I am not sure how we can properly retry until we have (2f+1) addresses as the committee that is passed in will never change. Do we want to switch to using a SharedCommittee or do we have to wait for a NewEpoch or a UpdateCommittee signal?

arun-koshy avatar Aug 24 '22 21:08 arun-koshy

Let's re-open this in https://github.com/MystenLabs/sui if it's still relevant

akichidis avatar Oct 17 '22 13:10 akichidis