Skip to content

Conversation

@terencechain
Copy link
Collaborator

No description provided.

@terencechain terencechain force-pushed the add-gloas-block branch 2 times, most recently from da955a3 to 8bbcf1f Compare August 31, 2025 03:16
@terencechain terencechain marked this pull request as ready for review November 5, 2025 16:23
@terencechain terencechain force-pushed the add-gloas-block branch 3 times, most recently from 8ecec5d to 4370e34 Compare November 5, 2025 21:10
_ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20)
)

type ExecutionPayloadEnvelope struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm why was this file included without the protobuf file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch, the file was a left over and i checked it in by accident
in the latest commit i have that removed and addressed your other feedback

copied.SyncAggregate = body.SyncAggregate.Copy()
}

copied.SignedExecutionPayloadBid = copySignedExecutionPayloadBid(body.SignedExecutionPayloadBid)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit can we move this to the bottom since it's new?

james-prysm
james-prysm previously approved these changes Nov 18, 2025
Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +2808 to +2811
if testVersion == version.Gloas {
// TODO(16027): Unskip light client tests for Gloas
continue
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note we are going to redo this, we are experimenting a different approach in: #16030
but it shouldn't block anyone reviewing this PR

Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

A bunch of getters should now error on Gloas, at least Execution, KZG commitments, Execution Requests.

First serious pass, will do another in a couple of hours.

@@ -82,13 +82,15 @@ func NewSignedBeaconBlock(i any) (interfaces.SignedBeaconBlock, error) {
return initBlindedSignedBlockFromProtoFulu(b)
case *eth.GenericSignedBeaconBlock_BlindedFulu:
return initBlindedSignedBlockFromProtoFulu(b.BlindedFulu)
case *eth.SignedBeaconBlockGloas:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a "Generic" version like for previous forks, something like b.Gloas.Block?

@@ -138,13 +140,15 @@ func NewBeaconBlock(i any) (interfaces.ReadOnlyBeaconBlock, error) {
return initBlindedBlockFromProtoFulu(b)
case *eth.GenericBeaconBlock_BlindedFulu:
return initBlindedBlockFromProtoFulu(b.BlindedFulu)
case *eth.BeaconBlockGloas:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a generic version like previous forks? b.Gloas.Block

@@ -629,6 +641,8 @@ func BuildSignedBeaconBlockFromExecutionPayload(blk interfaces.ReadOnlySignedBea
},
Signature: sig[:],
}
case version.Gloas:
Copy link
Contributor

Choose a reason for hiding this comment

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

This case should fail much earlier when trying to get the Execution from the body. It's better to return early from this function with an error. Also, the fact that this function passed tests shows that this couldn't have been covered. So a regression test is needed.

@@ -157,14 +159,17 @@ func (b *SignedBeaconBlock) PbGenericBlock() (*eth.GenericSignedBeaconBlock, err
return &eth.GenericSignedBeaconBlock{
Block: &eth.GenericSignedBeaconBlock_Fulu{Fulu: bc},
}, nil
case version.Gloas:
// Gloas doesn't support GenericSignedBeaconBlock yet
Copy link
Contributor

Choose a reason for hiding this comment

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

This answers my pervious questions.... Probably better to discuss this offline

Comment on lines 682 to 685
if err := pb.UnmarshalSSZ(buf); err != nil {
return err
}
var err error
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the design on all previous cases, but is there any reason to not have done:

Suggested change
if err := pb.UnmarshalSSZ(buf); err != nil {
return err
}
var err error
err := pb.UnmarshalSSZ(buf);
if err != nil {
return err
}

Comment on lines 1582 to 1583
signedExecutionPayloadBid: pb.SignedExecutionPayloadBid,
blsToExecutionChanges: pb.BlsToExecutionChanges,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the ordering?

@@ -182,3 +182,21 @@ func (b *SignedBeaconBlock) SetExecutionRequests(req *enginev1.ExecutionRequests
b.block.body.executionRequests = req
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

SetExecutionRequests should also fail if the version is Gloas or more, same for the Execution Payload setter.

@@ -80,6 +80,8 @@ func (b *SignedBeaconBlock) Copy() (interfaces.SignedBeaconBlock, error) {
return initBlindedSignedBlockFromProtoFulu(pb.(*eth.SignedBlindedBeaconBlockFulu).Copy())
}
return initSignedBlockFromProtoFulu(pb.(*eth.SignedBeaconBlockFulu).Copy())
case version.Gloas:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this line. But the getters file needs to be futher modified. Some getters do not make sense for Gloas and instead of returning nil and risking a panic they should return an error. For example the current path for Execution() will return nil instead of erroring out. The same for the ExecutionRequests(). We should add errors for all getters that would fail.

Comment on lines 60 to 61
payloadAttestations []*eth.PayloadAttestation
signedExecutionPayloadBid *eth.SignedExecutionPayloadBid
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverse order here.

potuz
potuz previously approved these changes Nov 24, 2025
potuz
potuz previously approved these changes Nov 24, 2025
terencechain and others added 3 commits November 24, 2025 14:42
**Review after #16033 is merged**

**What type of PR is this?**
Other

**What does this PR do? Why is it needed?**
This PR refactors the code to find the corresponding slot of a given
block root using state summary or the block itself, into its own
function `SlotByBlockRoot(ctx, blockroot)`.

Note that there exists a function `slotByBlockRoot(ctx context.Context,
tx *bolt.Tx, blockRoot []byte)` immediately below the new function. Also
note that this function has two drawbacks, which led to creation of the
new function:
- the old function requires a boltdb tx, which is not necessarily
available to the caller.
- the old function does NOT make use of the state summary cache. 

edit: 
- the old function also uses the state bucket to retrieve the state and
it's slot. this is not something we want in the state diff feature,
since there is no state bucket.
@terencechain terencechain added this pull request to the merge queue Nov 25, 2025
Merged via the queue into develop with commit 5bbdebe Nov 25, 2025
18 checks passed
@terencechain terencechain deleted the add-gloas-block branch November 25, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants