mycognosist / solar

A minimal Secure Scuttlebutt replication node.
Other
22 stars 2 forks source link

Improve sharing of configuration context #64

Open mycognosist opened 1 year ago

mycognosist commented 1 year ago

Right now we have a clumsy hybrid of configuration values which are passed down to actors and other functions (starting in src/node.rs) and those which are set and retrieved from OnceCells.

The OnceCell pattern generally makes it easier to deal with the situation where we have to pass a value into an actor in a loop. Generally these values do not implement the Copy trait and are therefore only able to be moved once before the compiler complains:

^^^^^^^ value moved here, in previous iteration of loop

The following values are currently set via OnceCell:

// NETWORK_KEY
config.network.key
// PEERS_TO_REPLICATE
config.replication.peers
// RESYNC_CONFIG
config.replication.resync
// SECRET_CONFIG
config.secret

One option is simply to chuck the whole ApplicationConfig instance into a OnceCell and avoid passing down config values entirely. Such a "global store" is generally discouraged in Rust.

Another option is to create a context which can be shared with all actors. Each actor is then able to access config values from the context.

mycognosist commented 1 year ago

This could be tackled after https://github.com/mycognosist/solar/issues/63 is complete.

mycognosist commented 1 year ago

Another option is to create a context which can be shared with all actors. Each actor is then able to access config values from the context.

This is the approach taken by aquadoggo : https://github.com/p2panda/aquadoggo/blob/main/aquadoggo/src/context.rs

The ServiceManager (equivalent of the broker in the solar context) holds the context object and passes it into each new service upon creation : https://github.com/p2panda/aquadoggo/blob/main/aquadoggo/src/manager.rs#L94

mycognosist commented 1 year ago

Something to consider: we may want mutability for some aspects of the shared state. I'm thinking in particular about an address book, the likes of which currently lives in the ReplicationConfig as the peers field. The peers field could be updated to only contain a HashSet of public keys (these are the peers with whom we wish to replication). A separate peer_addresses field could exist in the NetworkConfig as a HashMap (or similar type) of public key -> address mappings. The peer_addresses would need to be mutable so that addresses discovered during node operation can be added and queried.

Solution: use an Arc<Mutex<_>> only on the fields which require mutability (not the whole context object). For example:

https://github.com/p2panda/aquadoggo/blob/c47cfee3f7c5bda120d9c049d92e0f8a1b19d2c5/aquadoggo/src/schema/schema_provider.rs#L12-L22