0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
27 stars 23 forks source link

Consume Unauthenticated Input Notes #418

Closed mFragaBA closed 5 days ago

mFragaBA commented 1 week ago
  1. As it stands, TransactionRequest does not support input notes with delayed inclusion proofs introduced recently in miden-base and miden-node. For this, we may want to introduce a separate field to keep track of these notes - e.g., something like this:
pub struct TransactionRequest {
    account_id: AccountId,
    input_notes: BTreeMap<NoteId, Option<NoteArgs>>,
    unauthenticated_input_notes: Vec<Note>,   // new field
    expected_output_notes: Vec<Note>,
    expected_partial_notes: Vec<NoteDetails>, // I wonder if expected_future_notes is a better name here
    tx_script: Option<TransactionScript>,
    advice_map: AdviceMap,                    // new field
}

Though, there may be other ways of doing this.

Originally posted by @bobbinth in https://github.com/0xPolygonMiden/miden-client/issues/398#issuecomment-2198458827

mFragaBA commented 1 week ago

also relevant

Regarding your last point @bobbinth (maybe we should open a new issue for this), I don't think we need to add unauthenticated_input_notes separately. The input_notes field should cover this: Eventually the DataStore retrieves note data from the store, where a note can either be InputNote::Authenticated or InputNote::Unauthenticated. I'll let @mFragaBA explain further since he's currently testing and working on this and noticed this should be sufficient, but there might be something else you could have been thinking about.

Originally posted by @igamigo in https://github.com/0xPolygonMiden/miden-client/issues/398#issuecomment-2207079598

mFragaBA commented 1 week ago

cc @bobbinth

I think @igamigo explained it quite well. When we want to consume notes in a transaction, the only thing we need in TransactionRequest is the mapping NoteId -> Option<NoteArgs>. Then during get_transaction_inputs (from DataStore) we fetch the corresponding records and convert them into either InputNote::Authenticated (if we have both the metadata and the inclusion proof) or InputNote::Unauthenticated (if we only have the metadata), failing if we're missing both.

One thing I'm wondering now if whether there is any value in using authenticated notes, since I could treat everything as unauthenticated and cover the same flows and some more.

Btw, I submitted a PR with a temptative change in #417

bobbinth commented 1 week ago

the only thing we need in TransactionRequest is the mapping NoteId -> Option<NoteArgs>. Then during get_transaction_inputs (from DataStore) we fetch the corresponding records and convert them into either InputNote::Authenticated (if we have both the metadata and the inclusion proof) or InputNote::Unauthenticated (if we only have the metadata), failing if we're missing both.

There are a couple of things I was trying to achieve the the proposed structure:

  1. Avoiding a two-step process for consuming unauthenticated notes. Specifically, if all the note info is present in the TransactionRequest, we skip the need for importing the note first and then executing transaction request. The motivating use case is an orderbook exchange where the operator of the exchange maintains a set of unauthenticated notes and builds transaction requests for the user to consume these notes. If these notes are not a part of the request, then the DEX operator would need to attach separate note files to each request.
  2. Reduce the amount of changes in the client code. Specifically, if we want to follow the two-step process described above, we need to let the user consume notes which have not been authenticated yet. This means that we need to somehow indicate that some unauthenticated notes are OK to consume and I think that may require quite a few changes in the client/CLI.

One thing I'm wondering now if whether there is any value in using authenticated notes, since I could treat everything as unauthenticated and cover the same flows and some more.

Using unauthenticated notes has a significant effect on privacy. Specifically, with authenticated notes, the network does not know which note was consumed in a given transaction. With unauthenticated notes, the network needs to make sure that note which is being consumed has been previously created, and for this it needs to know note IDs of all unauthenticated notes. This means, that for unauthenticated notes the network will know exactly which notes are consumed by a given transaction.

bobbinth commented 1 week ago
  1. Avoiding a two-step process for consuming unauthenticated notes. Specifically, if all the note info is present in the TransactionRequest, we skip the need for importing the note first and then executing transaction request. The motivating use case is an orderbook exchange where the operator of the exchange maintains a set of unauthenticated notes and builds transaction requests for the user to consume these notes. If these notes are not a part of the request, then the DEX operator would need to attach separate note files to each request.

To clarify this point: unauthenticated notes may never get recorded on chain - they may be erased during batch/block building process. So, retrieving them by ID from the chain may not be an option.

mFragaBA commented 1 week ago

the only thing we need in TransactionRequest is the mapping NoteId -> Option<NoteArgs>. Then during get_transaction_inputs (from DataStore) we fetch the corresponding records and convert them into either InputNote::Authenticated (if we have both the metadata and the inclusion proof) or InputNote::Unauthenticated (if we only have the metadata), failing if we're missing both.

There are a couple of things I was trying to achieve the the proposed structure:

1. Avoiding a two-step process for consuming unauthenticated notes. Specifically, if all the note info is present in the `TransactionRequest`, we skip the need for importing the note first and then executing transaction request. The motivating use case is an orderbook exchange where the operator of the exchange maintains a set of unauthenticated notes and builds transaction requests for the user to consume these notes. If these notes are not a part of the request, then the DEX operator would need to attach separate note files to each request.

2. Reduce the amount of changes in the client code. Specifically, if we want to follow the two-step process described above, we need to let the user consume notes which have not been authenticated yet. This means that we need to somehow indicate that some unauthenticated notes are OK to consume and I think that may require quite a few changes in the client/CLI.

One thing I'm wondering now if whether there is any value in using authenticated notes, since I could treat everything as unauthenticated and cover the same flows and some more.

Using unauthenticated notes has a significant effect on privacy. Specifically, with authenticated notes, the network does not know which note was consumed in a given transaction. With unauthenticated notes, the network needs to make sure that note which is being consumed has been previously created, and for this it needs to know note IDs of all unauthenticated notes. This means, that for unauthenticated notes the network will know exactly which notes are consumed by a given transaction.

Thanks for the context! I'll work on the TransactionRequest approach tommorow. In that case, should we only allow unauthenticated notes via the TransactionRequest's unauthenticated_input_notes field?

mFragaBA commented 1 week ago
  1. Avoiding a two-step process for consuming unauthenticated notes. Specifically, if all the note info is present in the TransactionRequest, we skip the need for importing the note first and then executing transaction request. The motivating use case is an orderbook exchange where the operator of the exchange maintains a set of unauthenticated notes and builds transaction requests for the user to consume these notes. If these notes are not a part of the request, then the DEX operator would need to attach separate note files to each request.

To clarify this point: unauthenticated notes may never get recorded on chain - they may be erased during batch/block building process. So, retrieving them by ID from the chain may not be an option.

how would you know they got consumed successfully then? By the Account updates received?

bobbinth commented 1 week ago

should we only allow unauthenticated notes via the TransactionRequest's unauthenticated_input_notes field?

Yep - that's what I was thinking.

how would you know they got consumed successfully then? By the Account updates received?

Would we not be able to infer this from committed transaction info received during sync request? (I thought we migrated all the logic for marking notes as consumed to work based on transaction IDs).

mFragaBA commented 6 days ago

should we only allow unauthenticated notes via the TransactionRequest's unauthenticated_input_notes field?

Yep - that's what I was thinking.

how would you know they got consumed successfully then? By the Account updates received?

Would we not be able to infer this from committed transaction info received during sync request? (I thought we migrated all the logic for marking notes as consumed to work based on transaction IDs).

If the TX info is not lost (unlike the nullifier info) then we could do it, yes (with some refactoring on the way we mark notes as consumed).

mFragaBA commented 6 days ago

@bobbinth we were looking at all the updates in base regarding the unauthenticated notes and figuring out an approach to implement this in the client. We added the unauthenticated_input_notes and the advice_map to the TransactionRequest struct as suggested.

Then, when we convert the TransactionRequest into TransactionArgs (before calling TransactionExecutor::execute_transaction) we use the struct's advice map instead of creating a new one (the advice_map is used to extend the advice_inputs during get_kernel_inputs). It's a bit unclear how to use the unauthenticated_input_notes from there though.

The InputNotes are built during get_transaction_inputs from DataStore, and the only thing we have there are the list of NoteId that get consumed in the tx. If we're not tracking the notes we cannot build the full Note object from the NoteId only.

To overcome this we tried a different approach. We looked at how advice inputs are built, particularly what the add_input_notes_to_advice_inputs does and tried replicating the same logic to update the advice_map from the TransactionRequest accordingly. But we still need to provide the unauthenticated note id as part of the note_ids from execute_transaction (which in turn will be used in get_transaction_inputs and fail)

Perhaps we're overcomplicating things and there's a simpler approach.

bobbinth commented 6 days ago

The changes here may be a bit more complicated than I originally thought - but generally, I've imagined this working like this:

  1. During processing of the TransactionRequest the client would insert the unauthenticated notes into its store.
  2. Then, when the DataStore::get_transaction_inputs() is called, it would get the note data from the store to build TransactionInputs struct with unauthenticated input notes in it.

So, the bulk of the work would be done in the DataStore::get_transaction_inputs() and this would require getting notes from the client, figuring out if these supposed to be authenticated or not, and building the correct transaction inputs based on that.

mFragaBA commented 6 days ago

The changes here may be a bit more complicated than I originally thought - but generally, I've imagined this working like this:

1. During processing of the `TransactionRequest` the client would insert the unauthenticated notes into its store.

2. Then, when the `DataStore::get_transaction_inputs()` is called, it would get the note data from the store to build `TransactionInputs` struct with unauthenticated input notes in it.

So, the bulk of the work would be done in the DataStore::get_transaction_inputs() and this would require getting notes from the client, figuring out if these supposed to be authenticated or not, and building the correct transaction inputs based on that.

I've just pushed to the branch in #417, we'll do some testing but the approach should match this

bobbinth commented 5 days ago

Closed by #417.