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

Reduce usd_per_tx #7058

Merged
merged 8 commits into from Jan 30, 2018
Merged

Reduce usd_per_tx #7058

merged 8 commits into from Jan 30, 2018

Conversation

MysticRyuujin
Copy link
Contributor

@MysticRyuujin MysticRyuujin commented Nov 14, 2017

Currently the default minimum transaction fee is .0025 however, a huge amount of transactions are falling below this default minimum. I don't feel like it's Parity's duty to set a default minimum transaction fee, especially when that default is about 4x higher than what ETH Gas Station calls the "Safe Low" for a transaction.

While any number of arguments can be made for what a reasonable transaction fee should be, I don't think the place to decide that is in the default configuration...

I've submitted this pull request as a place to have that discussion πŸ˜„


This change is Reviewable

@parity-cla-bot
Copy link

It looks like @MysticRyuujin hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@MysticRyuujin
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @MysticRyuujin signed our Contributor License Agreement. πŸ‘

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview πŸ€“ Pull request needs code review. A2-insubstantial πŸ‘Ά Pull request requires no code review (e.g., a sub-repository hash update). M2-config πŸ“‚ Chain specifications and node configurations. labels Nov 15, 2017
@5chdn 5chdn added this to the 1.9 milestone Nov 15, 2017
@5chdn
Copy link
Contributor

5chdn commented Nov 15, 2017

I have no objections, really. Just want to say that 0.25 US cents are already cheap. 0.01 US cents is good too. Potential blockchain spammers will use a custom gas price anyways.

@folsen
Copy link
Contributor

folsen commented Nov 15, 2017

The only thing I'd be worried about with setting it too low is that in times of high pressure, a lot of transactions will get stuck and we'll get a huge influx of support requests saying "Your application isn't working and my transactions aren't going through."

25c may be too high, 1c is pretty damn low though. Do we have any data at all to say "95% of the time, X would get a transaction through reliably"?

I think the default should be data-driven to as large extent as possible.

@tomusdrw
Copy link
Collaborator

@folsen Note this setting only affect miners, not regular users of Parity Wallet. The eth_gasPrice RPC method is calculating a median gas price from last 100 blocks anyway.

@5chdn
Copy link
Contributor

5chdn commented Nov 15, 2017

"Closes" #6973 ;)

@5chdn
Copy link
Contributor

5chdn commented Nov 28, 2017

25c may be too high, 1c is pretty damn low though.

Note, it's 0.25c and 0.01c.

Do we have any data at all to say "95% of the time, X would get a transaction through reliably"?

https://ethgasstation.info/ says;

  • Std Cost for Transfer $0.01 (1.00c)
  • SafeLow Cost for Transfer $0.001 (0.10c)

I don't see why we should default to something 10x lower than the statistical SafeLow Cost. Paying a quarter cent for a transaction is perfectly reasonable.

And if any miner disagrees, they can still override it with a custom configuration.

@5chdn 5chdn closed this Nov 28, 2017
@MysticRyuujin
Copy link
Contributor Author

What? Let's stop changing the decimal places for clarity here...

Currently the default minimum transaction fee is $0.0025
SafeLow Cost for Transfer $0.001

$0.001 < $0.0025

So I don't see why you should default to rejecting transactions that are within the safe low cost by a factor of 2.5x either...

@5chdn 5chdn reopened this Nov 28, 2017
@5chdn
Copy link
Contributor

5chdn commented Nov 28, 2017

Reopening for 2nd opinion.

@tomusdrw
Copy link
Collaborator

The setting will affect network propagation (relaying) of low gas transactions which seems reasonable for me.
The miners are also affected, but they should run with custom settings anyway and set the price according to their own analysis.

That said it should be mentioned in release notes. @MysticRyuujin Would you mind fixing the tests? (don't mind the RpcList, they are not related, just the price needs to be updated).

@tomusdrw tomusdrw added A7-looksgoodtestsfail πŸ€– Pull request is reviewed well, but cannot be merged due to tests failing. B7-releasenotes πŸ“œ Changes should be mentioned in the release notes of the next minor version release. and removed A0-pleasereview πŸ€“ Pull request needs code review. A2-insubstantial πŸ‘Ά Pull request requires no code review (e.g., a sub-repository hash update). labels Nov 30, 2017
@MysticRyuujin
Copy link
Contributor Author

@tomusdrw can you point me at which test file(s) need to be updated?

* Iterate over both buffered and unbuffered database entries

* Fix iterator issues

* no default uncles

* prepare cargo configuration for upload of crates

* update bigint version number

* update ethcore-bigint version

* rename hash crate to keccak-hash

* update memorydb

* update rlp

* update patricia-trie cargo.toml

* use error-chain in ethcore-network

* interleaved-ordered 0.1.1

* static linking for snappy

* removed redundant imports

* Add the desktop file for the snap

Now that we have added plugs to allow accessing the display, the snap needs
a desktop file. And bonus point, it will appear on the menu when it's
installed, and once you make a stable relase, it will appear in the gnome
software center app! So, one-click install for parity :)

Closes: #7056

* update icon for desktop

* Properly display Signer errors (Snackbar display popup) (#7053)

* Update to fixed @parity/ui (Errors component)

* Update ParityBar radius to align with Snackbar/Errors

* Update to latest @parity/ui

* Update dependencies @parity/signer-plugin-*

* Really pull in @parity/signer-plugin-* deps

* CHANGELOG for 1.7.8, 1.7.9, 1.8.2, and 1.8.3 (#7055)

* Update changelog for 1.7.8 stable

* Update changelog for 1.7.9 stable

* Improve wording in Changelog

* Update changelog for 1.8.2 beta

* Update changelog for 1.8.3 beta

* [ci skip] js-precompiled 20171115-103846

* ECIP-1039: Monetary policy rounding specification

Fix potential rounding errors between geth and parity in the long-term future.

* Change reward calculation to only use divide once

* SecretStore: servers set change session api (#6925)

* SecretStore: first key versions flush

* SecretStore: key versions in encryption session

* SecretStore: flush key versions negotiation session

* SecretStore: connected key version negotiation session to cluster

* SecretStore: cluster sessions container refactoring

* SecretStore: flush

* SecretStore: flush key versions

* SecretStore: flush

* SecretStore: delegation proto

* SecretStore: decryption_session_is_delegated_when_node_does_not_have_key_share

* SecretStore: fixed version in decryption session

* SecretStore: signing_session_is_delegated_when_node_does_not_have_key_share

* SecretStore: started restoring admin sessions

* SecretStore: restoring admin sessions

* SecretStore: removed obsolete ShareRemove && ShareMove sessions

* SecretStore: ShareAdd math tests only require old_t+1 nodes

* SecretStore: ShareAdd revamp using new math backend

* SecretStore: do not include isolated nodes into consensus_group

* SecretStore: ServersSetChange + ShareAdd revamp

* removed debug printlns

* SecretStore: key version negotiation tests

* SecretStore: removed debug/merge artifacts

* SecretStore: fixed master node selection

* SecretStore: cleanup + tests + fixes

* SecretStore: uncommented tests

* SecretStore: cleaning up

* SecretStore: cleaning up + tests

* SecretStore: cleaning up

* SecretStore: cleaning up && tests

* SecretStore: fixing TODOs

* SecretStore: fixing TODOs + cleanup

* SecretStore: fixing TODOs

* SecretStore: nodes_add_to_the_node_with_obsolete_version

* SecretStore: nodes_add_fails_when_not_enough_share_owners_are_connected

* SecretStore: tests

* SecretStore: signing && delegation tests

* SecretStore: signing && decryption tests when some nodes are isolated

* SecretStore: sessions_are_removed_when_initialization_fails

* SecretStore: ceaning up

* SecretStore: removed obsolete comments

* SecretStore: signing_session_completes_if_node_does_not_have_a_share

* SecretStore: initial ServersSetChange API

* SecretStore: added secretstore_signServersSet RPC

* SecretStore: ChangeServersSet parse tests

* SecretStore: fixes after manual ServersSetChange tests

* lost file

* fixed network ports overlap in tests

* lost files

* fix tests on patricia-trie

* updated eth-secp256k1

* Fix no-default-features.

* Parse payload from panic

Impl payload

empty str is none

Update tests

Clean

Update wasm-tests

* Allow localUrl in manifest

* Improve Github Issue Template: IT CROWD approved version.

* Remove seperator that causes issue descriptions to become headlines sometimes

* Add to all icon_url places

* Add appId as needed to local dapps

* localUrl only from manifest

* Update panic_payload.rs

* Use query-string for search parsing

* spaces to tabs.

* Add localUrl to serialization

* Make storage_read/write return nothing

* Update gas values

* Update wasm-tests

* Cleanup debug info

* Remove debug log

* Optimize & group dapp requests (#7083)

* Group similar methods in same grouping

* Add a shell_getMethodGroups API

* Small code clean changes

* Fix bug dapp.name not showing

* Additional error handling

* Store sources in own map

* Remove observable variables where not needed

* Refactor code and fix bug dapp not showing on approve

* [ci skip] js-precompiled 20171121-150329

* Remove unused and duplicated files in js-old (#7082)

* Cleanup v1 build process, application-only

* Remove built-in dapps from build (duplicated)

* User @parity/api instead of local version

* Update references to @parity/abi

* Remove unused js-old api/abi folders

* Remove duplicated v1 jsonrpc

* Cleanup unused routes

* Update manifest with wallet image

* Update wallet logo

* Re-add missing test.sh

* Update rpc mocks

* Update tests for Providers

* Use flex for iframe & status

* Additional cleanups (Home screen for embed)

* Keep statusbar fixed (and non-overallping with dapps)

* [ci skip] js-precompiled 20171121-164807

* Cleanup top bar, add Home icon for navigation (#7118)

* Localise images to config.js file

* Remove sample status plugin (commented)

* Update image references from config

* Remove Unknown capability & Capable (only display actions)

* Update to @parity/ui 2.2.14

* Add Home icon on statusbar (go back)

* 2.2.14 -> 2.2.x

* Builtin dapp icons where dappreg not available

* [ci skip] js-precompiled 20171122-140247

* fixed RotatingLogger after migrating to new arrayvec

* Update packages, pull in compiled-only repos (#7125)

* Update packages, pull in compiled-only repos

* Update js-precompiled to point to js-dist-paritytech

* Trigger both js & js-old builds to force update

* Update to bring scripts 100% in-sync

* Fixed build && test (#7128)

* fixed build && test

* fixed rpc tests

* Update js-precompiled ref, trigger JS build

* Add test for ECIP1017 at block 250000000

* Wrong era used in ECIP1017 test

It is era 49, and should correspond to ECIP1017/ECIP1039's era 50.

* [ci skip] js-precompiled 20171124-124119

* Push to correct shell branch (#7135)

* Push to correct shell branch

* Trigger both js & js-old builds

* [ci skip] js-precompiled 20171124-134823

* pwasm-run-test utility

* WASM Remove blockhash error (#7121)

* Remove blockhash error

* Update tests.

* Pull in new dapp-{methods,visible} dapps (#7150)

* [ci skip] js-precompiled 20171128-091552

* fixes typo in user config path (#7159)

* Cleanup JS build artifacts (#7164)

* Cleanup JS build artifacts

* Trigger js & js-old

* [ci skip] js-precompiled 20171129-135441

* Use git flag to remove old js artifacts (#7165)

* [ci skip] js-precompiled 20171129-144917

* Remove *.css.map & *.js.map (#7168)

* [ci skip] js-precompiled 20171129-172021

* Delete unused package.json (dist) (#7173)

* [ci skip] js-precompiled 20171130-103432

* Assorted improvements for ethstore and ethkey (#6961)

* Testing many passwords for presale wallet.

* Add multiple threads.

* WiP: ethkey brain wallets recover.

* Refactor pre-sale-wallet cracking.

* Generate in multiple threads. Brain with prefix.

* Validate bain wallet phrase.

* Brain wallet recovery.

* Self-review fixes.

* Fix tests.

* More docs.

* Bump versions.

* Remove cmd_find from borked merge.

* Update wasm submodules.

* Use threadpool.

* upper limit is gas limit * 10 in estimate gas

* React 16 (#7174)

* Update packages to use React 16

* Rollback to react-router v3

* Use component instead of pure one

* Remove warning about mobx

* Make webpack load css from @parity/ui

* Update enzyme to support react16

* Fix lint

* Use @parity/ui v3

* Update refs of plugin-signer-* deps

* Exclude plugin-signer-* from babel processing

* Reupdate refs to old method

* Update refs again

* [ci skip] js-precompiled 20171201-114538

* pwasm-run-test utility upgrade

* Removed ethcore-util dependency from ethcore-network (#7180)

* Removed ethcore-util dependency

* Removed snappy

* New account selector UI in top bar (#7179)

* Add a dropdown popup for account selector

* Install sui latest version for hideOnScroll bug fix

* Update ui

* Update package-lock after rebase

* Require parity/ui v3.0.3

* Pass accountStore as props

* Require parity/ui v3.0.4

* [ci skip] js-precompiled 20171204-115345

* Update mocha import stubs (#7191)

* Update mocha import stubs

* Add .md files to ignore list

* [ci skip] js-precompiled 20171205-084709

* Update FirstRun for UI-2 (#7195)

* WIP

* Update after @parity/ui update

* Update to latest

* Update semver for @parity

* Update & -> &amp;

* [ci skip] js-precompiled 20171205-102703

* Maximum uncle count transition (#7196)

* Enable delayed maximum_uncle_count activation.

* Fix tests.

* Defer kovan HF.

* mistake comment in calc difficulty (#7154)

* Send each log as a separate notifications. (#7175)

* Update config.full.toml
This reverts commit 2fa0af6.
@5chdn
Copy link
Contributor

5chdn commented Dec 6, 2017

Now the safe low is 30c. :-p

@tomusdrw
Copy link
Collaborator

tomusdrw commented Dec 21, 2017

@MysticRyuujin values here needs to be updated:
https://github.com/paritytech/parity/blob/de8d20cea9eab9ab563a7e6314a40bd5432ed26a/parity/params.rs#L229
to

Calibrated { initial_minimum: 476190464, usd_per_tx: 0.0001, }

@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Dec 30, 2017
@5chdn 5chdn modified the milestones: 1.9, 1.10 Dec 30, 2017
@5chdn 5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jan 9, 2018
@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@5chdn
Copy link
Contributor

5chdn commented Jan 28, 2018

Can you fix tests and conflicts?

@MysticRyuujin
Copy link
Contributor Author

MysticRyuujin commented Jan 28, 2018

So it currently says:

initial_minimum: 11904761856u64.into(),
usd_per_tx: 0.0025f32,

But I don't know how to convert Calibrated { initial_minimum: 476190464, usd_per_tx: 0.0001, } into the proper values to replace the above...

Or do I literally just change it to:

initial_minimum: 476190464,
usd_per_tx: 0.0001,

Unclear

@rphmeier
Copy link
Contributor

@MysticRyuujin yes, that would fix it

@5chdn
Copy link
Contributor

5chdn commented Jan 29, 2018

Fixed the build but the tests still fail

@MysticRyuujin
Copy link
Contributor Author

Should 476190474u64 be 476190464u64 ?

@5chdn
Copy link
Contributor

5chdn commented Jan 30, 2018

Yes sorry

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A7-looksgoodtestsfail πŸ€– Pull request is reviewed well, but cannot be merged due to tests failing. B7-releasenotes πŸ“œ Changes should be mentioned in the release notes of the next minor version release. M2-config πŸ“‚ Chain specifications and node configurations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants