Skip to content

Find PendingIterator in Transaction Pool#218

Merged
dvdplm merged 2 commits into
masterfrom
ng-tx-pool-pending-iterator
Sep 10, 2019
Merged

Find PendingIterator in Transaction Pool#218
dvdplm merged 2 commits into
masterfrom
ng-tx-pool-pending-iterator

Conversation

@ngotchac
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Comment thread transaction-pool/src/pool.rs
Comment thread transaction-pool/src/pool.rs Outdated
Copy link
Copy Markdown
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.

👌

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

if tx_state == Readiness::Ready {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
* 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants