Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wait_for_finalized behavior if the tx dropped, usurped or invalid #897

Merged

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Apr 5, 2023

Fixes #889 the following changes are made:
When a TxProgress gets any of the following events via RpcSubscription, the stream should end:

  • Usurped
  • Finalized
  • FinalityTimeout
  • Invalid
  • Dropped
    Before this change, Usurped, Invalid and Dropped would cause functions like wait_for_finalized() and wait_for_in_block() to run indefinitely.

The OnlineClient trait bound for TxProgress impl blocks that contain wait_for_finalized() and wait_for_in_block() was not lifted, because too much depends on it. But the trait bound of the Stream impl block was downgraded to Clone.

Tests

I added a few unit tests, that generate a predetermined stream substription, create a TxProgress from it and check if the stream implementation of TxProgress behaves as desired, especially in wait_for_finalized().

@tadeohepperle tadeohepperle changed the title wait_for_finalized behavior if the tx dropped or usurped wait_for_finalized behavior if the tx dropped, usurped or invalid Apr 6, 2023
@tadeohepperle tadeohepperle marked this pull request as ready for review April 6, 2023 07:05
@@ -166,7 +166,8 @@ impl<Res> std::fmt::Debug for Subscription<Res> {
}

impl<Res> Subscription<Res> {
fn new(inner: RpcSubscription) -> Self {
/// Creates a new [`Subscription<Res>`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type param in doc links is needless and could be simplified?!

Suggested change
/// Creates a new [`Subscription<Res>`].
/// Creates a new [`Subscription`].

/// **Note:** transaction statuses like `Invalid`/`Usurped`/`Dropped` indicate with some
/// probability that the transaction will not make it into a block but there is no guarantee
/// that this is true. In those cases the stream is closed however, so you currently have no
/// way to find out if they finally made it into a block or not.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally, we should have an integration test against substrate if this behavior gets changed but should be fine for now and the old RPC API will be replaced "soon" :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to elaborate the docs in substrate on the transaction status says one thing and the RPC implementation does something else :P

/// **Note:** transaction statuses like `Invalid`/`Usurped`/`Dropped` indicate with some
/// probability that the transaction will not make it into a block but there is no guarantee
/// that this is true. In those cases the stream is closed however, so you currently have no
/// way to find out if they finally made it into a block or not.
Copy link
Member

@niklasad1 niklasad1 Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible to create another subscription and watch the extrinsic again if any of the transaction statuses wasn't finalized.

For example

let xt = create_xt()
submit(xt.clone).await?;
let mut watch_ext = rpc.watch_extrinsic(xt).await?;
loop {
   match watch_xt.next().await {
       TxStatus::Finalized(_) => break Ok(()),
       other => {
           log::warning!("Watch extrinsic {xt} failed: {other}, retrying again);
           // create a new subscription
           watch_ext = rpc.watch_extrinsic(xt).await?; 
       }
   }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure complicated but possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, I did not know this, should I add your code example to the doc comment?

Copy link
Member

@niklasad1 niklasad1 Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code doesn't really work for the TxProgress as it wraps subscription and extrinsic.
So you can just modify your comment In those cases the stream is closed however, so you currently have no way to find out to something like In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized

I'm just picking hair here but this is tricky to understand for sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we should provide an API for this, what do you guys think?

Copy link
Collaborator

@jsdw jsdw Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh hold on; I didn't think that it was possible to "re subscribe" to a transaction again!

This would definitely be a good thing to try out, and I think a nice API to do that (maybe some function on this TxProgress thing) would be very cool.

Lemme write an issue and we can try it out!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we should provide an API for this, what do you guys think?

You mentioned above that "the old RPC API will be replaced soon", so does that mean that also our stream handling will change? If that is the case, will the logic of this API need to change? If not it is certainly a good Idea to have an API for resubscribing right now, something like tx_progress.wait_until_finalized_until_timeout() that does the resubscribing under the hood.

subxt/src/tx/tx_progress.rs Outdated Show resolved Hide resolved
Comment on lines 468 to 473
let c = MockClient;
let stream_elements: Vec<SubstrateTxStatus<H256, H256>> =
vec![SubstrateTxStatus::Ready, SubstrateTxStatus::Dropped];
let sub = create_substrate_tx_status_subscription(stream_elements);
let tx_progress: TxProgress<PolkadotConfig, MockClient> =
TxProgress::new(sub, c, Default::default());
Copy link
Collaborator

@jsdw jsdw Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think I'd wrap this duplicated stuff into some function with a signature like:

fn mock_tx_progress(statuses: Vec<SubstrateTxStatus>) -> TxProgress

Then it'll be really easy to see the logic that matters in each test because they will just look like:

let tx_progress = mock_tx_progress(vec![SubstrateTxStatus::Ready, SubstrateTxStatus::Dropped]);
let finalized_result = tx_progress.wait_for_finalized().await;
assert!(matches!(
    finalized_result,
    Err(Error::Transaction(crate::error::TransactionError::Dropped))
));

Comment on lines 246 to 255
/// Even though these cases can happen, the server-side of the stream is closed, if one of the following is encountered:
/// - Usurped
/// - Finalized
/// - FinalityTimeout
/// - Invalid
/// - Dropped
/// In any of these cases the client side TxProgress stream is also closed.
/// So there is currently no way for you to tell if an Dropped`/`Invalid`/`Usurped` transaction
/// reappears in the pool again or not.
/// You are free to unsubscribe from notifications at any point.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd move this to the bottom fo the comment, in part because the rest of this comment is copied from the TxStatus struct in Substrate :)

Comment on lines 103 to 104
/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
/// TxProgress repeatedly until the extrinsic is finalized.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
/// TxProgress repeatedly until the extrinsic is finalized.
/// that this is true. In those cases the stream is closed however, so you currently have no
/// way to find out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

Comment on lines 133 to 134
/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
/// TxProgress repeatedly until the extrinsic is finalized.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
/// TxProgress repeatedly until the extrinsic is finalized.
/// that this is true. In those cases the stream is closed however, so you currently have no
/// way to find out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

@@ -172,7 +172,7 @@ impl<T: Config, C: Clone> Stream for TxProgress<T, C> {
//
// Even though `Dropped`/`Invalid`/`Usurped` transactions might make it into a block eventually,
// the server considers them final and closes the connection, when they are encountered.
// curently there is no way of telling if that happens, because the server ends the stream before.
// In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized.
Copy link
Collaborator

@jsdw jsdw Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs reverting :)

/// finalized. The `FinalityTimeout` event will be emitted when the block did not reach finality
/// within 512 blocks. This either indicates that finality is not available for your chain,
/// or that finality gadget is lagging behind.
/// In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized.
Copy link
Collaborator

@jsdw jsdw Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized.
/// In those cases the stream is closed however, so you currently have no way to find
/// out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome; just need to revert a few doc comments :)

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, LGTM

@tadeohepperle tadeohepperle merged commit a69b3e4 into master Apr 6, 2023
@tadeohepperle tadeohepperle deleted the tadeohepperle-tx-progress-handle-stream-failures branch April 6, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does wait_for_finalized hang indefinitely if the tx dropped or usurped
3 participants