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

Return events from blocks skipped over during Finalization, too #473

Merged
merged 15 commits into from
Mar 10, 2022

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Mar 8, 2022

If you subscribe to receive finalized block headers, blocks may be skipped over whenever a bunch of them are finalized at the same time. This PR makes sure to return events from all of the skipped-over blocks too.

Closes #468

@jsdw
Copy link
Collaborator Author

jsdw commented Mar 8, 2022

@paulormart I wonder whether you could give this change a go; I haven't given it much of a workout yet but it'd be great to get some initial impression of whether it seems to be doing the right thing :)

subxt/src/rpc.rs Outdated
Comment on lines 361 to 373
#[doc(hidden)]
pub async fn block_hash_internal(
&self,
block_number: T::BlockNumber,
) -> Result<Option<T::Hash>, BasicError> {
let block_number = ListOrValue::Value(block_number);
let params = rpc_params![block_number];
let list_or_value = self.client.request("chain_getBlockHash", params).await?;
match list_or_value {
ListOrValue::Value(hash) => Ok(hash),
ListOrValue::List(_) => Err("Expected a Value, got a List".into()),
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not a fan of duplicating this block_hash function. The problem is that Config::BlockNumber isn't something that we can easily work with.

I could also require that Config::BlockNumberimplements something like Into<u128>, or I could remove the "other" copy of this function, but it sounds like some consideration went into it, so I was hesitant. What do you think @ascjones?

Copy link
Collaborator Author

@jsdw jsdw Mar 8, 2022

Choose a reason for hiding this comment

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

The nice thing abour requiring Into<u128> or similar would be that I can get rid of num_traits again, and the other required traits I added to BlockNumber, which I was just using to add a minimal amount of information about the BlockNumber so that I could iterate over them.

Is it reasonable to assume that nobody would use something more esoteric than a rust unsigned int to represent block numbers, and so Into<u128> shouldn't cause any issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is reasonable to use Into<u128>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went for Into<u64> in the end because u128's don't serialize to JSON so easily; I'm hoping u64 is also a safe target here (I assume it's basically a choice between u32 and u64 that people would tend to make?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Into<u64> should also be fine.

@paulormart
Copy link
Contributor

Awesome stuff @jsdw.

I have just run my initial test against this branch and there is no skipped blocks. Seems to work fine, thank you.

Received 10 events from finalized block 11731282
Received 5 events from finalized block 11731283
Received 25 events from finalized block 11731284
Received 27 events from finalized block 11731285
Received 4 events from finalized block 11731286
Received 18 events from finalized block 11731287
Received 37 events from finalized block 11731288
Received 34 events from finalized block 11731289
Received 9 events from finalized block 11731290
Received 36 events from finalized block 11731291
Received 28 events from finalized block 11731292
Received 17 events from finalized block 11731293
Received 27 events from finalized block 11731294
Received 26 events from finalized block 11731295
Received 10 events from finalized block 11731296
Received 6 events from finalized block 11731297
Received 25 events from finalized block 11731298
Received 19 events from finalized block 11731299
Received 7 events from finalized block 11731300
Received 4 events from finalized block 11731301
Received 28 events from finalized block 11731302
Received 28 events from finalized block 11731303
Received 25 events from finalized block 11731304
Received 22 events from finalized block 11731305
Received 15 events from finalized block 11731306
Received 38 events from finalized block 11731307
Received 28 events from finalized block 11731308
Received 2 events from finalized block 11731309
Received 41 events from finalized block 11731310
Received 30 events from finalized block 11731311
Received 6 events from finalized block 11731312
Received 31 events from finalized block 11731313

@jsdw
Copy link
Collaborator Author

jsdw commented Mar 9, 2022

Fantastic; thank you for doing that!

#[doc(hidden)]
pub fn subscribe_to_block_headers_filling_in_gaps<'a, S, E, T: Config>(
client: &'a Client<T>,
mut last_block_num: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to assume that the block number is u64 here? Or would Option<Into<u64>> work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since block numbers can always be converted into u64s now, it's just easier to do the conversion first and then pass in the u64's here (this function isn't "public" anyway).

@jsdw jsdw requested a review from ascjones March 10, 2022 10:11
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Brilliant

@jsdw jsdw merged commit 4144a76 into master Mar 10, 2022
@jsdw jsdw deleted the jsdw-finalized-event-all-blocks branch March 10, 2022 10:24
@jsdw jsdw mentioned this pull request Mar 21, 2022
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.

Fix skipped finalized blocks under EventSubscription
4 participants