Don't remove invalid transactions when skipping. #5121
Conversation
test-utils/runtime/src/system.rs
Outdated
| @@ -192,6 +192,10 @@ pub fn validate_transaction(utx: Extrinsic) -> TransactionValidity { | |||
| /// This doesn't attempt to validate anything regarding the block. | |||
| pub fn execute_transaction(utx: Extrinsic) -> ApplyExtrinsicResult { | |||
| let extrinsic_index: u32 = storage::unhashed::get(well_known_keys::EXTRINSIC_INDEX).unwrap(); | |||
| // Arbitrary limit on test-runtime block size. | |||
| if extrinsic_index > 15 { | |||
There was a problem hiding this comment.
Could we not add a custom extrinsic type here: https://github.com/paritytech/substrate/blob/master/test-utils/runtime/src/lib.rs#L110
That always returns ExhaustsResources in validate_transaction?
There was a problem hiding this comment.
+1 on that, I remember several times it was hard figuring out what mocked result is returned and based on what
There was a problem hiding this comment.
Great idea. Addressed.
| @@ -230,7 +230,7 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A> | |||
| Ok(()) => { | |||
| debug!("[{:?}] Pushed to the block.", pending_tx_hash); | |||
| } | |||
| Err(sp_blockchain::Error::ApplyExtrinsicFailed(sp_blockchain::ApplyExtrinsicFailed::Validity(e))) | |||
| Err(sp_blockchain::Error::ApplyExtrinsicFailed(ApplyExtrinsicFailed::Validity(e))) | |||
There was a problem hiding this comment.
We really need to factor this out and make it reusable for Polkadot: https://github.com/paritytech/polkadot/blob/master/validation/src/block_production.rs#L299
There was a problem hiding this comment.
Agree, I think it deserves a separate issue though. I'll make a companion PR for now.
EDIT: Seems the companion PR is not necessary, cause the skipping logic is not present there.
| // then | ||
| // block should have some extrinsics although we have some more in the pool. | ||
| assert_eq!(block.extrinsics().len(), 2); | ||
| assert_eq!(txpool.ready().count(), 6); |
There was a problem hiding this comment.
I still would like to see that we build a second block here.
There was a problem hiding this comment.
Then it means that the transaction cannot always exhaust resources. I guess it's fine, I'll change it so that it exhaust resources if it's not the ONLY/FIRST transaction in the block. Then keeping that variant as Transfer extension makes even more sense to me.
test-utils/runtime/src/lib.rs
Outdated
| Transfer { | ||
| transfer: Transfer, | ||
| signature: AccountSignature, | ||
| exhaust_resources: bool, |
There was a problem hiding this comment.
Why isn't this just a distinct variant in this enum?
There was a problem hiding this comment.
It has to share most of the logic with Transfer, since it has to come from some sender and have some nonce. I could simplify it to:
Extrinsic::ExhaustResources(from, nonce)
but I felt it's too arbitrary and it made more sense to just extend transfer.
gavofyork
added
A8-looksgood
and removed
A0-please_review
* Don't remove invalid transactions when skipping. * Use a special kind of extrinsic instead of arbitrary limit. * Fix txpool tests. * Attempt to create more blocks. * Bump lock Co-authored-by: Gavin Wood <github@gavwood.com> Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
* Don't remove invalid transactions when skipping. * Use a special kind of extrinsic instead of arbitrary limit. * Fix txpool tests. * Attempt to create more blocks. * Bump lock Co-authored-by: Gavin Wood <github@gavwood.com> Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
The check was intially added by: #5121 But a later pr fixed it properly: #9789 TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because the nonce for example is incorrect. Fixes: #13911
The check was intially added by: #5121 But a later pr fixed it properly: #9789 TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because the nonce for example is incorrect. Fixes: #13911
The check was intially added by: paritytech#5121 But a later pr fixed it properly: paritytech#9789 TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because the nonce for example is incorrect. Fixes: paritytech#13911
Resolves #5028