-
Notifications
You must be signed in to change notification settings - Fork 22
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
Deneb Support #324
Deneb Support #324
Conversation
@@ -47,8 +47,8 @@ require ( | |||
go.uber.org/atomic v1.10.0 // indirect | |||
go.uber.org/multierr v1.11.0 // indirect | |||
go.uber.org/zap v1.24.0 // indirect | |||
golang.org/x/crypto v0.7.0 // indirect | |||
golang.org/x/sys v0.7.0 // indirect | |||
golang.org/x/crypto v0.10.0 // indirect |
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.
This less matters for us in the spec project, but when ssv is forced to dot he same update it may affect performance as we learnt for the RSA debacle
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.
That's true. We shall do performance tests when included. Just to remember, for the RSA case, we're actually using microsoft's openssl so it's less of an issue.
panic(err.Error()) | ||
} | ||
|
||
return &SSZSpecTest{ |
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 we only created an SSZSpecTest for Withdrawals
because we had some custom logic for it (don't remember why). I think because the latest go-eth2-client release at the time didn't have it.
I think if BeaconBlock
now supports withdrawals (and also Deneb blobs) we can remove the old withdrawals test and code (maybe in separate PR after this because we have enough code here)
Anyhow, you are aware of my evil plan with value check
that will make all this logic/tests unnecessary 😈
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.
types/ssz.go
Outdated
func (b SSZBlobZGCommitments) GetTree() (*ssz.Node, error) { | ||
return ssz.ProofTree(b) | ||
} | ||
|
||
func (b SSZBlobZGCommitments) HashTreeRootWith(hh ssz.HashWalker) error { | ||
// taken from https://github.com/attestantio/go-eth2-client/blob/a05485e0e75749f2b6912db2972a35ec2ec37c3b/spec/deneb/beaconblockbody_ssz.go#L577C3-L586C49 | ||
if size := len(b); size > 4096 { | ||
err := ssz.ErrListTooBigFn("BeaconBlockBody.BlobKZGCommitments", size, 4096) | ||
return err | ||
} | ||
subIndx := hh.Index() | ||
for _, i := range b { | ||
hh.PutBytes(i[:]) | ||
} | ||
numItems := uint64(len(b)) | ||
hh.MerkleizeWithMixin(subIndx, numItems, 4096) | ||
return 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.
did you do this because we did the same thing with Withdrawals?
I think we did this to Withdrawals because there wasn't a release available at the time (am I correct @olegshmuelov ?)
If the updated go-eth2-client has this logic then I don't think we need to copy it..
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.
Please see previous discussion
#234 (comment)
There seems to be some epoch activation epochs here Not sure if it is final but maybe let's put this in and later change it so we can all approve this PR |
@GalRogozinski If I update it, I start getting:
because of the dutyValueCheck. |
case spec.DataVersionCapella: | ||
require.NotNil(t, vBlk.Capella) | ||
blkSSZ, err = vBlk.Capella.MarshalSSZ() | ||
require.NoError(t, err) | ||
case spec.DataVersionDeneb: |
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 you scroll up you see a comment for issue 219. IMO you can delete the comment and close the issue
Closes #219 |
if vBlk.Deneb.Block == nil { | ||
panic("empty block") | ||
} | ||
return &deneb.SignedBeaconBlock{ |
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.
Question: there is also api/v1/deneb/signedblindedbeaconblock.go
shouldn't this one be used here?
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.
Well noticed. I'm fixing it now. At first, I used version.SignedBeaconBlock
because it was already being used for the blinded proposer beacon roots, strangely enough. But the correct one should be the SignedBlindedBeaconBlock
indeed.
types/consensus_data.go
Outdated
// we can upper limit transactions to 2^21, together with the rest of the data 2*2^21 = 2^22 = 4,194,304 bytes | ||
// Exmplanation on why transaction sizes are so big https://github.com/ethereum/consensus-specs/pull/2686 | ||
DataSSZ []byte `ssz-max:"4194304"` // 2^22 | ||
// we can upper limit transactions to 2^21, together with the rest of the data 1315964 + 2^21 = 3,413,116 bytes |
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 did the max decrease from ~4MB to ~3MB (2*2^21 to 2^21)? If anything, the introduction of blobs should have increased our ssz-max, no?
BTW, they're thinking of increasing gas size from 30M to 40M very soon, we should probably adapt our calculations (if needed).
I think we could maybe check what was Prysm's approach to this. Prior to receiving blocks from peers, they must do some sort of validation on the size, I guess.
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.
See this comment where we corrected a previous mistake regarding attestations:
#324 (comment)
checkout the commented python script
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.
they're thinking of increasing gas size from 30M to 40M very soon
When they are done thinking we'll fix this
it will only be on the next hard fork (best case) so there is time
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.
they're thinking of increasing gas size from 30M to 40M very soon
When they are done thinking we'll fix thisit will only be on the next hard fork (best case) so there is time
I think maybe we'd like to support a probable gas increase without having to fork?
This reverts commit f2d5899.
This reverts commit 0a559e2.
* Revert "Revert "Deneb Support (#324)"" This reverts commit 0a559e2. * use custom old encodings * generate-jsons * Add phase0 version * Bellatrix -> Capella on a single test * Generate jsons * Revert Capella -> Deneb on a single test --------- Co-authored-by: MatheusFranco99 <[email protected]>
…compatibility (#362) * Revert "Deneb Support (#324)" This reverts commit f2d5899. * Add jsons (#357) * create jsons * compare with jsons * generate jsons for qbft signed message * compare qbft signed message with json state * fix lint error * generate jsons for create msg test * generate jsons for create msg test * use correct test type * use correct test name * Return deneb with old encoding (#360) * Revert "Revert "Deneb Support (#324)"" This reverts commit 0a559e2. * use custom old encodings * generate-jsons * Add phase0 version * Bellatrix -> Capella on a single test * Generate jsons * Revert Capella -> Deneb on a single test --------- Co-authored-by: MatheusFranco99 <[email protected]> --------- Co-authored-by: MatheusFranco99 <[email protected]>
Major changes
VersionedProposal
orVersionedBlindedProposal
) to the Beacon node, Deneb uses:VersionedBlindedProposal
: aBlindedBeaconBlock
, similarly to other forks.VersionedProposal
:BlockContents
, differently from older forks which usedBeaconBlocks
. Nonetheless, the validator signature is still over the beacon block (BlockContents.Block
), i.e. the signing root is kept the same.Minor changes
ConsensusData
andFullData
fields, since Deneb uses aBlockContents
object which contains a Beacon block, KZGProofs (288 bytes) and Blobs of 786432 bytes.SSZ generation notes
make generate-ssz
you may see new logsINFO: Skipped ssz generated object:
.consensus_data_encoding.go
file. It imports an unused library. For that, we created an issue in Fastssz which remains unsolved. As a temporary arrangement, we added, in thegenerate.go
in./types
, a command to remove the unused import.Attestantio update notes
DataVersion
used to bephase0
. This was was changed to becomeunknown
. Therefore, some testing utils that didn't define theDataVersion
in theConsensusData
had to be updated.Leftovers To-Do
References