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.

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

chore(builder): store decoded constraints in the cache #68

Merged
merged 8 commits into from
Jun 7, 2024

Conversation

namn-grg
Copy link
Contributor

@namn-grg namn-grg commented Jun 5, 2024

This PR changes the way constraints are stored in the cache.
Before, constraints were stored in a signed form, which needed to be decoded during the building. Now it is stored in the decoded form in the first place.
fixes #51

  • Change the cache to type - *shardmap.FIFOMap[uint64, types.HashToConstraintDecoded]
  • Add function to utils for decoding constraints DecodeConstraint
  • Decode the constraint when received the first time from the relay and store it in the cache.

Implementation details in the review below

Comment on lines +368 to +372
decodedConstraints, err := DecodeConstraint(constraint)
if err != nil {
log.Error("Failed to decode transaction RLP: ", err)
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decode the constraints from the function in utils

Comment on lines +394 to +395
// Update the slot constraints
b.constraintsCache.Put(constraint.Message.Slot, slotConstraints)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We put the updated slot constraints in the cache. If there are no changes to it, we put the same back

Comment on lines +387 to +390
for hash := range decodedConstraints {
if !seenTx[hash] {
// The constraint is new, we will add this to the slot constraints
slotConstraints[hash] = decodedConstraints[hash]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we iterate over the decoded constraints and see if any of the constraints present in it is different and if so we add it to the slotConstraints

@namn-grg namn-grg requested a review from thedevbirb June 5, 2024 13:25
@namn-grg namn-grg changed the title feat(builder): store decoded constraints in the cache chore(builder): store decoded constraints in the cache Jun 6, 2024
@namn-grg namn-grg marked this pull request as ready for review June 6, 2024 04:50
@namn-grg namn-grg added C: block-builder Component: block builder T: chore Type: Chore labels Jun 6, 2024
Copy link
Collaborator

@merklefruit merklefruit left a comment

Choose a reason for hiding this comment

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

LGTM! just one nit

b.constraintsCache.Put(constraint.Message.Slot, append(slotConstraints, constraint))

// Update the slot constraints
b.constraintsCache.Put(constraint.Message.Slot, slotConstraints)
}
}

return nil
}

func (b *Builder) GetConstraintsForSlot(slot uint64) types.HashToConstraintDecoded {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just remove this function now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, removed it!

@merklefruit merklefruit merged commit 11af503 into unstable Jun 7, 2024
@merklefruit merklefruit deleted the feat/builder/constraint-cache branch June 7, 2024 11:39
Copy link
Contributor

@thedevbirb thedevbirb left a comment

Choose a reason for hiding this comment

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

Some nits! Thanks for the PR

for _, constraint := range constraintsSigned {
decodedConstraints, err := DecodeConstraint(constraint)
if err != nil {
log.Error("Failed to decode transaction RLP: ", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Error("Failed to decode transaction RLP: ", err)
log.Error("Failed to decode constraint: ", err)

Comment on lines +381 to +385
// Temporary map to keep track of the constraints that are already in the slot
seenTx := make(map[common.Hash]bool)
for hash := range slotConstraints {
seenTx[hash] = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this additional hashmap, you can just check if slotConstraint[hash] == nil, or writing every without doing this check

@@ -712,7 +710,7 @@ func (b *Builder) runBuildingJob(slotCtx context.Context, proposerPubkey phase0.
log.Debug("runBuildingJob", "slot", attrs.Slot, "parent", attrs.HeadHash, "payloadTimestamp", uint64(attrs.Timestamp))

// fetch constraints here
constraints := b.GetConstraintsForSlot(attrs.Slot)
constraints, _ := b.constraintsCache.Get(attrs.Slot)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should handle the other value returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get returns the value and true if the value is present, otherwise it returns the default value and false.

zero value for the map's value type is returned if key is not present.

I don't think it will make a difference?

)

var errHTTPErrorResponse = errors.New("HTTP error response")

func DecodeConstraint(constraint *common.SignedConstraints) (types.HashToConstraintDecoded, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func DecodeConstraint(constraint *common.SignedConstraints) (types.HashToConstraintDecoded, error) {
func DecodeConstraints(constraints *common.SignedConstraints) (types.HashToConstraintDecoded, error) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: block-builder Component: block builder T: chore Type: Chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't decode constraints when sealing blocks
3 participants