Generalize expiry based de-duplication #1782
Conversation
tsachiherman
requested review from
aaronbuchwald and
joshua-kim
as code owners
chain/accepter.go
Outdated
| @@ -7,17 +7,18 @@ import ( | |||
| "context" | |||
|
|
|||
| "github.com/ava-labs/avalanchego/trace" | |||
| "github.com/ava-labs/hypersdk/internal/validity_window" | |||
There was a problem hiding this comment.
nit: can we make this validitywindow and avoid underscores?
chain/processor.go
Outdated
| @@ -41,13 +42,13 @@ func NewExecutionBlock(block *StatelessBlock) *ExecutionBlock { | |||
| } | |||
| } | |||
|
|
|||
| func (b *ExecutionBlock) initTxs() error { | |||
| if b.txsSet.Len() == len(b.Txs) { | |||
| func (b *ExecutionBlock) InitTxs() error { | |||
There was a problem hiding this comment.
Can we get rid of this extra call by loading the txs set on parse, so that we don't need to push this into the internal validity window package?
|
|
||
| var ErrDuplicateTx = errors.New("duplicate transaction") | ||
|
|
||
| type ExecutionBlock[TxnTypePtr emap.Item] interface { |
There was a problem hiding this comment.
nit: Container instead of TxnTypePtr
There was a problem hiding this comment.
Same comment to the other function names that refer to Tx. We should also use Tx rather than txn as shorthand for txs since we've used that nomenclature throughout the repo.
| Timestamp() int64 | ||
| Height() uint64 | ||
| Txs() []TxnTypePtr | ||
| InitTxs() error |
There was a problem hiding this comment.
Can we remove the need for InitTxs ?
chain/syncer.go
Outdated
| chainIndex validity_window.ChainIndex[*Transaction] | ||
| timeValidityWindow validity_window.TimeValidityWindow[*Transaction] |
There was a problem hiding this comment.
Can we move the syncer into the validity window package as well since it is required to find the point where we can safely start to apply de-duplication?
chain/transaction.go
Outdated
| // create a local name for the imported interface. | ||
| type ValidityWindow validitywindow.TimeValidityWindow[*Transaction] |
There was a problem hiding this comment.
Can we move the type alias to where it's used?
| "github.com/ava-labs/hypersdk/internal/emap" | ||
| ) | ||
|
|
||
| type GetValidityWindowFunc func(int64) int64 |
There was a problem hiding this comment.
What does this function do?
There was a problem hiding this comment.
comment updated.
…/hypersdk into tsachi/refactor_validity_window
internal/validitywindow/syncer.go
Outdated
| seenValidityWindow = lastAcceptedBlock.Timestamp()-parent.Timestamp() > s.getValidityWindow(lastAcceptedBlock.Timestamp()) | ||
| if seenValidityWindow { | ||
| break | ||
| } |
There was a problem hiding this comment.
Can we calculate the validity window for the accepted block once rather than in each iteration of this loop to improve readability?
| "github.com/ava-labs/hypersdk/internal/emap" | ||
| ) | ||
|
|
||
| var ErrDuplicateTx = errors.New("duplicate transaction") |
There was a problem hiding this comment.
nit: rename to ErrDuplicateContainer and update error string to "duplicate container"
| v.lock.Lock() | ||
| defer v.lock.Unlock() | ||
|
|
||
| lastAcceptedBlkHeight := v.chainIndex.LastAcceptedBlockHeight() |
There was a problem hiding this comment.
(not needed for this PR because it's an existing bug) note: we should move tracking the last accepted block internal to the validity window (#1780)
I introduced a subtle bug here where we need to trace back not to the last accepted block, but up to the last block where v.seen has been updated. This is much simpler if we track the last accepted block by updating it and v.seen during Accept.
|
|
||
| type ChainIndex[Container emap.Item] interface { | ||
| GetExecutionBlock(ctx context.Context, blkID ids.ID) (ExecutionBlock[Container], error) | ||
| LastAcceptedBlockHeight() uint64 |
There was a problem hiding this comment.
we should be able to remove this in favor of tracking the last accepted height from within the validity window
chain/stateless_block.go
Outdated
|
|
||
| // authCounts can be used by batch signature verification | ||
| // to preallocate memory | ||
| authCounts map[uint8]int | ||
| txsSet set.Set[ids.ID] |
There was a problem hiding this comment.
Could we keep this in ExecutionBlock so that we only populate it as needed (ie for blocks that we may verify)?
If we put it here, then every type that uses this type will have to populate these fields.
What ?