• Alexandru Gheorghe's avatar
    Fix kusama validators getting 0 backing rewards the first session they enter the active set (#3722) · 8d0cd4ff
    Alexandru Gheorghe authored
    There is a problem in the way we update `authorithy-discovery` next keys
    and because of that nodes that enter the active set would be noticed at
    the start of the session they become active, instead of the start of the
    previous session as it was intended. This is problematic because:
    
    1. The node itself advertises its addresses on the DHT only when it
    notices it should become active on around ~10m loop, so in this case it
    would notice after it becomes active.
    2. The other nodes won't be able to detect the new nodes addresses at
    the beginning of the session, so it won't added them to the reserved
    set.
    
    With 1 + 2, we end-up in a situation where the the new node won't be
    able to properly connect to its peers because it won't be in its peers
    reserved set. Now, the nodes accept by default`MIN_GOSSIP_PEERS: usize =
    25` connections to nodes that are not in the reserved set, but given
    Kusama size(> 1000 nodes) you could easily have more than`25` new nodes
    entering the active set or simply the nodes don't have slots anymore
    because, they already have connections to peers not in the active set.
    
    In the end what the node would notice is 0 backing rewards because it
    wasn't directly connected to the peers in its backing group.
    
    ## Root-cause
    
    The flow is like this:
    1. At BAD_SESSION - 1, in `rotate_session` new nodes are added to
    QueuedKeys
    https://github.com/paritytech/polkadot-sdk/blob/02e1a7f476d7d7c67153e975ab9a1bdc02ffea12/substrate/frame/session/src/lib.rs#L609
    ```
     <QueuedKeys<T>>::put(queued_amalgamated.clone());
    <QueuedChanged<T>>::put(next_changed);
    ```
    2. AuthorityDiscovery::on_new_session is called with `changed` being the
    value of `<QueuedChanged<T>>:` at BAD_SESSION - **2** because it was
    saved before being updated
    https://github.com/paritytech/polkadot-sdk/blob/02e1a7f476d7d7c67153e975ab9a1bdc02ffea12/substrate/frame/session/src/lib.rs#L613
    3. At BAD_SESSION - 1, `AuthorityDiscovery::on_new_session` doesn't
    updated its next_keys because `changed` was false.
    4. For the entire durations of `BAD_SESSION - 1` everyone calling
    runtime api `authorities`(should return past, present and future
    authorities) won't discover the nodes that should become active .
    5. At the beginning of BAD_SESSION, all nodes discover the new nodes are
    authorities, but it is already too late because reserved_nodes are
    updated only at the beginning of the session by the `gossip-support`.
    See above why this bad.
    
    ## Fix
    Update next keys with the queued_validators at every session, not matter
    the value of `changed` this is the same way babe pallet correctly does
    it.
    https://github.com/paritytech/polkadot-sdk/blob/02e1a7f476d7d7c67153e975ab9a1bdc02ffea12/substrate/frame/babe/src/lib.rs#L655
    
    
    
    ## Notes
    
    - The issue doesn't reproduce with proof-authorities changes like
    `versi` because `changed` would always be true and `AuthorityDiscovery`
    correctly updates its next_keys every time.
    - Confirmed at session `37651` on kusama that this is exactly what it
    happens by looking at blocks with polkadot.js.
    
    ## TODO
    - [ ] Move versi on proof of stake and properly test before and after
    fix to confirm there is no other issue.
    
    ---------
    
    Signed-off-by: default avatarAlexandru Gheorghe <[email protected]>
    Co-authored-by: default avatarBastian Köcher <[email protected]>
    8d0cd4ff