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

Slate V4 (Compact Slates) #49

Merged
merged 24 commits into from
May 25, 2020

Conversation

yeastplume
Copy link
Member

@yeastplume yeastplume commented Apr 6, 2020

//"tx": null,
"amt": "1000000000",
"fee": "8000000",
"hgt": "437088",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you figure out why this was needed? I don't believe it is, or at minimum, it can probably be optional

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's needed at all, so will try removing it.

@DavidBurkett
Copy link
Contributor

It would probably be a good idea to specify the encoding used for pubkeys, commitments, and signatures.

@ghost
Copy link

ghost commented Apr 7, 2020

As discussed in chat, we could use a slate stage field that denotes what kind of slate we are dealing with (invoice, send, receive, finalize etc). The wallet GUI could display one UI 'receive' slate interface and handle which endpoint to use behind the scenes. That would improve UX flow considerably for P2P transactions.

@yeastplume
Copy link
Member Author

As discussed in chat, we could use a slate stage field that denotes what kind of slate we are dealing with (invoice, send, receive, finalize etc). The wallet GUI could display one UI 'receive' slate interface and handle which endpoint to use behind the scenes. That would improve UX flow considerably for P2P transactions.

Yes, I agree, and it will also remove a lot of awkwardness from code that tires to infer what state the slate is in. Going to add this field.

@yeastplume
Copy link
Member Author

  • Added proposed stage (sta) field and description
  • Removed hgt field
  • Added more precise descriptions of hex string fields

@yeastplume yeastplume changed the title [WIP] Compact Slates Compact Slates Apr 15, 2020
@i1skn
Copy link

i1skn commented Apr 15, 2020

Great work on this one! I especially like sta field, because now I do:
https://github.com/i1skn/ironbelly/blob/d55bcb70c52e36815f9f2380396722e112248021/src/common/index.ts#L170-L177 to identify the stage of a slate and to have this info inside a slate is a good thing!

I have a small question, why do we need to pass fee in a slate?

  • Standard flow: fee can be added between S2 and S3, right?
  • Invoices flow: fee is unknown in I1 because payee can use different amount of inputs, right? So, fee could be calculated only between I2 and I3, right?

Probably, I'm missing something here...

* `excess` - Hex-String encoded short form public key on the secp256k1 curve representing the public blind excess for the participants inputs/outputs. The first party to add signature data must also generate and add a random kernel offset to this value.
* `part` - Hex-String encoded Aggregated (Schnorr) secp2561k signature represeting the participant's partial sig. May be omitted if the participant does not yet have enough data to create it
* `nonce` - The public key of the nonce chosen by the participant for their partial signature
* `proof` - An optional payment proof requet that must be filled out by the recipient if requested (only valid for basic transaction flow). This contains:
Copy link

Choose a reason for hiding this comment

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

typo: requet

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@yeastplume
Copy link
Member Author

yeastplume commented Apr 16, 2020

Great work on this one! I especially like sta field, because now I do:
https://github.com/i1skn/ironbelly/blob/d55bcb70c52e36815f9f2380396722e112248021/src/common/index.ts#L170-L177 to identify the stage of a slate and to have this info inside a slate is a good thing!

I have a small question, why do we need to pass fee in a slate?

  • Standard flow: fee can be added between S2 and S3, right?
  • Invoices flow: fee is unknown in I1 because payee can use different amount of inputs, right? So, fee could be calculated only between I2 and I3, right?

Probably, I'm missing something here...

Fee needs to be included in the signature message, and needs to be specified by the person inserting the inputs, so it has to be specified at S1 and I2. It can be dropped at S2 since the originator knows it.

@j01tz
Copy link
Member

j01tz commented Apr 20, 2020

Looks great overall @yeastplume, this definitely makes the material clear and digestible.

A quick question about the "return" slate in the basic workflow: can we (or do we already) prune the senders xs and nonce keys in sigs?

My (possibly naive) understanding is that to finish building the transaction the only additional signature data that the sender needs from the receiver is a partial schnorr signature, a commitment to their blinding factor (a new xs), and a commitment to the nonce from the partial signature.

@yeastplume yeastplume changed the title Compact Slates Slate V4 (Compact Slates) Apr 24, 2020
@yeastplume
Copy link
Member Author

Looks great overall @yeastplume, this definitely makes the material clear and digestible.

A quick question about the "return" slate in the basic workflow: can we (or do we already) prune the senders xs and nonce keys in sigs?

My (possibly naive) understanding is that to finish building the transaction the only additional signature data that the sender needs from the receiver is a partial schnorr signature, a commitment to their blinding factor (a new xs), and a commitment to the nonce from the partial signature.

That's correct, and no we're not currently pruning the sender's xs and nonce data on the return trip and probably should. Will amend

@DavidBurkett
Copy link
Contributor

What will the flow look like for sending or receiving using a multisig wallet? Does this slate model work for that?

@yeastplume
Copy link
Member Author

What will the flow look like for sending or receiving using a multisig wallet? Does this slate model work for that?

No, this is definitely optimized for a single sender/recipient workflow at the moment. For multisig, I'd anticipate adding a new set of status stages and another set of definitions as to what fields need to be present at each stage.

@antiochp
Copy link
Member

antiochp commented Apr 28, 2020

We specify the following on the slate itself -

  • fee
  • lock_hgt

Fee makes sense but "lock height" is sort a hold over from legacy v1 (this defaults to 0 if not in use, but its not actually a field on a plain kernel, so we ignore it if 0).
It would be nice if we could specify a "kernel feature" along with associated feature specific fields.

This is related to "relative kernel locks" but a more general purpose approach here would make our live significantly easier when dealing with different kinds of kernels.

In slate.rs we do the following (basically map slate.lock_height==0 to plain kernel) -

	// Construct the appropriate kernel features based on our fee and lock_height.
	// If lock_height is 0 then its a plain kernel, otherwise its a height locked kernel.
	fn kernel_features(&self) -> KernelFeatures {
		match self.lock_height {
			0 => KernelFeatures::Plain { fee: self.fee },
			_ => KernelFeatures::HeightLocked {
				fee: self.fee,
				lock_height: self.lock_height,
			},
		}
	}

Ideally the slate contains enough information to fully represent a kernel feature.

@yeastplume
Copy link
Member Author

yeastplume commented Apr 29, 2020

We specify the following on the slate itself -

  • fee
  • lock_hgt

Fee makes sense but "lock height" is sort a hold over from legacy v1 (this defaults to 0 if not in use, but its not actually a field on a plain kernel, so we ignore it if 0).
It would be nice if we could specify a "kernel feature" along with associated feature specific fields.

This is related to "relative kernel locks" but a more general purpose approach here would make our live significantly easier when dealing with different kinds of kernels.

In slate.rs we do the following (basically map slate.lock_height==0 to plain kernel) -

	// Construct the appropriate kernel features based on our fee and lock_height.
	// If lock_height is 0 then its a plain kernel, otherwise its a height locked kernel.
	fn kernel_features(&self) -> KernelFeatures {
		match self.lock_height {
			0 => KernelFeatures::Plain { fee: self.fee },
			_ => KernelFeatures::HeightLocked {
				fee: self.fee,
				lock_height: self.lock_height,
			},
		}
	}

Ideally the slate contains enough information to fully represent a kernel feature.

How about something like this:

{
  "ver": "4:3",
  "id": "6f9c730a-1193-4123-948c-f6c1dc49e904",
  "sta": "S1",
  "amt": "60000000000",
  "fee": "4000000",
  "feats": {
    "feat" : 1,
    "lock_hgt": 3434,
  },
  "sigs": [
    {
      "xs": "AibJx7P1F3b+MYgAdk3mKkL6NhH/O2DZ65GLCSd5Ej0d",
      "nonce": "AuzSBEMitFkX2OiJSUToH4Fu/athEnsdQwDmBLBqdFiF"
    }
  ],
  "proof": {
    "saddr": "KvIz/vtzzNJdvh/p72ItytezxuU8N+OhrEsykmX3WAk=",
    "raddr": "GtKjKihZs4cbFe8UNgcPrXxXCBsYxOCvuyzsuH4RK44="
  }
}

feats.feat is the enumeration of the kernel feature
fee remains where it is (as it's likely to be universal)
If feats is not provided, the kernel is assumed to be plain (0).
feats is just a struct that contains all possible non-fee kernel feature arguments. It can be extended over-time with whatever args are needed for new features. Although the internal struct will contain all possible feature args, args not relevant to a feature can be left out of the serialization.

@lehnberg lehnberg added the wallet dev Related to wallet dev team label May 4, 2020
@yeastplume
Copy link
Member Author

Updated with more generic approach to KernelFeatures arguments

@quentinlesceller
Copy link
Member

In line with our governance process, this RFC can be considered being in Final Comment Period, with a disposition to merge in two weeks time, on May 21.

Please ensure any comments are made before then!

@quentinlesceller quentinlesceller added the in FCP Currently in Final Comment Period label May 7, 2020
Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

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

Added a few small comments, great job with this overall 🚀

"ver": "4:3",
"id": "7haE++9ZScKLOqM2jvRsjA==",
"sta": "S2",
"sigs": [
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to add new offset field here before sigs begins.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

#### Top-Level Slate Struct

* The `version_info` struct is removed, and is replaced with `ver`, which has the format "[version]:[block header version]"
* `id` becomes a short-form base-64 encoding of the UUID binary
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the UUID hex encoded rather than base64?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as a matter of that this is no longer a change, remove that line


This RFC is envasaged as a necessary first step for all slate-exchange possibilities that would benefit from compactness, e.g:

* [Slate Serialization](https://github.com/j01tz/grin-rfcs/blob/slate-serialization/text/0000-slate-serialization.md)
Copy link
Member

Choose a reason for hiding this comment

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

Can replace slate-serialization and armored-slates here with slatepack

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.

Big RFC :) A lot of work! Everything makes sense and really well written.
Few comments below. I think after they are addressed we can process with the merging.

RFC PR: https://github.com/mimblewimble/grin-rfcs/pull/49

And can you rename to 0012-compact-slates.md

text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
text/0000-compact-slates.md Outdated Show resolved Hide resolved
@lehnberg
Copy link
Contributor

@quentinlesceller is the FCP period extended?

In line with our governance process, this RFC can be considered being in Final Comment Period, with a disposition to merge in two weeks time, on May 21.

Please ensure any comments are made before then!

@quentinlesceller
Copy link
Member

@lehnberg I was waiting for @yeastplume to address the comments above. After that we can merge it.

@yeastplume
Copy link
Member Author

Thanks for reviewing, comments all addressed and rfc updated

@quentinlesceller quentinlesceller merged commit e6d4bff into mimblewimble:master May 25, 2020
@quentinlesceller
Copy link
Member

🎉 Wohooo! This RFC has now been merged! 🤸‍♀️
Tracking issue: mimblewimble/grin-wallet#317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in FCP Currently in Final Comment Period wallet dev Related to wallet dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants