Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

fix: latest value cleanups #248

Closed
wants to merge 1 commit into from
Closed

fix: latest value cleanups #248

wants to merge 1 commit into from

Conversation

tynes
Copy link
Collaborator

@tynes tynes commented Mar 4, 2021

Description

Moves the timestamp logic into the same general area and also deduplicates the logic around setting the index/queue index

Metadata

Fixes

  • Fixes # [Link to Issue]

Contributing Agreement

Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked some clarifying questions.

@@ -313,7 +313,6 @@ func (s *SyncService) VerifierLoop() {
if err != nil {
log.Error("Cannot apply transaction", "msg", err)
}
s.SetLatestIndex(&i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed because it now occurs within applyTransaction which would be called by maybeApplyTransaction. Is that correct?

@@ -604,6 +586,25 @@ func (s *SyncService) maybeApplyTransaction(tx *types.Transaction) error {
// Lower level API used to apply a transaction, must only be used with
// transactions that came from L1.
func (s *SyncService) applyTransaction(tx *types.Transaction) error {
meta := tx.GetMeta()
if tx.QueueOrigin().Uint64() == uint64(types.QueueOriginL1ToL2) {
s.SetLatestEnqueueIndex(meta.QueueIndex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a situation in which meta.QueueIndex could be greater than the previous enqueue index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean less than? It should not
See the note below the Types section here: https://github.com/ethereum-optimism/specs/blob/main/transaction-ingestor.md#types

}
// Queue origin sequencer transactions should not have an index here
if meta.Index != nil {
s.SetLatestIndex(meta.Index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above for this SetLatestIndex and the one below.

@@ -630,5 +631,7 @@ func (s *SyncService) ApplyTransaction(tx *types.Transaction) error {
if err != nil {
return fmt.Errorf("invalid transaction: %w", err)
}
tx.SetL1Timestamp(s.GetLatestL1Timestamp())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this ever be a race condition?

@tynes
Copy link
Collaborator Author

tynes commented Mar 5, 2021

Really what we want to do is implement the spec seen here: https://github.com/ethereum-optimism/specs/blob/main/transaction-ingestor.md

@tynes
Copy link
Collaborator Author

tynes commented Mar 27, 2021

Closing in favor of #271

@tynes tynes closed this Mar 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants