transport: Fix double lock and state overwrite on disconnected peers

[lexnv]

This PR fixes an issue where the secondary connection is rejected because it has a different connection ID.

The issue comes from fn dial_address which can overwrite the state of a disconnected peer with in-flight dial requests.

High level repro case

• T0: dial_address 1
• T1: dial_address 2 which overwrites address 1
• T2: dialed address 1 responds with a connection
• T3: connection is rejected

While at it, this PR also:

• removes the need to double lock the peer state (once to read and once to write)
• enforces connection ID checks on dial records in on_dial_failure
• adjust the repro case from transport: Reject secondary connections with different connection IDs #176 for the fix

Detailed repro case

• T0: Dial address A connection ID 1 -> State : Dialing { in-flight-1 }
• T1: Connection is established by remote address C connection ID 3 -> State : Connected { in-flight-dial 1 }
• T2: Closed connection established in T2 -> State : Disconnected { in-flight-dial 1}

// This is where the state was wrongfully overwritten

• T3: Dial address B connection ID 2 -> State: Dialing { in-flight-2 }

• T4: Connection is established by remote address C connection ID 3 -> State : Connected { in-flight-dial 2 }

// The issue was detected here

• T5: Dial from T0 receives a response however it expects in-flight-dial 1 and the state has in-flight-dial 2

Closes: #172

[lexnv]

Believe there might be another path that triggers #172, will see after some more triaging

[dmitry-markin]

Isn't this triggered if we dial two or more addresses simultaneously? Basically, the case when we know more than one address of a peer and employ dialing parallelism?

[lexnv]

Thinking out loud, I remember the state machine being similar to:

fn dial_addr (single_address) -> PeerState::Dialing { record: Some( single_address) } 

fn dial () -> this is where we employ the parallel dial. All dial records share the same connection ID
   -> PeerState::Opening { records, connection_id, ...transports } 
   
              fn dial() -> multiple parallel dials (max 8)
Disconnected  ---------------------------------> Opening 
  
              on_open_sucesss: negotiate + cancel other dials
Opening   -----> Dialing { record: first successful record }
              if other dial attempts succeed before canceling, then we return InvalidState (and just log it)
              on_open_failure: lose track of the dial
Opening ------> Opening
    

That points me towards adding a nice state machine and documenting these states.
It gets complicated by just looking at the code and we can easily forget it.

A few other things that I could address here:

• remove debug_assert from on_open_failure on_open_sucess; since we can race with the transports
• a dial failure can come later (same as above) and on_dial_failure we have a todo if we are in Opening

[dmitry-markin]

You are right, looked into the sources and now understand I was missing we replace the address record with the first succeeded record.

[dmitry-markin]

May be this is what you mean already by this TODO, but for me it looks like we have to store multiple records at least in PeerState::Dialing state to correctly handle concurrent dials (do we dial 8 addresses in parallel?) and track all connection_ids. This way the debug_assert!s in the on_dial_failure() shouldn't be triggered.

May be it's slightly more complicated than this, as we have to track what dials succeeded and drop the extra connections when there is more than two of them, so more bookkeeping might be needed.

// However, the record score cannot be updated in the future.

Can you also elaborate on this?

[lexnv]

I think the Opening state should handle this, but let me double check

Yeaa, the comment is wrong it should be "The record score can be updated in the future" :D
Basically, we can keep track of this potential address in the future, instead of discarding it. So if we ever try to fn dial() 8 addresses, this address can be one of the dialed ones

[dmitry-markin]

(Actually wanted just to comment, but misclicked Approve. Don't merge the PR yet, please.)

[dmitry-markin]

Looks good! (This time the approval was intentional.)

[dmitry-markin]

How do we check for the unknown connection id being rejected below? Or we don't have the issue of getting the unknown connection id after this PR?

[lexnv]

Indeed the edge-case that goes from those states is fixed now, I've added another test to check we reject connections from a theoretical state.

We might still have some other gaps, the state machine is quite complex

[lexnv]

Few followups from my side:

• document the state machine
• fix the Opening todo! panic on dial failure