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

Kernels v2 (variable size) #3034

Merged
merged 8 commits into from
Sep 19, 2019
Merged

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Sep 10, 2019

RFC (Variable Size Kernels) is here -

Tracking issue is here -

Resolves #3010.

TODO -

  • fix version negotiation min(our_version, peer_version)

Support for kernel serialization/deserialization with protocol version 2 ("variable size kernels").

Protocol version 1 preserves backward compatibility -

  • always serialize/deserialize bytes for fee and lock_height
  • always verify these are 0 if unused

Protocol version 2 introduces flexibility -

  • only serialize/deserialize fee if applicable for kernel variant
  • only serialize/deserialize lock_height if applicable for kernel variant

When writing for the purposes of hashing we use protocol version 1 to maintain backward compatibility. We can only change the serialization strategy for kernel in a HF and not for existing kernels.

These changes only apply to the p2p layer currently - nodes will negotiate protocol version during Hand/Shake and only use v2 if both peers support this.

The local db will continue to use v1 for full backward compatibility. This will be tackled in a separate PR with support for local db migration etc.

The txhashset MMR files will continue to use v1 for full backward compatibility (both on disk and for txhashet.zip archive file during fast sync). See #3011 for separate tracking issue for this.


TODO (Testing)

p2p messages -

  • v1 -> v1
  • v1 -> v2
  • v2 -> v1
  • v2 -> v2

@antiochp antiochp self-assigned this Sep 10, 2019
@antiochp antiochp added this to the 2.1.0 milestone Sep 10, 2019
@antiochp antiochp changed the title Kernels v2 Kernels v2 (variable size) Sep 10, 2019
@antiochp antiochp marked this pull request as ready for review September 11, 2019 12:20
@antiochp antiochp requested a review from hashmap September 17, 2019 11:41
@antiochp antiochp mentioned this pull request Sep 17, 2019
@antiochp
Copy link
Member Author

Ok I think this works but I don't think the "version negotiation" is particularly robust.

If we are on version 1 and our peer is on version 2 then we will simply try to send and receive using version 2 (what they told us via hand/shake).
And conversely our peer will try to send and receive using version 1 (what we told them via hand/shake).

Which is fine as long as we are on version 1 and do not know how to serialize/deserialize version 2.
If we tell our peer we use version 1 but we actually know how to serialize version 2 then we cause problems because our peer expects version 1 messages but we send them version 2 messages.

We get away with this because we do not know anything about version 2 and simply treat version 2 the same as version 1 (version>=1). This (version>=1) is necessary to support backward compatibility on the network as versions increase. If a node joins the network that support version 99 then everyone else will simply attempt to serialize/deserialize using their current (highest known) version.

I think the version negotiation should always negotiate min(peer_version, our_version) to cover this case where we use a codebase supporting version 2 but configured to only support version 1.

tl;dr I skipped the actual version negotiation because right now nodes have a hardcoded version and we made (a possibly incorrect) assumption that this would never be modified to conflict with the versions actually supported in the codebase).

@tromp
Copy link
Contributor

tromp commented Sep 19, 2019

If we are on version 1 and our peer is on version 2 then we will simply try to send and receive using version 2 (what they told us via hand/shake).

If we are on version 1, then surely we do all sending and receiving in version 1, which is the only thing we know how to do?!

Copy link
Contributor

@tromp tromp left a comment

Choose a reason for hiding this comment

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

Can kernel_sig_msg now be rewritten in terms of the hash of write_v2 ?

@antiochp
Copy link
Member Author

If we are on version 1, then surely we do all sending and receiving in version 1, which is the only thing we know how to do?!

Yes, that was poorly phrased by me. The way it was implemented we would ask our serializer to serialize using v2, not know about v2 and use v1 which is the highest we know about (and presumably the most compatible with v2).
So we do all sending and receiving using version 1 but only as a result of not knowing anything about v2.

This initial impl was very implicit and there is a hole there that needs plugged. We may know about v2 but only advertise support for v1.

@antiochp
Copy link
Member Author

antiochp commented Sep 19, 2019

Can kernel_sig_msg now be rewritten in terms of the hash of write_v2 ?

👍

Oh I believe it can yes - let me think that through a bit more and confirm. We hash tuples when building the sig msg but the tuple container itself is transparent in terms of the data actually being hashed, the data in the tuple is just concatenated together.
Probably makes sense to rewrite that as a separate PR to keep things clean.

Edit: Its tricky to hook into this though. Currently we have 2 different writers - the BinWriter and the HashWriter and we use the HashWriter for hashing things.
We always want to use v1 when hashing tx kernels except when hashing them for the kernel_sig_msg when we want to use v2.
And hashing has no context around this, we just call x.hash().

We need to think this through a bit more.

@antiochp antiochp merged commit bc6108c into mimblewimble:master Sep 19, 2019
@antiochp antiochp deleted the kernels_v2 branch September 19, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable size kernels: Initial protocol version support for p2p messages
3 participants