feat: de-duplicate payloads from persisted beacon blocks #6029
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
Performance Report✔️ no performance regression detected Full benchmark results
|
|
Some todos:
|
@dapplion I am working on that conversion now. When I did it this evening the unit tests for capella broke. I had logic to convert the mainnet mocks to work with the
I remembered chatting with you about this a couple weeks ago and got it ready for you :) Apologies, I should have brought this up when we spoke before standup. I forgot with all the other stuff we chatted about. I posted those results from the perf test on the issue: I copied the results in that comment below so they are part of this PR too for visibility. The test file is in this commit of this PR: The results seem like the serialize is the way to go, is a couple of orders of magnitude faster, but would love to get your opinion before I delete the losing codepath. The perf test is in the commit linked above so you can check the methodology. I will leave the perf test as part of this PR if serialize is how you want to go. I was thinking about removing the generator function and just returning a promise after our discussion before standup. I will rerun the perf tests like that to compare and post them in a comment below tomorrow once I sort out the mock serialization bug. fullOrBlindedBlock
BlindedOrFull to full
phase0
✔ phase0 to full - deserialize first 9646.737 ops/s 103.6620 us/op - 4856 runs 0.617 s
✔ phase0 to full - convert serialized 2865330 ops/s 349.0000 ns/op - 1740003 runs 0.909 s
altair
✔ altair to full - deserialize first 5352.431 ops/s 186.8310 us/op - 2699 runs 0.697 s
✔ altair to full - convert serialized 2967359 ops/s 337.0000 ns/op - 1598138 runs 0.808 s
bellatrix
✔ bellatrix to full - deserialize first 3991.474 ops/s 250.5340 us/op - 1208 runs 0.553 s
✔ bellatrix to full - convert serialized 2463054 ops/s 406.0000 ns/op - 879455 runs 0.505 s
capella
✔ capella to full - deserialize first 3660.175 ops/s 273.2110 us/op - 1846 runs 0.783 s
✔ capella to full - convert serialized 2364066 ops/s 423.0000 ns/op - 2012155 runs 1.21 s
deneb
✔ deneb to full - deserialize first 3621.915 ops/s 276.0970 us/op - 1827 runs 0.806 s
✔ deneb to full - convert serialized 2398082 ops/s 417.0000 ns/op - 506726 runs 0.303 s
BlindedOrFull to blinded
phase0
✔ phase0 to blinded - deserialize first 12937.95 ops/s 77.29200 us/op - 4230 runs 0.404 s
✔ phase0 to blinded - convert serialized 1.000000e+7 ops/s 100.0000 ns/op - 3120420 runs 0.606 s
altair
✔ altair to blinded - deserialize first 7185.198 ops/s 139.1750 us/op - 2170 runs 0.439 s
✔ altair to blinded - convert serialized 9900990 ops/s 101.0000 ns/op - 2588758 runs 0.505 s
bellatrix
✔ bellatrix to blinded - deserialize first 100.1679 ops/s 9.983241 ms/op - 76 runs 1.26 s
✔ bellatrix to blinded - convert serialized 92.22430 ops/s 10.84313 ms/op - 117 runs 1.77 s
capella
✔ capella to blinded - deserialize first 45.29530 ops/s 22.07735 ms/op - 48 runs 1.58 s
✔ capella to blinded - convert serialized 43.09465 ops/s 23.20474 ms/op - 50 runs 1.66 s
deneb
✔ deneb to blinded - deserialize first 45.42834 ops/s 22.01269 ms/op - 51 runs 1.63 s
✔ deneb to blinded - convert serialized 46.20545 ops/s 21.64247 ms/op - 46 runs 1.50 s |
btw @dapplion I added these, and one for converting from generator and retesting perf, to the checklist above |
|
@matthewkeil thanks! the differences in performance do not justify doing the complex byte manipulation IMO. Just merge structs. |
g11tech
requested changes
Oct 16, 2023
@dapplion there is a benchmark regression after removing the serialized blinding/unblinding. There is not a big difference in time for the blinding process and the increase in the |
matthewkeil
force-pushed
the
mkeil/dedup-beacon-block-2
branch
from
697870f to
158cb40
Compare
g11tech
force-pushed
the
mkeil/dedup-beacon-block-2
branch
from
8d804ea to
2fb8f48
Compare
g11tech
reviewed
Oct 30, 2023
| return firstByte - readExtraDataOffsetAt > 92; | ||
| } | ||
|
|
||
| // same as isBlindedSignedBeaconBlock but without type narrowing |
There was a problem hiding this comment.
what is the issue with type narrowing?
g11tech
reviewed
Oct 30, 2023
| canonical, | ||
| header: { | ||
| message: blockToHeader(config, block.message), |
There was a problem hiding this comment.
its cleaner to extend blockToHeader to accept full or blinded,
also then the root above can be calulated from the header returned by hashtree root of the blockheader ... it should be more efficient since body won't be merklized twice
g11tech
force-pushed
the
mkeil/dedup-beacon-block-2
branch
from
2fb8f48 to
adfa2f5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #6029 +/- ##
============================================
+ Coverage 62.52% 62.73% +0.21%
============================================
Files 575 578 +3
Lines 60985 61367 +382
Branches 2125 2114 -11
============================================
+ Hits 38130 38500 +370
- Misses 22816 22829 +13
+ Partials 39 38 -1 |
matthewkeil
force-pushed
the
mkeil/dedup-beacon-block-2
branch
from
adfa2f5 to
c36a0c9
Compare
nflaig
reviewed
Jul 1, 2024
| @@ -29,6 +29,7 @@ import { | |||
| isBlindedBeaconBlock, | |||
| BeaconBlock, | |||
| SignedBeaconBlock, | |||
| FullOrBlindedSignedBeaconBlock, | |||
There was a problem hiding this comment.
Why did we reintroduce this type? Based on discussion in type refactor we don't want this type
|
Closed in favor of #7034 |
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.
NOTE: The Sim Merge Test is not going to pass. The container that it runs one test in needs to be updated. @g11tech is going to look for the Dockerfile and I will help get it updated and published so it will pass. The image is based on a pre-shanghai image that does not have
engine_getPayloadBodiesByHashV1available. This is the image:https://hub.docker.com/r/g11tech/mergemock
Two things still need to be double checked before moving to ready:
getBlockworks as expecteddenebblock (ask @g11tech how to generate with valid data. perhaps can pull from devnet 9??)denebblock unit tests. need to add a value for the fork epoch and spoof valid slots in the mockspackages/beacon-node/src/util/fullOrBlindedBlock.tsengine_getPayloadBodiesByHashV1Motivation
Lodestar is saving data that is also saved in the execution client database. In particular we are persisting transactions and withdrawals in the
blockandblockArchivedatabases.Description
Stores blinded blocks in both the hot and cold db. Modifies calls for data retrieval that require the full block, ReqResp and API, to splice in the missing transactions and withdrawals.
Closes #5671
** How to test **
Extensive unit and perf testing was conducted to make sure that this should work correctly.