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

Removing old transactions from the queue #4046

Merged
merged 2 commits into from
Jan 5, 2017
Merged

Removing old transactions from the queue #4046

merged 2 commits into from
Jan 5, 2017

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Jan 5, 2017

I'm not sure about the block time choice, or balance re-validation.
Would like to hear some second thoughts on this.

Closes #3928
Explains: #4007, #3996, #3358

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 5, 2017
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 5, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 86.655% when pulling 1cfae5f on txqueue-fix into 9323704 on master.

@@ -488,6 +495,11 @@ pub enum PrioritizationStrategy {
GasFactorAndGasPrice,
}

/// Point in time when transaction was inserted.
/// (implying block numbers)
pub type InsertionTime = u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is blocks, might want to change type to BlockNumber, and name to QueuingPeriod.

for sender in senders {
self.remove_all(sender, fetch_nonce(&sender));
for (sender, details) in senders.iter() {
self.remove_all(*sender, details.nonce);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove_all is a rather misleading name. there is very specific logic here that the name should imply. suggest remove_dead or even just cull.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jan 5, 2017
@gavofyork
Copy link
Contributor

minor grumbles.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jan 5, 2017

Grumbles addressed, renamed InsertionTime to QueuingInstant, cause it's actually used more as a point in time, not period.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 5, 2017
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 5, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.53% when pulling 9337f2d on txqueue-fix into 9323704 on master.

@gavofyork gavofyork merged commit eb0daea into master Jan 5, 2017
@gavofyork gavofyork deleted the txqueue-fix branch January 5, 2017 20:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants