`Arc` definition in `TransactionPool` #7042

dharjeezy · 2025-01-04T16:43:35Z

michalkucharczyk · 2025-01-06T16:02:17Z

substrate/client/network/transactions/src/config.rs

 	/// Get hash of transaction.
-	fn hash_of(&self, transaction: &B::Extrinsic) -> H;
+	fn hash_of(&self, transaction: &Arc<B::Extrinsic>) -> H;


I think Arc is not needed here.

I mean we are already passing reference to the extrinsics data.

michalkucharczyk · 2025-01-06T16:23:46Z

substrate/client/network/transactions/src/config.rs

 	/// Import a transaction into the pool.
 	///
 	/// This will return future.
-	fn import(&self, transaction: B::Extrinsic) -> TransactionImportFuture;
+	fn import(&self, transaction: Arc<B::Extrinsic>) -> TransactionImportFuture;


This currently does not make sense. We would need to change submit_one/ submit_and_watch API to accept Arc.
Otherwise we create Arc, then dereference it and create new one within transcaction pool implementation.

michalkucharczyk · 2025-01-06T16:33:37Z

For the sake of the simplicity I would only rework transactions function in this PR - it makes sense not to copy/clone extrinsics data to propagate transactions over network.

hash_of IMO does not need Arc - it already takes ref.

When it comes to import - if we really want to change it, we could it in dedicated PR. But I think it is not really needed because submit_one/ submit_and_watch already take an owenership of the extrinsic's data so Arc is not helping here with anything.

michalkucharczyk · 2025-01-23T14:35:36Z

@dharjeezy are you planning to continue working on this?

dharjeezy · 2025-01-23T14:59:44Z

@dharjeezy are you planning to continue working on this?

yes. i will get it done through the week/weekend.

…tion-pool-trait

michalkucharczyk · 2025-01-25T20:47:46Z

substrate/client/network/transactions/src/lib.rs

@@ -471,7 +471,8 @@ where

 		debug!(target: LOG_TARGET, "Propagating transaction [{:?}]", hash);
 		if let Some(transaction) = self.transaction_pool.transaction(hash) {
-			let propagated_to = self.do_propagate_transactions(&[(hash.clone(), transaction)]);
+			let propagated_to =
+				self.do_propagate_transactions(&[(hash.clone(), Arc::new(transaction))]);


Could we avoid Arc::new here?

I think that this transaction method could also provide an Arc reference, here:

polkadot-sdk/substrate/client/service/src/lib.rs

Lines 562 to 567 in 682f8cd

fn transaction(&self, hash: &H) -> Option<B::Extrinsic> {

self.pool.ready_transaction(hash).and_then(

// Only propagable transactions should be resolved for network service.

|tx| if tx.is_propagable() { Some((**tx.data()).clone()) } else { None },

)

}

This is just method of the same trait (sc-network-transactions::config::TransactionPool), so should be easily doable:

polkadot-sdk/substrate/client/network/transactions/src/config.rs

Line 70 in 49c2f22

fn transaction(&self, hash: &H) -> Option<B::Extrinsic>;

Would you change it too?

michalkucharczyk · 2025-01-25T20:52:49Z

prdoc/pr_5978.prdoc

+title: `networking::TransactionPool` should accept `Arc`
+doc:
+- audience: Node Dev
+  description: The TransactionPool trait now accepts an `Arc` for transactions.


Suggested change

description: The TransactionPool trait now accepts an `Arc` for transactions.

description: The `sc_network_transactions::config::TransactionPool` trait now accepts an `Arc` for transactions.

michalkucharczyk · 2025-01-27T10:34:15Z

substrate/client/service/src/lib.rs

 		self.pool.ready_transaction(hash).and_then(
 			// Only propagable transactions should be resolved for network service.
-			|tx| if tx.is_propagable() { Some((**tx.data()).clone()) } else { None },
+			|tx| if tx.is_propagable() { Some(Arc::new((**tx.data()).clone())) } else { None },


We don't need new Arc, something like this should work:

Suggested change

|tx| if tx.is_propagable() { Some(Arc::new((**tx.data()).clone())) } else { None },

|tx| tx.is_propagable().then(|| { tx.data().clone() }),

michalkucharczyk · 2025-01-27T11:11:08Z

prdoc/pr_5978.prdoc

@@ -0,0 +1,9 @@
+title: `networking::TransactionPool` should accept `Arc`


Name of the file is incorrect. Please rename it to: pr_7042.prdoc

michalkucharczyk · 2025-01-27T11:13:10Z

prdoc/pr_5978.prdoc

+title: `networking::TransactionPool` should accept `Arc`
+doc:
+- audience: Node Dev
+  description: The `sc_network_transactions::config::TransactionPool` trait now accepts an `Arc` for transactions.


Suggested change

description: The `sc_network_transactions::config::TransactionPool` trait now accepts an `Arc` for transactions.

description: The `sc_network_transactions::config::TransactionPool` trait now returns an `Arc` for transactions.

michalkucharczyk · 2025-01-27T11:14:45Z

Looks good, thank you!

michalkucharczyk · 2025-01-27T11:53:18Z

bot fmt

command-bot · 2025-01-27T11:53:25Z

@michalkucharczyk https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8096196 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-652d0981-885a-4723-be4b-575df729f3ea to cancel this command or bot cancel to cancel all commands in this pull request.

github-actions · 2025-01-27T11:53:27Z

We have migrated the command bot to GHA

Please, see the new usage instructions here or here. Soon the old commands will be disabled.

command-bot · 2025-01-27T11:55:16Z

@michalkucharczyk Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8096196 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8096196/artifacts/download.

dharjeezy added 3 commits December 30, 2024 23:23

arc in transaction param for pool

66bfbf4

make use of an Arc

80d6d09

prdoc

33ae9c1

michalkucharczyk reviewed Jan 6, 2025

View reviewed changes

michalkucharczyk mentioned this pull request Jan 6, 2025

networking::TransactionPool should accept Arc. #5978

Closed

dharjeezy added 2 commits January 25, 2025 10:33

Merge remote-tracking branch 'origin/master' into dami/arc-in-transac…

12da149

…tion-pool-trait

resolving conversations

49c2f22

dharjeezy requested a review from michalkucharczyk January 25, 2025 10:05

michalkucharczyk reviewed Jan 25, 2025

View reviewed changes

dharjeezy added 2 commits January 27, 2025 11:14

nit conversations

8d409eb

fmt

c33da5a

dharjeezy requested a review from michalkucharczyk January 27, 2025 10:18

michalkucharczyk reviewed Jan 27, 2025

View reviewed changes

nit

3d8b91d

dharjeezy requested a review from michalkucharczyk January 27, 2025 10:52

michalkucharczyk reviewed Jan 27, 2025

View reviewed changes

michalkucharczyk added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Jan 27, 2025

rename pr doc

24f321f

michalkucharczyk reviewed Jan 27, 2025

View reviewed changes

nit

9fa2a0d

dharjeezy requested a review from michalkucharczyk January 27, 2025 11:14

michalkucharczyk approved these changes Jan 27, 2025

View reviewed changes

lexnv approved these changes Jan 27, 2025

View reviewed changes

iulianbarbu approved these changes Jan 27, 2025

View reviewed changes

".git/.scripts/commands/fmt/fmt.sh"

b6547d0

Merge branch 'master' into dami/arc-in-transaction-pool-trait

6039985

michalkucharczyk enabled auto-merge January 27, 2025 12:02

michalkucharczyk added this pull request to the merge queue Jan 27, 2025

Merged via the queue into paritytech:master with commit b2004ed Jan 27, 2025
201 of 205 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Arc` definition in `TransactionPool` #7042

`Arc` definition in `TransactionPool` #7042

dharjeezy commented Jan 4, 2025

michalkucharczyk Jan 6, 2025

michalkucharczyk Jan 6, 2025

bkchr Jan 6, 2025

michalkucharczyk Jan 6, 2025 •

edited

Loading

michalkucharczyk commented Jan 6, 2025 •

edited

Loading

michalkucharczyk commented Jan 23, 2025

dharjeezy commented Jan 23, 2025

michalkucharczyk Jan 25, 2025 •

edited

Loading

michalkucharczyk Jan 25, 2025

michalkucharczyk Jan 27, 2025 •

edited

Loading

michalkucharczyk Jan 27, 2025

michalkucharczyk Jan 27, 2025

michalkucharczyk commented Jan 27, 2025

michalkucharczyk commented Jan 27, 2025

command-bot bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025

command-bot bot commented Jan 27, 2025

	fn transaction(&self, hash: &H) -> Option<B::Extrinsic> {
	self.pool.ready_transaction(hash).and_then(
	// Only propagable transactions should be resolved for network service.
	\|tx\| if tx.is_propagable() { Some((**tx.data()).clone()) } else { None },
	)
	}

	description: The TransactionPool trait now accepts an `Arc` for transactions.
	description: The `sc_network_transactions::config::TransactionPool` trait now accepts an `Arc` for transactions.

	\|tx\| if tx.is_propagable() { Some(Arc::new((**tx.data()).clone())) } else { None },
	\|tx\| tx.is_propagable().then(\|\| { tx.data().clone() }),

		@@ -0,0 +1,9 @@
		title: `networking::TransactionPool` should accept `Arc`

Arc definition in TransactionPool #7042

Arc definition in TransactionPool #7042

Conversation

dharjeezy commented Jan 4, 2025

michalkucharczyk Jan 6, 2025

Choose a reason for hiding this comment

michalkucharczyk Jan 6, 2025

Choose a reason for hiding this comment

bkchr Jan 6, 2025

Choose a reason for hiding this comment

michalkucharczyk Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk commented Jan 6, 2025 • edited Loading

michalkucharczyk commented Jan 23, 2025

dharjeezy commented Jan 23, 2025

michalkucharczyk Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk Jan 25, 2025

Choose a reason for hiding this comment

michalkucharczyk Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk Jan 27, 2025

Choose a reason for hiding this comment

michalkucharczyk Jan 27, 2025

Choose a reason for hiding this comment

michalkucharczyk commented Jan 27, 2025

michalkucharczyk commented Jan 27, 2025

command-bot bot commented Jan 27, 2025 • edited Loading

github-actions bot commented Jan 27, 2025

command-bot bot commented Jan 27, 2025

`Arc` definition in `TransactionPool` #7042

`Arc` definition in `TransactionPool` #7042

michalkucharczyk Jan 6, 2025 •

edited

Loading

michalkucharczyk commented Jan 6, 2025 •

edited

Loading

michalkucharczyk Jan 25, 2025 •

edited

Loading

michalkucharczyk Jan 27, 2025 •

edited

Loading

command-bot bot commented Jan 27, 2025 •

edited

Loading