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

Avoid using MemBatch in ValidateChain #10678

Merged
merged 19 commits into from
Jun 11, 2024
Merged

Conversation

Giulio2002
Copy link
Contributor

@Giulio2002 Giulio2002 commented Jun 9, 2024

We take advantage of sharedDoms

@Giulio2002 Giulio2002 changed the title Avoid using MemBatch in `Validaet Avoid using MemBatch in ValidateChain Jun 9, 2024
@@ -284,10 +283,6 @@ func (e *EthereumExecutionModule) updateForkChoice(ctx context.Context, original
return
}
}
if e.executionPipeline.HasUnwindPoint() {
unwindToGenesis = e.executionPipeline.UnwindPoint() == 0
}
Copy link
Contributor Author

@Giulio2002 Giulio2002 Jun 9, 2024

Choose a reason for hiding this comment

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

this was useless btw

@@ -251,3 +248,68 @@ func DeserializeKeys(in []byte) [kv.DomainLen][]DomainEntryDiff {
}
return ret
}

const diffChunkLen = 8*1024 - 32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to move it here so that I can incorporate it with Flushing of domains

Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

I don't understand how ValidateChain can do BeginRw.
What if node does execution at this time?
It means we have less time for pruning?
What if you need validate 2 forks?
Or did you mean BeginRo?
Plz add this info in description of PR, otherwise hard to understand the intent.

@Giulio2002
Copy link
Contributor Author

Giulio2002 commented Jun 10, 2024

Ok, so the "Write" part of RwTx is only used for unwinding beforehand. If you look at the code the RwTx is rollbacked right after ValidatePayload. It does not mean we have less time for pruning. As a matter of fact, the flushing is done through SharedDomains and as you can see we keep it and use that to Flush, so functionally, it is as if we had diff like before but in the form of SharedDomain's writers. if you need to validate 2 big forks, you can do that with no issue, because it is functionally equivalent to the previous one.

Regarding intent, Intent are:

  • to remove the FlushExtendingFork timing, before we were flushing domains on membatch and then flush membatch onto a RwTx, this was quite expensive in terms of performance
  • Make the routine more stable, MemoryMutation is quite buggy, so it removes one of the things we have to worry about.

@awskii
Copy link
Member

awskii commented Jun 10, 2024

So this issue fixes DupSort issue in MemBatch causing
[5/7 Execution] ExecV3: loadIntoTable : fn error: prune history commitment got invalid txNum: found 2434374999 != 2433726825 wanted"

@Giulio2002
Copy link
Contributor Author

Giulio2002 commented Jun 10, 2024

yes that too, it is mostly a performance improvement though. removes 15ish% of block time. and also paves the way for persistence of some kind of cache across domains. maybe just the Btree

@AskAlexSharov AskAlexSharov merged commit f23d636 into main Jun 11, 2024
10 checks passed
@AskAlexSharov AskAlexSharov deleted the nomembatch-2nd-attempt branch June 11, 2024 02:11
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.

3 participants