Introduce OpaqueBundle and integrate tx pool in bundle production #206
Conversation
`OpaqueBundle` with opaque extrinsics will be used on primary chain as the primary chain does not care about the extrinsic format, it can be evolved as `CompactBundle` or something like that in the future. This PR also starts an initial transaction pool integration in bundle production, not the final version, but should be a step towards it.
|
|
||
| /// Encoded extrinsic. | ||
| #[derive(Decode, Encode, TypeInfo, PartialEq, Eq, Clone, RuntimeDebug)] | ||
| pub struct EncodedExtrinsic(Vec<u8>); |
There was a problem hiding this comment.
There is sp_runtime::OpaqueExtrinsic, which is exactly like this. Any specific reason to not use it?
There was a problem hiding this comment.
See paritytech/substrate#10504. But After this has been merged, I realized that EncodedExtrinsic is more straightforward because using OpaqueExtrinsic you'll have to deal with Result using expect(), EncodeExtrinsic could be just cleaner.
There was a problem hiding this comment.
OK, I had another PR merged a few days ago that would be useful to get in, maybe I can just go ahead and upgrade substrate to the latest state of upstream's master, WDYT?
Also would be nice to have a TODO with link to an upstream PR explaining that this is a temporary measure.
There was a problem hiding this comment.
I already have a branch with Substrate upgrade and switch to OpaqueExtrinsic
There was a problem hiding this comment.
OK, I had another PR merged a few days ago that would be useful to get in, maybe I can just go ahead and upgrade substrate to the latest state of upstream's master, WDYT?
Yeah, it's totally fine to use the latest master of substrate IMO.
Also would be nice to have a TODO with link to an upstream PR explaining that this is a temporary measure.
That's what I usually would do. The reason for not doing it here is that now I don't fully sure that OpaqueExtrinsic is better than EncodedExtrinsic in this sutiation because you have to use expect in https://github.com/subspace/subspace/pull/207/files#diff-c49314f00f4e4f47d787bfe52ec77eef8dcbd2e37fab3357910b86ff4cd49d5aR83 . So the small difference between OpaqueExtrinsic and EncodedExtrinsic is that EncodedExtrinsic implies the source of input but OpaqueExtrinsic assumes any bytes leading to the fallible result and you have to deal with that. No big deal, just EncodedExtrinsic can be more specific and cleaner.
There was a problem hiding this comment.
I was actually thinking it might make sense to do impl<E: Extrinsic> From<E> for OpaqueExtrinsic upstream, since it will, obviously, always be correct, it will also allow to avoid one memory allocation (no need to copy Vec into another Vec), do you think this is a good idea?
There was a problem hiding this comment.
Make sense to me, I was expecting there is a more convenient way to convert Extrinsic to OpaqueExtrinsic.
| block_number, | ||
| "Timeout fired waiting for transaction pool, proceeding with production.", | ||
| ); | ||
| self.transaction_pool.ready() |
There was a problem hiding this comment.
Can you explain why we even call .ready_at() then, what is the difference between those 2 for this use case?
There was a problem hiding this comment.
This pattern was simply copied from https://github.com/paritytech/substrate/blob/a404abd3b3/client/basic-authorship/src/basic_authorship.rs#L394-L408 :P
There was a problem hiding this comment.
Sure, still I'd like to know why that particular construction is actually used 🙂 If no one on our team fully understands it, we're in a rough shape.
There was a problem hiding this comment.
Have a quick look at ready_at and ready, basically ready_at is a subset of ready where ready_at is guaranteed that the transaction pool was updated since block_number while ready doesn't have this guarantee.
There was a problem hiding this comment.
Should be possible to reduce to .ready() then as we will check that transactions collected are not in previous bundle and we have bundles completely decoupled from block production.
There was a problem hiding this comment.
But you still have to check some states(if the transaction is able to pay the fee), right?
There was a problem hiding this comment.
Yes, but either way we may have more than one bundle produced depending on how many things there is in transaction pool (I think we'll need to have an upper limit on any particular bundle).
There was a problem hiding this comment.
I don't have a pretty clear vision here, we could revisit it in the future, we're iterating fastly anyway.
|
|
||
| /// Encoded extrinsic. | ||
| #[derive(Decode, Encode, TypeInfo, PartialEq, Eq, Clone, RuntimeDebug)] | ||
| pub struct EncodedExtrinsic(Vec<u8>); |
There was a problem hiding this comment.
I already have a branch with Substrate upgrade and switch to OpaqueExtrinsic
OpaqueBundlewith opaque extrinsics will be used on primary chain, it can beevolved as
CompactBundleor something like that in the future.This PR also starts an initial transaction pool integration in the bundle
production, the logic of pushing transactions is borrowed from the current proposing function https://github.com/paritytech/substrate/blob/a404abd3b3/client/basic-authorship/src/basic_authorship.rs.
Nothing is the final version, but should be a step towards it.