Bug: not updating view of peer burnchain state appropriately

[pavitthrap]

The following logic in chat.rs updates the stored stats we have for our view of a peer's burnchain state:

                // update chain view from preamble
                if msg.preamble.burn_block_height > self.burnchain_tip_height {
                    self.burnchain_tip_height = msg.preamble.burn_block_height;
                    self.burnchain_tip_burn_header_hash = msg.preamble.burn_block_hash.clone();
                }

                if msg.preamble.burn_stable_block_height > self.burnchain_stable_tip_height {
                    self.burnchain_stable_tip_height = msg.preamble.burn_stable_block_height;
                    self.burnchain_stable_tip_burn_header_hash =
                        msg.preamble.burn_stable_block_hash.clone();
                }

We only update our view if the sent burn heights are strictly greater than the ones we have stored for that peer. Doing so fails to account for re-orgs in the underlying chain (bitcoin).

I plan to make the height vars of type Option<u64>, so that we can properly distinguish between the var being unset and the var being equal to 0.

@jcnelson @kantai thoughts?

[jcnelson]

It's not a bug. Two burnchain blocks with the same height are concurrent -- neither one is definitively the chain tip, so it doesn't matter which one gets reported as the remote chaintip view. If you want, you can update the code to check both burn_block_height and burn_block_hash (and burn_stable_block_height and burn_stable_block_hash), but it won't meaningfully change the way the code behaves.

[kantai]

It's not a bug. Two burnchain blocks with the same height are concurrent -- neither one is definitively the chain tip, so it doesn't matter which one gets reported as the remote chaintip view. If you want, you can update the code to check both burn_block_height and burn_block_hash (and burn_stable_block_height and burn_stable_block_hash), but it won't meaningfully change the way the code behaves.

But a peer's burn tip can regress -- in the event of PoX re-orgs.

[jcnelson]

So, the only code that cares about the burn block height and burn stable block height is the inventory state machine, which uses them as a hint to determine which block inventories it can ask for. If the the remote peer undergoes a PoX reorg, but the local peer asks for a block inventory for a now-invalid list of sortitions, the local peer will just get a NACK, which it interprets as the remote peer being on a different PoX fork. This is acceptable, because it causes the local peer to stop asking for later block inventories and to try again later.

We could update this code path to simply reset the burn block heights to whatever is in the preamble, and it probably won't change anything, but we'd want to test it live first.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jcnelson]

Circling back to this, I think it might be sufficient to just remove the conditional check. The remote peer's burnchain view is just whatever it claims it is.

[kantai]

It seems like you probably have the most context for this, so I'm adding you to the assignees, @jcnelson