-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add state diff serialization #15250
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
base: develop
Are you sure you want to change the base?
Add state diff serialization #15250
Conversation
d9f6001
to
c461552
Compare
f41008d
to
1beba7a
Compare
} | ||
ret.eth1VotesAppend = ((*data)[0] == nilMarker) | ||
eth1DataVotesLength := int(binary.LittleEndian.Uint64((*data)[1 : 1+8])) // lint:ignore uintcast | ||
if len(*data) < 1+8+eth1DataVotesLength*eth1DataLength { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should do a max() check either 9 or 1+8+eth1DataVotesLength*eth1DataLength whichever is longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The length cannot be known until after checking for the first 8 bytes. Since the nilmarker anyway needs to be present, that's why I check for 9 instead of just 9
if err := ret.readSlot(&data); err != nil { | ||
return nil, err | ||
} | ||
if err := ret.readFork(&data); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would be very easy to forget something here, I wonder what would be the best way to keep track to make sure we add a new time every time we touch the state
earliestExitEpoch primitives.Epoch // override. | ||
consolidationBalanceToConsume primitives.Gwei // override. | ||
earliestConsolidationEpoch primitives.Epoch // override. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil { | ||
return nil, err | ||
} | ||
ret.currentSyncCommittee, err = target.CurrentSyncCommittee() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using a nil marker when there is no diff in the sync committee? Same for execution payload header
) | ||
|
||
// ConvertToElectra converts a Deneb beacon state to an Electra beacon state. It does not perform any fork logic. | ||
func ConvertToElectra(beaconState state.BeaconState) (state.BeaconState, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i think you should reuse the code and reduce the duplicate LOC
b.sharedFieldReferences[types.PendingPartialWithdrawals] = stateutil.NewRef(1) | ||
|
||
b.pendingPartialWithdrawals = pendingPartialWithdrawals | ||
b.markFieldAsDirty(types.PendingPartialWithdrawals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that could be an issue. @rkapka what do you think?
proposerLookahead[i] = primitives.ValidatorIndex(v) | ||
} | ||
// Proposer lookahead must be exactly 2 * SLOTS_PER_EPOCH in length. We fill in with zeroes instead of erroring out here | ||
for i := len(proposerLookahead); i < 2*fieldparams.SlotsPerEpoch; i++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's supposed to be params.BeaconConfig().MinSeedLookahead + 1
, not just 2
, although it is 2
in both cases.
@@ -0,0 +1,109 @@ | |||
package helpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we need tests for all of these 😱 . Maybe a friendly AI could help with some of that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1bdd78
Adds serialization code for state diffs. Adds code to create and apply state diffs Adds fuzz tests and benchmarks for serialization/deserialization Co-authored-by: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
@@ -0,0 +1,109 @@ | |||
package helpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use proto.Equal(s, t)
for all of these functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally dislike proto.Equal as it requires an extra serialization/allocation and it fails in an obscure manner. Eventually we want to move away from protobufs so it would even be backwards-looking
vals = append(vals, v.WithdrawalCredentials...) | ||
} | ||
vals = binary.LittleEndian.AppendUint64(vals, v.EffectiveBalance) | ||
if v.Slashed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we reverse this to be
if !v.Slashed {
als = append(vals, nilMarker)
else {
vals = append(vals, notNilMarker)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? this is just an arbitrary choice, but generally, I've used that the default value should always serialize to zero because that way the default diff is smaller. Wouldn't want to change this.
Bast Co-authored-by: Bastin <[email protected]>
Adds custom functions to create differences between beacon states, serialize them and apply them.
This adds a package
hdiff
which exposes one type and two functionsThe type is exposed and the members are accessible because in a future PR we will expose functions to apply differences only to the validator set and the balance slice.
Current benchmarks for applications of a diff between a state 6 months ago in Deneb vs current Electra state point to 1.1" for deserialization and application of the diff, on AMD Ryzen 5 3600. Creation and serialization of the diff takes 530ms on this computer and produces a 27MB diff.