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.

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

Re-enable gas price #624

Closed
wants to merge 12 commits into from
Closed

Re-enable gas price #624

wants to merge 12 commits into from

Conversation

aroommen
Copy link

@aroommen aroommen commented Jan 22, 2019

The changes here allow users to specify a non-zero gas price for Quorum transactions. Previously Quorum only worked with a zero gas price. This is useful to enable network metering especially in consortium projects where the network enablers can be compensated for their costs. It's backward compatible in that the gas price can still be zero.
@fixanoid @joelburget I have incorporated code changes from the below pull request
#288

Copy link
Contributor

@SatpalSandhu61 SatpalSandhu61 left a comment

Choose a reason for hiding this comment

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

I've done some initial testing and found one issue (on raft) - if I submit a transaction with zero gasPrice, it goes into pending state and is never completed, e.g.:
eth.sendTransaction({from:"0xed9d02e382b34818e88b88a309c7fe71e65f419d",to:"0xcc71c7546429a13796cf1bf9228bff213e7ae9cc",value:1,gasPrice:0,gas:21000})

Also, the changes are causing a number of acceptance tests to fail (https://github.com/jpmorganchase/quorum-acceptance-tests). I'll try and dig deeper into the reasons for these - but it may just be due to some transactions getting stuck in pending and causing following tests to fail.

@aroommen
Copy link
Author

Thanks @SatpalSandhu61
am looking into the pending transfer issue.

@prd-fox
Copy link
Contributor

prd-fox commented Jan 24, 2019

Further to Satpal's comment above, this is due to the transactions not being accepted on remote nodes due to a too low gas price.
The minimum accepted gas price can be used using the --txpool.pricelimit flag, but when set below 1, it gets sanitised back to 1; this means that a gas price of 0 is never allowed on remote nodes.

See txpool.go:172 for the condition, which should be reviewed. The check should compare against a Price Limit of 0.

@aroommen
Copy link
Author

I have adjusted the flow in tx_pool.go to reset the default Transaction price limit for Quorum to zero and all tests are passing. Do check and let me know if the changes are ok

core/tx_pool.go Outdated Show resolved Hide resolved
core/tx_pool.go Outdated Show resolved Hide resolved
@SatpalSandhu61
Copy link
Contributor

Note that this PR is on our backlog, pending some associated changes that are being worked on, and some further testing.

@pjworrall
Copy link

Note that this PR is on our backlog, pending some associated changes that are being worked on, and some further testing.

Great to here @SatpalSandhu61 . As soon as this pull is in we'll be able to migrate to Quorum because I need transactions to require Eth. As there is no mining I am keen to know where the transaction fee goes: to the Validator?

@SatpalSandhu61
Copy link
Contributor

@pjworrall the account where the fees go is actually the bit we're working on. It will be the coinbase account, or the account associated with the minting node. There are some nuances which are currently being ironed out, around the relationship of the account on the type of consensus used.

@fixanoid fixanoid removed this from the QE - May Deliverable milestone Dec 4, 2019
@SatpalSandhu61
Copy link
Contributor

Note that this PR should not be merged as it's not complete. Work to enable gas price is ongoing on a different branch.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Aravind Oommen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Yur13L
Copy link

Yur13L commented May 30, 2022

Any news on this?

@antonydenyer
Copy link
Contributor

Superseded by #1446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants