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

Market-orientated transaction pricing #1963

Merged
merged 7 commits into from
Aug 23, 2016
Merged

Conversation

gavofyork
Copy link
Contributor

Avoid a strict gas-limit and let the market decide through using
a priority queue based around gas pricing for transactions. In
periods of low transaction volume, they'll be processed for a lower
fee.

Avoid a strict gas-limit and let the market decide through using
a priority queue based around gas pricing for transactions. In
periods of low transaction volume, they'll be processed for a lower
fee.
@gavofyork gavofyork added the A0-pleasereview 🤓 Pull request needs code review. label Aug 19, 2016
return false;
}
// Operation maybe ok: only if hash not found in gas-price Set.
from.remove(gas_price).is_some()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move this to line #334 - cause it will happen only in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would like to, but hashes has captured an &mut of into in this scope so it has to escape before it can be removed.

Copy link
Collaborator

@tomusdrw tomusdrw Aug 19, 2016

Choose a reason for hiding this comment

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

Maybe explicit drop(hashes) would work?

Copy link
Contributor

Choose a reason for hiding this comment

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

not until non-lexical lifetimes are designed and implemented in rust.

@tomusdrw
Copy link
Collaborator

Test would be nice :)

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 19, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Aug 19, 2016

I believe that any new logic introduced without a test that triggers it should be considered a major issue

@gavofyork gavofyork added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 19, 2016
@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 Aug 19, 2016
@gavofyork
Copy link
Contributor Author

@tomusdrw please review change in tests/logic carefully.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.161% when pulling 1c1fe81 on tx-gas-price-prioritisation into 070a215 on master.

@coveralls
Copy link

coveralls commented Aug 21, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.917% when pulling 7576f4f on tx-gas-price-prioritisation into 070a215 on master.

@tomusdrw
Copy link
Collaborator

AFAIR enforce_limit is called everytime transaction is inserted. Wouldn't enforce_limit remove a transaction anyway?

So behaviour in this PR would be: favor transactions that were inserted prior to the limit, when limit is reached you need to provide sufficient gas price to (potentialy) replace one transaction from the queue.

Is this correct/expected?

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 22, 2016
assert_eq!(txq.status().pending, 2);
assert_eq!(txq.last_nonce(&sender), Some(nonce));
*/
assert!(res.is_err());
Copy link
Collaborator

@tomusdrw tomusdrw Aug 22, 2016

Choose a reason for hiding this comment

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

I would check exact error here if possible:

assert_eq!(res, Err(TransactionError::InsufficientGasPrice {
                minimal: <min>,
                got: <gas_price>,
            }));

@gavofyork
Copy link
Contributor Author

yes - that's the expected behaviour.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 22, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 86.911% when pulling 0750b06 on tx-gas-price-prioritisation into 070a215 on master.

@gavofyork gavofyork merged commit 35ecb39 into master Aug 23, 2016
@gavofyork gavofyork deleted the tx-gas-price-prioritisation branch August 23, 2016 11:30
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.

5 participants