Milestone 1 #1
Milestone 1 #1
Conversation
There was a problem hiding this comment.
Caveat: I haven't spent tons of time considering the coinswap proposal and its development.
From an engineering perspective, this all looks like a good start to me and appears to cover phase 1 of the development plan as proposed in the original thread. It appears to present the API as derived in the thread and everything thus far demonstrates a solid understanding of the requirements and how to go about developing them (and we don't need to remind you to write tests covering everything) so glad to have you around @scilio!
This project is in its infancy here, and I'm an advocate of not putting up a load of premature hurdles to early development, based on what I've seen so far I'd be comfortable providing scilio with write access to this repo and letting him run with it for the time being (while trying to keep everything in PRs for review purposes). It will be easier to keep up with changes that way than trying to do a mega-review on all of the proposed phase 2 work, and I wouldn't want progress slowed down waiting for formal reviews on every single change just yet.
I just have one comment for now about some apparently redundant code (there may have been a reason for it,) and I'll let @tromp evaluate the API for the time being, but I'll get into more detailed reviews as we progress. It looks as if the real meat is going to come in the next phase, and I look forward to starting to test-run a few instance of this as the project progresses!
There was a problem hiding this comment.
I think this is a great start. I've only left a few small notes rather than requests for a change.
I'm a bit weak on the cryptographic side, but I'll try to grasp the implementation concepts better as the projects goes to the next milestones. Thanks for bringing this to life!
| Ok(()) | ||
| } | ||
|
|
||
| // milestone 2 - add tests to cover invalid comsig's & inputs not in utxo set |
There was a problem hiding this comment.
Another one we'll eventually want to add is to check that we accept only 1 swap for a given input - this way we close the attack vector when someone creates two swaps for the same input commitment. Similarly, the last node will want to check that no two swaps create the same output commitment.
There was a problem hiding this comment.
Multiple occurrences of the same input or output indicates either malice, a malfunctioning wallet, or multiple wallets sharing the same seed (which can be considered a form of malfunctioning too). In this case we can either drop all but one of the spends, or drop all of them.
There was a problem hiding this comment.
I vote for dropping all of them. This way we can see the malfunctioning frequency faster because honest users will complain.
There was a problem hiding this comment.
Wallets sharing the same seed is probably too common for us to just drop all duplicate inputs. We should decide between the earliest submission or the most recent one.
Duplicate outputs does seem more like a form of maliciousness or wallet malfunction. For that, it may be better to drop both. node n wouldn't know which one was submitted first anyway.
There was a problem hiding this comment.
If you keep one of two inputs sent from two seed-sharing wallets, then the wallet whose input you dropped will malfunction anyway when its input moves in a way it doesn't understand. So I'm inclined to agree with phyro, that it's better to drop all.
There was a problem hiding this comment.
When wallets restore from a seed they have no way of knowing what the last used derivation index was (they can guess from the last output belonging to it, but it's obviously not 100%), so non-malicious duplicate outputs can occur with non-zero frequency. The wallet stores an output's MMR position (once the output is found on-chain) to disambiguate it internally in this instance. It might be possible to make MMR position an optional field here, so duplicate outputs can be dropped if they're the same and neither has an MMR position, of if they both have the same MMR position. If only one provides an MMR pos it is preferred and other dropped. (note, I need to check whether wallet stores mmr position or insertion index, I think it's position)
There was a problem hiding this comment.
what scenario do you envision in which a user is hampered by duplicate invalidation?
does it involve a user mixing within one day both before and after restore?
btw, i'd rather not complicate the protocol with optional fields.
There was a problem hiding this comment.
Suppose we have n wallets sharing the same seed w1..wn. Only one wallet will create the output, so perhaps we could say that if the output was created with wi, then it's up to wi to create the coinswap. A wallet can know whether it created an output or not unless there has been a restore, but if a restore was done, you can just create a coinswap after 25 hours or so.
| } | ||
| } | ||
|
|
||
| /// Serializes a ComSignature to and from hex |
There was a problem hiding this comment.
Not sure if really needed, but when the comsig is implemented, we could write a simple test that asserts the validity of the signature is preserved e.g. is_valid(deser(ser(sig)), pubkey, msg) still holds.
| **jsonrpc:** `2.0` | ||
| **method:** `swap` | ||
| **params:** | ||
| ``` |
There was a problem hiding this comment.
How do these params relate to the data provisioning below? I don't understand why there is a proof of ownership but no rangeproof. And what is data?
There was a problem hiding this comment.
data refers to the array of encrypted payloads Pi...Pn. Maybe payloads would be a better name.
There was a problem hiding this comment.
I had concluded that data was too short (at a mere 34 bytes) to contain a rangeproof:-(
The other fields seem to have realistic lengths.
But I agree, payloads is more descriptive.
What is the role of msg?
There was a problem hiding this comment.
True, I should've included a rangeproof in the payload. I will change to payloads and replace this example with a real one for 3 nodes. We'll see 2 of these small payloads followed by a larger one with the rangeproof included.
I wasn't sure what message should be signed for the ComSig, so I just added a msg field. I guess the Onion field could be signed instead. Thoughts?
There was a problem hiding this comment.
I'm fine with signing with the onion field as message.
| /// When asked to read too much data | ||
| #[fail(display = "TooLargeReadErr")] | ||
| TooLargeReadErr, | ||
| } |
There was a problem hiding this comment.
There should perhaps be something like a DuplicateInput and DuplicateOutput error.
There was a problem hiding this comment.
Good idea. I'll include those errors when I write the duplicate checking logic in milestone 2
There was a problem hiding this comment.
This PR seems to implement the basics as described in the first milestone. Beyond that, I don't have much to add that hasn't already been brought up in these comments.
I do have some comments about the specification itself but will keep them in the forum thread.
It's nice to have you here @scilio, looking forward to seeing your next work.
|
I've opened an issue #2 to track the minor leftovers from this discussion so we don't forget them in milestone 2. |
Contains all changes for milestone 1 of https://forum.grin.mw/t/request-for-funding-scilio-coinswap-implementation/9149?u=scilio
Fee will be included in payload as part of milestone 2.