diff --git a/internal/validitywindow/validitywindow.go b/internal/validitywindow/validitywindow.go index 5d8d10d418..0393df9724 100644 --- a/internal/validitywindow/validitywindow.go +++ b/internal/validitywindow/validitywindow.go @@ -9,6 +9,7 @@ import ( "fmt" "sync" + "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/trace" "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/set" @@ -69,6 +70,15 @@ func (v *TimeValidityWindow[Container]) VerifyExpiryReplayProtection( if dup.Len() > 0 { return fmt.Errorf("%w: duplicate in ancestry", ErrDuplicateContainer) } + // make sure we have no repeats within the block itself. + blkTxsIDs := make(map[ids.ID]bool, len(blk.Txs())) + for _, tx := range blk.Txs() { + id := tx.GetID() + if _, has := blkTxsIDs[id]; has { + return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) + } + blkTxsIDs[id] = true + } return nil } diff --git a/x/dsmr/node.go b/x/dsmr/node.go index ff19917b31..b26631a0e7 100644 --- a/x/dsmr/node.go +++ b/x/dsmr/node.go @@ -34,8 +34,6 @@ var ( ErrTimestampNotMonotonicallyIncreasing = errors.New("block timestamp must be greater than parent timestamp") ) -const validityWindowDuration = int64(5) - type ( ChainIndex = validitywindow.ChainIndex[*ChunkCertificate] timeValidityWindow = *validitywindow.TimeValidityWindow[*ChunkCertificate] @@ -60,6 +58,7 @@ func New[T Tx]( log logging.Logger, tracer trace.Tracer, chainIndex ChainIndex, + validityWindowDuration int64, ) (*Node[T], error) { storage, err := newChunkStorage[T](NoVerifier[T]{}, memdb.New()) if err != nil { @@ -93,10 +92,11 @@ func New[T Tx]( ChunkCertificateGossipHandler: &ChunkCertificateGossipHandler[T]{ storage: storage, }, - storage: storage, - log: log, - tracer: tracer, - validityWindow: validitywindow.NewTimeValidityWindow(log, tracer, chainIndex), + storage: storage, + log: log, + tracer: tracer, + validityWindow: validitywindow.NewTimeValidityWindow(log, tracer, chainIndex), + validityWindowDuration: validityWindowDuration, }, nil } @@ -119,6 +119,7 @@ type ( storage *chunkStorage[T] log logging.Logger tracer trace.Tracer + validityWindowDuration int64 } ) @@ -200,7 +201,7 @@ func (n *Node[T]) BuildBlock(ctx context.Context, parent Block, timestamp int64) } chunkCerts := n.storage.GatherChunkCerts() - oldestAllowed := timestamp - validityWindowDuration + oldestAllowed := timestamp - n.validityWindowDuration if oldestAllowed < 0 { oldestAllowed = 0 } @@ -245,7 +246,12 @@ func (n *Node[T]) Execute(ctx context.Context, parentBlock Block, block Block) e // TODO: Verify header fields // Find repeats - if err := n.validityWindow.VerifyExpiryReplayProtection(ctx, block, parentBlock.Tmstmp); err != nil { + + oldestAllowed := block.Timestamp() - n.validityWindowDuration + if oldestAllowed < 0 { + oldestAllowed = 0 + } + if err := n.validityWindow.VerifyExpiryReplayProtection(ctx, block, oldestAllowed); err != nil { return err } diff --git a/x/dsmr/node_test.go b/x/dsmr/node_test.go index ebc311770a..e0d3532809 100644 --- a/x/dsmr/node_test.go +++ b/x/dsmr/node_test.go @@ -32,6 +32,8 @@ var ( _ Verifier[tx] = (*failVerifier)(nil) ) +const testingDefaultValidityWindowDuration = int64(5) + type testingChainIndex struct { blocks map[ids.ID]validitywindow.ExecutionBlock[*ChunkCertificate] } @@ -143,6 +145,7 @@ func TestNode_BuildChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -210,6 +213,7 @@ func TestNode_GetChunk_AvailableChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -304,6 +308,7 @@ func TestNode_GetChunk_PendingChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -384,6 +389,7 @@ func TestNode_GetChunk_UnknownChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -600,6 +606,7 @@ func TestNode_BuiltChunksAvailableOverGetChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -736,6 +743,7 @@ func TestNode_GetChunkSignature_SignValidChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -787,6 +795,7 @@ func TestNode_GetChunkSignature_SignValidChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) chunk, err := node2.BuildChunk( @@ -908,6 +917,7 @@ func TestNode_GetChunkSignature_DuplicateChunk(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1005,6 +1015,7 @@ func TestGetChunkSignature_PersistAttestedBlocks(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1044,6 +1055,7 @@ func TestGetChunkSignature_PersistAttestedBlocks(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1348,6 +1360,7 @@ func TestNode_NewBlock_IncludesChunkCerts(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1437,6 +1450,7 @@ func TestDuplicateChunksElimination(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1543,12 +1557,13 @@ func TestNode_Execute_Chunks(t *testing.T) { return chunks } testCases := []struct { - name string - parentBlocks [][]int // for each parent, a list of the chunks included. - chunks []int - timestamp int64 - executeWantErr error - buildWantErr error + name string + parentBlocks [][]int // for each parent, a list of the chunks included. + chunks []int + timestamp int64 + executeWantErr error + buildWantErr error + validalidityWindowDuration int64 }{ { name: "three empty blocks", @@ -1566,22 +1581,43 @@ func TestNode_Execute_Chunks(t *testing.T) { }, { name: "two blocks one duplicate chunk", - parentBlocks: [][]int{{0, 1}, {1, 2}}, - chunks: []int{}, - timestamp: 4, + parentBlocks: [][]int{{0, 1}}, + chunks: []int{1, 2}, + timestamp: 2, executeWantErr: validitywindow.ErrDuplicateContainer, + buildWantErr: nil, // build would filter out duplicate chunks, hence no error. }, { name: "one block duplicate chunks", - parentBlocks: [][]int{{1, 1}}, - chunks: []int{}, - timestamp: 4, + parentBlocks: [][]int{{}}, + chunks: []int{1, 1}, + timestamp: 2, executeWantErr: validitywindow.ErrDuplicateContainer, + buildWantErr: nil, // build would filter out duplicate chunks, hence no error. + }, + { + name: "three blocks non consecutive duplicate chunks", + parentBlocks: [][]int{{1}, {2}}, + chunks: []int{1}, + timestamp: 3, + executeWantErr: validitywindow.ErrDuplicateContainer, + }, + { + name: "three blocks non consecutive duplicate chunks outside validity window", + parentBlocks: [][]int{{1}, {2}}, + chunks: []int{1}, + timestamp: 3, + executeWantErr: nil, + validalidityWindowDuration: 1, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { indexer := newTestingChainIndexer() + valWind := testCase.validalidityWindowDuration + if valWind == 0 { + valWind = testingDefaultValidityWindowDuration + } node, err := New[tx]( ids.EmptyNodeID, networkID, @@ -1614,6 +1650,7 @@ func TestNode_Execute_Chunks(t *testing.T) { logging.NoLog{}, trace.Noop, indexer, + valWind, ) r.NoError(err) @@ -1631,9 +1668,9 @@ func TestNode_Execute_Chunks(t *testing.T) { for _, chunkIndex := range chunkList { blk.ChunkCerts = append(blk.ChunkCerts, makeChunkCert(chunks[chunkIndex])) } - if blockNum > 0 { - r.ErrorIs(node.Execute(context.Background(), parentBlk, blk), testCase.executeWantErr) - } + + r.NoError(node.Execute(context.Background(), parentBlk, blk)) + r.NoError(node.Accept(context.Background(), blk)) indexer.set(blk.GetID(), blk) parentBlk = blk @@ -1642,8 +1679,20 @@ func TestNode_Execute_Chunks(t *testing.T) { for _, chunkIdx := range testCase.chunks { r.NoError(node.storage.AddLocalChunkWithCert(chunks[chunkIdx], makeChunkCert(chunks[chunkIdx]))) } - _, err = node.BuildBlock(context.Background(), parentBlk, testCase.timestamp) + newBlk, err := node.BuildBlock(context.Background(), parentBlk, testCase.timestamp) r.ErrorIs(err, testCase.buildWantErr) + + // create the block so that we can test it against the execute directly. + newBlk = Block{ + ParentID: parentBlk.GetID(), + Hght: uint64(testCase.timestamp), + Tmstmp: int64(testCase.timestamp), + blkID: ids.GenerateTestID(), + } + for _, chunkIndex := range testCase.chunks { + newBlk.ChunkCerts = append(newBlk.ChunkCerts, makeChunkCert(chunks[chunkIndex])) + } + r.ErrorIs(node.Execute(context.Background(), parentBlk, newBlk), testCase.executeWantErr) }) } } @@ -1691,6 +1740,7 @@ func TestAccept_RequestReferencedChunks(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) @@ -1748,6 +1798,7 @@ func TestAccept_RequestReferencedChunks(t *testing.T) { logging.NoLog{}, trace.Noop, newTestingChainIndexer(), + testingDefaultValidityWindowDuration, ) r.NoError(err) r.NoError(node2.Accept(context.Background(), blk))