Return error from TxPool level if the `BlobId` is known #2067

xgreenx · 2024-08-09T13:59:32Z

Return error from TxPool level if the BlobId is known. Currently, we only return it from the block producer level.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

xgreenx · 2024-08-09T14:01:23Z

crates/services/txpool/src/containers/dependency.rs

            messages: HashMap::new(),
            max_depth,
            utxo_validation,
        }
    }

-    /// find all dependent Transactions that are inside txpool.
-    /// Does not check db. They can be sorted by gasPrice to get order of dependency
-    pub(crate) fn find_dependent(


It is not used anywhere, so I removed it=)

MitchTurner · 2024-08-09T17:26:22Z

crates/services/txpool/src/containers/dependency.rs

+            }
+
+            if let Some(state) = self.blobs.get(blob_id) {
+                if state.tip >= tx.tip() {


This doesn't seem like a "collision" error, but we are returning NotInsertedCollisionBlobId

If new transaction that deploys the same Blob has lower tip, it is a collision. We can't have two transactions that deploy the same Blob

Sure. It's a collision if it's < as well though. What we're really doing here is choosing which tx we like more. To the end user the collision error is helpful, I guess, and not inaccurate. NotInsertedCollisionBlobIdTipLowerThanOtherTx would be more encompassing.

I guess I'm having a hard time parsing this function. It would help if it was a little more declarative.

It would be nice to change all other places as well with a better name. But it is not part of this PR and will be addressed during the refactoring of the TxPool=) You can check https://github.com/FuelLabs/fuel-core/tree/feature/txpool-v2 it already uses better names.

crates/services/txpool/src/containers/dependency.rs

MitchTurner · 2024-08-09T19:59:41Z

crates/services/txpool/src/containers/dependency.rs

@@ -268,7 +222,34 @@ impl Dependency {
        let mut max_depth = 0;
        let mut db_coins: HashMap<UtxoId, CoinState> = HashMap::new();
        let mut db_contracts: HashMap<ContractId, ContractState> = HashMap::new();
+        let mut db_blobs: HashMap<BlobId, BlobState> = HashMap::new();


This is missing from the docstring.

nit: it shouldn't be a docstring and should just be a comment since it's private.

MitchTurner · 2024-08-09T20:02:39Z

crates/services/txpool/src/containers/dependency.rs

@@ -268,7 +222,34 @@ impl Dependency {
        let mut max_depth = 0;
        let mut db_coins: HashMap<UtxoId, CoinState> = HashMap::new();
        let mut db_contracts: HashMap<ContractId, ContractState> = HashMap::new();
+        let mut db_blobs: HashMap<BlobId, BlobState> = HashMap::new();


Also, does this need to be a HashMap? Couldn't it just be an Option<(BlobId, BlobState)>? We shouldn't ever insert more than one.

Not as relevant considering comment below.

MitchTurner · 2024-08-09T20:07:26Z

crates/services/txpool/src/containers/dependency.rs

@@ -475,7 +463,7 @@ impl Dependency {
    where
        DB: TxPoolDb,
    {
-        let (max_depth, db_coins, db_contracts, db_messages, collided) =
+        let (max_depth, db_coins, db_contracts, db_blobs, db_messages, collided) =
            self.check_for_collision(txs, db, tx)?;


Do you know why check_for_collision is taking a dyn TxPoolDb instead of a generic DB like db here?

I don't know=) But let's leave it for the refactoring of the TxPool

MitchTurner · 2024-08-09T20:16:42Z

crates/services/txpool/src/containers/dependency.rs

+            }
+
+            if let Some(state) = self.blobs.get(blob_id) {
+                if state.tip >= tx.tip() {


Sure. It's a collision if it's < as well though. What we're really doing here is choosing which tx we like more. To the end user the collision error is helpful, I guess, and not inaccurate. NotInsertedCollisionBlobIdTipLowerThanOtherTx would be more encompassing.

I guess I'm having a hard time parsing this function. It would help if it was a little more declarative.

MitchTurner · 2024-08-09T20:17:54Z

crates/services/txpool/src/containers/dependency.rs

@@ -461,7 +442,14 @@ impl Dependency {
            // collision of other outputs is not possible.
        }

-        Ok((max_depth, db_coins, db_contracts, db_messages, collided))
+        Ok((


The name is check_for_collision. I'd expect a function named that to return Result<()> or maybe Result<Vec<TxId>>, but it's returning a bunch of HashMaps as well. Ideally we don't have single function responsible for multiple things, but at the very least we could name the function something more accurate.

I'd prefer to move the db extension logic to separate functions.

I just did it the same way it had worked before. I don't see a point in refactoring the code that will be removed soon during the refactoring of the TxPool.

MitchTurner · 2024-08-09T20:23:17Z

crates/services/txpool/src/txpool/tests.rs

@@ -1421,3 +1355,139 @@ async fn predicate_that_returns_false_is_invalid() {
        "unexpected error: {err}",
    )
 }
+
+#[tokio::test]
+async fn insert_blob_works() {


Suggested change

async fn insert_blob_works() {

async fn insert_single__blob_tx_works() {

MitchTurner · 2024-08-09T20:23:38Z

crates/services/txpool/src/txpool/tests.rs

+    let result = txpool.insert_single(tx);
+
+    // Then
+    let _ = result.expect("Should insert blob");


Can we check that the tx is included rather than looking for an Ok?

MitchTurner · 2024-08-09T20:24:44Z

crates/services/txpool/src/txpool/tests.rs

+}
+
+#[tokio::test]
+async fn insert_the_same_blob_fails_in_txpool_level() {


Suggested change

async fn insert_the_same_blob_fails_in_txpool_level() {

async fn insert_single__blob_tx_fails_if_blob_already_inserted_and_lower_tip() {

MitchTurner · 2024-08-09T20:25:58Z

crates/services/txpool/src/txpool/tests.rs

+}
+
+#[tokio::test]
+async fn insert_the_same_blob_works_with_higher_tip() {


Suggested change

async fn insert_the_same_blob_works_with_higher_tip() {

async fn insert_single__blob_tx_succeeds_if_blob_already_inserted_but_higher_tip() {

MitchTurner · 2024-08-09T20:29:06Z

crates/services/txpool/src/txpool/tests.rs

+}
+
+#[tokio::test]
+async fn insert_the_same_blob_fails_in_database_level() {


Suggested change

async fn insert_the_same_blob_fails_in_database_level() {

async fn insert_single__blob_tx_fails_if_blob_already_exists_in_database() {

@MitchTurner

### Added - [2061](#2061): Allow querying filled transaction body from the status. ### Changed - [2067](#2067): Return error from TxPool level if the `BlobId` is known. - [2064](#2064): Allow gas price metadata values to be overridden with config ### Fixes - [2060](#2060): Use `min-gas-price` as a starting point if `start-gas-price` is zero. - [2059](#2059): Remove unwrap that is breaking backwards compatibility - [2063](#2063): Don't use historical view during dry run. ## What's Changed * Bugfix: Remove unwrap in Consensus Param conversion by @MitchTurner in #2059 * Don't use historical view during dry run by @xgreenx in #2063 * Weekly `cargo update` by @github-actions in #2050 * Allow gas price metadata values to be overridden with config by @MitchTurner in #2064 * Use `min-gas-price` as a starting point if `start-gas-price` is zero by @xgreenx in #2060 * Allow querying transaction from the status by @xgreenx in #2061 * Return error from TxPool level if the `BlobId` is known by @xgreenx in #2067 **Full Changelog**: v0.32.0...v0.32.1

Return error from TxPool level if the BlobId is known

d7e995b

xgreenx requested a review from a team August 9, 2024 13:59

xgreenx self-assigned this Aug 9, 2024

Update CHANGELOG.md

bd8897f

xgreenx commented Aug 9, 2024

View reviewed changes

Fixed integration test

48717b0

danielbate mentioned this pull request Aug 9, 2024

feat!: fuel-core@0.32.1 and large contract deployments FuelLabs/fuels-ts#2827

Merged

4 tasks

Merge branch 'master' into feature/return-error-for-the-same-blob-id

737bdbe

MitchTurner reviewed Aug 9, 2024

View reviewed changes

crates/services/txpool/src/containers/dependency.rs Show resolved Hide resolved

MitchTurner reviewed Aug 9, 2024

View reviewed changes

xgreenx and others added 2 commits August 9, 2024 22:53

Merge branch 'master' into feature/return-error-for-the-same-blob-id

76fa525

Applied comments from the PR

9e6391d

xgreenx requested review from MitchTurner and a team August 9, 2024 20:53

MitchTurner approved these changes Aug 9, 2024

View reviewed changes

MitchTurner merged commit 279b123 into master Aug 9, 2024
34 checks passed

MitchTurner deleted the feature/return-error-for-the-same-blob-id branch August 9, 2024 21:54

MitchTurner mentioned this pull request Aug 9, 2024

Version 0.32.1 #2069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error from TxPool level if the `BlobId` is known #2067

Return error from TxPool level if the `BlobId` is known #2067

xgreenx commented Aug 9, 2024

xgreenx Aug 9, 2024

MitchTurner Aug 9, 2024

xgreenx Aug 9, 2024

MitchTurner Aug 9, 2024

xgreenx Aug 9, 2024

MitchTurner Aug 9, 2024

MitchTurner Aug 9, 2024

MitchTurner Aug 9, 2024

MitchTurner Aug 9, 2024

xgreenx Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024

MitchTurner Aug 9, 2024 •

edited

Loading

xgreenx Aug 9, 2024

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

	async fn insert_blob_works() {
	async fn insert_single__blob_tx_works() {

	async fn insert_the_same_blob_fails_in_txpool_level() {
	async fn insert_single__blob_tx_fails_if_blob_already_inserted_and_lower_tip() {

	async fn insert_the_same_blob_works_with_higher_tip() {
	async fn insert_single__blob_tx_succeeds_if_blob_already_inserted_but_higher_tip() {

	async fn insert_the_same_blob_fails_in_database_level() {
	async fn insert_single__blob_tx_fails_if_blob_already_exists_in_database() {

Return error from TxPool level if the BlobId is known #2067

Return error from TxPool level if the BlobId is known #2067

Conversation

xgreenx commented Aug 9, 2024

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

MitchTurner Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

MitchTurner Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Return error from TxPool level if the `BlobId` is known #2067

Return error from TxPool level if the `BlobId` is known #2067

xgreenx Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading

MitchTurner Aug 9, 2024 •

edited

Loading