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

Bugfix: Calling of the contract with enabled utxo_validation fails #1717

Merged
merged 7 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Changed

- [#1717](https://github.com/FuelLabs/fuel-core/pull/1717): The fix for the [#1657](https://github.com/FuelLabs/fuel-core/pull/1657) to include the contract into `ContractsInfo` table.
- [#1657](https://github.com/FuelLabs/fuel-core/pull/1657): Upgrade to `fuel-vm` 0.46.0.
- [#1671](https://github.com/FuelLabs/fuel-core/pull/1671): The logic related to the `FuelBlockIdsToHeights` is moved to the off-chain worker.
- [#1663](https://github.com/FuelLabs/fuel-core/pull/1663): Reduce the punishment criteria for mempool gossipping.
Expand Down
13 changes: 10 additions & 3 deletions crates/fuel-core/src/service/adapters/consensus_module/poa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ use fuel_core_types::{
BlockImportInfo,
UncommittedResult as UncommittedImporterResult,
},
executor::UncommittedResult,
executor::{
Error as ExecutorError,
UncommittedResult,
},
txpool::ArcPoolTx,
},
tai64::Tai64,
Expand Down Expand Up @@ -82,8 +85,12 @@ impl TransactionPool for TxPoolAdapter {
self.service.total_consumable_gas()
}

fn remove_txs(&self, ids: Vec<TxId>) -> Vec<ArcPoolTx> {
self.service.remove_txs(ids)
fn remove_txs(&self, ids: Vec<(TxId, ExecutorError)>) -> Vec<ArcPoolTx> {
self.service.remove_txs(
ids.into_iter()
.map(|(tx_id, err)| (tx_id, err.to_string()))
.collect(),
)
}

fn transaction_status_events(&self) -> BoxStream<TxId> {
Expand Down
7 changes: 5 additions & 2 deletions crates/services/consensus_module/poa/src/ports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ use fuel_core_types::{
BlockImportInfo,
UncommittedResult as UncommittedImportResult,
},
executor::UncommittedResult as UncommittedExecutionResult,
executor::{
Error as ExecutorError,
UncommittedResult as UncommittedExecutionResult,
},
txpool::ArcPoolTx,
},
tai64::Tai64,
Expand All @@ -35,7 +38,7 @@ pub trait TransactionPool: Send + Sync {

fn total_consumable_gas(&self) -> u64;

fn remove_txs(&self, tx_ids: Vec<TxId>) -> Vec<ArcPoolTx>;
fn remove_txs(&self, tx_ids: Vec<(TxId, ExecutorError)>) -> Vec<ArcPoolTx>;

fn transaction_status_events(&self) -> BoxStream<TxId>;
}
Expand Down
2 changes: 1 addition & 1 deletion crates/services/consensus_module/poa/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ where
tx_id,
err
);
tx_ids_to_remove.push(tx_id);
tx_ids_to_remove.push((tx_id, err));
}
self.txpool.remove_txs(tx_ids_to_remove);

Expand Down
11 changes: 6 additions & 5 deletions crates/services/consensus_module/poa/src/service_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,15 @@ impl MockTransactionPool {
.sum()
});
let removed = txs.clone();
txpool
.expect_remove_txs()
.returning(move |tx_ids: Vec<TxId>| {
txpool.expect_remove_txs().returning(
move |tx_ids: Vec<(TxId, ExecutorError)>| {
let mut guard = removed.lock().unwrap();
for id in tx_ids {
for (id, _) in tx_ids {
guard.retain(|tx| tx.id(&ChainId::default()) == id);
}
vec![]
});
},
);

TxPoolContext {
txpool,
Expand Down Expand Up @@ -308,6 +308,7 @@ async fn remove_skipped_transactions() {
let mut txpool = MockTransactionPool::no_tx_updates();
// Test created for only for this check.
txpool.expect_remove_txs().returning(move |skipped_ids| {
let skipped_ids: Vec<_> = skipped_ids.into_iter().map(|(id, _)| id).collect();
// Transform transactions into ids.
let skipped_transactions: Vec<_> = skipped_transactions
.iter()
Expand Down
17 changes: 16 additions & 1 deletion crates/services/executor/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ use fuel_core_types::{
MintAssetId,
MintGasPrice,
OutputContract,
Salt,
TxPointer as TxPointerField,
},
input,
Expand Down Expand Up @@ -1017,7 +1018,7 @@ where
.into();
let reverted = vm_result.should_revert();

let (state, mut tx, receipts) = vm_result.into_inner();
let (state, mut tx, receipts): (_, Tx, _) = vm_result.into_inner();
#[cfg(debug_assertions)]
{
tx.precompute(&self.config.consensus_parameters.chain_id)?;
Expand Down Expand Up @@ -1073,6 +1074,20 @@ where
})
}

// TODO: Move to an off-chain worker: https://github.com/FuelLabs/fuel-core/issues/1721
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it is a temporary solution, I'm okay with doing it here. Later, we will move this code to the off-chain worker.

if let Some(create) = tx.as_create() {
let contract_id = create
.metadata()
.as_ref()
.expect("The metadata always should exist after VM execution stage")
.contract_id;
let salt = *create.salt();
tx_st_transaction
.as_mut()
.storage::<ContractsInfo>()
.insert(&contract_id, &(salt.into()))?;
}

let final_tx = tx.into();

// Store tx into the block db transaction
Expand Down
8 changes: 4 additions & 4 deletions crates/services/txpool/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,8 @@ impl<P2P, ViewProvider> SharedState<P2P, ViewProvider> {
self.txpool.lock().consumable_gas()
}

pub fn remove_txs(&self, ids: Vec<TxId>) -> Vec<ArcPoolTx> {
self.txpool.lock().remove(&self.tx_status_sender, &ids)
pub fn remove_txs(&self, ids: Vec<(TxId, String)>) -> Vec<ArcPoolTx> {
self.txpool.lock().remove(&self.tx_status_sender, ids)
}

pub fn find(&self, ids: Vec<TxId>) -> Vec<Option<TxInfo>> {
Expand All @@ -330,8 +330,8 @@ impl<P2P, ViewProvider> SharedState<P2P, ViewProvider> {
sorted_txs
}

pub fn remove(&self, ids: Vec<TxId>) -> Vec<ArcPoolTx> {
self.txpool.lock().remove(&self.tx_status_sender, &ids)
pub fn remove(&self, ids: Vec<(TxId, String)>) -> Vec<ArcPoolTx> {
self.txpool.lock().remove(&self.tx_status_sender, ids)
}

pub fn new_tx_notification_subscribe(&self) -> broadcast::Receiver<TxId> {
Expand Down
13 changes: 10 additions & 3 deletions crates/services/txpool/src/service/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,16 @@ async fn simple_insert_removal_subscription() {
}

// remove them
service
.shared
.remove(vec![tx1.cached_id().unwrap(), tx2.cached_id().unwrap()]);
service.shared.remove(vec![
(
tx1.cached_id().unwrap(),
"Because of the test purposes".to_string(),
),
(
tx2.cached_id().unwrap(),
"Because of the test purposes".to_string(),
),
]);

let update = tx1_subscribe_updates.next().await.unwrap();
assert_eq!(
Expand Down
17 changes: 11 additions & 6 deletions crates/services/txpool/src/txpool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,20 @@ impl<ViewProvider> TxPool<ViewProvider> {
pub fn remove(
&mut self,
tx_status_sender: &TxStatusChange,
tx_ids: &[TxId],
tx_ids: Vec<(TxId, String)>,
) -> Vec<ArcPoolTx> {
let mut removed = Vec::new();
for tx_id in tx_ids {
let rem = self.remove_by_tx_id(tx_id);
tx_status_sender.send_squeezed_out(*tx_id, Error::Removed);
for (tx_id, reason) in tx_ids.into_iter() {
let rem = self.remove_by_tx_id(&tx_id);
tx_status_sender.send_squeezed_out(tx_id, Error::SqueezedOut(reason.clone()));
for dependent_tx in rem.iter() {
if tx_id != &dependent_tx.id() {
tx_status_sender.send_squeezed_out(dependent_tx.id(), Error::Removed);
if tx_id != dependent_tx.id() {
tx_status_sender.send_squeezed_out(
dependent_tx.id(),
Error::SqueezedOut(
format!("Parent transaction with {tx_id}, was removed because of the {reason}")
)
);
}
}
removed.extend(rem.into_iter());
Expand Down
6 changes: 6 additions & 0 deletions crates/types/src/entities/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,9 @@ impl From<Salt> for ContractsInfoTypeV1 {
Self { salt }
}
}

impl From<Salt> for ContractsInfoType {
fn from(salt: Salt) -> Self {
ContractsInfoType::V1(salt.into())
}
}
125 changes: 121 additions & 4 deletions tests/tests/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,15 @@ use crate::helpers::{
TestContext,
TestSetupBuilder,
};
use fuel_core::service::{
Config,
FuelService,
use fuel_core::{
database::{
database_description::on_chain::OnChain,
Database,
},
service::{
Config,
FuelService,
},
};
use fuel_core_client::client::{
pagination::{
Expand All @@ -18,12 +24,123 @@ use fuel_core_types::{
fuel_asm::*,
fuel_tx::*,
fuel_types::canonical::Serialize,
fuel_vm::*,
fuel_vm::{
checked_transaction::IntoChecked,
*,
},
};
use rand::SeedableRng;

use fuel_core::chain_config::{
ChainConfig,
CoinConfig,
StateConfig,
};
use rstest::rstest;

const SEED: u64 = 2322;

#[tokio::test]
async fn calling_the_contract_with_enabled_utxo_validation_is_successful() {
let mut rng = rand::rngs::StdRng::seed_from_u64(0xBAADF00D);
let secret = SecretKey::random(&mut rng);
let amount = 10000;
let owner = Input::owner(&secret.public_key());
let utxo_id_1 = UtxoId::new([1; 32].into(), 0);
let utxo_id_2 = UtxoId::new([1; 32].into(), 1);

let config = Config {
debug: true,
utxo_validation: true,
chain_conf: ChainConfig {
initial_state: Some(StateConfig {
coins: Some(vec![
CoinConfig {
tx_id: Some(*utxo_id_1.tx_id()),
output_index: Some(utxo_id_1.output_index()),
owner,
amount,
asset_id: AssetId::BASE,
..Default::default()
},
CoinConfig {
tx_id: Some(*utxo_id_2.tx_id()),
output_index: Some(utxo_id_2.output_index()),
owner,
amount,
asset_id: AssetId::BASE,
..Default::default()
},
]),
..Default::default()
}),
..ChainConfig::local_testnet()
},
..Config::local_node()
};
let node = FuelService::from_database(Database::<OnChain>::in_memory(), config)
.await
.unwrap();
let client = FuelClient::from(node.bound_address);

// Given
let contract_input = {
let bytecode: Witness = vec![].into();
let salt = Salt::zeroed();
let contract = Contract::from(bytecode.as_ref());
let code_root = contract.root();
let balance_root = Contract::default_state_root();
let state_root = Contract::default_state_root();

let contract_id = contract.id(&salt, &code_root, &state_root);
let output = Output::contract_created(contract_id, state_root);
let create_tx = TransactionBuilder::create(bytecode, salt, vec![])
.add_unsigned_coin_input(
secret,
utxo_id_1,
amount,
Default::default(),
Default::default(),
)
.add_output(output)
.finalize_as_transaction()
.into_checked(Default::default(), &Default::default())
.expect("Cannot check transaction");

let contract_input = Input::contract(
UtxoId::new(create_tx.id(), 1),
balance_root,
state_root,
Default::default(),
contract_id,
);

client
.submit_and_await_commit(create_tx.transaction())
.await
.expect("cannot insert tx into transaction pool");

contract_input
};

// When
let contract_tx = TransactionBuilder::script(vec![], vec![])
.add_input(contract_input)
.add_unsigned_coin_input(
secret,
utxo_id_2,
amount,
Default::default(),
Default::default(),
)
.add_output(Output::contract(0, Default::default(), Default::default()))
.finalize_as_transaction();
let tx_status = client.submit_and_await_commit(&contract_tx).await.unwrap();

// Then
assert!(matches!(tx_status, TransactionStatus::Success { .. }));
}

#[rstest]
#[tokio::test]
async fn test_contract_balance(
Expand Down
Loading