chore: remove decorators from Note's masm#1194
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-avoid-decorators-in-note
branch
from
18b63a5 to
194d8f5
Compare
SantiagoPittella
requested review from
Mirko-von-Leipzig,
bobbinth and
igamigo
and removed request for
Mirko-von-Leipzig
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM. Not that it makes a difference, but would this cover the network transaction builder mempool events? AFAIK when a transaction gets added to the mempool, new notes are sent to the ntx builder so maybe it would receive notes with the decorators.
Yeah it would still have the decorators. Good point. Maybe this should be done in the RPC then to strip them at arrival? Bit of a waste but its the single point of entry for now.. |
|
Should this be moved to |
|
Yeah, doing it in the PRC is a bit more robust. We could still have a check before inserting into DB, just in case. Basically, at the DB level we'd check if there are any decorators, and if so, remove them. |
|
Implemented this change, leaving the decorator removal before inserting in the DB too. wdyt? |
There was a problem hiding this comment.
This works but feels extremely fragile and expensive since we build everything twice. I don't think there is currently a better way though.
Maybe notes should be stripped as part of building a transaction already? I'm a bit unclear on when the decorators are desired.
As a side note ProvenTransactionBuilder::new feels like it really needs wrapper types since it has many words and block numbers which can be trivially swapped around. cc @PhilippGackstatter
There was a problem hiding this comment.
As a side note
ProvenTransactionBuilder::newfeels like it really needs wrapper types since it has many words and block numbers which can be trivially swapped around.
Agreed, but we need 0xMiden/crypto#430 for this first.
There was a problem hiding this comment.
Maybe notes should be stripped as part of building a transaction already? I'm a bit unclear on when the decorators are desired.
In a mock-chain context, we often consume notes that were created by a previous transaction, so we'd ideally retain the decorators. It would be sufficient to have that on testing though, and strip them in non-testing during ProvenTransactionBuilder::build. I think that could work - but there may be more cases where we'd want decorators that I'm not thinking about.
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM in terms of code. Although it feels redundant to do it in the DB if we are already stripping them at the only available RPC entrypoints, if we do it we might want to check if the note had decorators before attempting to do the reconstruction as suggested here.
SantiagoPittella
deleted the
santiagopittella-avoid-decorators-in-note
branch
| let mut builder = ProvenTransactionBuilder::new( | ||
| tx.account_id(), | ||
| tx.account_update().initial_state_commitment(), | ||
| tx.account_update().final_state_commitment(), | ||
| tx.account_update().account_delta_commitment(), | ||
| tx.ref_block_num(), | ||
| tx.ref_block_commitment(), | ||
| tx.fee(), | ||
| tx.expiration_block_num(), | ||
| tx.proof().clone(), | ||
| ) | ||
| .account_update_details(tx.account_update().details().clone()) | ||
| .add_input_notes(tx.input_notes().iter().cloned()); |
There was a problem hiding this comment.
This is fine for now, but ideally, we should have a less error-prone way to strip decorators from any note in a transaction. I'm not sure what this way would be - but let's create an issue about for this.
5 participants
partially address 0xMiden/protocol#1812
Removes decoratos from the note before inserting it in the DB.