-
Notifications
You must be signed in to change notification settings - Fork 15
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
Receipt listener event streams #515
Conversation
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
…tself as the unblock Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
…icker Signed-off-by: Peter Broadhurst <[email protected]>
…make eth_subscribe a sample Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
…rder of numbers with commits Signed-off-by: Peter Broadhurst <[email protected]>
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.
leaving few comments here mid review while I time out for a couple of hours. Will hopefully get to end of review later today.
@@ -164,7 +164,7 @@ func NewUnitTestPersistence(ctx context.Context, suite string) (p Persistence, c | |||
DSN: dbDSN(utDBName), | |||
MigrationsDir: migrationsDirRelative, | |||
AutoMigrate: &autoMigrate, | |||
DebugQueries: false, | |||
DebugQueries: true, |
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.
did you mean to leave this in? Not sure of the tradeoff here? Does it significantly increase test run time or log output on the CI actions?
} | ||
} | ||
|
||
// We use a post-commit handler to send back any acks to the other side that are required | ||
return cleanup, make([]flushwriter.Result[*noResult], len(values)), nil | ||
return postCommit, make([]flushwriter.Result[*noResult], len(values)), 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.
Not strictly a point about this PR but reviewing this PR has given me an opportunity to reflect on this. I am not sure how I feel about how we return the postCommit function from each function in the stack. It is coupled to the fact that we are passing a DB transaction down the stack so another approach would have been to pass a wrapper object that contains the gorm.DB
and also includes a method to register post commit handlers.
The way we have it is more explicit which is good but it does force PRs like this touch a ton of files that have no behavior change and causes a lot more "noop" code scattered around.
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.
Yep - been thinking a lot about this, and overall GORM's plusses and minuses.
The lack of tx.AddPostCommit()
is very frustrating.
But fixing it is likely a day of refactor, and I think it was right not to couple it to this PR
FailureMessage *string `gorm:"column:failure_message"` | ||
RevertData tktypes.HexBytes `gorm:"column:revert_data"` | ||
ContractAddress *tktypes.EthAddress `gorm:"column:contract_address"` | ||
Block *persistedReceiptGap `gorm:"foreignKey:Source;references:Source;"` |
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 a compound foreignKey? Is there a chance that there are 2 rows in receipt_listener_gap
with the same Source
but different Listener
? Not sure how the gorm magic works in that case.
Not sure I understand what this field is for so I may be misreading the intent here. At first I thought it was something to do with BlockNumber
but I think now that it is saying whether this receipt should be blocked from being delivered because of a known gap?
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.
Think I understand this now. I see we are adding the listener name to the where clause when we construct the join SQL.
So the only takeaway here is that the name Block
could be confused with blockchain block but I think here it is actually a Gap
or Blocker
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.
Darn, yes. I did the rename to Gap
everywhere else, but not on this Go struct. Will submit this in a FUP PR.
} | ||
if mh.methodType == rpcMethodTypeAsyncStart { | ||
rpcRes = wsc.handleNewAsync(ctx, rpcReq, mh.async) | ||
} else { |
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 we be explicit here (else if mh.methodType == rpcMethodTypeAsyncLifecycle
) just in case a new enum is added in future?
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 did consider this, and just felt adding the extra errors and test cases wasn't justified. But happy to be pushed to do that as a follow-up (as this merged without that pushback)
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 with your assessment. This is one of the things that annoys me about 100% code coverage. If you had done this, the coverage tool would whinge at you but the behavior wouldn't be any less covered than it is now.
if spec.Filters.Domain != "" { | ||
return nil, i18n.NewError(ctx, msgs.MsgTxMgrBadReceiptListenerTypeDomain, spec.Name, spec.Filters.Type, spec.Filters.Domain) | ||
} | ||
q = q.Where("domain = ''") // private |
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.
q = q.Where("domain = ''") // private | |
q = q.Where("domain = ''") // public |
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 add this to the Gap
rename PR.
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Adds receipt listeners for streaming completed transactions from a Paladin node.
Architecture
A
sequence
column is added to thetransaction_receipts
table, to provide a reference point for checkpoints in the receipt listeners to determine which receipts have/haven't been processed.Then three new tables are introduced to manage the listeners:
receipt_listeners
- the definitions of the listeners, each with a uniquename
receipt_listener_gap
- a blocking record when an individual smart contract is missing statesreceipt_listener_checkpoints
- the recovery point processing should restart from for a listener, to detect new receiptsEach started listener has a go routine that queries the database to detect new receipts to deliver.
Where possible, DB filters are used to pre-filter receipts that do not match the filters of the listener. As filters become more complex, some might only be filterable through post-filtering - that is already supported in the code (used in practice today to avoid waking listeners with a should-tap to poll when new receipts are written to the DB that do not match).
For each receipt the outcome might be:
gap
in the DB that must be resolved before receipts can be delivered, for this particular private smart contractGaps are created for
"incompleteStateReceiptBehavior": "block_contract"
listeners, when the states for a particular smart contract have not been received yet.This is very important for indexing cases where full visibility of a transaction data is required to process the receipt, and those transactions must be processed in order.
EVM Privacy Groups (Pente) are the primary example of this, where each receipt represents a transaction on the blockchain. Every transaction must be processed by every party maintaining the privacy group in the same order.
JSON/RPC details
Create receipt listener
Note the ability to filter on particular receipt types, and most importantly the ability to block delivery on a given contract address while a "gap" exists in the available states.
Subscribe (WebSockets only)
Ack
Confirms receipt, so the next batch is delivered.
Nack
Drives redelivery/retry for this batch
Unsubscribe
Delete receipt listener