Increase usage of new Fuel_txpool

[ControlCplusControlV]

I've changed the submit tx and find tx methods to use the new fuel_txpool rather than the old one as part of #152

One note I do want to make is that I currently use an ugly fallback method where if includable doesn't return the tx's correctly I default to previous behavior. I really did try to avoid this behavior but after spending so long diving through the codebase I think it should be a seperate issue as it will take significantly more time to fix and seems to be a bug with precompute_metadata as on only some transactions that are passed in that lack metadata precompute_metadata seems to be adding potentially wrong info, which results in them not being returned by includable, specifically in the

    tx::get_owned_transactions
    tx::get_transactions

tests where this behavior was observed.

Otherwise submit_tx will use this new behavior adding transactions to the new txpool, and find tx performs a quick check in the mempool, although that won't be used yet until tx's can actually persist in the mempool for any amount of time (we execute them right away currently)

[rakita]

You need to check the result of the insert, tx needs to be valid to be included inside the pool.
Maybe we can make the insert and insert_vec functions to better facilitate this.

[ControlCplusControlV]

So on the tests that currently require the fallback (ie don't work with the new txpool) it is because I am running into the [Err(Transaction is not inserted. UTXO is not existing: 0x99dd7fc1ad584d9b174275ef9de7bda04fc61e38899fdce22fd31a49f3fc47d6)] error, which seems to stem from TestContext's transfer method generating a utxo_id: via self.rng.gen(), so when precompute_metadata is called it requests an input transaction which doesn't exist causing it to not be inserted.

Or do you just mean runtime validation that inserting was successful?

[rakita]

yea, there are not check there and data can't be random, it is expected for them to fail. Changing those test to reflect error seems resonable.

runtime validation is needed here, you need to return error there for user to know what is happening.

[Voxelot]

Hmm, we've been spoofing utxos in tests in the early days, but it seems like the txpool actually validates their existence. Currently in other parts of the system, we validate utxo existence only when the utxo-validation flag is enabled. This is because full validation will be a fairly breaking change to all the downstream test suites in the SDK and in sway, so we've been holding off on enabling utxo-validation by default until they are adapted to initialize coins properly.

Given that integrating this new txpool would conflict with our current utxo-validation default being false, we should gate insertion on the new txpool using the same utxo-validation feature flag. That way we can begin writing tests with utxo-validation enabled which will go through this txpool, without breaking everything else downstream.

[Voxelot]

Another note is that the utxo lookup mechanism used by the txpool will need to be adjusted, since it looks up coins only from other pre-existing transactions, instead of directly checking the utxo set. This will cause issues for us since we need to be able to add coins directly to the utxo set from the genesis block without an associated transaction.

It's also an issue because the current approach doesn't take the spent status of those coins into account.

[Voxelot]

I'll make a new pr to fix the utxo lookup issue I described above

[ControlCplusControlV]

@rakita I just had a quick question as I am new to rust errors. Looking through it appears I would need to make a new error along the lines of Tx insertion failed, or is there a way to just pass the error to the result? When I try to do so I end up being unable with the error expected enum tx_pool::Error, found struct anyhow::Error`?

[rakita]

@Voxelot Yea, i should have used utxo db table for this, it seems like straightforward fix, if you want I can do it.

@ControlCplusControlV in general we should probably change error type in fuel-core to return anyhow::Error as we dont care what error is we just want to propagate it to user. But for fuel-txpool i feel like we need to chainge it to proper error type.
In rust you have two main error libs thiserror and anyhow this is good read: https://github.com/dtolnay/anyhow

There is good comparison there:

Use Anyhow if you don't care what error type your functions return, you just want it to be easy. This is common in
application code. Use [thiserror](https://github.com/dtolnay/thiserror) if you are a library that wants to design your own 
dedicated error type(s) so that on failures the caller gets exactly the information that you choose.

[Voxelot]

@rakita feel free to go ahead and make the fix, I haven't started on it yet.

As a side-note, is there any need for UtxoId to be still be txid + out_idx after this fix? It may help consolidate things like deposit coins vs utxos into the same db table if the id was just a simple 32 byte hash.

[rakita]

will make bug issue.

txid+out_idx is still relevent and helpfull for dependencies

[adlerjohn]

converting to draft since still WIP

[ControlCplusControlV]

@rakita the error handling reading was very helpful, I now understand how Rust and fuel-core by extension is handling errors now. Changing the return type of submit_tx seemed to have lots of downstream implications even if it was just the error, so I added a value to Error which just wraps the value of anyhow::Error

Also wanted to thank Voxelot and rakita for helping me so much with this first PR, and if all is well on this one, hopefully my next PR will be much faster.

[ControlCplusControlV]

Marked as ready for review as it should have both error handling and utxo_validation feature switching now, if it isn't ready though will move back to draft

[Voxelot]

Up till now we had unit tests to cover any cases where the utxo_validation setting was used. But we don't have any integration tests with utxo_validation enabled, so these changes currently aren't being tested. We'll need to update the fuel-tests/tests/tx.rs to use properly generated and signed transactions (see TransactionBuilder from fuel-tx) and enable utxo-validation there to ensure we don't break the SDK now that they have utxo-validation enabled by default. Or add unit tests here in txpool.rs to verify the new txpool is happy with the changes.

There are examples of creating properly valid transactions that pass utxo_validation in the fuel-core/src/executor.rs tests.

[ControlCplusControlV]

Ok, will check those out then and add in a test to make sure the utxo_validation submit_tx is working fully then, as well as fix the current generic issue with anyhow errors

[ControlCplusControlV]

@Voxelot Let me know if you want to change the test transaction as I know using just Default may not be ideal, submit_tx test was always working though even with utxo_validation turned on, so I modeled my test closer to that, with a similar test that just ensures the utxo_validation flag turned on.

Decided to put it in fuel-tests as since its an endpoint I felt it made the most sense for it to be tested as an external piece rather than an internal componet.

[Voxelot]

Should the test verify that the transaction was actually included in a block, (i.e. check the TransactionStatus and verify its' successful)? Just returning the transaction object submitted via the API isn't a guarantee that the node is operating properly with the new txpool.

[ControlCplusControlV]

submit_tx also checks this way, however I definitely agree the block is a better source of truth than just a returned transaction, will modify my test to check the block

[ControlCplusControlV]

Test should check to ensure the transaction was actually included in the block now

[Voxelot]

Transparent transaction is just a different client encoding type (json instead of binary). We should actually check the TransactionStatus variant if it's successful. client.transaction_status

[Voxelot]

TransactionStatus looks like this:

pub enum TransactionStatus {
    Submitted {
        submitted_at: DateTime<Utc>,
    },
    Success {
        block_id: String,
        time: DateTime<Utc>,
        program_state: ProgramState,
    },
    Failure {
        block_id: String,
        time: DateTime<Utc>,
        reason: String,
        program_state: Option<ProgramState>,
    },
}

So we can confirm it was included in a block if we get the Success variant and the block_id is real.

[Voxelot]

We should also test that the pool is still well behaved after submitting multiple transactions, since we had to add logic in this PR to manually remove txs from the pool after including them in a block. If we don't verify all these things properly then it could break the SDK if we release this.

[rakita]

We should also test that the pool is still well behaved after submitting multiple transactions, since we had to add logic in this PR to manually remove txs from the pool after including them in a block. If we don't verify all these things properly then it could break the SDK if we release this.

If there is bug in txpool, shouldn't we just fix it if problem arises? Usage by sdk will be good test.

[ControlCplusControlV]

Test now

• Runs with 10 transactions to ensure mempool is working and enduring (variation between tx's atm is gas price, if they need to be more complex lmk)
• Transaction Status is checked to make sure each transaction was a success
• The block_id of each transaction is then checked to ensure that the block its from actually exists

[ControlCplusControlV]

I have the ability to merge this, can I merge or do I need a second approval?

[adlerjohn]

Ideally let's get a review from @rakita as the txpool OP

[rakita]

@Voxelot this seems like it needs a rework, updating status before it is executed is not a good flow, everything before block/tx inclusion shouldn't leave any trace in database, only after block is included we could have status of it.
Expected status of transaction is:

• Pending (inside txpool)
• Included (included in chain)

Is sdk using this information? Maybe we need to change how we get status, and do it in the same as it is done in TxQuery::transaction

[Voxelot]

I agree this needs rework and this was called out on the original task:

Update transaction status API to fallback to the txpool if the status doesn't exist in the database, using the Submitted status

I'm assuming there will be follow-up PR's to finish the rest of the planned work? @ControlCplusControlV

[ControlCplusControlV]

@Voxelot yes I am planning on using a fallback based on tx status rather than existence in a separate PR, as I figured there may be some overlap with #183 in part atleast, by tackling

if the transaction was very invalid (i.e. not includable in a block) this seems like it might need to be separate type of error from a tx that was partially processed within a block (i.e. utxos were spent and outputs created, but the script panicked).

As I could generate the first status based on if it errors when inserting into the tx pool (which would require utxo verification to be on to determine if a tx is invalid) and then the second would be a result of a failure but the tx was includable.

Unless you would rather that separate PR just cover lookup in mempool by status?

[rakita]

lgtm