Fix Transaction Status API #263

ControlCplusControlV · 2022-04-14T03:32:08Z

Closes #154

This updates the transaction status api to use the mempool as a shortcut to determine transaction status, as if a transaction is in the mempool it's status is submitted. There is a small TODO where the time used for the submitted status is the current time of when the status if fetched rather than the actual time it was submitted.

The dependent_transactions api was shelved as I understand it isn't that important, and I thought that if it is implemented it may make more sense to do with once utxo_validation is turned on.

Voxelot · 2022-04-14T17:36:26Z

fuel-core/src/schema/tx/types.rs

+
+        let tx_pooldb = Box::new(db.clone()) as Box<dyn TxPoolDb>;
+
+        let txpool = TxPoolService::new(tx_pooldb, Arc::new(Config::default()));


Isn't this creating an brand new instance of a txpool on each status request? We should be fetching the shared txpool out of the ctx like we do with the db. The db backs the txpool, but a lot of the data in the txpool is managed in memory, so creating an ephemeral txpool like this will not share any data with the txpool used in other places.

I wasn't sure if this was the best way to access the txpool, I went with this approach as I wanted to avoid side effects (hence the clone), but I see why this could be extremely expensive and how the shared data could end up being helpful, so I will move this to fetch from the context

The clone is of the db, which is just cloning a pointer and virtually has no cost. The issue is that creating a new txpool instance wouldn't know about the transaction submitted by the user, as it's a different instance with it's own hashmaps in memory. So the transaction the user is looking for would never exist in the temp pool you're making here.

Ok, will keep that in mind when deciding between whether to clone or pull in an object from the context.

What is the best way to fetch the TxPool from a context - let txpool = ctx.data::<Arc<dyn TxPool>>().unwrap(); however this results in many tests breaking with Custom { kind: Other, error: Curl("Send failed since rewinding of the data stream failed"): Send failed since rewinding of the data stream failed }, what would be the correct way to fetch the TxPool or TxPoolService from the current context?

It's injected into the context here: https://github.com/FuelLabs/fuel-core/blob/master/fuel-core/src/service/graph_api.rs#L37

So based on the type use there it would be the concrete Arc<TxPool> instead of dyn TxPool.

Voxelot · 2022-04-14T17:36:41Z

tests?

ControlCplusControlV · 2022-04-14T22:47:20Z

Oh I thought the transaction status api unit tests would cover this, however I will in one to specifically test this new behavior and make sure it is behaving properly

ControlCplusControlV · 2022-04-17T03:09:13Z

I've been running into some conversion issues. I noticed the submit_utxo_verified_tx test was actually able to catch a bug (with the api returning submitted too much), and the get_transaction_by_id test as well, so I was looking to base my new test off one of them. However, submit_tx on FuelClient currently deletes a transaction immediately after adding it to the txpool, and while to test the Submitted status I need a transaction that is currently inside the txpool. Then when testing with initializing just a txpool from TxPoolService where it is possible to insert a tx without deleting it, the Transaction objects I am using are of fuel_tx::Transaction and cannot be converted to the type where their status is checkable through the status api (fuel_core::schema::tx::types::Transaction )

So my question boils down to how would this behavior be tested? As the only way to get a transaction from FuelClient which is of the type I could check its status using the api would mean its not in the txpool (so it couldn't be submitted, as we are deleting transactions from the txpool atomically), and using a fresh TxPoolService doesn't seem to make the transaction accessible in the correct type to check its status via the api.

ControlCplusControlV · 2022-04-18T22:40:44Z

So the Submitted Status isn't currently testable with the new fix without significant modification to they way the txpool currently works. There is still some coverage testing Success and Failed, but there are no current calls which would result in a Submitted status because submit_tx deletes and executes a transaction from the txpool as soon as its added. The TxPool isn't directly accessible either otherwise I would do a quick test to insert a tx and check status.

I have been able to test it though by removing the block execution and deletion of the tx from txpool by commenting out parts of submit_tx. I would prefer to make tests for this to be made a new issue for when tx's will persist in the mempool as it will be a much more natural test, and as of now I haven't found a way to test this update without changes other parts of the Client.

rakita · 2022-04-19T05:31:09Z

fuel-core/src/schema/tx/types.rs

+
+        let txpool = ctx.data::<Arc<TxPool>>().unwrap().pool();
+
+        let transaction_in_pool = txpool.find(&[self.0.id()]).await;


You can just extend TxPool to support find_one function it should be easy to add as you just need to see how find is made, this should simplify a bit logic here.
Other than that this seems okay.

Voxelot · 2022-04-19T08:41:40Z

You could include the test that passed when you modified the txpool and add the #[ignored] annotation with a todo linking to #50, so that when that task is implemented the test can be re-enabled.

ControlCplusControlV · 2022-04-19T23:11:30Z

I've added an empty test with a todo!() macro and explanation of what the test should do for testing the Submitted status and a link to the issue which is blocking it, then ignored the test the todo!()

…tus api finally working

ControlCplusControlV · 2022-04-22T21:41:21Z

So to explain this (hopefully) final commit

Moves to new find_one method
Extends TxPool trait in fuel_core_interface/src/txpool.rs to use the new TxInfo struct rather than just Arc<Tx> as mentioned to me by @rakita . This required TxInfo being moved to avoid a circular dependency between fuel-core-interfaces and fuel-txpool, which now resides in fuel-core-interfaces as its a type so I thought it was most fitting there.
Fixes time so that the Submitted Status returned has the accurate time of when the transaction was submitted
Added a mock test with annotations to an issue and a note to add in an automatic unit test for when execution is decoupled from submit_tx, so that Submitted can be tested in the future when it's a reachable status

I have re-done manual testing as well to ensure Submitted still works properly after this change (modifying submit_tx in submit_utxo_verified tx test to leave transactions in the txpool, and then checking their status)

rakita · 2022-04-23T09:05:07Z

fuel-core-interfaces/src/info.rs

+use fuel_tx::Transaction;
+use std::sync::Arc;
+
+pub type ArcTx = Arc<Transaction>;

 #[derive(Debug, Clone)]
 pub struct TxInfo {


nit: I would just move this file to fuel-core-interfaces/src/model/info.rs and maybe even rename it to txpool.rs it feels like modules specific to txpool, that is why a little bit a specific naming of file.
other than that lgtm :)

Ok, I wasn't sure what name so I chose info but I agreetxpool is better to make it more clear in what it does and easier to find. It is now moved it to fuel-core-interfaces/src/model/txpool.rs

fuel-core/src/schema/tx/types.rs

Co-authored-by: Brandon Kite <brandonkite92@gmail.com>

Voxelot · 2022-04-27T22:30:44Z

fuel-core/src/schema/tx/types.rs

+        if transaction_in_pool.is_some() {
+            let time = transaction_in_pool.unwrap().submited_time();


[nit] use if-let syntax to avoid .is_some followed by unwrap.

ControlCplusControlV added 3 commits April 13, 2022 17:26

Changes re-done to be more in line with things

a7340b6

Removed dependent_txs

936dbfb

Cleaned up code

3ef1c1c

ControlCplusControlV requested review from Voxelot and rakita April 14, 2022 03:32

ControlCplusControlV self-assigned this Apr 14, 2022

ControlCplusControlV added 2 commits April 13, 2022 21:32

Removed dependent_txs

dca62b0

Fmt + Clippy

601425a

Voxelot reviewed Apr 14, 2022

View reviewed changes

ControlCplusControlV marked this pull request as draft April 15, 2022 03:20

Fixed minor bug, test almost done

5f84599

ControlCplusControlV and others added 3 commits April 18, 2022 16:33

cleanup and remove test

c07af7f

last fix

b8ccda6

Merge branch 'master' into controlc/152_pt2

5173669

ControlCplusControlV marked this pull request as ready for review April 18, 2022 22:41

rakita reviewed Apr 19, 2022

View reviewed changes

rakita mentioned this pull request Apr 19, 2022

Add txpool TxInfo time submitted #275

Merged

ControlCplusControlV and others added 3 commits April 19, 2022 17:04

Test todo!()

b3a430c

Test annotation

6989750

Merge branch 'master' into controlc/152_pt2

a1ba2dd

ControlCplusControlV added 4 commits April 20, 2022 12:17

Merge remote-tracking branch 'origin/master' into controlc/152_pt2

8e68f9f

minor fix

750812b

fmt

de9b918

Use find_one, use correct time, TxInfo refactor, ArcTx Move, also sta…

20d05cd

…tus api finally working

ControlCplusControlV added 3 commits April 22, 2022 15:43

Weird fmt fix

32e0059

forgot git add

f2d29bb

mod fmt is hard

33c54bc

ControlCplusControlV requested review from rakita and Voxelot April 22, 2022 22:07

rakita reviewed Apr 23, 2022

View reviewed changes

ControlCplusControlV added 2 commits April 23, 2022 18:33

rename+mv file

844f40e

rename+mv file

a3b972e

ControlCplusControlV requested a review from rakita April 25, 2022 22:02

Voxelot reviewed Apr 26, 2022

View reviewed changes

fuel-core/src/schema/tx/types.rs Outdated Show resolved Hide resolved

Voxelot reviewed Apr 26, 2022

View reviewed changes

fuel-core/src/schema/tx/types.rs Outdated Show resolved Hide resolved

ControlCplusControlV and others added 5 commits April 26, 2022 18:10

Fixed import

6c063e4

Remove redundant check

7ab9b57

Co-authored-by: Brandon Kite <brandonkite92@gmail.com>

Merge branch 'master' into controlc/152_pt2

c984ae7

Fixed Remove bug

74104af

Merge branch 'master' into controlc/152_pt2

19201cf

ControlCplusControlV requested a review from Voxelot April 27, 2022 21:53

Voxelot reviewed Apr 27, 2022

View reviewed changes

Voxelot approved these changes Apr 27, 2022

View reviewed changes

ControlCplusControlV merged commit 0b78de5 into master Apr 27, 2022

ControlCplusControlV deleted the controlc/152_pt2 branch April 27, 2022 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Transaction Status API #263

Fix Transaction Status API #263

ControlCplusControlV commented Apr 14, 2022

Voxelot Apr 14, 2022

ControlCplusControlV Apr 14, 2022

Voxelot Apr 14, 2022 •

edited

Loading

ControlCplusControlV Apr 15, 2022

Voxelot Apr 15, 2022

Voxelot commented Apr 14, 2022

ControlCplusControlV commented Apr 14, 2022

ControlCplusControlV commented Apr 17, 2022

ControlCplusControlV commented Apr 18, 2022

rakita Apr 19, 2022

Voxelot commented Apr 19, 2022

ControlCplusControlV commented Apr 19, 2022

ControlCplusControlV commented Apr 22, 2022

rakita Apr 23, 2022

ControlCplusControlV Apr 24, 2022

Voxelot Apr 27, 2022


		let tx_pooldb = Box::new(db.clone()) as Box<dyn TxPoolDb>;

		let txpool = TxPoolService::new(tx_pooldb, Arc::new(Config::default()));


		let txpool = ctx.data::<Arc<TxPool>>().unwrap().pool();

		let transaction_in_pool = txpool.find(&[self.0.id()]).await;

		if transaction_in_pool.is_some() {
		let time = transaction_in_pool.unwrap().submited_time();

Fix Transaction Status API #263

Fix Transaction Status API #263

Conversation

ControlCplusControlV commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot commented Apr 14, 2022

ControlCplusControlV commented Apr 14, 2022

ControlCplusControlV commented Apr 17, 2022

ControlCplusControlV commented Apr 18, 2022

Choose a reason for hiding this comment

Voxelot commented Apr 19, 2022

ControlCplusControlV commented Apr 19, 2022

ControlCplusControlV commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Apr 14, 2022 •

edited

Loading