Does wait_for_finalized hang indefinitely if the tx dropped or usurped

[furoxr]

subxt/subxt/src/tx/tx_progress.rs

Lines 102 to 116 in 42bcdde

	pub async fn wait_for_finalized(mut self) -> Result<TxInBlock<T, C>, Error> {
	while let Some(status) = self.next_item().await {
	match status? {
	// Finalized! Return.
	TxStatus::Finalized(s) => return Ok(s),
	// Error scenarios; return the error.
	TxStatus::FinalityTimeout(_) => {
	return Err(TransactionError::FinalitySubscriptionTimeout.into())
	}
	// Ignore and wait for next status event:
	_ => continue,
	}
	}
	Err(RpcError::SubscriptionDropped.into())
	}

I have a question regarding a scenario where I send a transaction and invoke this method to wait for its finalization. Is there a risk that the program may hang indefinitely? This is because the status of the transaction may be usurped or dropped, causing the method to continue waiting without returning an error in such cases.

[niklasad1]

/// Note: transaction statuses like Invalid and Usurped are ignored, because while they
/// may well indicate with some probability that the transaction will not make it into a block,
/// there is no guarantee that this is true. Thus, we prefer to "play it safe" here. Use the lower
/// level [TxProgress::next_item()] API if you'd like to handle these statuses yourself.

This documentation is a bit tricky to understand I reckon but if you take a look at https://github.com/paritytech/substrate/blob/master/client/transaction-pool/api/src/lib.rs#L59-#L104,

The stream is considered finished only when either Finalized or FinalityTimeout

It will wait up to 512 blocks until the FinalityTimeout is triggered so not indefinitely but based on your usecase you can maybe regard Usurped, Dropped and other events as failed up to you.

[furoxr]

/// Note: transaction statuses like Invalid and Usurped are ignored, because while they
/// may well indicate with some probability that the transaction will not make it into a block,
/// there is no guarantee that this is true. Thus, we prefer to "play it safe" here. Use the lower
/// level [TxProgress::next_item()] API if you'd like to handle these statuses yourself.

This documentation is a bit tricky to understand I reckon but if you take a look at https://github.com/paritytech/substrate/blob/master/client/transaction-pool/api/src/lib.rs#L59-#L104,

The stream is considered finished only when either Finalized or FinalityTimeout

It will wait up to 512 blocks until the FinalityTimeout is triggered so not indefinitely but based on your usecase you can maybe regard Usurped, Dropped and other events as failed up to you.

Thank you for your help. However, based on the documentation, I understand that the FinalityTimeout only applies to transactions that are included in a block. If a transaction is usurped or dropped and not included in any block, would the method still trigger a FinalityTimeout?

subxt/subxt/src/tx/tx_progress.rs

Lines 244 to 246 in 42bcdde

	/// A block containing the transaction did not reach finality within 512
	/// blocks, and so the subscription has ended.
	FinalityTimeout(T::Hash),

[niklasad1]

The extrinsic can be kicked out from the pool (dropped, invalid etc) and then later be re-entered in the pool again (Retracted) but I'm not sure whether that can happen for Usurped (i.e, replaced probably not) ^^

/cc @jsdw do you know? :)

[jsdw]

When I wrote this logic, I had these comments in mind:

/// The stream is considered finished only when either `Finalized` or `FinalityTimeout`
/// event is triggered. You are however free to unsubscribe from notifications at any point.

And

/// Note that there are conditions that may cause transactions to reappear in the pool.

It seemed like one can't guarantee that a transaction won't go from eg Invalid or Dropped to being back in the pool and eventually in a block. It also sounds like the notifications will end in Finalized or FinalityTimeout, but otherwise Subxt also will handle when Substrate closes the subscription for any other reason.

I decided to ask this question on the stackexchange to see if I can get any further clarification, because it is a good one! I'd suggest following up on this line of questioning there rather than here, so that people more familiar with Substrate internals can chime in:

See: https://substrate.stackexchange.com/questions/7892/when-will-i-stop-receiving-transactionstatus-updates-for-a-transaction-submitted

[jsdw]

I'll close this now, and if we learn something interesting from the StackExchange answer that leads to tweaking Subxt then I'll open a new issue for that. Otherwise, You currently have the option to handle transaction status updates however you'd like if the built-in wait_for_finalized* methods aren't suitable :)

[furoxr]

I have made an investigation, and find something interesting. The Stream trait implemented for TxProgress will end only when the event FinalityTimeout or Finalized received. But I found the RPC server of substrate will remove subscribers when the usurped, invalid, dropped, finalized or finality_timeout received. So I'm curious whether TxProgress would drop the connection and stop waiting for new events?

Please find below some relevant code snippets:

subxt/subxt/src/tx/tx_progress.rs

Lines 137 to 181 in a2b8dde

	impl<T: Config, C: OnlineClientT<T>> Stream for TxProgress<T, C> {
	type Item = Result<TxStatus<T, C>, Error>;
	fn poll_next(
	mut self: std::pin::Pin<&mut Self>,
	cx: &mut std::task::Context<'_>,
	) -> std::task::Poll<Option<Self::Item>> {
	let sub = match self.sub.as_mut() {
	Some(sub) => sub,
	None => return Poll::Ready(None),
	};
	sub.poll_next_unpin(cx).map_ok(|status| {
	match status {
	SubstrateTxStatus::Future => TxStatus::Future,
	SubstrateTxStatus::Ready => TxStatus::Ready,
	SubstrateTxStatus::Broadcast(peers) => TxStatus::Broadcast(peers),
	SubstrateTxStatus::InBlock(hash) => {
	TxStatus::InBlock(TxInBlock::new(hash, self.ext_hash, self.client.clone()))
	}
	SubstrateTxStatus::Retracted(hash) => TxStatus::Retracted(hash),
	SubstrateTxStatus::Usurped(hash) => TxStatus::Usurped(hash),
	SubstrateTxStatus::Dropped => TxStatus::Dropped,
	SubstrateTxStatus::Invalid => TxStatus::Invalid,
	// Only the following statuses are actually considered "final" (see the substrate
	// docs on `TxStatus`). Basically, either the transaction makes it into a
	// block, or we eventually give up on waiting for it to make it into a block.
	// Even `Dropped`/`Invalid`/`Usurped` transactions might make it into a block eventually.
	//
	// As an example, a transaction that is `Invalid` on one node due to having the wrong
	// nonce might still be valid on some fork on another node which ends up being finalized.
	// Equally, a transaction `Dropped` from one node may still be in the transaction pool,
	// and make it into a block, on another node. Likewise with `Usurped`.
	SubstrateTxStatus::FinalityTimeout(hash) => {
	self.sub = None;
	TxStatus::FinalityTimeout(hash)
	}
	SubstrateTxStatus::Finalized(hash) => {
	self.sub = None;
	TxStatus::Finalized(TxInBlock::new(hash, self.ext_hash, self.client.clone()))
	}
	}
	})
	}
	}

This is Listener, the server side corresponding to the client of subscribition. It will fire a event when the transaction status changed, and it will close the connection if h.is_done() returns true. You could find the code here

impl<H: hash::Hash + traits::Member + Serialize, C: ChainApi> Listener<H, C> {
	fn fire<F>(&mut self, hash: &H, fun: F)
	where
		F: FnOnce(&mut watcher::Sender<H, ExtrinsicHash<C>>),
	{
		let clean = if let Some(h) = self.watchers.get_mut(hash) {
			fun(h);
			h.is_done()
		} else {
			false
		};


		if clean {
			self.watchers.remove(hash);
		}
	}

And h is a instance of Sender. It's method is_done returns true if is_finalized is true, or it's subscribers is empty.

pub fn is_done(&self) -> bool {
        self.is_finalized || self.receivers.is_empty()
}

And you could find relevent codes here, the Sender will change is_finalized to true, if the event is usurped, finalized, finality_timeout, invalid or dropped.

[niklasad1]

So I'm curious whether TxProgress would drop the connection and stop waiting for new events?

I assume that you are referring to the substrate RPC server implementation, it will just drop the subscription and no more message will be sent on the subscription. Then, no the connection will not be dropped in that scenario.

To conclude, subxt will wait "forever/until the connection closed" on wait_for_finalized if any of the events usurped, invalid, dropped, finalized occurred.

[jsdw]

Ah very intersting; thanks @furoxr for your digging! Based on that code then we should probably tweak Subxt to do the same; if the subscription is dropped after those messages then we should also be ending it. I'll re-open this as reminder to do so

[jsdw]

Just to elaborate on the plan here; Subxt should end the TxProgress stream when any of the following is encountered:

• Usurped
• Finalized
• FinalityTimeout
• Invalid
• Dropped

(We currently only end when Finalized or FinalityTimeout is encountered).

Make sure to return an appropriate error when wait_for_finalized() or wait_for_in_block() are called.

While we're here, I wonder whether we can unit test the TxProgress stream to check that we get the expected errors back in each case; I'd have a go and see whether we could:

• Remove the OnlineClient<T> constraint from the TxProgress Stream impl and the impl block with next_item, wait_for_in_block etc in (offhand I can't see why it'd be needed).
• Now you can manually write a test which can create a new TxProgress with a hand made test Subscription, I think. That way, you can check that it does what we want.