~~proof of reserves,~~ proof of a transfer#799
~~proof of reserves,~~ proof of a transfer#799skaunov wants to merge 15 commits intoNeptune-Crypto:masterfrom
Conversation
aszepieniec
left a comment
There was a problem hiding this comment.
I left some comments inline, but I think those superficial comments are subordinate to high-level directional feedback which follows here.
Please explain how the CLI commands ought to work. What information do they take, and what information do they produce? How these CLI commands work will establish constraints that the tasm program must satisfy.
I'm thinking out loud here. What happens if you (re-)derive a perturbed RemovalRecord. Essentialy a different AsboluteIndexSet derived the same way except for also including the block hash in the input. Then your proof certifies the claim "this PerturnedAsboluteIndexSet tailored to block X, corresponds to a UTXO in the UTXO set at block X (so in particular it was confirmed earlier and has not been spent yet), amounts to Y Neptune coins with release date Z." So the claim is (W: PerturbedAsboluteIndexSet, X: BloclHeight, Y: NativeCurrencyAmount, Z: Timestamp).
With this construction, you can combine (claim, proof) pairs. What you care about is that all the proofs are valid, the block heights are identical, and all the PerturbedAsboluteIndexSets are unique.
If that's the goal, then the CLI commands probably look like something like this:
neptune-cli prove-reservestakes
- a UTXO index
- a block height
- an output file name
and writes the (claim, proof) pair to the given file.
neptune-cli verify-reservestakes
- an input file name or list of them
and verifies each proof, verifies that allPerturbedAbsoluteIndexSets are unique, and outputs one line per file containing the amount and, if any, release date.
| /// to parallelization. | ||
| /// | ||
| /// # running | ||
| /// Correct me if I'm wrong: for proving a new Neptune CLI command should be added. |
There was a problem hiding this comment.
/// Correct me if I'm wrong: for proving a new Neptune CLI command should be added.
Yes that makes sense.
| /// # running | ||
| /// Correct me if I'm wrong: for proving a new Neptune CLI command should be added. | ||
| /// # checks out of Triton | ||
| /// - AOCL must be from the block of interest |
There was a problem hiding this comment.
/// - AOCL must be from the block of interest
More precisely: the AdditionRecord must live in the AOCL of the block of interest.
There was a problem hiding this comment.
that's proven and the AOCL digest is output, so a verifier must find the digest in a canonical block
| /// | ||
| /// # running | ||
| /// Correct me if I'm wrong: for proving a new Neptune CLI command should be added. | ||
| /// # checks out of Triton |
There was a problem hiding this comment.
Checks outside of Triton VM, meaning that the verifier verifies the proof and some extra data. Doesn't this extra data not leak privacy? For instance, if you need to include the AdditionRecords, then that leaks exactly when the reserves were obtained.
There was a problem hiding this comment.
That's why it keeps these private. X) And the prover can prove for the latest block AOCL which is relevant to the task in hand.
In a sense a good proof of the reserves requires to leak some privacy. Like it makes sense to leak the lock post-image. So the question is where to draw the line. I start the current version with the comment on the choice I present here.
| NativeCurrencyAmount, | ||
| ), | ||
| } | ||
| impl crate::protocol::proof_abstractions::tasm::program::ConsensusProgram for PublicData { |
There was a problem hiding this comment.
Consider naming the program something more specific than PublicData. Just about every proof type used in NeptuneCash has some kind of public data.
| crate::util_types::mutator_set::ms_membership_proof::MsMembershipProof, | ||
| >::default(), | ||
| )); // TODO | ||
| let payload = triton_asm! { |
There was a problem hiding this comment.
What is the stack signature of this blob of tasm code?
There was a problem hiding this comment.
Oh, I had this question! How do you denote the empty stack? Jan Ferdinand told me recently the stack is never empty, but I feel like that doesn't impact a stack signature.
There was a problem hiding this comment.
There are always at least 16 elements on the stack. Any less -- crash!
If the stack contains nothing interesting, you can denote it with // _.
|
|
||
| /* finish of the things from the RR integrity file | ||
| ____________________________ */ | ||
|
|
There was a problem hiding this comment.
Please add a stack signature.
| /* pasted '8.' from <removal_records_integrity.rs> | ||
| ============== */ | ||
| /* 8. */ |
There was a problem hiding this comment.
But what does "8." do? Please summarize.
If significant chunks of tasm code is being copied, consider factoring it out into a separate blob or even BasicSnippet. In this case, an appropriate name for the blob or snippet will obviate the need for explanatory comment.
There was a problem hiding this comment.
It's not certain that subsections of RemovalRecordsIntegrity can be factored out into a BasicSnippet without changing the function signature of RemovalRecordsIntegrity, which would be a hard-fork (which we certainly don't want!).
Maybe something can be factored out into a block of code but even that is not certain, as the relevant chunk of code in RemovalRecordsIntegrity might depend on a very specific stack setup, or statically-allocated memory addresses.
|
I'm not sure I entirely grasped your idea. My design goal here was enabling someone with just the verified proof and an explorer to watch the proof is still valid. I mean they can even set a notification in such an explorer on when the UTXO backing the reserves is spent. And I don't yet understand how to achieve this with that approach. Another problem seem to re-arise here is that one can reuse the UTXO in the different blocks.
What would be the benefits to combine it? Why would someone want to put their resources on that? I can only think of putting a proof on-chain, but that implies an use-case where it should be proven recursively. I can think of that, but it feels so much in the future yet.
|
That is quite different from what I propose. Can you be more explicit? How to represent the claim and what does that sound like in human words? In my proposal, the proof is tailored to a block. And so if a new block is found, you have no guarantees about the reserves any more -- they could have been spent and you would not know. Your proposal requires publishing the
I don't think this is correct. The proof is tailored to a block. Different block -> proof is invalid. Different proof -> |
I'm thinking that exchanges typically sit on thousands of UTXOs. If they have to produce one proof covering all UTXOs, that might be prohibitively expensive -- unless if you use recursion to combine multiple smaller proofs ;-) And putting the proofs on chain is not the only application. They could publish the proof on their website. Or make smart contracts that are capable of verifying them -- in this case the proofs do not need to be on-chain because they can be non-deterministic input to the smart contract. |
|
Thank you, I expected that this thing will take some discussion and am glad to look at the thing from different aspects! As I tried my best to address below the concerns which were noted I finally could formulate what was bugging me! I believe without disclosing the RR it should be called a proof of historical reserve since each time a verifier receives the proof he should expect that a tx spending this reserve is already in the mempool. Could you paint a use-case, pls. At least what resources an exchange would need for this on their thousands of the UTXO. Like I'm thinking about an exchange which doesn't do mining and doesn't want to pay a lot for proving but publishes their reserves to attract the users. They say: sorry we can't afford to prove every block so we do it less often (idk every other block, or once a day). Which leave different ways to reuse the funds to make the proofs for amounts more than the coins holding.
Absolutely, just guide me with questions, pls -- I tried to do that in that explanation already. X) I might be wrong and am not insisting but here's my reasoning. A verifier don't want the proof to be invalidated by a new block and to have this window of uncertainty / leap of faith between discovery of a new block (even not a canonical!) in the network and receiving the proof for that block. A prover hardly appreciate the cost of proving for each block too. I mean what the problem (what's exposed) when the RR are published for the reserves: the public will know the date the UTXO existed already, the public will track when the UTXO is spent, the amount of the UTXO, and the address unlocking it. To me it looks like exactly what you want when checking someone reserves, so I'd call it 'selective disclosure' instead of privacy compromisation (as the consequences are controlled, confined, and wanted). Also it feels like a smaller hurdle to prepare the UTXO for this than making a proof per block.
Correct me if I'm wrong: you're still speaking here about one proof for the whole publication amount? My point that currently it makes more sense to produce a proof per the UTXO. And I thought it would be actually cheaper to do. As some UTXO get spent they can add more proofs for the fresher UTXO, and not immediately but as they see fit to show their reserves. Providing them all from a website is what I had in mind. Also per an UTXO approach allows smaller guys to make the proves for other than an exchange purposes (and cheaper proving and not needing to do it for each block makes the difference for a smaller setup). I thought about using in a smart contract too, but with the current state of the smart contracts making the UTXO for it seems very feasible work-around.
|
I think that's a fine name stressing the important downside. Let's use this name going forward.
If the hypothetical exchange is sitting on 600 UTXOs, and if the time cost to produce one proof of historical reserve is 1 second, then the exchange cannot feasibly prove all reserves with every block. Unless they parallelize this task but even then the cost grows rather quickly. "It's too expensive so we do it less often -- once a week." All good, except: that defeats the purpose because no-body in their right mind is going to accept a one-block-old proof of historical reserves, let alone a one-week-old one. What they can do instead is put up a bond, say 10% of the claimed reserves, into a smart contract that releases it to a challenger if the the exchange fails to produce a full proof of historical reserves within, say, one day. The challenger has to pay to activate the challenge. As long as no-one challenges, the exchange can save on the cost of proving. And if they do challenge, then the challenge fee can pay for the cost of producing the proofs.
I think you make a compelling case for having durable proofs of reserves -- proofs that don't expire unless identifiable removal records are spent. But I do think there is a trade-off; the question is whether the cost of the information leakage is worth the benefits. Clearly, if proving were free and instantaneous, then there would be no benefit.
The public will know:
I'm thinking that the first three are unavoidable for durable proofs of reserves. By making a single proof over all UTXOs in custody, you could publish only a lower bound on the total amount. Except of course: that single proof will expire once any one of the individual UTXOs is spent.
Yes. If you aggregate proofs, you improve privacy. That holds for both proofs of historical reserves as well as durable proofs of reserves. For durable proofs of reserves aggregation has the downside of making them more prone to expiry, thus defeating the purpose to some degree. |
|
Yes! Let's allow this to sit for a few days to think which is the better way/path to take now. (Perspectively both can just co-exist.) Meanwhile I can just start to really look into a transfer proof which probably just doesn't has such public/private tug. P.s. I thought I've added those privacy/anonymity sets reduction that publishing of a RR does, but I thought they're quite insignificant and ultimately forgot to edit this in. I mean that Monero approach seems to be working here: if an user shared his like viewing key but doesn't want to show the transfer he just includes the intermediate tx to a burner address, which restores all the properties (especially when it's not back-to-back, which seems to have even less effect in Neptune). |
aszepieniec
left a comment
There was a problem hiding this comment.
For proof-of-payment, the claim should have the following format.
- program digest: hash of proof-of-payment program
- input:
- UTXO
- sender randomness
- receiver digest
- aocl leaf index
- block digest
- output: (empty)
Including the entire UTXO means that this proof-of-payment mechanism works for other tokens beyond NativeCurrency, in addition to NativeCurrency coins. Moreover, in this case you do not need to take explicit care of a potential TimeLock. Lastly, note that the lock script hash is already part of the UTXO.
I'm afraid I cannot make sense of how the proof-of-reserves is supposed to work. Could you write a draft RPC endpoint? That ought to be enough that ought to be enough to reverse engineer my way through.
Make sure to add tests for the programs. Both positive tests and negative tests -- the latter ones trigger specific assert codes.
| } else { | ||
| Err(todo![]) | ||
| } |
There was a problem hiding this comment.
A let-else pattern instead of an if-let-else pattern avoids needless indentation and puts the failure actions close to the failure condition.
There was a problem hiding this comment.
I don't know how to avoid return with it, so I use it only for procedural/effects things, as return takes much more effort/attention to reason about the types compared to identation and code folding. If that's possible, it would be magnificient if you could show/teach me how to employ let _ else without return.
And you're absolutely right on everything else: just added ? as a better, more idiomatic, and types-friendly solution to that!
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct The(WitnessMemory, Claim); |
There was a problem hiding this comment.
Please avoid overly generic struct names.
There was a problem hiding this comment.
I mostly covered this in the other comments. 1) Just give it a better name. 2) In the example/RPC it doesn't even come up explicitly. I don't remember if I was already saying this: I was thinking to promote it the module up, but then the data and the logic in the different files which is sad. And here I just want to communicate that it's the only structure of the module which is not really important per se, like "focus on the other items in the module."
| @@ -0,0 +1,2 @@ | |||
| pub mod reserves; | |||
| pub mod sent; | |||
There was a problem hiding this comment.
I would suggest that "proof of payment" or even just "payment" is a better description of what this module is about.
| /// `receiver_digest`; | ||
| /// `release_date` ([add `MINING_REWARD_TIME_LOCK_PERIOD`](https://github.com/Neptune-Crypto/neptune-core/blob/5c1c6ef2ca1e282a05c7dc5300e742c92758fbfb/neptune-core/src/protocol/consensus/type_scripts/native_currency.rs#L365)) | ||
| input: (Digest, Timestamp), | ||
| /// `lock_postimage` of the address; | ||
| /// the record to check if the reserve is still unspent; | ||
| /// AOCL digest; | ||
| /// the 'reserve' | ||
| /// the timelocked 'reserve' | ||
| output: ( | ||
| Digest, | ||
| crate::util_types::mutator_set::removal_record::RemovalRecord, | ||
| Digest, | ||
| NativeCurrencyAmount, | ||
| NativeCurrencyAmount, | ||
| ), |
There was a problem hiding this comment.
These types are too complex. Please separate into many fields and implement convenience functions input() and output().
For tests it seems I'd need a wallet with an outgoing tx. I glanced over the tests which |
|
I'm constraining an UTXO by its (I was a bit surprised to see that (Also, it was a surprise the basic snippet takes a pointer to the element next to the SI.) I'm out of ideas how to further fix this, so your advice would be very helpful! |
Unfortunately, I don't know where to find this one.
I don't think this is correct. The size-indicator does take into account fields with a statically known size. Are you sure you are not looking at the size-indicator for
I'm not sure what the problem is; can you describe in more elaborate terms please? |
I discovered it with https://github.com/skaunov/neptune-core/blob/16038b1e79469d1c5180a781af328ee3d0145ea4/neptune-core/src/application/util_proof/sent/tests.rs#L135: length is obviously 18 but the first element which is SI is 13. |
|
Size-indicator value of 13 suggests to me something like this layout.
So I do think the size indicator you're looking at is the size indicator of the field |
I remembered this imprecise -- I thought just to hit each assertion. But a LLM reminded me there are often error ids (sadly I'm out of the LLM tokens), and I guess that's what you meant. If so, my first question is how do I choose a new error code value/number? |
|
The document Feel free to define your own error codes and add to this file. |
|
Thank you! An unexpected format tbh. I can't stop thinking why it's not a Rust file which would be included in the docs. |
@aszepieniec , could you give a review again. Proof of a transfer progressed a lot and I'd like to get directions on finishing it further; especially yours suggestions that I don't feel enough to press resolve but they need another look, a discussion, or an OK. Meanwhile I'll |
I'm not sure if https://github.com/skaunov/neptune-core/tree/reserves_sketch is what you meant -- pls guide if not. But it's to demonstrate the idea: fast verification and fast proving thanks to a proof per an UTXO. The down thing is it discloses the removal record in a claim, but I still argue it's not really an issue since the owner always can choose to restore his funds privacy just by sending it himself. Then the reserves will be not proved anymore -- a verifier would see the removal record on the chain. For huge numbers of the UTXO this design is actually better then a compression via succinctness as a prover only needs to make the new proofs/arguments only for the spent UTXO and they're so much easier than a proof over few UTXO. And doesn't need to redo the proof(s) for each new block. It only worse for putting such a proof on-chain. But 1) that would be a different story, 2) can be worked around via putting the reserves into a single UTXO. |
aszepieniec
left a comment
There was a problem hiding this comment.
Please use meaningful struct names, such as e.g., ProofOfPayment and ProofOfPaymentWitness. I cannot orient myself without these markers. Once you've applied that refactor I will scrutinize the tasm code. The spec looks correct at this point.
Please add documentation:
- as docstrings on the API functions, including an example
- as a separate .md file in the mdbook.
I think the thing you want to achieve is a little under-specified and you need to make some choices. There may be other questions but on my mind is this one: disclose the UTXO or not? If not, then I see no reason to disclose the sender randomness. Also, if not, then I don't think allowing any other coins but one NativeCurrency is a good idea -- how else will the recipient prove knowledge of the UTXO when they try to spend it? But if yes, then you need to decide over which channel to send the UTXO (as it does not have to be the program's input or output -- the hash of the UTXO suffices there).
In general I am concerned that the recipient of a payment that comes with a proof of payment, can still be incapable of spending the "received" UTXO. I wonder if there is a unit test or property of the source code that can convince me or other readers that this nightmare can never happen.
| /// A [`NeptuneProof`] will argument that the tx UTXO (determined by the indices, see below) transferred | ||
| /// the `amount` (it discloses) of [`NativeCurrency`] to the address with the components it discloses | ||
| /// (`receiver_digest` & `lock_postimange`). The UTXO is binded by hashing `sender_randomness` which serves for | ||
| /// identification between the similar transfers. | ||
| /// | ||
| /// The data is taken from the wallet of this node. `tx_ix` & `utxo_ix` are indicies for getting the UTXO you | ||
| /// want to prove from the wallet. | ||
| /// | ||
| /// The `block` is needed to prove the UTXO was mined. It's determined by its [`Digest`], the relevant data | ||
| /// is taken from this node DB. During verification the same data will be pulled from the same block. | ||
| /// | ||
| /// A verifier should use [`super::super::super::protocol::proof_abstractions::verifier::verify`] exposed on his API/RPC, or use the [TUI](https://github.com/TritonVM/triton-tui). | ||
| /// | ||
| /// # Panics | ||
| /// The implementation detail is when `tx_ix` is out of its bound it crashes the node until | ||
| /// <https://github.com/Neptune-Crypto/neptune-core/issues/816> is done. |
There was a problem hiding this comment.
I appreciate this docstring but it's not perfectly clear to me yet. Let me try to rephrase and then you can correct where I made the wrong guess.
Produce a proof of payment.
The result is a [`NeptuneProof`] that certifies that a given transaction output transfers the given
amount of [`NativeCurrency`] to the identified receiving address. The amount, the addition record,
the sender randomness, and the `receiver_digest` and `lock_postimage` (which identify the
receiving address) are disclosed by the claim. Besides verifying this proof against the claim, the
verifier must also test membership of the given addition record in the AOCL.
It is a zero-knowledge proof of knowledge revolving around a [`Utxo`] (not disclosed) that hashes
to the given addition record using the sender randomness and receiver digest. The UTXO in
question has the given `lock_script_hash` and k > 1 [`Coin`]s of type script hash [`NativeCurrency`]
whose states are valid positive amounts and sum to the given amount.
Also:
It is not clear to me from this description whether the UTXO is disclosed or not. It strikes me that there might be a problem if the UTXO is not disclosed: I produce a proof of payment for a UTXO that contains, besides the NativeCurrency coin of the right amount, some other coin. The recipient has no way of spending this UTXO. One way to get around this obstacle would be to prove instead that the UTXO has only one coin, which is the NativeCurrency coin with the given value.
I don't get this sentence:
The UTXO is binded by hashing
sender_randomnesswhich serves for identification between the similar transfers.
If I read correctly, this function does 3 things that I probably would have written as separate functions:
- Gather the data that the prover needs.
- Produce the proof.
- Produce the claim.
There was a problem hiding this comment.
Also, please add a doctest example how to use prove in conjunction with verify.
There was a problem hiding this comment.
a doctest
I spent some time thinking about this when working on the earlier an example suggestion. As it needs a mined spent tx, two addresses, and the wallet --- could you show me anything similar in the code, I'd build off something existing, especially the testing/mocking utils.
another Coin
I remember we had the discussion on this like two months ago after you explained me the mechanism in general on the Discourse. (I don't remember where was the discussion - on IRC, or Tg, or there, or here. 😵💫) I left with the feeling I raised this concern and we concluded it's not an issue at this stage. Like a thing which should be added when this simpler approach/iteration works (in the wild). I think adding an issue on it when merged is the best choice.
The rephrase preformat...
...seems to be more like a summary of the code, but that's just me. Coin can be single. But generally no objects for it instead the docs I wrote.
Two items mentioned are not disclosed!
- The sender randomness digest is disclosed which serves just to be able to distinguish the proofs/arguments with the same parameters. Does that helps with the quoted sentence?
- The addition record is only proved AFAIR.
The three things
are there indeed. I'd make it a post-merge issue just because I'd separate it not without fine types as all the interactions are not that general/reusable, but introducing the types now could restrict the next proof features designs. I mean it would make sense to do that basing on the needs and hopefully more similar modules to structure.
| let sent = sent::new( | ||
| sent::claim_outputs( | ||
| sent::claim_inputs( | ||
| tasm_lib::triton_vm::proof::Claim::new(sent::hash()), | ||
| tx_output.receiver_digest(), | ||
| // TODO this can be developed further | ||
| utxo.release_date().unwrap_or_default(), | ||
| ), | ||
| sender_randomness.hash(), | ||
| // aocl_digest, | ||
| utxo.lock_script_hash(), | ||
| tx_output.native_currency_amount(), | ||
| ), | ||
| block_aocl, | ||
| sender_randomness, | ||
| aocl_leaf_ix, | ||
| utxo, | ||
| aocl_membership_proof, | ||
| ); |
There was a problem hiding this comment.
This code block gives me no clue about the type of the variable sent. Furthermore, since "sent" is a very generic word, I have no idea what it does or is supposed to do.
There was a problem hiding this comment.
Reading ahead slightly, I guess that the type of sent is a struct that implements ConsensusProgram. Is that right? If so, why not call StructName::new explicitly?
There was a problem hiding this comment.
Because I'm bad at naming entities. I'm glad to put a better name on this.
I found it more natural to use the module name, but am not insisting at all.
|
|
||
| // TODO tune `cases` but the single speeds up the development | ||
| #[test_strategy::proptest(async = "tokio", cases = 1, rng_seed = RngSeed::Fixed(0))] | ||
| async fn property_test_happy_path( |
There was a problem hiding this comment.
Please describe the property that you are testing in the name of the test. The motivation is so that you can write
cargo test test_name
and run only one test without triggering a whole bunch of pattern matches.
|
|
||
| // Consolidated negative test: AOCL proof verification failure. | ||
| #[test_strategy::proptest(async = "tokio", cases = 1, rng_seed = RngSeed::Fixed(0))] | ||
| async fn aocl_proof_verification_failed( |
There was a problem hiding this comment.
Good job! This test function has the right template. Please add one for every assert statement in the specification / tasm code of the TritonProgram.
There was a problem hiding this comment.
It's the only one left. 🤷 Other potential assertions were basically pushed out to be the public outputs to be checked outside of Triton. (There was another one which left the yet commented out error code, but it has its own thread here.)
The path is cleared: the names are added. 🏎️ |
aszepieniec
left a comment
There was a problem hiding this comment.
Proof of Payment
General
Module name: -> proof_of_payment
A proof of payment spans one UTXO. Not obvious, but after reflecting I think it is the right choice.
A proof-of-payment discloses:
- the (?) release date
- the receiver digest
- the lock script hash
- the hash of the sender randomness
- the AOCL hash
- the (?) amount
In regards to (1.) and (6.) the question mark indicates that the use of the word "the" is poorly defined because there may be more than one TimeLock or NativeCurrency Coin. What if the UTXO contains multiple Timelocks or multiple NativeCurrency Coins? If the proof covers only the first TimeLock then you might be exposing the recipient to an attack whereby the proof-of-payment resolves the dispute in his disfavor (meaning he was paid) but he cannot spend the UTXO because the second time-lock has not expired yet. Likewise, if there is a second NativeCurrency Coin, that the recipient is not knowledgeable of, then he cannot spend the UTXO.
I see two ways to get around this problem:
- Disallow UTXOs that have more than one
TimeLock, more than oneNativeCurrencyCoins; and disallow any otherCoins. I would expect there to be a documented standard along with an implementation for deriving the UTXO from the lock script hash, the release date (if any), and the amount. I don't think I saw it by I may have missed it. - Alternatively, you allow arbitrary UTXOs and disclose the entire UTXO. The TASM program should be modified to iterate over all the
NativeCurrencyCoins to take their sum; and over all theTimeLockCoins to take their latest release date. While this solution is more extendable, the problem of proving payment with an unspendable UTXO persists: what if an unknown type script hash is included? There would still need to be some sort of standard stipulating which type scripts are allowed and which are not; and we would moreover have to keep updating that standard whenever anything changes. So all in all I do prefer option one.
In relation to (4.), what's the motivation for disclosing the hash of the sender randomness and not the sender randomness itself? I'm afraid it can lead to the same situation where you receive a proof-of-payment that shows that you were paid, but you can't spend the UTXO because you don't know the sender randomness.
If that motivation is to protect privacy, then you need a data structure that is separate from the claim to contain the raw sender randomness. In case of dispute, you need to transmit this data structure anyway. In this case you might as well include the release date and amount and AOCL leaf index in this other data structure .... so that in the end the proof is only relative to the receiver digest, lock script hash, and AOCL hash.
I think it's fine to go for a non-privacy-preserving proof-of-payment for now. In this case I do not see why the sender randomness should be hashed.
With respect to field ProofOfTransferWitness::membership_proof I was confused because membership proof can also refer to mutator set membership proof. I would suggest disambiguating: aocl_membership_proof.
Spec
About the time lock: I noticed that the source is not doing anything with the _time_locked variable. (And I think the tasm code does not do anything with this information either.) So as far as the proof goes, a UTXO without time-lock behaves the same way as a UTXO whose time-lock has expired, or one whose time-lock is still active. If the intention is to ignore them, then why look at the TimeLock coins to begin with? Otherwise, how about extending the Claim with extra data in the input, a timestamp? Then the Claim effectively says, "You own a UTXO that is spendable at this point in time."
IMO the variable name ram is confusing. The witness happens to live in non-deterministically initialized RAM, but but for the subsequent logic it does not matter where that witness comes from. Moreover, RAM can be used for other things besides storing witnesses. I would suggest changing it to proof_of_payment_witness.
Tasm
While reviewing I find myself inserting a lot of comments describing the stack. FYI, more such comments would make reading and reviewing the code faster.
There is a discrepancy between the spec and the tasm code. The first thing the spec reads is the release date. The first thing the tasm code reads is the receiver digest.
Even if the spec and the tasm code do the same things, if they do them in a different order then it is very difficult to verify that they match. Please ensure as best as possible that the two implementations agree on the sequence of events.
Please add tests showing that the tasm code and rust source agree in input-output behavior.
Proof of Reserves
Applies to a single UTXO. Well motivated.
Discloses the removal record. Well motivated. If privacy becomes an issue, we can think of improving a working protocol to fix this issue.
Disclosed:
- the lock script hash. Why?
- the receiver digest. Why?
- hash of absolute index set. This is even better than disclosing the raw removal record because the prover retains privacy until the UTXO in question is spent.
- the AOCL hash
- a release date
I think (1.) and (2.) can and probably should not be disclosed. But I'm willing to entertain a counter-argument ;-)
What's the specific responsibility of payload? It lives next to main, which does a lot of things. There must be a reason why it was separated out. Does the stack continue when jumping from the end of payload to the start of main?
Please rename WitnessMemory to something more specific, like e.g. ProofOfReservesWitness. And add the suffix Memory if there is another witness with largely the same function but that is too expensive to put into memory.
About WitnessMemory::utxo_digest -- why include this field? Just hash the UTXO and you're doing this anyway.
I might be missing it but I don't see any code that panics unless the prover is capable of satisfying the lock script. Without this feature the proof-of-reserves certifies "these coins exist somewhere" and not "these coins are being custodied by me".
Please add an implementation of ConsensusProgramSpecification so that there is a source function to cross-reference against, as an aid to parse what's going on in the tasm.
Please add error IDs to the assert statements so that we can test that the right one is being triggered in negative tests.
Please add tests. Positive tests, negative tests targeting every error id or assert statement, and tests that verify that the tasm code agrees with the rust source.
| // _ *coins | ||
| addi 0 read_mem 1 addi 1 | ||
| addi 1 | ||
| // _ SI_coins *coins[0]_si |
There was a problem hiding this comment.
Unless I am mistaken, memory is laid out as
size_indicator length_indicator [element 0] [element 1] ...
where every element itself has a variable length and therefore comes prepended with a size indicator of its own.
And so the stack evolves as
| stack | instruction |
|---|---|
_ *coins |
|
addi 0 |
|
_ *coins |
|
read_mem 1 addi 1 |
|
_ coins_si *coins |
|
addi 1 |
|
_ coins_si *coins_si |
which disagrees with the comment. I might be wrong -- please double-check.
Also note that you can collapse two consecutive addi instructions.
There was a problem hiding this comment.
I'm ok to just trust you! X) Would be nice to have a piece of documentation on length_indicator anyway to start learning about it; I could not find any yet.
What do you mean by coins_si? I thought that *coins and *coins_si is the same thing, so if I used that previously I took an effort to unify it to *coins IIRC.
Is length_indicator takes a single field element, so should last addi be just one bigger?
There was a problem hiding this comment.
The trait and derive macro for BFieldCodec are defined in twenty-first. I'm afraid there is no documentation but there are examples. And in the worst case you can BFieldCodec-encode an object and inspect the resulting words.
The length_indicator is only used for lists where the number of elements is not known at compile time. It is a single word -- you guessed it --indicating the number of elements in the list.
The difference between *coins_si and *coins is subtle because of nested data structures. They both contain the same address. The second one contains the address of the first word in the data object coins. This first word happens to coincide with the size indicator for that object. That size indicator is itself a data object called coins_si and *coins_si points to its first word. Obviously the two pointers' values are identical. The difference between *coins_si and coins_si is the difference between the pointer and the value pointed to.
| push {FIRST_NON_DETERMINISTICALLY_INITIALIZED_MEMORY_ADDRESS} | ||
| {&rustfield_membershipproof} | ||
| {&rustfield_aoclleafindex} | ||
| read_mem {u64_stack_size} |
There was a problem hiding this comment.
| read_mem {u64_stack_size} | |
| addi {u64_stack_size - 1} read_mem {u64_stack_size} |
| // *aocl *aocl_peaks [canonical_commitment] [num_leafs] [aocl_leaf_index] *auth_path | ||
| swap 2 | ||
| // *aocl *aocl_peaks [canonical_commitment] *auth_path [aocl_leaf_index] [num_leafs] | ||
| swap 3 | ||
| // *aocl *aocl_peaks [num_leafs] *auth_path [aocl_leaf_index] [canonical_commitment] | ||
| swap 1 | ||
| // *aocl *aocl_peaks [num_leafs] *auth_path [canonical_commitment] [aocl_leaf_index] | ||
| swap 2 | ||
| // *aocl *aocl_peaks [num_leafs] [aocl_leaf_index] [canonical_commitment] *auth_path |
There was a problem hiding this comment.
I don't think this will work. Instruction swap n swaps the BFieldElement on top of the stack with the one at index n. The objects [num_leafs] and [aocl_leaf_index] are pairs of BFieldElements, representing u64s.
| push {FIRST_NON_DETERMINISTICALLY_INITIALIZED_MEMORY_ADDRESS} | ||
| {&rustfield_membershipproof} | ||
| {&rustfield_aoclleafindex} | ||
| read_mem {u64_stack_size} | ||
| pop 1 | ||
| // *aocl [aocl_leaf_index] | ||
|
|
||
| push {FIRST_NON_DETERMINISTICALLY_INITIALIZED_MEMORY_ADDRESS} | ||
| {&rustfield_membershipproof} | ||
| {&rustfield_receiverpreimage} | ||
| read_mem {Digest::LEN} | ||
| pop 1 | ||
| // *aocl [aocl_leaf_index] [receiver_preimage] | ||
|
|
||
| push {FIRST_NON_DETERMINISTICALLY_INITIALIZED_MEMORY_ADDRESS} | ||
| {&rustfield_membershipproof} | ||
| {&rustfield_senderrandomness} | ||
| read_mem {Digest::LEN} | ||
| pop 1 | ||
| // *aocl [aocl_leaf_index] [receiver_preimage] [sender_randomness] | ||
|
|
||
| push {FIRST_NON_DETERMINISTICALLY_INITIALIZED_MEMORY_ADDRESS} | ||
| {&rustfield_utxodigest} | ||
| read_mem {Digest::LEN} | ||
| pop 1 |
There was a problem hiding this comment.
Note that the field helpers set the pointer to the memory object's first element in memory. In memory, objects are laid out forwards meaning that the rest of the object is located at higher addresses. However, read_mem decreases the memory pointer so that after the read operation the same memory object is laid out forwards on stack, with the first word at stack index 0 and later words at higher indices. Right now you're reading in the wrong direction: you're reading the first word of the targeted object and the last few words of the object that came before it.
| push 0 | ||
| push 0 | ||
| push 0 | ||
| // SI_coins 0 *coins[0] amount timelocked_amount utxo_amount utxo_is_timelocked |
There was a problem hiding this comment.
There are a bunch of instructions missing here, right? Because I think the stack at this point has this shape:
_ SI_coins 0 *coins[0] [amount]
|
Thank you for bringing all the current things together! I only address the first part now, as I will return to a reserve with a positive experience from current part. an unconventional UTXOI appreciate the discussion on the cases different from a single native coin and less than two time locks as it's something to deal with. I just want to bring up the idea to deal that after the conventional cases start to work. I mean I like defined it as the limitation of the current solution, and of course it outlines the full future as it can. The rationale is that this part benefits from splitting, and that it would be nice to first observe its first steps in the wild before putting the efforts into covering the rarer cases. IIRC correctly I thought about specThe elephant in the room is I guess it won't make it without I thought it both easier and beneficiary to try to implement the trait with a nearest thing ( TasmHave you shared those comments? It's a complicated topic... A code during reading is seen quite differently so I'd argue (not too hard though) that sometimes readers can comment it in a more useful way. (I tried to convey a similar idea to Thorkill when we were discussing comments.) Ofc I will try to 'switch an eye' for writing better comments (for a proof of reserve?). Also with a critical part a more thorough review is probably good. You actually see/reconstruct the logic from the opcode patterns instead of matching comments with the code (a brain/mind is lazy and it will play 'looks convincing' trick here and there). Though I have no idea how critical this one is; it's auxiliary, on the other hand I think having a Tasm bug is especially bad at least for public perception of the whole. Any way this take is just a side thought -- not an intention. namesSpecial thank you for providing new names instead of those deserve changing; I'm trying not to miss those and make the changes. Pls, indicate when/if I miss something (if we have not discussed the item separately)! Btw, for me it's much easier to do to the end of a PR when most of reasoning on them is done. As with flattening the imports -- I kinda remember about it but due to the purpose of that it makes sense to do just once before merging when those parts of the code is settled already. |
I'm afraid I did not save them. |
link the issue fix reading "from right to left" mistake (Neptune-Crypto#799 (comment)) resolve few review suggestions resolve <Neptune-Crypto#799 (comment)> optimize out `if let` fix couple of the review suggestions a bit of docs which were suggested in the review add `impl ConsensusProgramSpecification` replace `swap` with `pick` and a smarter approach first version of `ConsensusProgramSpecification` expansion with the generalized trait the better version of the trait expansion debugging debugging debugging debugging debugging the happy/positive test passes well negative tests move `prove_transfer` to `api` develop the comments in Tasm code verification fixes regarding the review make `new` a method process review suggestions - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) Update neptune-core/src/application/util_proof/sent/mod.rs Co-authored-by: aszepieniec <alan.szepieniec+github@gmail.com> process the suggestions - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) process naming suggestions extract the module from huge <server.rs>
… program in a VM queue
link the issue fix reading "from right to left" mistake (Neptune-Crypto#799 (comment)) resolve few review suggestions resolve <Neptune-Crypto#799 (comment)> optimize out `if let` fix couple of the review suggestions a bit of docs which were suggested in the review add `impl ConsensusProgramSpecification` replace `swap` with `pick` and a smarter approach first version of `ConsensusProgramSpecification` expansion with the generalized trait the better version of the trait expansion debugging debugging debugging debugging debugging the happy/positive test passes well negative tests move `prove_transfer` to `api` develop the comments in Tasm code verification fixes regarding the review make `new` a method process review suggestions - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) Update neptune-core/src/application/util_proof/sent/mod.rs Co-authored-by: aszepieniec <alan.szepieniec+github@gmail.com> process the suggestions - Neptune-Crypto#799 (comment) - Neptune-Crypto#799 (comment) process naming suggestions extract the module from huge <server.rs> the missing changes from the dirty/development files fmt docs improvement add example / process <https://github.com/Neptune-Crypto/neptune-core/pull/799/changes#r2644656254> ditch multiline `use` [rename `ConsensusProgram` to `TritonProgram`](https://github.com/Neptune-Crypto/neptune-core/pull/799/changes#r2684962708) groom the module with Tasm fix the test setup `api::wallet` has its own locking approach, so the proving preparation moved back to the RPC can be moved again somewhere - but I have no idea for a right place yet develop the tests groom new modules deal with the warnings separate reserves into the other stage lint
a transfer
the review suggestions progressing are in progress
reserves
here's a good draft of the Triton code
the discussion is below, will be get back to it after the transfer part