-
Notifications
You must be signed in to change notification settings - Fork 17
Event based triggers #627
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?
Event based triggers #627
Conversation
- Implement generic MultiEventSyncer - Implement TriggerRegisteredEvent syncing and trigger syncing with MultiEventSyncer - Filter triggers based on TTL and log matching - Remove previous TriggerRegisteredEvent syncer (which replaced IdentityRegisteredEvent syncer)
Previous commits temporarily changed it to sync EventTriggerRegisteredEvents. This restores it to the original state.
This splits the triggering up into trigger generation and sending.
Instead, we use just the hash directly. This simplifies the code and means no information is lost during marshalling (even before, we only marshalled the hash).
It used to be [][]byte (which just got concatenated during unmarshaling), now it's just []byte. Presumably, the original idea was to have blocks of 32 bytes to make it easier for the EVM to handle, but ABI encoding already takes care of this for us.
The code did not match the comment and umarshaling logic.
Topics should not be specified (not even if they're empty) if they don't exist in the event signature, otherwise the filter doesn't match.
This aligns the config parameter with the contract name. This makes automatically setting the config value to the deployment address easier.
When looking for triggers in a certain block range, the trigger registration event does not have to be from this range. Rather, it may have been registered a long time prior to this, as long as it hasn't expired yet.
The event needs to have access to the trigger register event, otherwise it doesn't know the identity prefix etc.
Similar to how it is handled with time based triggers, trigger events are now marked as "decrypted" when a corresponding decryption key has been produced. To do that, we store the full identity in the database even though it could be derived from prefix and sender. In the middleware, we cannot distinguish between keys that have been generated due to a time based or an event based trigger, so we just look in both tables if we can find the corresponding identity.
This simplifies the data structures and avoids interfaces. It allows using RLP for encoding. The commit also adds proper validation of the data structures. The functionality is broadly equivalent.
In practice, the contract will probably apply reasonable bounds, but it doesn't hurt to check it here too.
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.
Pull Request Overview
This PR implements event-based triggers for the shutter service, enabling triggers to fire based on specific Ethereum events rather than just time-based triggers. The implementation includes event syncing infrastructure, trigger processing logic, and database schema updates to support event-based trigger registration and execution.
Key changes:
- Added
EventTriggerDefinition
system for defining complex event-based triggers with predicates on log topics and data - Implemented
MultiEventSyncer
for synchronizing multiple types of blockchain events with reorg handling - Added database support for event trigger registration and fired trigger tracking
Reviewed Changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
eventtrigger.go | Core event trigger definition and validation logic with RLP encoding/decoding |
multieventsyncer.go | Generic event synchronization framework with reorg detection and rollback capabilities |
triggerprocessor.go | Processes trigger events to check if registered triggers should fire |
eventtriggerregisteredprocessor.go | Handles EventTriggerRegistered events from the trigger registry contract |
newblock.go | Integrates event-based triggers into the main block processing flow |
database schema/queries | Database support for event trigger storage and tracking |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
return nil | ||
select { | ||
case kpr.decryptionTriggerChannel <- event: |
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 channel send operation could block if the channel is full, but there's no timeout or non-blocking alternative. Consider adding a timeout or making this non-blocking to prevent the goroutine from hanging.
Copilot uses AI. Check for mistakes.
} | ||
|
||
err = s.setSyncStatus(ctx, tx, toBlock, []byte{}) | ||
if err != nil { |
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.
Setting block hash to empty bytes during rollback could cause issues. Consider setting it to the actual block hash at the rollback target block for consistency.
if err != nil { | |
// Fetch the block hash for the rollback target block | |
blockHeader, err := s.EthClient.BlockByNumber(ctx, big.NewInt(toBlock)) | |
if err != nil { | |
return errors.Wrapf(err, "failed to fetch block header for block %d during rollback", toBlock) | |
} | |
blockHash := blockHeader.Hash().Bytes() | |
err = s.setSyncStatus(ctx, tx, toBlock, blockHash) | |
if err != nil { |
Copilot uses AI. Check for mistakes.
026b6a8
to
a7c41bc
Compare
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.
looks good overall, just some small changes requested
rolling-shutter/keyperimpl/shutterservice/database/sql/queries/shutterservice.sql
Show resolved
Hide resolved
rolling-shutter/keyperimpl/shutterservice/database/sql/schemas/shutterservice.sql
Outdated
Show resolved
Hide resolved
type ValuePredicate struct { | ||
Op Op | ||
IntArgs []*big.Int | ||
ByteArgs [][]byte |
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.
maybe something for next version or future improvements, we can just use BytesArgs for all data types, after encoding them.
FromBlock: nil, | ||
ToBlock: nil, | ||
Addresses: []common.Address{d.Contract}, | ||
Topics: topics, |
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.
will this work correctly, if there are multiple LogPredicates and one of which is topic and others are not? It could just fetch the topic then and not other events (non topic ones)?
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 it should work correctly. LogPredicates are supposed to be "anded" together, so we want to fetch events that fulfill all predicates. The filter will return all events that match all topic predicates. The Match
afterwards (in newblock.go) will filter it down further using the remaining predicates.
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 I meant to say is, if one of the LogPredicate is for a different event which contains topic and other LogPredicate is of different event which does not have topic specified, can it fetch the second event as well, which we will match afterwards?
continue | ||
} | ||
|
||
filterQuery, err := trigger.ToFilterQuery() |
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.
one future improvement here could be combining these filter queries to do a single query to the rpc? wdyt?
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.
Agree, but at this stage I think it would be premature optimization.
Depending on the contract design, identity prefix and sender might not be unique, but with the eon they should be.
|
||
# share genesis | ||
if [ $num -eq 0 ]; then | ||
for destination in data/chain-seed/config/ data/chain-{1..2}-validator/config/; do |
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 know, this is a copy and past from elsewhere, but I think this could just be:
for destination in data/chain-seed/config/ data/chain-{1..2}-validator/config/; do | |
for destination in data/chain-seed/config/ data/chain-${num}-validator/config/; do |
or it could be pulled out of the for num in {0..2}; do
-loop and just be
for destination in data/chain-seed/config/ data/chain-{1..2}-validator/config/; do
${BB} cp -v data/chain-0-validator/config/genesis.json "${destination}"
done
This PR implements event based triggers. It is based on the work done in #613. It is mostly complete, but still a draft because
ShutterTriggerRegistryContract
in the contracts repository to be finalized, merged and added as a dependency to go.mod (currently, go.mod references this early version in theevent-trigger-registry
branch)For reviewing I suggest largely ignoring the commit history (the commits should be squashed before merging). The core logic is implemented in eventtrigger.go. multieventsyncer.go contains a generalized event syncing interface that is implemented once for trigger registration events and once for the actual trigger events. Everything is tied together in newblock.go.