core: respect price bump threshold #15401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karalabe
requested changes
Oct 30, 2017
core/tx_pool_test.go
Outdated
| t.Fatalf("original queued transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) | ||
| } | ||
| if err := pool.AddRemote(pricedTransaction(2, big.NewInt(100000), big.NewInt(threshold+1), key)); err != nil { | ||
| if err := pool.AddRemote(pricedTransaction(2, big.NewInt(100000), big.NewInt(threshold), key)); err != nil { |
There was a problem hiding this comment.
Please retain both checks to ensure correct behavior on both sides of the boundary (i.e. just rejected vs. just accepted). I.e. in your code threshold - 1 should fail, but threshold should succeed.
core/tx_pool_test.go
Outdated
| t.Fatalf("original proper pending transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) | ||
| } | ||
| if err := pool.AddRemote(pricedTransaction(0, big.NewInt(100000), big.NewInt(threshold+1), key)); err != nil { | ||
| if err := pool.AddRemote(pricedTransaction(0, big.NewInt(100000), big.NewInt(threshold), key)); err != nil { |
There was a problem hiding this comment.
Please retain both checks to ensure correct behavior on both sides of the boundary (i.e. just rejected vs. just accepted). I.e. in your code threshold - 1 should fail, but threshold should succeed.
karalabe
approved these changes
Oct 30, 2017
vincentserpoul
pushed a commit
to vincentserpoul/go-ethereum
that referenced
this pull request
Nov 22, 2017
* core: allow price bump at threshold * core: test changes to allow price bump at threshold * core: reinstate tx replacement test underneath threshold * core: minor test failure message cleanups
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the usage information the transaction price bump value is defined as "Price bump percentage to replace an already existing transaction". However a replacement value at the price bump fails to replace the transaction.
To give an example: if a transaction is sent with a gas price of 1GWei then given the definition of price bump and the default value of 10(%) it would be expected that a replacement gas price of 1.1GWei would result in a successful replacement. This patch alters the code so that rather than reject this as underpriced the transaction is accepted.
The patch ensures that low gas prices are handled correctly by ensuring that the replacement gas price is absolutely larger than the original gas price as well as above the threshold.