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

Allow configuration of when to reseal blocks. #1460

Merged
merged 14 commits into from
Jun 28, 2016
Merged

Conversation

gavofyork
Copy link
Contributor

No description provided.

@gavofyork gavofyork added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 27, 2016
- Optional limit for the amount of gas transactions may have;
- option to restruct transactions returned/queried to only those
which have been executed.
_ => {
queue.top_transactions()
}
let sw = self.sealing_work.lock().unwrap();
Copy link
Contributor Author

@gavofyork gavofyork Jun 27, 2016

Choose a reason for hiding this comment

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

anyone know if this is the correct order for the lock? (it's the same as before, so should be good...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 27, 2016
@arkpar
Copy link
Collaborator

arkpar commented Jun 27, 2016

Looks good to me. @tomusdrw should take a look

};
match (&self.options.pending_set, sealing_set) {
(&PendingSet::AlwaysQueue, _) | (&PendingSet::SealingOrElseQueue, None) => queue.pending_hashes(),
(_, sealing) => sealing.map(|s| s.transactions().iter().map(|t| t.hash()).collect()).unwrap_or(Vec::new()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

sealing.map_or_else(Vec::new, |s| ...)

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 27, 2016
@arkpar
Copy link
Collaborator

arkpar commented Jun 28, 2016

needs rebase

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 28, 2016
}

/// Create new instance of this Queue with specified limits
pub fn with_limit(limit: usize) -> Self {
pub fn with_limits(limit: usize, tx_gas_limit: U256) -> Self {
Copy link
Collaborator

@tomusdrw tomusdrw Jun 28, 2016

Choose a reason for hiding this comment

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

Sorry to complain again, but maybe removing it from constructor would be good too? No strong feelings though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's changed by setter anyway, other stuff like minimal_gas_price,gas_limit,etc is not here also (because there are sensible defaults / it's unbounded without negative consequences)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants