Always use flexible identifier selection

[cwgoes]

Closes #491

[ancazamfir]

Not sure about this part. We still want to abort if we have a previous (which means desiredIdentifier != "" and we found a connection with that id) that doesn't match the counterpartyPrefix, clientIdentifier and counterpartyClientIdentifier. So this part would be almost the same as before.

Suggested change

	// use the provided identifier only if the handshake can progress with it
	if ((previous === null) ||
	!(previous.state === INIT &&
	previous.counterpartyConnectionIdentifier === "" &&
	previous.counterpartyPrefix === counterpartyPrefix &&
	previous.clientIdentifier === clientIdentifier &&
	previous.counterpartyClientIdentifier === counterpartyClientIdentifier)) {
	identifier = generateIdentifier()
	} else {
	identifier = desiredIdentifier
	}
	abortTransactionUnless(
	(previous === null) ||
	(previous.state === INIT &&
	previous.counterpartyConnectionIdentifier === "" && // unless there's some state corruption this should always be true since connOpenInit always stores ""
	previous.counterpartyPrefix === counterpartyPrefix &&
	previous.clientIdentifier === clientIdentifier &&
	previous.counterpartyClientIdentifier === counterpartyClientIdentifier))
	// we are here either with null previous or one stored under desiredIdentifier.
	// previous can be null in two cases:
	// 1. desiredIdentifier === "" or
	// 2. desiredIdentifier != "" and there is no stored connection with that id.
	// We only want to generate the identifier in case 1
	if (previous === null) && (desiredIdentifier === "") {
	identifier = generateIdentifier()
	} else {
	identifier = desiredIdentifier
	}

[cwgoes]

Hmm, I think this is up to us. If we find a previous connection with the identifier the relayer is trying to use, but the fields mismatch, we cannot continue the handshake with that identifier - so we can either abort, or pick a new identifier and continue the handshake (with the fields as requested by the relayer). I think both are safe, I guess it's a question of whether we want to throw an explicit error or continue but without the requested identifier.

I figured that there might be a second relayer racing the first to try to cause its transaction to fail and so it might be nice to continue (but with a different identifier), what do you think? Are there disadvantages of doing so?

[ancazamfir]

I see, good point! Looking over a few relayer types:

A - a relayer similar to the current Go relayer where client, connection and channel IDs are statically configured, ID-s having a "mandatory" meaning.
The relayer could init both ends (send OpenInit to each chain) and ensure the transactions are successful. If they are then OpenTry is guaranteed to succeed. If not, it will just stop relaying. In this case we don't need this feature. It could also OpenInit one end only and then figure out after OpenTry that the configured ID cannot be used.

B - a relayer without much configuration except client ids and, maybe (*), one end of the connection. In general it tries to relay everything.
This relayer would OpenInit one end and then send OpenTry with empty desired connection id. Then just continue the handshake regardless of the id that was selected. Again in this case we won't need the feature.

C - somewhere in the middle there is one relayer that would benefit from your proposal. This relayer has the IDs configured with the "desired" meaning, i.e. could continue to relay if the OpenTry succeeds with a different connection ID. The relayer determines the new connection id and adjust its configuration.

(*) Maybe useful here to also allow OpenInit with empty identifier. Or generate one if desired is taken. Also, could the clients be created with empty client ids (relayer would need a chain-id in the CreateClient event to figure out what is going on). Just trying to minimize the configuration for a B relayer.

[colin-axner]

I realize I implemented this doing partly abort, partly pick a new identifier.

If the previous state existed but the fields mismatch, the go implementation would currently abort. I did this because I imagine proof validation would likely fail, for example if the counterparty client id didn't match.

If the previous state didn't exist, I had the handshake continue with a generated identifier. I have no preferences here, but it was requested during code review to return an error instead. see comment

[AdityaSripal]

I'm not sure it makes sense for us to just create a new channel or connection ID in this case.

As I see it there are two cases:

1. The relayer submits a TRY msg with an empty connection/channel ID. This signals to the chain that the relayer intends to create a new channel/connection that doesn't already exist on our chain. This makes sense to me.

2. The second case is when the relayer submits a TRY msg with a non-empty connection/channel ID. In my view, this is a signal to the chain that the relayer wants to update the state of an already existing connection/channel on our chain. In that case, it is not appropriate to just continue with a new id. Rather, we should return an error if the proposed update is invalid.
   I don't think we really need to worry about race conditions as much with handshakes. IBC Channels and Connections are common goods (at the moment, at least) so it doesn't matter who created them. If a relayer submits a TRY msg before me, I don't really care. I just want to know that information so I can relay the ack back or whatever is needed for the handshake to complete.
   If a competing relayer submits an INIT msg such that my TRY msg fields mismatch, then I could simply resubmit without a filled-in connection/channel ID if that is what I really want to do. I'm not sure we should be doing that automatically here.

[cwgoes]

Right, I agree that we should give the option of what to do in this case to the relayer, as @AdityaSripal proposes, meaning that:

• If the relayer submits an empty identifier, the chain will pick the next available identifier and the handshake will continue
• If the relayer submits a specific identifier, and the proofs succeed, the handshake will continue
• If the relayer submits a specific identifier, and the proofs fail, the handshake will abort and an error will be returned

The tricky bit is that always picking an identifier can mean that we may leave "garbage" (partially completed handshakes) around in the case of crossing-hellos when we might not have needed to. One option is to have the relayer provide a boolean to indicate the behaviour they prefer (whether or not to pick a new identifier on proof-mismatch and continue).

[cwgoes]

However, I don't really like the idea of adding even more complexity to this handshake step and this would be a non-protocol-breaking change, so I think we can do the simpler thing for now (as you propose @ancazamfir) and add this case-C handling later on if it ends up becoming necessary.

I'll make an issue just to track - https://github.com/cosmos/ics/issues/499

[cwgoes]

Just a note - I think we can simplify this implementation a bit since previous should always be null if the proposed identifier is "" since nothing can be stored under "" - so we can just generate the identifier at the start of the function, which seems a bit cleaner to me.

[colin-axner]

Just to make sure I follow, it's entirely possible after open init on chainA, that chainB creates multiple connections or channels connected to that connection or channel open init on chainA. The step that determines which handshake will be successful will be open ack?

[cwgoes]

Just to make sure I follow, it's entirely possible after open init on chainA, that chainB creates multiple connections or channels connected to that connection or channel open init on chainA. The step that determines which handshake will be successful will be open ack?

That is possible, correct. However, it should not happen e.g. in the presence of only one correct relayer.

[cwgoes]

@ancazamfir If you could take a quick look at the revised diff and let me know if it all seems in order I'd be very thankful 🙏!

[colin-axner]

LGTM

[colin-axner]

we cannot do this since the code cannot distinguish between a relayer supplied identifier and a generated identifier without extra logic. I'd suggest changing the if to be more so:

if desiredIdentifier != "" {
    // check that previous connection exists
    // check that previous connection fields match
} else {
    identifier = generateIdentifier()
}

[colin-axner]

I'd also recommend changing desiredIdentifier to previousIdentifier

[colin-axner]

see implementation changes

[ancazamfir]

Yes, I think this if (desiredIdentifier === "") { block should move further down, after all validations are done and were a previous has been determined. Also, this will prevent an identifier to be generated if checks below fail.
(see #493 (comment))

[cwgoes]

we cannot do this since the code cannot distinguish between a relayer supplied identifier and a generated identifier without extra logic

Hmm, why would we need to do this? Empty-string is not a valid identifier, it's a sentinel value.

see implementation changes

I think this structure is fine too, we can update the spec to reflect it if you think it's cleaner, it looks cleaner to me too.

I'd also recommend changing desiredIdentifier to previousIdentifier

Sure.

Yes, I think this if (desiredIdentifier === "") { block should move further down, after all validations are done and were a previous has been determined. Also, this will prevent an identifier to be generated if checks below fail.
(see #493 (comment))

It doesn't matter if an identifier is generated and checks later fail, because the transaction is atomic.

[colin-axner]

Hmm, why would we need to do this? Empty-string is not a valid identifier, it's a sentinel value.

The case I'm thinking of is if the relayer passed in myownconnectionid as the connection identifier (a connection that doesn't exist), then the previous would equal null and pass all the checks and continue with the handshake.

[cwgoes]

I've made changes to reflect your suggestion in 43bbbb3 @colin-axner

[cwgoes]

The case I'm thinking of is if the relayer passed in myownconnectionid as the connection identifier (a connection that doesn't exist), then the previous would equal null and pass all the checks and continue with the handshake.

Ah, I see - yes, that's right.

[cwgoes]

(we could also change the validation functions but your proposal is cleaner)

[ancazamfir]

It doesn't matter if an identifier is generated and checks later fail, because the transaction is atomic.

True. Just looked over the latest changes, lgtm.

[cwgoes]

@colin-axner Any further comments? Does this LGTM from your side? Just double-checking.

[colin-axner]

LGTM, just one thing that I think differs with the implementation