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

eth/catalyst: shouldOverrideBuilder flag #26829

Closed

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Mar 8, 2023

An idea to track dropped transactions and warn the user if transactions have been reorged out twice
This PR implements the shouldOverrideBuilder flag which is specified here: ethereum/execution-apis#425
The heuristic that is currently implemented sets the flag to true if a transaction is reorged out multiple times
This change is part of the cancun spec

@holiman
Copy link
Contributor

holiman commented Mar 8, 2023

Sorry but.. what is the user supposed to do with that information?

core/blockchain.go Outdated Show resolved Hide resolved
core/blockchain.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Mar 8, 2023

Also you're measuring the wrong thing, you are picking up all txs which are in the block(s) that was removed, but you do not detect whether the same transactions are maybe also included in the new chain. I guess the intention is to detect when a series of reorgs explicitly tries to avoid certain transactions?
If so, this will give false positives

@MariusVanDerWijden
Copy link
Member Author

MariusVanDerWijden commented Mar 8, 2023

Sorry but.. what is the user supposed to do with that information?

Thats why this is still a draft... The idea is that this could act as a circuit breaker for mev boost. E.g. if nodes detects these reorgs it can stop producing mev-boost blocks and only use the locally produced blocks

@potuz
Copy link

potuz commented Mar 8, 2023

Just for clarity, builders have very little influence on forking and censorship by forking requires a cartel of consensus validators being involved. However, if such a cartel appears, it would benefit from the builders being part of it and these features could help in at least honest validators falling back to local execution decentralizing this aspect.

@@ -2170,6 +2175,11 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error {
// transaction indexes, canonical chain indexes which above the head.
indexesBatch := bc.db.NewBatch()
for _, tx := range types.HashDifference(deletedTxs, addedTxs) {
if bc.droppedTxCache.Contains(tx) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: count the times the tx got reorged

@holiman
Copy link
Contributor

holiman commented Jun 6, 2023

Triage discussion: is this block dropping really feasible? And maybe it would be easier to form an image of this by looking at the reorg events on the CL layer?

@holiman
Copy link
Contributor

holiman commented Jun 8, 2023

From @potuz via discord:

what we're asking is for geth to implement two heuristics:

  1. track if some valid tx are paying highly and are not included in blocks
  2. track if some transactions are being reorged when they appear in blocks.

There's an open issue in the engine API but it was deemed necessary that some EL implements some of these heuristics before opening the PR.

@MariusVanDerWijden MariusVanDerWijden marked this pull request as ready for review July 24, 2023 09:33
@MariusVanDerWijden MariusVanDerWijden changed the title core: track dropped transactions and warn if dropped twice eth/catalyst: shouldOverrideBuilder flag Jul 26, 2023
@rjl493456442
Copy link
Member

rjl493456442 commented Aug 30, 2023

I am a bit skeptical about the efficiency of this indicator.

Typically, the process of block building is

  • validator sets the canonical chain by ForkChoiceUpdate event
  • validator may or may not ask for a local generated payload along with ForkChoiceUpdate
  • validator tells EL to process a payload locally by NewPayload event, the payload has the consensus agreement of the network

In the whole process, EL will only execute the payload given by CL in NewPayload event, and this payload can already be censored. In another word, I don't think reorg will occur in the first place, the censored transactions will just be stuck in txpool forever.

@MariusVanDerWijden
Copy link
Member Author

The idea is that the CL node will stop using MEV-boost if it sees that blocks/transactions are censored. But yeah I agree the implemented metric is not really useful

@potuz
Copy link

potuz commented Aug 30, 2023

In the whole process, EL will only execute the payload given by CL in NewPayload event, and this payload can already be censored. In another word, I don't think reorg will occur in the first place, the censored transactions will just be stuck in txpool forever.

The idea is that we want to mitigate two ways of censoring. A "soft" censoring in which simply builders won't include txs. In this case an honest proposer whose EL tells it: " we've had this high paying tx in the mempool for this long", then the CL would just send a local payload.

The other is the hard censorship in which an honest validator includes a censored tx and then the next one reorgs the block because it doesn't like that tx in. In this case I hope that honest validators get a message from the EL that says "I have seen this transaction on a payload that is no longer canonical, this has happened twice or thrice already".. and then the next honest proposer will build a local block without calling the builder.

@MariusVanDerWijden
Copy link
Member Author

closed because of #28029

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.

4 participants