feat: Track more information about a note's status and consumability

[tomyrd]

closes #328

This PR looks to add more information about a note status and consumability in the client. This is achieved by a few new features:

• A new NoteStatus variant called Processing was created to represent the state between Comitted and Consumed. It's a state where locally you executed a transaction to consume a note but the client doesn't know if the node confirmed it.
• More metadata about a note's specific status was added inside the different NoteStatus variants:
  • For Pending: A timestamp of the creation of the note in the client.
  • For Commited: The block height where the note was commited in the node.
  • For Processing: The id of the consumer account of the note and the timestamp of the submition of transaction that is currently consuming the note.
  • For Consumed: The id of the consumer account of the note (if known) and the block height where the note was consumed.
• In terms of consumability status, this PR adds a new function (get_note_consumability) to allow the user to check a note's consumability (in addition to the previous get_consumable_notes).

Screenshot of the new status info:

[mFragaBA]

Not a final review (still need to test locally), but left some comments

[mFragaBA]

LGTM!

[bobbinth]

Maybe not for this PR, but I do think that we should eventually change Pending to something like Expected (i.e., the note is expected to appear on chain) to improve clarity.

[tomyrd]

Created a PR to address this.

[bobbinth]

There might ben an alternative way to do this: we keep the status field as it was (with 3 variants), but instead change the consumer_transaction_id into an array of transaction IDs (and probably rename it to consumer_transactions).

The update logic would then be as follows:

• When a new transaction involving the note is submitted to the network, we append the corresponding transaction ID to the array. This way, we could track 2 or more in-flight transactions for the same note.
  • The status remains as Committed.
• Once a note is consumed, we remove all transactions IDs from the array except for the one for which the transaction was committed.
  • We also change the status from Committed to Consumed as we do now.
• If a transaction gets canceled (we can't detect this now, but in the future we may be able to determine this), we remove the transaction ID of the canceled transaction from the array.

[tomyrd]

This implementation would be something similar to the first iteration of this PR where the "Processing" status was implied and not stored in the database. I have a few questions:

• Do we want to let 2 accounts try to consume the same note with the same client? I could add a check that if a note has a consumer_transaction_id then the transaction is cancelled even before sending to the node. Clarifying just in case, the consumer_transaction_id only tracks transactions made by the same client. Transactions made by other clients are not tracked with that column.
• If we receive the nullifier of a note with multiple consumer_transactions, how do we know which transaction ended up consuming the note?

[bobbinth]

If we receive the nullifier of a note with multiple consumer_transactions, how do we know which transaction ended up consuming the note?

This should be possible after #370. Basically, we'll get a list of committed transactions from the node as well. So, if we see a nullifier for a note, but the transactions we expected to consume the note is not in the list of committed transactions, the note must have been consumed by some other transaction.

Do we want to let 2 accounts try to consume the same note with the same client? I could add a check that if a note has a consumer_transaction_id then the transaction is cancelled even before sending to the node.

I'm wondering if I'm overthinking this a bit. With a centralized operator (something will have for the next 1 - 2 years), if a transaction is stale (i.e., has been sent to the node but is not getting committed) we can try to consume notes in a new transaction. If the node accepts the new transaction, we know that the stale transaction is canceled (i.e., dropped by the node for some reason), and so we can update its status and also replace consumer_transaction_id with the ID of the new transaction.

This won't quite work in a decentralized setting (and that's when we might need to track more than one transaction ID per note) - but maybe we don't need to tackle this now.

To summarize: maybe it still makes sense to keep consumer_transaction_id as a single value field. Note states can then be as follows:

1. expected - we are expecting the note to appear on chain.
   a. Here, it would also be useful to track the time at which the note was imported into the client (probably as a timestamp) so that we can tell how long have we been expecting this note to appear.
2. committed and consumer_transaction_id is NULL - the note is on chain, but we haven't executed a transaction to consume it yet.
3. committed and consumer_transaction_id is not NULL - we've executed a transaction to consume the note. consumer_transaction_id tracks ID of the last transaction accepted by the node to consume this note.
   a. Here, it would also be useful to track when the last transaction was submitted to the node so that we know how long we have been waiting for the transaction to be processed.
4. consumed - the note has been consumed. If consumer_transaction_id is set, it will be the ID of the transaction which consumed the note; if it is NULL, we don't know which transaction consumed the note.
   a. Here, it might be useful to know when the note was consumed (i.e., in which block).

[tomyrd]

I've been thinking on how can we represent all this additional information (timestamps, block heights, consumer ids, etc.). I started by just adding a new field in InputNoteRecord for each one but that doesn't scale well.

One way would be to store specific information inside the NoteStatus variants. Something like this:

pub enum NoteStatus {
    /// Note is pending to be commited on chain.
    Pending{tracking_start_timestamp: u64},
    /// Note has been commited on chain
    Committed{commit_height: u64},
    /// Note has been consumed locally but not yet nullified on chain
    Processing{consumer_account_id: AccountId, submition_timestamp: u64},
    /// Note has been nullified on chain
    Consumed{consumer_account_id: Option<AccountId>, nullification_height: u64},
}

The problem with this is that we wouldn't be able to access past information. Another way would be to add an optional struct with additional information but I feel that that would be too vague (at least with that name):

pub struct InputNoteRecord {
    additional_information: AdditionalInfo,
}
pub struct AdditionalInfo{
   tracking_start_timestamp: u64,
   submition_timestamp: u64,
   consumer_account_id: Option<AccountId>,
   nullification_height: Option<u64>,
}

cc @igamigo

[igamigo]

Is there any reason why we would need past data? In that case we can always factor out this status into its own table which maps Note IDs with NoteStatus and a timestamp so we can know how it evolved.
In any case I think placing the data within the enum is a good approach. Although your suggestion does not reflect the transaction ID approach that Bobbin was mentioning. I think it could look something like this:

pub enum NoteStatus {
    /// Note is pending to be commited on chain.
    Pending{created_at: u64},
    /// Note has been commited on chain
    Committed{commit_height: u64},
    /// Note has been consumed locally but not yet nullified on chain
    Processing{transaction_id: TransactionId, submitted_at: u64},
    /// Note has been nullified on chain
    Consumed{consumer_transaction_id: Option<TransactionId>, block_height: u64},
}

or if we follow Bobbin's suggestion more directly (essentially removing processing as a separate status):

pub enum NoteStatus {
    /// Note is pending to be commited on chain.
    Pending{created_at: u64},
    /// Note has been commited on chain
    Committed{commit_height: u64, consumer_transaction_id: Option<TransactionId>},
    /// Note has been nullified on chain
    Consumed{consumer_transaction_id: Option<TransactionId>, block_height: u64},
}

[tomyrd]

Oh, when Bobbin talks about removing processing I thought he meant "removing it from the sql table" but to keep it as a variant. Both approaches would work.

Is there any reason why we would need past data?

Not that I can think of. Maybe it's a downside that wouldn't bother us. In any case I think the information would be stored in the SqliteStore so we can always retreive it if we need it.

[bobbinth]

Is there any reason why we would need past data?

This may be useful for audit/log purposes - but it is a "nice-to-have". So, if it simplifies things a lot, we can assume that we don't need this data.

Bobbin talks about removing processing I thought he meant "removing it from the sql table" but to keep it as a variant.

Originally, I thought about removing it from both - but given more recent discussion (about making the solution more tailored to the centralized operator for now), it may make sense to have the status on the Rust side, and maybe database side as well.

Another way would be to add an optional struct with additional information but I feel that that would be too vague (at least with that name)

On the database side, we could have a field called something like audit_log which would keep track of some of this info (i.e., created_on, submitted_on etc.). We probably still want to keep status as a separate field to simplify querying - but also not a strong preference from my end.

On the Rust side, another alternative is to turn the whole InputNoteRecord into an enum based on the status. Something like this (though, I'm sure I missed some detail):

pub enum InputNote {
    Expected(ExpectedInputNote),
    Committed(CommittedInputNote),
    ConsumedInputNote(ConsumedInputNote),
}

pub struct ExpectedInputNote {
    note: NoteDetails,
    created_on: Timestamp,
}

pub struct CommittedInputNote {
    note: Note,
    proof: NoteInclusionProof, // this also contains commit_height for the note
    created_on: Timestamp,
    transaction: Option<TransactionId>, // maybe submitted_on can come from the transactions table?
}

pub struct ConsumedInputNote {
    note: Note,
    proof: NoteInclusionProof,
    created_on: Timestamp,
    consumed_height: u32,
    consumed_by: Option<TransactionId>,
}

Introducing Processing(ProcessingInputNote) may make this cleaner still.

[tomyrd]

I ended up implementing it with the struct variants for NoteStatus. I feel like if we add all the changes/features discussed here this PR will get really big. So I summerized the changes in the last comment, I can create an issue for each one and tackle them in the future.

[bobbinth]

Yep - this approach makes sense.

[tomyrd]

Just finished implementing the new additional information for the note status. I also modified the PR title and description to better reflect the new changes.

I feel like all of the other changes discussed here should be implemented in separate PRs. These are the changes that are left unfinished:

• Check if the local consumer transaction actually consumed the note when the nullifier is received. The issue Update transaction commitment check logic after miden-node#371 #370 needs to be implemented first.
• Allow for multiple transactions to try to consume the same note locally. We should store each tx trying to consume a note and when it gets nullified we check which tx was commited. This depends on the previous point.
• Find a way to check if a transaction gets cancelled in the node and update the client.

[igamigo]

Looking good! I want to take a deeper look but it seems fine at a first glance.

[mFragaBA]

Nice Work! Left some comments.

Do you think we could do a similar thing for the note status as we did for, for example, NoteDetails? We could store the status as a not NULL json field. It'll probably clean up the serialization / deserialization code a bit as well.

[mFragaBA]

we could add constraints here, like

• if status is Committed or Processing, submitted_at should not be null
• if status is consumed, nullifier_height should not be null

I think we can do it in a follow up PR though, let's open an issue for it

[mFragaBA]

(same thing for input notes)

[tomyrd]

The submitted_at may be null on a "Committed" status. Maybe the user just created the note and synced, in this case the note is Committed but it was never submitted to the node. What should be checked is that if a note is "Processing" then the submitted_at can't be null.

[tomyrd]

Done!

[mFragaBA]

Code Looks good! Tested locally and CLI output is neat! Worked with both public and private (with import/export) notes and worked without issues.

[igamigo]

LGTM! Just a small nit but we should be good to merge this after that.

[igamigo]

nit: I think it's worth mentioning here that this is a UNIX epoch-based timestamp

[tomyrd]

Done!