Skip to content

Commit

Permalink
Merge pull request #423 from onflow/improve-log-filtering-tests
Browse files Browse the repository at this point in the history 
`eth_getLogs` does not validate the topics length
  • Loading branch information
@sideninja
sideninja authored Aug 6, 2024
2 parents 2932f71 + 41ece11 commit 45c7454
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 46 deletions.
14 changes: 11 additions & 3 deletions api/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,9 +623,17 @@ func (b *BlockChainAPI) GetLogs(

// if filter provided specific block ID
if criteria.BlockHash != nil {
return logs.
NewIDFilter(*criteria.BlockHash, filter, b.blocks, b.receipts).
Match()
f, err := logs.NewIDFilter(*criteria.BlockHash, filter, b.blocks, b.receipts)
if err != nil {
return handleError[[]*types.Log](err, l, b.collector)
}

res, err := f.Match()
if err != nil {
return handleError[[]*types.Log](err, l, b.collector)
}

return res, nil
}

// otherwise we use the block range as the filter
Expand Down
12 changes: 10 additions & 2 deletions services/logs/filter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func NewRangeFilter(
criteria FilterCriteria,
receipts storage.ReceiptIndexer,
) (*RangeFilter, error) {
if len(criteria.Topics) > maxTopics {
return nil, fmt.Errorf("max topics exceeded, only %d allowed", maxTopics)
}

// check if both start and end don't have special values (negative values representing last block etc.)
// if so, make sure that beginning number is not bigger than end
if start.Cmp(big.NewInt(0)) > 0 && end.Cmp(big.NewInt(0)) > 0 && start.Cmp(&end) > 0 {
Expand Down Expand Up @@ -124,13 +128,17 @@ func NewIDFilter(
criteria FilterCriteria,
blocks storage.BlockIndexer,
receipts storage.ReceiptIndexer,
) *IDFilter {
) (*IDFilter, error) {
if len(criteria.Topics) > maxTopics {
return nil, fmt.Errorf("max topics exceeded, only %d allowed", maxTopics)
}

return &IDFilter{
id: id,
criteria: &criteria,
blocks: blocks,
receipts: receipts,
}
}, nil
}

func (i *IDFilter) Match() ([]*gethTypes.Log, error) {
Expand Down
145 changes: 117 additions & 28 deletions services/logs/filter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/onflow/flow-go/fvm/evm/types"
"github.com/onflow/go-ethereum/common"
gethTypes "github.com/onflow/go-ethereum/core/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -155,10 +156,9 @@ func receiptStorage() storage.ReceiptIndexer {
return receiptStorage
}

// todo check if empty address with only topics provided is a valid query

func TestIDFilter(t *testing.T) {
lgs := receipts[0].Logs
logs := receipts[0].Logs

tests := []struct {
desc string
id common.Hash
Expand All @@ -168,33 +168,72 @@ func TestIDFilter(t *testing.T) {
desc: "wildcard all logs",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0].Address},
Addresses: []common.Address{},
Topics: [][]common.Hash{},
},
expectLogs: lgs[:2], // first two are all logs from the address
expectLogs: logs[:], // block 0 has 3 logs in total
}, {
desc: "single topic no address match single log",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{},
Topics: [][]common.Hash{logs[0].Topics[:1]},
},
expectLogs: logs[:1],
}, {
desc: "single out of order topic no address match no logs",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{},
Topics: [][]common.Hash{logs[0].Topics[1:]},
},
expectLogs: []*gethTypes.Log{},
}, {
desc: "single topic with first position wildcard match single log",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{},
Topics: [][]common.Hash{
{},
{logs[0].Topics[1]},
},
},
expectLogs: logs[:1],
}, {
desc: "single topic with second position wildcard match single log",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{},
Topics: [][]common.Hash{
{logs[0].Topics[0]},
{},
},
},
expectLogs: logs[:1],
}, {
desc: "single topic, single address match single log",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0].Address},
Topics: [][]common.Hash{lgs[0].Topics[:1]},
Addresses: []common.Address{logs[0].Address},
Topics: [][]common.Hash{logs[0].Topics[:1]},
},
expectLogs: lgs[:1],
expectLogs: logs[:1],
}, {
desc: "single address no topic match two logs",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0].Address},
Addresses: []common.Address{logs[0].Address},
Topics: [][]common.Hash{},
},
expectLogs: lgs[:2],
expectLogs: logs[:2],
}, {
desc: "single address, both topics match single log",
id: mustHash(blocks[0]),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0].Address},
Topics: [][]common.Hash{lgs[0].Topics},
Addresses: []common.Address{logs[0].Address},
Topics: [][]common.Hash{logs[0].Topics},
},
expectLogs: lgs[:1],
expectLogs: logs[:1],
}, {
desc: "invalid topic match no logs",
id: mustHash(blocks[0]),
Expand All @@ -206,17 +245,43 @@ func TestIDFilter(t *testing.T) {

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
filter := NewIDFilter(tt.id, tt.criteria, blockStorage(), receiptStorage())
logs, err := filter.Match()
filter, err := NewIDFilter(tt.id, tt.criteria, blockStorage(), receiptStorage())
require.NoError(t, err)

matchedLogs, err := filter.Match()

require.NoError(t, err)
require.Equal(t, tt.expectLogs, logs)
require.Equal(t, tt.expectLogs, matchedLogs)
})
}

t.Run("with topics count exceeding limit", func(t *testing.T) {
_, err := NewIDFilter(
common.HexToHash("123"),
FilterCriteria{
Topics: [][]common.Hash{
{common.HexToHash("123")},
{common.HexToHash("456")},
{common.HexToHash("789")},
{common.HexToHash("101")},
{common.HexToHash("120")},
},
},
blockStorage(),
receiptStorage(),
)

require.Error(t, err)
assert.ErrorContains(
t,
err,
"max topics exceeded, only 4 allowed",
)
})
}

func TestRangeFilter(t *testing.T) {
lgs := [][]*gethTypes.Log{receipts[0].Logs, receipts[1].Logs, receipts[2].Logs, receipts[3].Logs, receipts[4].Logs}
logs := [][]*gethTypes.Log{receipts[0].Logs, receipts[1].Logs, receipts[2].Logs, receipts[3].Logs, receipts[4].Logs}

tests := []struct {
desc string
Expand All @@ -228,27 +293,27 @@ func TestRangeFilter(t *testing.T) {
start: big.NewInt(0),
end: big.NewInt(1),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0][0].Address},
Topics: [][]common.Hash{lgs[0][0].Topics[:1]},
Addresses: []common.Address{logs[0][0].Address},
Topics: [][]common.Hash{logs[0][0].Topics[:1]},
},
expectLogs: lgs[0][:1],
expectLogs: logs[0][:1],
}, {
desc: "single topic, single address, all blocks match multiple logs",
start: big.NewInt(0),
end: big.NewInt(4),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0][0].Address},
Topics: [][]common.Hash{lgs[0][0].Topics[:1]},
Addresses: []common.Address{logs[0][0].Address},
Topics: [][]common.Hash{logs[0][0].Topics[:1]},
},
expectLogs: []*gethTypes.Log{lgs[0][0], lgs[3][1]},
expectLogs: []*gethTypes.Log{logs[0][0], logs[3][1]},
}, {
desc: "single address, all blocks match multiple logs",
start: big.NewInt(0),
end: big.NewInt(4),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0][0].Address},
Addresses: []common.Address{logs[0][0].Address},
},
expectLogs: []*gethTypes.Log{lgs[0][0], lgs[0][1], lgs[1][0], lgs[3][1]},
expectLogs: []*gethTypes.Log{logs[0][0], logs[0][1], logs[1][0], logs[3][1]},
}, {
desc: "invalid address, all blocks no match",
start: big.NewInt(0),
Expand All @@ -262,7 +327,7 @@ func TestRangeFilter(t *testing.T) {
start: big.NewInt(5),
end: big.NewInt(10),
criteria: FilterCriteria{
Addresses: []common.Address{lgs[0][0].Address},
Addresses: []common.Address{logs[0][0].Address},
},
expectLogs: []*gethTypes.Log{},
}}
Expand All @@ -272,12 +337,36 @@ func TestRangeFilter(t *testing.T) {
filter, err := NewRangeFilter(*tt.start, *tt.end, tt.criteria, receiptStorage())
require.NoError(t, err)

logs, err := filter.Match()
matchedLogs, err := filter.Match()

require.NoError(t, err)
require.Equal(t, tt.expectLogs, logs)
require.Equal(t, tt.expectLogs, matchedLogs)
})
}

t.Run("with topics count exceeding limit", func(t *testing.T) {
_, err := NewRangeFilter(
*big.NewInt(0),
*big.NewInt(4),
FilterCriteria{
Topics: [][]common.Hash{
{common.HexToHash("123")},
{common.HexToHash("456")},
{common.HexToHash("789")},
{common.HexToHash("101")},
{common.HexToHash("120")},
},
},
receiptStorage(),
)

require.Error(t, err)
assert.ErrorContains(
t,
err,
"max topics exceeded, only 4 allowed",
)
})
}

func TestStreamFilter(t *testing.T) {
Expand Down
23 changes: 10 additions & 13 deletions tests/web3js/eth_logs_filtering_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ const conf = require('./config')
const helpers = require('./helpers')
const web3 = conf.web3

it('emit logs and retrieve them using different filters', async() => {
setTimeout(() => process.exit(1), 19*1000) // hack if the ws connection is not closed
it('emit logs and retrieve them using different filters', async () => {
setTimeout(() => process.exit(1), 19 * 1000) // hack if the ws connection is not closed

let deployed = await helpers.deployContract("storage")
let contractAddress = deployed.receipt.contractAddress
Expand All @@ -28,30 +28,27 @@ it('emit logs and retrieve them using different filters', async() => {
})
assert.equal(res.receipt.status, conf.successStatus)

let latest = await web3.eth.getBlockNumber()

// filter each event just emitted by both A and B matching the exact event
const events = await deployed.contract.getPastEvents('Calculated', {
filter: { numA: A, numB: B },
fromBlock: conf.startBlockHeight,
toBlock: latest, // todo 'latest' special value doesn't seem to work
toBlock: 'latest'
})

// Assert that the event is found and the result is correct
assert.equal(events.length, 1)
assert.equal(events[0].returnValues.sum, (A + B).toString())
}

let latest = await web3.eth.getBlockNumber()

// filter events by A value equal to 10 which should equal to 3 events with different B values
let events = await deployed.contract.getPastEvents('Calculated', {
filter: { numA: repeatA },
fromBlock: conf.startBlockHeight,
toBlock: latest,
fromBlock: 'earliest',
toBlock: 'latest',
})

assert.lengthOf(events, 3)

// this filters the test values by A = 10 and makes sure the response logs are expected
testValues
.filter(v => v.A == repeatA)
Expand All @@ -64,8 +61,8 @@ it('emit logs and retrieve them using different filters', async() => {

// make sure all events are returned
events = await deployed.contract.getPastEvents({
fromBlock: conf.startBlockHeight,
toBlock: latest,
fromBlock: 'earliest',
toBlock: 'latest',
})
assert.lengthOf(events, testValues.length)

Expand All @@ -79,8 +76,8 @@ it('emit logs and retrieve them using different filters', async() => {
// filter by value A being 1 or -1
events = await deployed.contract.getPastEvents('Calculated', {
filter: { numA: [-1, 1] },
fromBlock: conf.startBlockHeight,
toBlock: latest,
fromBlock: 'earliest',
toBlock: 'latest',
})
assert.lengthOf(events, 2)
assert.equal(events[0].returnValues.numB, 2)
Expand Down

0 comments on commit 45c7454

Please sign in to comment.