FuelLabs / fuel-core

Rust full node implementation of the Fuel v2 protocol.
Other
58.17k stars 2.76k forks source link

Rework TxPool dependency graph to avoid crashes and strange behaviours #1961

Open xgreenx opened 3 months ago

xgreenx commented 3 months ago

Overview

TxPool transactions management logic is complex and smeared across several files, leading to inconsistent updates.

As an example, it is possible to cause a crash on the line below:

image

Because the remove function missed some dependent transactions from the by_dependency field.

Solution

We need to rework the transaction pool and incorporate the logic of the transaction management side of one structure. Taking into account the work required for https://github.com/FuelLabs/fuel-core/issues/1229 and https://github.com/FuelLabs/fuel-core/issues/868, it seems we can simplify the logic of how transactions are validated. We don't need to replace transactions based on tips. We just can keep them in memory in another dependency graph.

More simple rules should reduce room for error.

xgreenx commented 1 month ago

Adding test for the panic

#[tokio::test]
async fn poc_txpool_dos() {
    let mut context = TextContext::default();

    // tx1 {inputs: {}, outputs: {coinA}, tip: 1}
    let (_, gas_coin) = context.setup_coin();
    let (output_a, unset_input) = context.create_output_and_input(1);
    let tx1 = TransactionBuilder::script(vec![], vec![])
        .tip(1)
        .max_fee_limit(1)
        .script_gas_limit(GAS_LIMIT)
        .add_input(gas_coin)
        .add_output(output_a)
        .finalize_as_transaction();

    // tx2 {inputs: {coinA}, outputs: {coinB}, tip: 1}
    let (_, gas_coin) = context.setup_coin();
    let input_a = unset_input.into_input(UtxoId::new(tx1.id(&Default::default()), 0));
    let (output_b, unset_input) = context.create_output_and_input(1);
    let tx2 = TransactionBuilder::script(vec![], vec![])
        .tip(1)
        .max_fee_limit(1)
        .script_gas_limit(GAS_LIMIT)
        .add_input(input_a.clone())
        .add_input(gas_coin)
        .add_output(output_b)
        .finalize_as_transaction();

    // tx3 {inputs: {coinA, coinB}, outputs:{}, tip: 20}
    let (_, gas_coin) = context.setup_coin();
    let input_b = unset_input.into_input(UtxoId::new(tx2.id(&Default::default()), 0));
    let tx3 = TransactionBuilder::script(vec![], vec![])
        .tip(20)
        .max_fee_limit(20)
        .script_gas_limit(GAS_LIMIT)
        .add_input(input_a)
        .add_input(input_b.clone())
        .add_input(gas_coin)
        .finalize_as_transaction();

    let (_, gas_coin) = context.setup_coin();
    let tx4 = TransactionBuilder::script(vec![], vec![])
        .tip(30)
        .max_fee_limit(30)
        .script_gas_limit(GAS_LIMIT)
        .add_input(input_b)
        .add_input(gas_coin)
        .finalize_as_transaction();

    let mut txpool = context.build();
    let tx1 = check_unwrap_tx(tx1, &txpool.config).await;
    let tx2 = check_unwrap_tx(tx2, &txpool.config).await;
    let tx3 = check_unwrap_tx(tx3, &txpool.config).await;
    let tx4 = check_unwrap_tx(tx4, &txpool.config).await;

    txpool
        .insert_single(tx1.clone())
        .expect("Tx1 should be OK, got Err");
    txpool
        .insert_single(tx2.clone())
        .expect("Tx2 should be OK, got Err");
    // Inserting tx3 should fail
    txpool
        .insert_single(tx3.clone())
        .expect("Tx3 insertion should be OK and have removed T2, got Err");
    // Since tx3 didn't fail, tx4 insertion tries to unwrap non-existance resource, and panics
    let _ = txpool.insert_single(tx4.clone());
}