This looks great! The only suggestion I have is to add an "internal" version of m.config.GetSecretKeys that caches the list of keys, just so that we are allocating a new slice every time we get a message. Should be a minor change. Thanks!

@armon Thanks, and good point. Maybe something like the updates I just pushed? Now whenever keys get modified using AddSecretKey, RemoveSecretKey, or UseSecretKey, we just order SecretKeys at that time and store it in the config structure so we can use it directly without allocating a new ordered slice.

@ryanuber I think this is going in the right approach, but there are some questionable thread safety issues in here. I think there should honestly be a private member installedKeys or something, that is protected by a mutex. Add/Remove/Install key should grab the lock, build the new slice, and just do a pointer swap on installedKeys. Internally we should only ever read the installedKeys. Otherwise we are changing the slice around while another goroutine is reading from it. Its okay for us to do the slice allocation on key changes (very rare), as long as its not in the hot loop of decoding.

Sorry to nitpick on this!

@armon no problem! You're right, I'll take another look tonight when I have more time. Thanks!

@armon, I added a mutex to the operations that read/write key data. I overhauled it a little and broke all of this stuff out into a Keyring structure which seemed more fitting than shoving it all into Config. One thing I haven't really done is test the Mutex, and I'm not sure what the right way to go about doing that is since the calls are blocking.

I also added an EncryptionEnabled bool to Config, rather than checking config.SecretKey != nil. This isn't really necessary now that I think about it so if you'd rather not have that we can just axe it.

Ok, I think I've got a decent amount of tests. @armon Maybe let's let this soak over the weekend. Let me know your thoughts whenever you have a few minutes. Thanks!

Sorry things have been busy! This is definitely the right direction. I think we can kill SecretKeys and EncryptionEnabled. We must keep SecretKey for backwards compatibility. If the secret key OR keychain is provided, encryption is enabled. Thanks! I will try to review more closely soon.

@armon no problem, how often are things not busy? No rush at all, just let me know what you think whenever you have time to do a code review on this.

I killed SecretKeys and EncryptionEnabled. So this would leave memberlist with 4 options for instantiation:

• Omitting encryption entirely
• Passing only SecretKey, which automatically gets installed as the primary key on the Keyring
• Passing only Keyring, which simply installs the provided keyring
• Passing both SecretKey and Keyring, which installs the keyring and sets the primary key to SecretKey.

Awesome work! LGTM! Thanks @ryanuber