0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

Track more information about note status #328

Closed tomyrd closed 1 week ago

tomyrd commented 1 month ago

One other thing that we should try to figure out how to do is to track more details information about note status. For example, I don't think we can currently tell if a note is in the process of being consumed (e.g., we created a transaction consuming this note, sent it to the network, but the transaction hasn't been included in a block yet) and how long it has remained in this state (e.g., have we sent a transaction 10 minutes or 10 days ago?).

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

tomyrd commented 1 month ago

Changed the issue title and description to better reflect the matter to discuss.

bobbinth commented 1 month ago

It feels like there are actually 4 note status variants for input notes:

bobbinth commented 1 month ago

Oh - one more variant I missed for the future: when the note has been recorded on-chain but we cannot consume it yet (e.g., it will become consumable in the future).

tomyrd commented 1 month ago

This is probably the most tricky state as we currently don't have a very robust mechanism of telling whether a given transaction succeeded or not.

Just tested this case, if tx1 consumes note1 and tx2 tries to consume note1 then the node responds the rpc SubmitProvenTransactionRequest with an error saying that the note was already consumed. This happens even if its in the same block and even without a sync in between. So we just have to wait after the confirmation to change from Recorded to Consuming. Then when we get the nullifier from the sync we change from Consuming to Consumed.

tomyrd commented 1 month ago

From the original comment:

how long it has remained in this state (e.g., have we sent a transaction 10 minutes or 10 days ago?)

Do we want to track when the note was consumed locally? If we do, should we track the local commit height of the transaction execution or the timestamp?

bobbinth commented 1 month ago

This is probably the most tricky state as we currently don't have a very robust mechanism of telling whether a given transaction succeeded or not.

Just tested this case, if tx1 consumes note1 and tx2 tries to consume note1 then the node responds the rpc SubmitProvenTransactionRequest with an error saying that the note was already consumed. This happens even if its in the same block and even without a sync in between. So we just have to wait after the confirmation to change from Recorded to Consuming. Then when we get the nullifier from the sync we change from Consuming to Consumed.

With the current node this shouldn't be a problem - but may be an issue in the future. Specifically:

  1. When the network is decentralized, the network may accept two conflicting transactions and eventually one of them will fail. This is probably some time away for now (e.g., 2 years into the future).
  2. If we add logic to the node to drop transactions for some reason (e.g., replace-by-fee type of transactions). This is also not something we'll have immediately - but might happen in the next year.

how long it has remained in this state (e.g., have we sent a transaction 10 minutes or 10 days ago?)

Do we want to track when the note was consumed locally? If we do, should we track the local commit height of the transaction execution or the timestamp?

Might make sense to track timestamps - but I also haven't thought about it too much.

tomyrd commented 1 month ago

Oh - one more variant I missed for the future: when the note has been recorded on-chain but we cannot consume it yet (e.g., it will become consumable in the future).

In this case, are we talking about the notes that are created but not yet consumable (e.g. a sender that cannot consume a p2idr note yet but will be able to do it after a specific height) or notes that we can't consume because we don't have all the information yet (e.g. the future note we expect after we create a swap note). AFAIK the latter wouldn't be recorded on-chain so I don't think it's that case but I want to be sure.

tomyrd commented 1 month ago

Some questions about the naming:

Also we should change the documentation to reflect this new nomenclature.

bobbinth commented 1 month ago

In this case, are we talking about the notes that are created but not yet consumable (e.g. a sender that cannot consume a p2idr note yet but will be able to do it after a specific height) or notes that we can't consume because we don't have all the information yet (e.g. the future note we expect after we create a swap note). AFAIK the latter wouldn't be recorded on-chain so I don't think it's that case but I want to be sure.

Yeah - it's the former case: we have all the required info to consume the note, but can't yet because some note-specific conditions are not met.

This case is actually more tricky than I originally thought as we can imagine notes which are "periodic" - e.g., consumable based on some conditions that happen periodically (e.g., a note that is consumable only on Mondays). Not sure how to deal with these ones yet.

If we don't like Consuming maybe Processing/InProcess is a better option?

Agreed, I don't like Consuming too. I'm not sure I love Processing, but I think it is better than than Consuming.

The Expected name may be better suited for the case where the note is not yet created and may arrive in the future (like for the swap tx). I don't know if we want to have a status for this case though.

Yes, this is how I thought about it too - i.e., a note that we expect to appear on chain, but it is not there yet.

mFragaBA commented 1 month ago

This case is actually more tricky than I originally thought as we can imagine notes which are "periodic" - e.g., consumable based on some conditions that happen periodically (e.g., a note that is consumable only on Mondays). Not sure how to deal with these ones yet.

should that be part of the state? As of now, the current note status reflects the note lifetime: it's created (locally) but not yet on chain, then gets committed, then gets consumed (locally), then gets consumed on the node.

I'm also not sure who would consume that state, considering we can already list consumable notes. And I think whether or not the note is consumable can be checked with:

a) storing its consumability status as a separate field (this would require updates on sync or to compute the consumability on each get_input_notes call) b) expose the NoteScreener (or expose its API through the Client) so users can do the check themselves

bobbinth commented 1 month ago

This case is actually more tricky than I originally thought as we can imagine notes which are "periodic" - e.g., consumable based on some conditions that happen periodically (e.g., a note that is consumable only on Mondays). Not sure how to deal with these ones yet.

should that be part of the state? As of now, the current note status reflects the note lifetime: it's created (locally) but not yet on chain, then gets committed, then gets consumed (locally), then gets consumed on the node.

I'm also not sure who would consume that state, considering we can already list consumable notes. And I think whether or not the note is consumable can be checked with:

a) storing its consumability status as a separate field (this would require updates on sync or to compute the consumability on each get_input_notes call) b) expose the NoteScreener (or expose its API through the Client) so users can do the check themselves

Yeah - probably storing this info in the DB would complicate things too much. I think the approach b makes more sense here: we can get a list of potentially consumable notes from the client (i.e., the notes which have been committed to the chain but not consumed yet), and then we can apply further filters on top of that to figure out what our of that list can actually be consumed at a given block.

mFragaBA commented 2 weeks ago

now that #355 got merged should we close this?

tomyrd commented 1 week ago

Yes! Closing this issue.