Added retry logic to minaTransactionSender#360
Conversation
|
So what I just discovered: When we setFee(), o1js erases all previous signatures in order for the tx to be signed again. And this makes sense - for the feepayer and some accountupdates, o1js uses the so-called "full commitment". This is simply just the hash of the whole transaction, including all accountupdates and the feepayer, whereas the non-full commitment would just the be hash of the current accountupdate. |
rpanic
left a comment
There was a problem hiding this comment.
This is great! I have a few minor requests and questions, but I like it generally
| messages IncomingMessageBatchTransaction[] | ||
| } | ||
|
|
||
| model PendingL1Transaction { |
There was a problem hiding this comment.
You forgot to create a migration for this new schema change - do this by cd-ing into the persistence pkg, then run prisma migrate dev and it'll you for a name and migrate
| const currentFee = tx.transaction.feePayer.body.fee; | ||
| const newFee = UInt64.from(this.bumpFee(Number(currentFee.toBigInt()))); | ||
| tx.setFee(newFee); | ||
| // Delay if needed |
There was a problem hiding this comment.
Can you explain what this delay here accomplishes? I'd expect this function to only craft the new transaction and return it...
There was a problem hiding this comment.
This is used to add a delay between retries, like retrying a transaction every 5 minutes.
packages/sequencer/src/storage/repositories/PendingL1TransactionStorage.ts
Outdated
Show resolved
Hide resolved
| lastError String? | ||
| sentAt DateTime? | ||
|
|
||
| @@id([sender, nonce]) |
There was a problem hiding this comment.
I'm not against this schema, but I think for the reorg support we'll have to change this - but that will happen when we tackle that piece of work.
| lastError String? | ||
| sentAt DateTime? | ||
|
|
||
| @@id([sender, nonce]) |
There was a problem hiding this comment.
What I'd love to see would be if we could add foreign-key out of settlement here. So in the Settlement entity, add a reference to the PendingTransaction so that we can establish that link
packages/persistance/src/services/prisma/PrismaPendingL1TransactionStorage.ts
Show resolved
Hide resolved
| const tx = Transaction.fromJSON(JSON.parse(record.transactionJson)); | ||
| const currentFee = tx.transaction.feePayer.body.fee; | ||
| const newFee = UInt64.from(this.bumpFee(Number(currentFee.toBigInt()))); | ||
| tx.setFee(newFee); |
There was a problem hiding this comment.
As mentioned in the comment earlier, we need to figure something out to prevent re-proving
packages/sequencer/src/settlement/transactions/DefaultL1TransactionRetryStrategy.ts
Show resolved
Hide resolved
packages/sequencer/src/settlement/transactions/MinaTransactionSender.ts
Outdated
Show resolved
Hide resolved
87ebda9 to
2fe0464
Compare
…endingL1TransactionStorage
…nId instead of transactionHash, PendingL1TransactionStorage to use id as the primary identifier.
- polling the chain and updating persistant storage is now handled by L1TransactionDispatcher - TxStatusWaiter is used for waiting on a txn to get sent, included.
087636c to
0ceb431
Compare
c88b741 to
70d884d
Compare
68399ea to
eb8c39c
Compare
5fcb3ed to
ccc3fdd
Compare
| useClass: TxStatusWaiter, | ||
| }, | ||
|
|
||
| L1TransactionDispatcherConfig: { |
There was a problem hiding this comment.
So - the pattern that we should use here is by using useFactory instead of useValue and then returning the Dispatcher instance - so inside the factory, resolve the dispatcher from the given dependencycontainer, then setting the config
| @inject("SettlementSigner") private readonly signer: MinaSigner, | ||
| @inject("FeeStrategy") private readonly feeStrategy: FeeStrategy, | ||
| private readonly dispatcher: L1TransactionDispatcher, | ||
| @inject("TxStatusWaiter") private readonly waiter: TxStatusWaiter |
There was a problem hiding this comment.
If I see it correctly, the waiter is only used inside the MinaTransactionSender and is not supposed to be overrideable - therefore I'd suggest resolving by class reference, not by token here
| private readonly dispatcher: L1TransactionDispatcher, | ||
| @inject("TxStatusWaiter") private readonly waiter: TxStatusWaiter | ||
| ) { | ||
| this.dispatcher.start(); |
There was a problem hiding this comment.
We can use @startable() here (like closeable) and then start the dispatcher in there
| this.pollingTimeout.unref?.(); | ||
| } | ||
| }; | ||
| this.pollingTimeout = setTimeout(poll, this.config.pollIntervalMs); |
There was a problem hiding this comment.
I didn't know about unref - that's cool!
Why is this not a interval? Because of the unref or other reasons?
There was a problem hiding this comment.
if a poll() call takes more than pollIntervalMs time, 2 calls can overlap
| * Optional retry delay. If provided, the dispatcher will schedule | ||
| * the next retry after the delay from the last sentAt time. | ||
| */ | ||
| getRetryDelayMs?(record: PendingL1TransactionRecord): number; |
There was a problem hiding this comment.
What happens if the user doesn't specify that function? When will we retry?
There was a problem hiding this comment.
the retry will happen after inclusionTimeoutMs (default: 10 mins).
this delay is after checking for status ends.
| attempts: number; | ||
| status: PendingL1TransactionStatus; | ||
| transaction: Transaction<any, any>; | ||
| hash?: string; |
There was a problem hiding this comment.
https://github.com/o1-labs/o1js/blob/main/CHANGELOG.md#added
The PR that we requested from florian is now merged, we should use a nightly version of o1js for now until this is released but already now integrate it into this PR and use the hash everywhere
removed hash field from PendingL1Transaction model
closes #340