-
Notifications
You must be signed in to change notification settings - Fork 3
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
attributes publisher #4
Conversation
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.
should this be behind a flag?
also can we make sure we match L1 payload attribute SSE stream as closely as possible? ethereum/beacon-APIs#244 |
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
op-node/node/node.go
Outdated
|
||
mux := http.NewServeMux() | ||
mux.HandleFunc("/events", eventStream.HTTPHandler) | ||
addr := net.JoinHostPort(cfg.RPC.ListenAddr, strconv.Itoa(cfg.RPC.ListenPort+1)) |
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 the rpc server not be reused for the event stream? Otherwise the address and port of the SSE stream should be a flag / config
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.
op-node/node/node.go
Outdated
n.log.Warn("failed to marshal payload attributes", "err", err) | ||
return err | ||
} | ||
n.log.Info("Publishing execution payload attributes on event stream", "attrs", builderAttrs, "json", string(jsonBytes)) |
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.
is "json", string(jsonBytes)
left from debugging? logs would be quite verbose
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.
op-node/rollup/clsync/clsync.go
Outdated
func (eq *CLSync) PublishAttributes(ctx context.Context, l2head eth.L2BlockRef) error { | ||
l1Origin, err := eq.l1OriginSelector.FindL1Origin(ctx, l2head) | ||
if err != nil { | ||
eq.log.Error("Error finding next L1 Origin", "err", err) |
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.
is there a need to log errors here when its logged in https://github.com/flashbots/optimism/pull/4/files#diff-606b28086f5f2338fa6ed7aabc59df9d304de4c88d2cbabc7228be5da9592abaR141
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.
withParent := &derive.AttributesWithParent{ | ||
Attributes: attrs, | ||
Parent: l2head, | ||
IsLastInSpan: false, |
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 is IsLastInSpan
used for?
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 derivation pipeline inserts blocks in batch which they call it a span. If it is the last block in span, it sets that to the safe head.
if attributes.IsLastInSpan { |
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.
since we are inserting unsafe payloads it should be set to false
transactions := make([]*types.Transaction, len(a.Attributes.Transactions)) | ||
for i, txBytes := range a.Attributes.Transactions { | ||
var ttx types.Transaction | ||
ttx.UnmarshalBinary(txBytes) |
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 this can return an error, good to log something here at least
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 to return error properly fd07061
ParentBeaconBlockRoot *common.Hash `json:"parentBeaconBlockRoot"` | ||
Transactions types.Transactions `json:"transactions"` | ||
GasLimit uint64 `json:"gasLimit"` | ||
NoTxPool bool `json:"noTxPool,omitempty"` |
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 is NoTxPool
? its not set in ToBuilderPayloadAttributes()
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.
it is used for a block that only contains deposit txs. it was my mistake to leave it unset
No description provided.