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.

Sign up for GitHub

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

Jump to bottom

Merge/continous block improvement #4488

Merged
merged 13 commits into from
Oct 4, 2022
Merged

Merge/continous block improvement #4488

merged 13 commits into from
Oct 4, 2022

Conversation

LukaszRozmej
Copy link
Member

@LukaszRozmej LukaszRozmej commented Aug 27, 2022 •
edited
Loading

Resolves #3592

This is based on 14.1 version. Merge ready if someone wants to use it.

Changes:

  • Adds the ability to constantly improve the block between ForkChoiceUpdated and GetPayload calls, this allows to create better and more profitable blocks if there is a larger gap between them. Between FCU and GetPayload can be realistically 7-10s, depending on CL, networking, and processing time. Currently picked delay between improvements to 3s, which could produce 3-4 blocks per slot with those timings.
  • This is carefully designed to:
  1. Not leak block improvements - they are disposed, there is only one improvement per payload id. The underlying task is canceled on dispose.
  2. On GetPayload if we already constructed non-empty block to return immediately
  3. Not to run further improvements when previous one was disposed or block slot has passed.
  • Moved all payload creation tests to separate file

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some
Should be tested on shadow-fork. Would be good to grab memory dump to ensure there is no leak.

@flcl42
Copy link
Contributor

flcl42 commented Aug 27, 2022 •
edited
Loading 

Environment.TickCount64 > DateTime.UtcNow > DateTime.Now
^ no sync/update issues   ^ no day-light saving time issues

I did not face such issues, but I believe if they will appear, it will be veery hard to debug

@LukaszRozmej
Copy link
Member Author

LukaszRozmej commented Aug 27, 2022 •
edited
Loading 

Environment.TickCount64 > DateTime.UtcNow > DateTime.Now
^ no sync/update issues   ^ no day-light saving time issues

I did not face such issues, but I believe if they will appear, it will be veery hard to debug

Yeah you are right. I somehow convinced myself that DateTime would not be a problem. Moved to DateTimeOffset.UtcNow as Environment.TickCount64 can have issues with resolution: https://docs.microsoft.com/en-us/dotnet/api/system.environment.tickcount?view=net-6.0#remarks

Those might not be a big issue in prod, but I want my test to have tight timings

@LukaszRozmej LukaszRozmej requested a review from asdacap August 29, 2022 11:44
@LukaszRozmej
Copy link
Member Author

MSF11 nodes looked, waiting for MSF12 good: https://seq.nethermind.io/#/events?filter=Contains(@Message,%20%22improved%22)%20and%20Logger%20%3D%20'Merge.Plugin.BlockProduction.PayloadPreparationService'

@LukaszRozmej
Merge remote-tracking branch 'origin/master' into merge/continous-blo…
1c8c69f 
…ck-improvement

# Conflicts:
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V1.cs
#	src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/BlockImprovementContextFactory.cs
#	src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/Boost/BoostBlockImprovementContextFactory.cs
#	src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/IBlockImprovementContextFactory.cs
@LukaszRozmej
Merge remote-tracking branch 'origin/master' into merge/continous-blo…
0233176 
…ck-improvement

# Conflicts:
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V1.cs
@LukaszRozmej
Copy link
Member Author

Sepolia tests showed no memory leak, everything is clearing up fine.

MarekM25
MarekM25 reviewed Oct 4, 2022
View reviewed changes
src/Nethermind/Nethermind.sln
@@ -230,6 +230,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Nethermind.Merge.AuRa.Test"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Nethermind.Merge.AuRa", "Nethermind.Merge.AuRa\Nethermind.Merge.AuRa.csproj", "{F166365D-6E31-401B-9753-59EADB800E2A}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MathGmp.Native", "..\Math.Gmp.Native\MathGmp.Native\MathGmp.Native.csproj", "{16E039D0-8944-4EFB-8624-FC9EB9FF75DB}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm looks like not related to the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but fixes latest master

@LukaszRozmej LukaszRozmej merged commit 415fd83 into master Oct 4, 2022
@LukaszRozmej LukaszRozmej deleted the merge/continous-block-improvement branch October 4, 2022 12:11
Andrew-Pohl pushed a commit to fuseio/nethermind that referenced this pull request Oct 7, 2022
@LukaszRozmej @Andrew-Pohl
Merge/continous block improvement (NethermindEth#4488)
e29c182 
* Make Payload Production continuous improving block while in slot.

* Remove whitespace

* Change timings for more reliable tests

* Comments + better naming

* Move to DateTimeOffset

* Add more logs

* Review improvements

* Move to TimeSpan's

* Fix merging master
rubo pushed a commit that referenced this pull request Oct 13, 2022
@LukaszRozmej @rubo
Merge/continous block improvement (#4488)
c21b094 
* Make Payload Production continuous improving block while in slot.

* Remove whitespace

* Change timings for more reliable tests

* Comments + better naming

* Move to DateTimeOffset

* Add more logs

* Review improvements

* Move to TimeSpan's

* Fix merging master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcindsobczak marcindsobczak marcindsobczak approved these changes

@MarekM25 MarekM25 MarekM25 approved these changes

@asdacap asdacap asdacap approved these changes

@flcl42 flcl42 Awaiting requested review from flcl42

@dceleda dceleda Awaiting requested review from dceleda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implement dynamic block building
5 participants
@LukaszRozmej @flcl42 @asdacap @MarekM25 @marcindsobczak