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

N(e)RD kernels #47

Merged
merged 29 commits into from
Jun 2, 2020
Merged

N(e)RD kernels #47

merged 29 commits into from
Jun 2, 2020

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Apr 3, 2020

Link to rendered text
Tracking issue: mimblewimble/grin#3288
Supersedes #19 "relative kernels".

No Recent Duplicate (NRD) Kernels


TODO -

  • HF3 rollout proposal (how do we introduce a new kernel variant safely?)
  • NRD fees (to be decided)
  • NRD weight (consensus rules for max block weight)
  • final edits based on feedback

@lehnberg
Copy link
Contributor

lehnberg commented Apr 3, 2020

Very nice job writing this. I can't comment on the technical specification other than that it reads VERY clear. 👍

One thought I had as I was reviewing is that it might be worth while to justify the 7 day maximum (relative_height <= 10,080) a bit better, i.e. why does this cover all proposed use cases, what is the trade-off for allowing a larger maximum, and why should it not be made smaller.

@antiochp antiochp marked this pull request as ready for review April 6, 2020 15:38
@antiochp antiochp changed the title [WIP] NeRD kernels N(e)RD kernels Apr 6, 2020
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.

Still reading, I did a quick typo check first.

text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
@antiochp
Copy link
Member Author

antiochp commented Apr 7, 2020

@quentinlesceller Thanks for review so far. I'll work through these and update.

@antiochp antiochp mentioned this pull request Apr 7, 2020
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jaspervdm jaspervdm left a comment

Choose a reason for hiding this comment

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

Nice work, very elegant implementation of relative locks.

In this proposal each NRD kernel is only inhibited by the presence of another NRD kernel with the same excess. Did you consider the possibility of inhibiting by any type of kernel (of the same excess)? I am not entirely sure it would enable new usecases, but I can see it being more general in that you don't have to know at tx construction time whether or not you would like to be able to refer to its kernel in the future. The obvious downside of this is that we would have to index every kernel for a period of 2 weeks instead of only NRD kernels..

@tromp
Copy link
Contributor

tromp commented Apr 10, 2020

The only use case we identified for relative kernels where absolute ones do not suffice, is the construction of unknown-future-time transactions A and B where B spends A's output unless it get revoked in time. To justify the higher implementation costs of what I'll call NoRecentSameCommitment, we would need to think of a convincing use case not handled by NRD.
There's also the problem that the implementation cost cannot be covered by higher fees on the NRSC kernels, since the former are incurred even in the absence of the latter.

Having an unrestricted kernel for A with the same commitment as an NRD kernel for B presents an even bigger problem, in that transaction B can just swap out the NRD kernel for the A kernel and proceed without delay, possibly with cut-through in the same block.

@antiochp
Copy link
Member Author

Did you consider the possibility of inhibiting by any type of kernel (of the same excess)? I am not entirely sure it would enable new usecases, but I can see it being more general in that you don't have to know at tx construction time whether or not you would like to be able to refer to its kernel in the future. The obvious downside of this is that we would have to index every kernel for a period of 2 weeks instead of only NRD kernels..

Yes. I do like it being more general, but comes at the cost of the index needing to be general also.

We also briefly considered the inverse of this -

  • add an NRD kernel on-chain (relative lock is "forward looking")
  • any future kernel with duplicate excess needs to meet lock requirements (can be any kernel type)

@tromp argued against this in terms of kernels restricting themselves vs. other kernels.

@antiochp
Copy link
Member Author

Having an unrestricted kernel for A with the same commitment as an NRD kernel for B presents an even bigger problem, in that transaction B can just swap out the NRD kernel for the A kernel and proceed without delay, possibly with cut-through in the same block.

Yes agreed. But this seems like a more general case of the "shorten the lock by replacing the NRD kernel with another NRD kernel". Although the cut-through does appear to make this more fragile.
Cut-though would make this harder to identify after the fact, given that it would never appear on-chain.

@lehnberg lehnberg added the node dev Related to node dev team label May 4, 2020
@lehnberg lehnberg self-assigned this May 4, 2020
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
@tromp
Copy link
Contributor

tromp commented May 5, 2020

In PIBD, the result of downloading a subrange of blocks should be all the MMR data plus a list of head and tail NRD constraints, generated from the presence of NRD kernels near the start and end of the range respectively. Then, for two adjacent subranges R1 and R2, the tail NRD constraints of R1 and the head NRD constraints of R2 will need to be resolved against each other. These lists should be small so we should set the NRD kernel fee appropriately high.

@antiochp
Copy link
Member Author

antiochp commented May 5, 2020

so we should set the NRD kernel fee appropriately high.

Outputs are weighted +4, inputs -1 and kernels +1 for fee calcs.
What fee do we think NRD kernels should have?

Related - "absolute height locked kernels" are treated as regular kernels for fee calcs (and they contain an additional 8 bytes of height info.

Edit: And block weight rules need to be decided on also. (Block weight is different to fee weight calcs).

@tromp
Copy link
Contributor

tromp commented May 5, 2020

So I wanted to keep the consensus (block) weight rule simple. It should correspond to rough multiples of 32 bytes, so I think all kernels have weight 3, which is fine with me.
But for fee, we should consider a higher cost to disincentivize uncooperative privacy leaking closes.

@antiochp
Copy link
Member Author

antiochp commented May 7, 2020

So I wanted to keep the consensus (block) weight rule simple. It should correspond to rough multiples of 32 bytes, so I think all kernels have weight 3, which is fine with me.
But for fee, we should consider a higher cost to disincentivize uncooperative privacy leaking closes.

As discussed on keybase - for simplicity we are treating NRD as the same as plain kernels for both weight and fee calcs.
Plan to revisit entire fee calculation structure in a future RFC.

@lehnberg
Copy link
Contributor

lehnberg commented May 7, 2020

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!

@lehnberg lehnberg added the in FCP Currently in Final Comment Period label May 7, 2020
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
text/000x-nrd-kernels.md Outdated Show resolved Hide resolved
@lehnberg
Copy link
Contributor

@antiochp the FCP period has now expired and I'm happy to merge this as is. However, I had two optional points that you may want to consider before doing so:

  1. @tromp raised the suggestion in the last governance meeting that we may want to reference Ruben Somsen's SAS protocol for atomic swaps as a use case, and also touch upon the question of whether NRD kernels are adaptor signature compatible somewhere in the RFC?
  2. I note there's a section in the Reference level explanation that outlines details of the two currently supported serialization versions for transaction kernels. I believe this is information is already outlined in RFC#0005, is that right? If so, I'd recommend not to replicate it here, as it would make it harder to keep documentation up to date should it change in the future (there would now be two places to update, one not-obvious). A reference to RFC0005 for the behaviour might suffice here, keeping the NRD RFC focused on new functionality that's only related to NRD kernels, what do you think? Apologies, I really should have raised this in previous reviews.

@antiochp
Copy link
Member Author

antiochp commented May 24, 2020

@lehnberg

  1. It probably makes sense to reference the SAS protocol.
    I still don't have a full handle on how adaptor sigs and NRD would interact. I suspect they are not directly compatible, cannot have an NRD kernel that itself has an adaptor sig. We can always use multiple kernels in a tx, so I don't think they are necessarily incompatible.

  2. I'd like to leave this section in as we need to expand the details in RFC#0005 to cover the new kernel type and define what bytes are used in the new kernel type. I'm worried this would be harder to read if the details were split across multiple RFCs and this RFC could not be read in isolation.

@lehnberg
Copy link
Contributor

I'll extend the FCP another two weeks from its original expiration date, until Jun 02.

@antiochp
Copy link
Member Author

2. I note there's a section in the Reference level explanation that outlines details of the two currently supported serialization versions for transaction kernels. I believe this is information is already outlined in RFC#0005, is that right? If so, I'd recommend not to replicate it here, as it would make it harder to keep documentation up to date should it change in the future (there would now be two places to update, one not-obvious). A reference to RFC0005 for the behaviour might suffice here, keeping the NRD RFC focused on new functionality that's only related to NRD kernels, what do you think? Apologies, I really should have raised this in previous reviews.

Changed my position on this. I added a section to describe NRD serialization with "fixed size kernels" and reworked the "variable size kernels" section to refer to RFC-0005 as you suggested. I think this is clearer now. 👍

@lehnberg lehnberg merged commit 2d62f3e into mimblewimble:master Jun 2, 2020
@lehnberg
Copy link
Contributor

lehnberg commented Jun 2, 2020

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

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 node dev Related to node dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants