Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore messages with duplicated nonces #2375

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]

### Fixed
- [2375](https://github.com/FuelLabs/fuel-core/pull/2375): Modify executor to ignore messages from the relayer that have a duplicate nonce
- [2366](https://github.com/FuelLabs/fuel-core/pull/2366): The `importer_gas_price_for_block` metric is properly collected.
- [2369](https://github.com/FuelLabs/fuel-core/pull/2369): The `transaction_insertion_time_in_thread_pool_milliseconds` metric is properly collected.

### Added
- [2321](https://github.com/FuelLabs/fuel-core/pull/2321): New metrics for the txpool: "The size of transactions in the txpool" (`txpool_tx_size`), "The time spent by a transaction in the txpool in seconds" (`txpool_tx_time_in_txpool_seconds`), The number of transactions in the txpool (`txpool_number_of_transactions`), "The number of transactions pending verification before entering the txpool" (`txpool_number_of_transactions_pending_verification`), "The number of executable transactions in the txpool" (`txpool_number_of_executable_transactions`), "The time it took to select transactions for inclusion in a block in nanoseconds" (`txpool_select_transaction_time_nanoseconds`), The time it took to insert a transaction in the txpool in milliseconds (`txpool_insert_transaction_time_milliseconds`).
- [2362](https://github.com/FuelLabs/fuel-core/pull/2362): Added a new request_response protocol version `/fuel/req_res/0.0.2`. In comparison with `/fuel/req/0.0.1`, which returns an empty response when a request cannot be fulfilled, this version returns more meaningful error codes. Nodes still support the version `0.0.1` of the protocol to guarantee backward compatibility with fuel-core nodes. Empty responses received from nodes using the old protocol `/fuel/req/0.0.1` are automatically converted into an error `ProtocolV1EmptyResponse` with error code 0, which is also the only error code implemented. More specific error codes will be added in the future.

### Breaking
- [](https://github.com/FuelLabs/fuel-core/pull/2375): Add new `ExecutorEvent` variant for ignored messages

## [Version 0.40.0]

### Added
Expand Down
72 changes: 72 additions & 0 deletions crates/fuel-core/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3285,6 +3285,78 @@ mod tests {
Ok(())
}

fn on_chain_db_containing_message(
message: &Message,
da_block_height: u64,
) -> Database<OnChain> {
let mut db = add_consensus_parameters(
Database::default(),
&ConsensusParameters::default(),
);
db.storage_as_mut::<Messages>()
.insert(message.id(), message)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a nonce method that returns the same thing as id. Very silly.

.unwrap();

let mut block = Block::default();
block.header_mut().set_da_height(da_block_height.into());
block.header_mut().recalculate_metadata();

db.storage_as_mut::<FuelBlocks>()
.insert(&0.into(), &block)
.expect("Should insert genesis block without any problems");
db
}

fn relayer_db_containing_message(message: &Message) -> Database<Relayer> {
let mut relayer_db = Database::<Relayer>::default();
add_message_to_relayer(&mut relayer_db, message.clone());
relayer_db
}

#[test]
fn produce_without_commit__messages_with_duplicate_nonces_are_ignored() {
// given
let previous_da_height = 1;
let shared_nonce = 1234.into();
let mut message = Message::default();
message.set_amount(10);
message.set_da_height(previous_da_height.into());
message.set_nonce(shared_nonce);
let on_chain_db =
on_chain_db_containing_message(&message, previous_da_height);

let mut message_with_matching_nonce = message.clone();
message_with_matching_nonce.set_amount(20);
let new_da_height = previous_da_height + 1;
message_with_matching_nonce.set_da_height(new_da_height.into());
let relayer_db = relayer_db_containing_message(&message_with_matching_nonce);

let producer = create_relayer_executor(on_chain_db, relayer_db);
let block_height = 1;
let block = test_block(block_height.into(), new_da_height.into(), 0);

// when
let (result, changes) = producer
.produce_without_commit(block.into())
.unwrap()
.into();

// then
let no_messages_in_changes = ChangesIterator::<Column>::new(&changes)
.iter_all::<Messages>(None)
.count()
== 0;
assert!(no_messages_in_changes);
// and
let event_contains_ignore = result.events.iter().any(|event| {
matches!(event, ExecutorEvent::MessageIgnored {
message,
..
} if message == &message_with_matching_nonce)
});
assert!(event_contains_ignore);
}

#[test]
fn execute_without_commit__block_producer_includes_correct_inbox_event_merkle_root(
) {
Expand Down
1 change: 1 addition & 0 deletions crates/fuel-core/src/graphql_api/worker_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ where
&(),
)?;
}
Event::MessageIgnored { .. } => {}
Event::MessageConsumed(message) => {
block_st_transaction
.storage_as_mut::<OwnedMessageIds>()
Expand Down
25 changes: 17 additions & 8 deletions crates/services/executor/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ where
D: KeyValueInspect<Column = Column>,
{
let forced_transactions = if self.relayer.enabled() {
self.process_da(
self.process_l1_events(
block_height,
da_block_height,
data,
Expand Down Expand Up @@ -875,7 +875,7 @@ where
}
}

fn process_da<D>(
fn process_l1_events<D>(
&mut self,
block_height: BlockHeight,
da_block_height: DaBlockHeight,
Expand Down Expand Up @@ -916,12 +916,21 @@ where
if message.da_height() != da_height {
return Err(ExecutorError::RelayerGivesIncorrectMessages)
}
block_storage_tx
.storage_as_mut::<Messages>()
.insert(message.nonce(), &message)?;
execution_data
.events
.push(ExecutorEvent::MessageImported(message));
let message_nonce = message.nonce();
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 }
};
Comment on lines +920 to +932
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe we want a data structure that allows us to store multiple messages with colliding nonces?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, only one message should be active at the time.

execution_data.events.push(message_event);
}
Event::Transaction(relayed_tx) => {
let id = relayed_tx.id();
Expand Down
19 changes: 13 additions & 6 deletions crates/types/src/services/executor.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
//! Types related to executor service.

#[cfg(feature = "alloc")]
use alloc::{
string::String,
vec::Vec,
};

use crate::{
blockchain::{
block::Block,
Expand Down Expand Up @@ -34,12 +40,6 @@ use crate::{
services::Uncommitted,
};

#[cfg(feature = "alloc")]
use alloc::{
string::String,
vec::Vec,
};

/// The alias for executor result.
pub type Result<T> = core::result::Result<T, Error>;
/// The uncommitted result of the block production execution.
Expand Down Expand Up @@ -131,6 +131,13 @@ impl<DatabaseTransaction> UncommittedResult<DatabaseTransaction> {
pub enum Event {
/// Imported a new spendable message from the relayer.
MessageImported(Message),
/// Ignored a message from the relayer.
MessageIgnored {
/// Ignored message
message: Message,
/// The reason for ignoring the message
reason: String,
},
Comment on lines +134 to +140
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. It was an afterthought and added a breaking section the the CHANGELOG to flag for this reason.

/// The message was consumed by the transaction.
MessageConsumed(Message),
/// Created a new spendable coin, produced by the transaction.
Expand Down
Loading