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.

Sign up for GitHub

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

Jump to bottom

Find PendingIterator in Transaction Pool #218

Merged
merged 2 commits into from
Sep 10, 2019
Merged

Find PendingIterator in Transaction Pool #218

merged 2 commits into from
Sep 10, 2019

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Sep 9, 2019

When encountering a Stale transaction in the PendingIterator, we should try to use the next best transaction for this sender.

@ngotchac ngotchac mentioned this pull request Sep 9, 2019
Merged
tomusdrw
tomusdrw reviewed Sep 9, 2019
View reviewed changes
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.

Looks good, small style grumble and it would be really good to add a test for it.

transaction-pool/src/pool.rs Show resolved Hide resolved
tomusdrw
tomusdrw approved these changes Sep 10, 2019
View reviewed changes
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.

👌

transaction-pool/src/pool.rs
},
_ => trace!("[{:?}] Ignoring {:?} transaction.", best.transaction.hash(), tx_state),

if tx_state == Readiness::Ready {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've actually meant to put the if inside previous match and leave the trace in _ pattern, but this is fine as well.

Just got an idea that for readability we could also change both this if and match above to if let, but it's up to you.

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 thought it would be clearer to have one match when we need to find the next transactions, and another one when we need to return the transaction, but I'm open to anything :)

Copy link
Contributor

Choose a reason for hiding this comment

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

My reasoning was that it's easier to see everything that happens in particular case(ses) in a single block, cause now you fall-through and need to analyze multiple blocks to think about what happens. But as I said, I'm not strong on this either.

@dvdplm dvdplm merged commit 67a9e7d into master Sep 10, 2019
@ngotchac ngotchac deleted the ng-tx-pool-pending-iterator branch September 11, 2019 07:23
ordian added a commit that referenced this pull request Sep 16, 2019
@ordian
Merge branch 'master' into ao-kvdb-indexeddb
3410968 
* master:
  Bump version (#219)
  Find PendingIterator in Transaction Pool (#218)
  Modified AsRef impl for U-type macros (#196)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm left review comments

@tomusdrw tomusdrw tomusdrw approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood M4-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ngotchac @dvdplm @tomusdrw @ordian