Ignore messages with duplicated nonces #2375
Conversation
|
Is this actually a breaking change? The contract is kinda changing, but only on the code side. It was implicitly the same before. |
| block_storage_tx | ||
| .storage_as_mut::<Messages>() | ||
| .insert(message_nonce, &message)?; | ||
| } | ||
| execution_data | ||
| .events | ||
| .push(ExecutorEvent::MessageImported(message)); |
There was a problem hiding this comment.
should this also be conditional to the message_nonce not being already present in the block storage?
There was a problem hiding this comment.
Good catch. I've moved it.
This makes me wonder if we should be including an event in the case that a message is skipped. Or add it to a list of skipped messages, similar to the skipped tx.
There was a problem hiding this comment.
I think we should have the info somewhere that we manage a message with this nonce at this height and ignored this can be important to debug, index...
There was a problem hiding this comment.
I think it is a bad idea to just ignore the error. If the message is emitted by the contract, it means that someone deposited some funds. Ignoring the messages means that funds are lost forever.
I think we need to return an error if the message already exists in the table and insert it otherwise. It is not our responsibility to track the validity of how messages are emitted on the L1. We just need to be sure that it will not override the state.
| /// Ignored a message from the relayer. | ||
| MessageIgnored { | ||
| /// Ignored message | ||
| message: Message, | ||
| /// The reason for ignoring the message | ||
| reason: String, | ||
| }, |
There was a problem hiding this comment.
We shouldn't add a new event at the middle since it is a breaking change for the WASM interface, basically you break all nodes in the network with this update and forces them to use a new fuel-core node.
Plus, I don't think that we even need MessageIgnored event since it doesn't change the state of the chain.
There was a problem hiding this comment.
That's fair. It was an afterthought and added a breaking section the the CHANGELOG to flag for this reason.
| let message_event = if !block_storage_tx | ||
| .storage_as_ref::<Messages>() | ||
| .contains_key(message_nonce)? | ||
| { | ||
| block_storage_tx | ||
| .storage_as_mut::<Messages>() | ||
| .insert(message_nonce, &message)?; | ||
| ExecutorEvent::MessageImported(message) | ||
| } else { | ||
| let reason = | ||
| "Message with the same nonce already exists".to_string(); | ||
| ExecutorEvent::MessageIgnored { message, reason } | ||
| }; |
There was a problem hiding this comment.
I'm not sure even how do we want to handle this case.
Right now you don't handle the situation mentioned by @Voxelot .
we should probably just skip nonces that have already been seen before
The Messages table contains only the list of unspent messages. If a message was seen before but spent after, it can still be inserted into the state in this solution.
There was a problem hiding this comment.
If a message was seen before but spent after, it can still be inserted into the state in this solution.
You mean if a message with the same nonce gets spent before we check for nonce collisions, it will get added still. Right?
There was a problem hiding this comment.
Yes. And I think it is okay behavior if L1 contracts emit the message with the same nonce. It is not our business to decide is it bad or not in this situation=D
There was a problem hiding this comment.
Then maybe we want a data structure that allows us to store multiple messages with colliding nonces?
There was a problem hiding this comment.
No, only one message should be active at the time.
| &ConsensusParameters::default(), | ||
| ); | ||
| db.storage_as_mut::<Messages>() | ||
| .insert(message.id(), message) |
There was a problem hiding this comment.
| .insert(message.id(), message) | |
| .insert(message.nonce(), message) |
May I ask you to rename this method please?=D message.id() for me means MessageId return type, while it is not true.
There was a problem hiding this comment.
There is already a nonce method that returns the same thing as id. Very silly.
Linked Issues/PRs
Closes #1834
Description
Ignore new messages that share a nonce with an old message.
This isn't the only way to solve the issue but it was the solution proposed
Checklist
Before requesting review