Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add transactions from retracted blocks back to the pool #3562

Merged
merged 3 commits into from
Sep 6, 2019

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Sep 6, 2019

Closes #3462

@arkpar arkpar added the A0-please_review Pull request needs code review. label Sep 6, 2019
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, small grumble about excessive warnings though.

if let Some(block) = client.block(&BlockId::hash(*r))? {
let extrinsics = block.block.extrinsics();
if let Err(e) = transaction_pool.submit_at(id, extrinsics.iter().cloned(), true) {
warn!("Error re-submitting transactions: {:?}", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be triggered quite often in case of re-orgs. I would either lower this to debug or figure out how to detect transactions that were already included in another chain and print a warning only for other transactions.

Copy link
Member Author

Choose a reason for hiding this comment

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

submit_at pushes a list of transactions. I believe it only fails when there's something fundamentally wrong with the pool. Otherwise it returns a list of individual results that we ignore here.

@@ -893,6 +894,16 @@ pub(crate) fn maintain_transaction_pool<Api, Backend, Block, Executor, PoolApi>(
Executor: client::CallExecutor<Block, Blake2Hasher>,
PoolApi: txpool::ChainApi<Hash = Block::Hash, Block = Block>,
{
// Put transactions from retracted blocks back into the pool.
for r in retracted {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to change to retracted.into_iter().rev(), so that depending transactions are imported in-order and not moving to/from future/ready queues? I mean if block #5 had a TX1 that provides OUT1 and block #6 had a TX2 that requires OUT1, then if we first import TX2, then it'll be inserted to future queue (because we miss OUT1). And after we import TX1, it'll be moved to ready queue (because TX1 provides OUT1). If we import TX1 first, then both transactions will be inserted in ready queue immediately.

(and hashes in retracted are in reverse order, afaik)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes retracted in notification to be in ascending order, as documented.

@gavofyork gavofyork merged commit e169b37 into master Sep 6, 2019
@gavofyork gavofyork deleted the a-resubmit-tx branch September 6, 2019 19:30
andresilva pushed a commit that referenced this pull request Sep 17, 2019
* Add transactions from retracted blocks back to the pool

* Line width

* Reverse retracted
Demi-Marie pushed a commit to Demi-Marie/substrate that referenced this pull request Sep 17, 2019
)

* Add transactions from retracted blocks back to the pool

* Line width

* Reverse retracted
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-import transactions from retracted blocks to the pool.
4 participants