Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix v2 conversion to ensure we provide blocks in correct format #3465

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Oct 7, 2020

Testing on testnet (was floonet) uncovered an issue in our protocol version handling when providing blocks to v2 peers.

I think we see this on testnet because there are relatively few nodes in sync.
This does not appear to be happening on mainnet but I suspect this is simply masked by the fact we have better connectivity and more peers available.

The following error occurs when attempting to provide a full block to a v2 peer -

20201007 14:54:15.459 DEBUG grin_p2p::conn - try_break: exit the loop: Serialization(UnsupportedProtocolVersion)

This is because we store v3 CommitOnly blocks in the local db and the bug resulted in us not converting the block into v2 compatible format prior to the serialization attempt. We do not have "input features" and cannot serialize successfully.

  • We need to do the conversion if the peer is v2.
  • For v3 peers and above we do not need to convert.

The bug had this logic flipped.


This would most obviously manifest itself if a 4.0.0 node attempted to sync against a number of 4.1.0 nodes.
Given 4.1.0 is the latest binary this is unlikely to occur. I think this is why we did not see this on mainnet and we are only seeing it on testnet.

@DavidBurkett
Copy link
Contributor

Given 4.1.0 is the latest binary this is unlikely to occur.

It will occur for new Grin++ and Niffler nodes though, because Niffler is still 4.0.0 IIRC, and Grin++ won't be v3 for a few more months. So it would be nice to include this if we have a patch release going out soon.

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Good catch 👍

@antiochp
Copy link
Member Author

antiochp commented Oct 8, 2020

Given 4.1.0 is the latest binary this is unlikely to occur.

It will occur for new Grin++ and Niffler nodes though, because Niffler is still 4.0.0 IIRC, and Grin++ won't be v3 for a few more months. So it would be nice to include this if we have a patch release going out soon.

Yes. Planning to get a patch release out today to include this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants