-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Espresso Streamer #66
base: celo-integration
Are you sure you want to change the base?
Conversation
7be4db2
to
403f472
Compare
push today's changes forget to add the file
log add caff node config fix config fix the bug...thanks... foget the error handling today's push remove FilterAndFind
added a Verify, but remember to debug commit today's change current change
foget to push the file again
403f472
to
57e2acf
Compare
// OverrideMessageExpiryTimeInterop is only used for testing purposes. | ||
// It is used to override the protocol-defined interop message time expiry. | ||
// DO NOT this read value directly. Use GetMessageExpiryTimeInterop instead. | ||
OverrideMessageExpiryTimeInterop uint64 `json:"override_message_expiry_time_interop,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.
Weird, this argument is in celo-integration
,but not in celo-tip
, and it's being added when I'm doing a rebase on celo-integration-rebased
op-batcher/batcher/driver.go
Outdated
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.
As discussed offline, we don't need the MultipleNodesClient
.
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'll go to this in next PR. A comment from Jeb https://github.com/EspressoSystems/book/pull/37#discussion_r2006587177
@@ -561,6 +561,10 @@ func (n *OpNode) Start(ctx context.Context) error { | |||
return err | |||
} | |||
} | |||
if n.cfg.CaffNodeConfig.IsCaffNode { | |||
// Sishan TODO: deal with this in a better way and add error handling |
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 do a very similar error handling like line 559 above.
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's a kind of different, I have to figure out how to do error handling for go routine.
case BatchUndecided: | ||
remaining = append(remaining, s.messageWithHeight[i:]...) | ||
s.messageWithHeight = remaining | ||
return nil, false, io.EOF |
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.
Do we need this case? It looks like CheckBatchEspresso
can't return BatchUndecided
.
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.
Here is a placeholder for the next task, it may be needed when I add more checks there.
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 you add a comment so we won't forget to remove or update this part?
// We dont need to check majority here because when we eventually go | ||
// to fetch a block at a certain height, | ||
// we will check that a quorum of nodes agree on the block at that height, | ||
// which wouldn't be possible if we were somehow are given a height | ||
// that wasn't finalized at all |
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.
As discussed offline, we won't use the majority rule.
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 understand that after discussion we will use this rule right?
// from eth_sign. | ||
func Verify(data []byte, signature []byte, expected common.Address) error { | ||
// Ensure the signature is 65 bytes long (r[32] || s[32] || v[1]) | ||
if len(signature) != 65 { |
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 do we need to verify these conditions? Why not relying on standard signature verification as described in https://goethereumbook.org/signature-verify/ ?
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.
Yes, this is exactly what I'm using in the follow-up PR #73
|
||
// Ethereum's eth_sign prefixes the data before signing. | ||
// The prefixed message is: "\x19Ethereum Signed Message:\n" + len(data) + data | ||
prefixedMsg := fmt.Sprintf("\x19Ethereum Signed Message:\n%d%s", len(data), data) |
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.
Not sure we need to do this ourselves. Again, unless we have a good reason not to, we should just use the functions described in https://goethereumbook.org/signature-verify/
Closes https://app.asana.com/0/1209392461754458/1209619016749013/f
And we can leave the remaining TODOs and smokey test to https://app.asana.com/0/1209392461754458/1209619016749015/f
This PR:
This PR does not:
Key places to review: