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

core: sort txs at the same gas price by received time #21358

Merged
merged 2 commits into from
Jul 30, 2020
Merged

core: sort txs at the same gas price by received time #21358

merged 2 commits into from
Jul 30, 2020

Conversation

hendrikhofstadt
Copy link
Contributor

@hendrikhofstadt hendrikhofstadt commented Jul 22, 2020

This PR changes the sorting of pool transactions with the same gas price.

Previously transactions were sorted randomly (due to the way go maps work).
Now they are sorted by the time that they were added to the mempool in ascending order.

This is supposed to remove the incentive for spamming transactions to backrun a tx (#21350).


Potential cons of this solution:

  • This provides stronger incentives for miner collusion than the current solution

@hendrikhofstadt hendrikhofstadt marked this pull request as ready for review July 22, 2020 18:15
@karalabe
Copy link
Member

While I agree with the general idea, I'm not sure this is the best approach. It feels wrong to hack in a new field into the transaction. The arrival time is relevant only for the txpool and the miner. Otherwise a transaction has no notion nor need for an arrival time. I don't think it makes sense to add it to a core type just to make the miner happy.

@MicahZoltu
Copy link
Contributor

just to make the miner happy.

To be clear, this isn't to make miners happy. It is to remove an incentive currently present that is resulting in a significant amount of spam against the network and blockchain, which is contributing to gas prices going up.

Is your issue with the general idea of sorting by seen-time, or is your concern with the specific implementation presented here?

@hendrikhofstadt
Copy link
Contributor Author

While I agree with the general idea, I'm not sure this is the best approach. It feels wrong to hack in a new field into the transaction. The arrival time is relevant only for the txpool and the miner. Otherwise a transaction has no notion nor need for an arrival time. I don't think it makes sense to add it to a core type just to make the miner happy.

I agree that the data does not really belong in the Transaction. I mean the caches of non-txdata are already there but I agree they are closer than a mempool-reliant concept like time of arrival.
Would you prefer storing a sync hash->time map in the txpool to store the timings?

@holiman
Copy link
Contributor

holiman commented Jul 23, 2020 via email

@karalabe
Copy link
Member

My initial gut approach would be to have the txpool track the arrival time along each transaction, maybe by wrapping the txs in some auxiliary wrapper. That seems better vs. a totally external map Martin suggested because you don't need to explicitly track liveliness/contets and keep it in lockstep with the pool contents.

That said, I do see that the txpool uses types.Transaction quite extensively, not sure what the implication would be of introducing a wrapper in there. All in all I do like the idea with the arrival time, just trying to figure out where exactly is it the most suitable to be injected. If it's only tracked by the txpool and the miner, we can farily easily reason about it. Once the field goes into the types.Transaction object itself, it's a can of worms because we never had to care about this field, so not sure where there might be a code segment that will lose it.

@karalabe
Copy link
Member

Another potential idea could be to have a types.TimestampedTransaction (just thinking out aloud here for a debate, not sure it's a good idea), then the pool and miner could use that object, whereas the remainder of the code would remain oblivious. This would also be clean-ish API wise because you'd know that there's something funky going on in the txpool/miner, it's not just boring txs. Not sure though that such a wrapper is best placed in the types package or core package. Maybe we should finally separate the txpool out already from core.

@lightuponlight
Copy link

lightuponlight commented Jul 27, 2020

If this is contributing significantly to Ethereum's current gas price (150 for fast confirmation) then it needs to be implemented and merged ASAP.

Here is a screenshot of the spam in real life:

Screen Shot 2020-07-27 at 11 51 43 AM

@karalabe
Copy link
Member

I've pushed a bit of a modification on top of this PR. Instead of leaking the receipt time into the tx API, I've made it a hidden fields only handled within the types package and only used for sorting. The time is set automatically whenever a transaction is constructed or unmarshalled from RLP or JSON. that should cover proper timestamping automatically without having to be aware of an extra notion. Could you check @hendrikhofstadt if this makes sense to you?

Regarding my original complaint that the receipt time has no place in the tx object, I'm happy with my above approach as long as the field is private and completely hidden, so all the rest of the code is oblivious that it exists. I did try to move all this out of types into core, but then things get further complicated because the tx object hides it's internals and only allows access via copy, so I would have had to add a ton of new accessors, or just bite the bullet and keep the timestamp in the original struct.

@holiman PTAL if this approach makes sense to you or not?

@karalabe karalabe added this to the 1.9.19 milestone Jul 28, 2020
@hendrikhofstadt
Copy link
Contributor Author

@karalabe Thanks for taking care of this. Looks good to me. Covering time setting in constructors and decode methods seems to cover all cases that involve the mempool.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@lightuponlight
Copy link

lightuponlight commented Jul 29, 2020

Awesome responsiveness, thanks Peter as always!

@@ -341,8 +358,8 @@ type TransactionsByPriceAndNonce struct {
// Note, the input map is reowned so the caller should not interact any more with
// if after providing it to the constructor.
func NewTransactionsByPriceAndNonce(signer Signer, txs map[common.Address]Transactions) *TransactionsByPriceAndNonce {
Copy link

@gubatron gubatron Aug 3, 2020

Choose a reason for hiding this comment

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

s/NewTransactionsByPriceAndNonce/NewTransactionsByPriceTimeAndNonce/g ?

@holiman
Copy link
Contributor

holiman commented Feb 24, 2021

@hendrikhofstadt do you have any thoughts on this alternate proposal: https://notes.ethereum.org/@holiman/H17hFNWfd

@palkeo
Copy link

palkeo commented Feb 24, 2021

Hey. Me and @livnev (author of #21350) have been researching these subjects and would be happy to give feedback / have a discussion, if you are interested.
(and I cannot leave notes on your proposal because registration is not public)

@holiman
Copy link
Contributor

holiman commented Feb 24, 2021

@palkeo sounds great. I can try to fix it so you have comment-access, or we could use some other forum. Should I post on e.g. fellowship of magicians, or use some discord channel?

@hendrikhofstadt
Copy link
Contributor Author

hendrikhofstadt commented Feb 24, 2021

@holiman @palkeo happy to start a fresh discussion.

A lot of context to the issue unfortunately got lost in the Ethereum devs discord history where I originally brought this up and discussed it with a couple of folks

The original idea behind the issue was to reduce block congestion due to “sorting bids” which I consider successful given that for most backrunning opportunities there are only some 4-10 bids from different bots.

My current take is that given the value of all MEV transactions (see Flashbots’ explorer), any change that incentivises shot gunning or bruteforcing hashes (and subsequently sending multiple attempts) might significantly increase congestion again (as if it wasn’t bad enough right now).

MaxMustermann2 added a commit to MaxMustermann2/harmony that referenced this pull request Apr 4, 2022
Closes harmony-one#4113 by sorting transactions by time received
instead of using a hashmap based transaction ordering. This sorting is
on top of the gas price and nonce sorting already implemented.

Effectively, this allows the production of almost a deterministic order
of transaction ordering, as opposed to a heap-based hash map ordering
which is affected by Golang's internal operations. Consequently,
arbitrageurs, who spam the network with a view to exist around
arbitrag-able transactions, will have to focus on latency instead of
network spamming.

See also bsc-chain/bsc#269 and ethereum/go-ethereum#21358 for related
issues in other chains.
MaxMustermann2 added a commit to MaxMustermann2/harmony that referenced this pull request Apr 4, 2022
Closes harmony-one#4113 by sorting transactions by time received
instead of using a hashmap based transaction ordering. This sorting is
on top of the gas price and nonce sorting already implemented.

Effectively, this allows the production of almost a deterministic order
of transaction ordering, as opposed to a heap-based hash map ordering
which is affected by Golang's internal operations. Consequently,
arbitrageurs, who spam the network with a view to exist around
arbitrag-able transactions, will have to focus on latency instead of
network spamming.

See also bnb-chain/bsc#269 and ethereum/go-ethereum#21358 for related
issues in other chains.
rlan35 pushed a commit to harmony-one/harmony that referenced this pull request Apr 4, 2022
Closes #4113 by sorting transactions by time received
instead of using a hashmap based transaction ordering. This sorting is
on top of the gas price and nonce sorting already implemented.

Effectively, this allows the production of almost a deterministic order
of transaction ordering, as opposed to a heap-based hash map ordering
which is affected by Golang's internal operations. Consequently,
arbitrageurs, who spam the network with a view to exist around
arbitrag-able transactions, will have to focus on latency instead of
network spamming.

See also bnb-chain/bsc#269 and ethereum/go-ethereum#21358 for related
issues in other chains.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants