Followup pre populate chain validity window #2012
Conversation
* Add NewPopulatedTimeValidityWindow for direct initialization * Simplify validity window population with consistent timestamp comparison * Improve VM initialization flow to handle validity window during restart * Unify calculateOldestAllowed function implementation * Update component initialization order to break circular dependencies
|
hypersdk/internal/validitywindow/syncer.go Line 138 in 5d39841 Line 83 in 5d39841 Line 727 in 5d39841 Line 718 in 5d39841 |
…-populate-chain-validity-window
…p safety checks enhance VW implementation with a populated flag that tracks whether a complete validity window has been observed. Reducing API surface area
…uring consistent view of state across the network
| log: log, | ||
| tracer: tracer, | ||
| chainIndex: chainIndex, | ||
| seen: emap.NewEMap[T](), | ||
| getTimeValidityWindow: getTimeValidityWindowF, | ||
| populated: false, | ||
| } | ||
| if lastAcceptedBlock != nil { |
There was a problem hiding this comment.
Allowing nil lastAcceptedBlock for testing purposes. While not ideal for production use, passing nil signals the creation of an empty validity window. The constructor handles this case gracefully. I'm open to improvements.
vw := NewTimeValidityWindow(ctx, log, tracer, chainIndex, nil, ...)
…`startNormalOp` ensuring both paths leading to normal operation require populated validity window
| // It maintains a record of transactions that have been seen within a | ||
| // configurable time window and rejects any duplicates. | ||
| // | ||
| // This component is critical as it: | ||
| // 1. Prevents transaction replay attacks | ||
| // 2. Enforces double-spend protection | ||
| // 3. Provides temporal validation boundaries | ||
| // 4. Maintains consensus safety across nodes i.e.; | ||
| // if different nodes had different rules for transaction uniqueness, | ||
| // they would disagree about the state of the blockchain. |
There was a problem hiding this comment.
I think we could improve this comment by focusing on what it does as opposed to how it's used. For example, enforces double-spend protection is a function of preventing replay attacks, but these are not really the same thing. Provides temporal validation boundaries is a bit vague and doesn't tell me much additional information.
There was a problem hiding this comment.
| // It maintains a record of transactions that have been seen within a | |
| // configurable time window and rejects any duplicates. | |
| // | |
| // This component is critical as it: | |
| // 1. Prevents transaction replay attacks | |
| // 2. Enforces double-spend protection | |
| // 3. Provides temporal validation boundaries | |
| // 4. Maintains consensus safety across nodes i.e.; | |
| // if different nodes had different rules for transaction uniqueness, | |
| // they would disagree about the state of the blockchain. | |
| // Maintains a window of all transactions included onchain, which have an expiry time that would | |
| // allow them to be included a second time. Once their expiry time passes, transactions can be pruned | |
| // from the window, since their expiry will mark them as invalid. |
There was a problem hiding this comment.
I updated the comment
| if head == nil { | ||
| return nil, fmt.Errorf("cannot construct time validity window: %w", ErrNilInitialBlock) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we remove this check in favor of requiring that the caller passes in a fully populated value?
The nil check won't work reliably for generic types and we commonly make this assumption rather than include defensive checks, so I'd lean towards removing it.
There was a problem hiding this comment.
I agree with removing the nil check - it makes sense to follow the common practice of assuming callers pass valid values rather than adding defensive checks.
My original thinking was that having an explicit check with a clear error message would make debugging easier if we ever had an inconsistency. When something goes wrong in a system like this, having specific error messages that point directly to the source of the problem (like explicitly calling out a nil head block) can save time during troubleshooting.
But you're right that for generic types, nil checks aren't reliable, and it's better to be consistent with our approach of requiring callers to pass fully populated values. I'll remove this check.
This PR is closing #2003 by addressing several issues related to the validity window initialization flow.
Improved Validity Window API:
NewPopulatedTimeValidityWindowas a factory method to create and populate a validity window in one steppopulateValidityWindowprivate (formerlyPopulateValidityWindow) to reduce API surface areapopulateValidityWindowCleaner VM Initialization Flow:
Validity Window Initialization