-
Notifications
You must be signed in to change notification settings - Fork 17
Primev poc #629
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
base: main
Are you sure you want to change the base?
Primev poc #629
Conversation
…r to use this config
received_bid_digest text NOT NULL, | ||
received_bid_signature text NOT NULL, | ||
bidder_node_address text NOT NULL, | ||
PRIMARY KEY (provider_address, commitment_digest, block_number) |
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.
isn't there a canonical commitment hash we can use as primary key? If not, commitment_digest
and provider_address
alone should be unique without including the block_number
, shouldn't 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.
the commitment hash is not sent back when submitting Bid via grpc, anyway we can use commitment_digest
and provider_address
as primary key.
optional TraceContext trace = 3; | ||
} | ||
|
||
message Commitment { |
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 we import this from the primev repo instead of copy and pasting 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.
it does not support our message handler interface, that's why I had to create this new message
@@ -0,0 +1,286 @@ | |||
package primev |
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 we use the multi event syncer from the service keyper here? The point of it was to avoid repeating all the event syncing code. Probably best to move it to medley so we don't have cross-keyperimpl dependencies. See e.g. eventtriggerregisteredprocessor.go for an example how to use 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.
makes sense, lets add it in a separate PR
-- the schema. We'll use this to check we're using the right schema. | ||
|
||
CREATE TABLE commitment( | ||
tx_hashes text[] NOT NULL, |
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.
We have a separate table for txs. Does tx_hashes correspond to them? If so, we should get rid of the duplication and rely on referencing them (if it's for convenience it's better to aggregate in a query).
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, the query is already aggregated as InsertMultipleTransactionsAndUpsertCommitment
|
||
// TODO: before sending the dec trigger, we need to check if majority of providers have generated commitments | ||
|
||
h.decryptionTriggerChannel <- broker.NewEvent(decryptionTrigger) |
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'm not sure if it's a good idea to send the trigger in the handler directly or if we should rather only dump the commitments in the db and then check in a separate goroutine if a trigger should be sent. I guess the advantage of doing it here is that it most tight timing-wise, but conceptually it seems wrong and we potentially delay propagation of the commitment.
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.
Also, in any case, this should check for context cancellation as it might block
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 was hoping the context would be managed by p2p package already as this is just a handler for a message, let me know if it still needs any update.
The trigger sending is done here, to ensure no extra complexity is added in the new go routine, maybe it would make sense to include it in a separate go routine, once we introduce checking for number of providers committed.
@@ -0,0 +1,19 @@ | |||
package p2pmsg |
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 is this in a separate file instead of just messages.go
where all the other ones are?
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.
Added in the same file, I just wanted to keep it separate as it is a new thing added, but yeah, makes sense to include in same file too
merge main to primev poc
* [rolling-shutter op-bootstrap](rolling-shutter_op-bootstrap.md) - Bootstrap validator utility functions for a shuttermint chain | ||
* [rolling-shutter op-keyper](rolling-shutter_op-keyper.md) - Run a Shutter optimism keyper node | ||
* [rolling-shutter p2pnode](rolling-shutter_p2pnode.md) - Run a Shutter p2p bootstrap node | ||
* [rolling-shutter primevkeyper](rolling-shutter_primevkeyper.md) - Run a Shutter keyper for PrimeV POC |
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.
(General) I think we should add a dedicated spec document for this new keyper implementation under the existing docs/spec.md that describes the high-level design, key components, and how it differs from other implementations. Ideally we should have that for all implementations, but we can start here as part of this PR.
received_bid_digest text NOT NULL, | ||
received_bid_signature text NOT NULL, | ||
bidder_node_address text NOT NULL, | ||
PRIMARY KEY (commitment_digest, provider_address) |
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 the commitment digest unique per commitment to use it as a primary key 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.
The commitment digest is the hash of Bid struct, which will be signed by any provider, so combination of commitment digest and provider address will always be unique
$11 | ||
FROM inserted_transactions | ||
ON CONFLICT (provider_address, commitment_digest) | ||
DO UPDATE SET |
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.
We're assuming here that we are receiving the same commitment with a different set of transactions? Is that possible? Given that you are using the digest and bidder address as a primary key, shouldn't these fields be immutable?
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 is using provider address, which is builder's address, which can be different for same commitment
|
||
type PrimevConfig struct { | ||
SyncStartBlockNumber uint64 `shconfig:",required"` | ||
SyncMonitorCheckInterval uint64 `shconfig:",required"` |
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 SyncStartBlockNumber
and SyncMonitorCheckInterval
can be moved to the general configuration as not specific to PRIMEV.
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 would be a change affecting all keyper implementations, I suggest we do it in a separate PR
return nil | ||
} | ||
|
||
func (c *PrimevConfig) SetDefaultValues() error { //nolint:unparam |
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.
SetDefaultValues()
and SetExampleValues()
both set empty or zero values for required configuration fields. I think it would be better to either provide meaningful defaults/example values or remove them altogether if not applicable.
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.
We cannot add exact values to these, as they depend on which chain it is running on, testnet/mainnet.
It is added here, just so that an example config file is generated, which would need to be modified to add exact values, like in other keyper implementations
closes https://github.com/shutter-network/shutter-technical-pm/issues/23