core/bloombits, eth/filters: handle null topics #15195
Conversation
| bloomBits[i] = calcBloomIndexes(clause) | ||
| } | ||
| m.filters = append(m.filters, bloomBits) | ||
| // Accumulate the filter rules if no nil rule was within | ||
| if bloomBits != nil { |
There was a problem hiding this comment.
You could drop this check if you continue-d the outer loop instead of just break-ing out of the inner loop.
There was a problem hiding this comment.
I don't really like it when multiple nestings are broken out of. Seems a lot harder to follow what's going on, especially since the label can be arbitrarily far away.
There was a problem hiding this comment.
I kind of agree with both positions, but in this case personally I think it'd be clearer if it did continue the outer loop; the code is compact, so it's clear what the label is referring to. Alternately, you could refactor the inner loop and append into a separate function.
| bloomBits[i] = calcBloomIndexes(clause) | ||
| } | ||
| m.filters = append(m.filters, bloomBits) | ||
| // Accumulate the filter rules if no nil rule was within | ||
| if bloomBits != nil { |
There was a problem hiding this comment.
I kind of agree with both positions, but in this case personally I think it'd be clearer if it did continue the outer loop; the code is compact, so it's clear what the label is referring to. Alternately, you could refactor the inner loop and append into a separate function.
interfaces.go
Outdated
| @@ -146,7 +146,7 @@ type FilterQuery struct { | |||
| // {{}, {B}} matches any topic in first position, B in second position | |||
| // {{A}}, {B}} matches topic A in first position, B in second position | |||
| // {{A, B}}, {C, D}} matches topic (A OR B) in first position, (C OR D) in second position | |||
| Topics [][]common.Hash | |||
| Topics [][]*common.Hash | |||
There was a problem hiding this comment.
I don't get why this change is needed. The wildcard for a topic is the empty slice as documented just above this line. Since (anything OR XXX) is the same as anything, the new possibility of including a nil hash as a wildcard is useless.
There was a problem hiding this comment.
E.g. [2][2]common.Hash means
[t1 OR t2] AND [t3 or t4]
You can't set t1 or t2 or t3 or t4 to nil, only the entire group.
There was a problem hiding this comment.
Ah, you mean interpreting the nulls already during parsing and getting rid of them.
karalabe
force-pushed
the
filter-null-fix-2
branch
from
ad17590
to
87af6ae
Compare
|
Simplified according to @fjl's request. @Arachnid @fjl @zsfelfoldi PTAL |
When implementing the new bloombits based filter, I've accidentally broke null
topics by removing the special casing of common.Hash{} filter rules, which
acted as the wildcard topic until now.
This PR fixes the regression, but instead of using the magic hash
common.Hash{} as the null wildcard, the PR reworks the code to handle nil
topics during parsing, converting a JSON null into nil []common.Hash topic.
When implementing the new bloombits based filter, I've accidentally broke null
topics by removing the special casing of common.Hash{} filter rules, which
acted as the wildcard topic until now.
This PR fixes the regression, but instead of using the magic hash
common.Hash{} as the null wildcard, the PR reworks the code to handle nil
topics during parsing, converting a JSON null into nil []common.Hash topic.
When implementing the new bloombits based filter, I've accidentally broke null
topics by removing the special casing of common.Hash{} filter rules, which
acted as the wildcard topic until now.
This PR fixes the regression, but instead of using the magic hash
common.Hash{} as the null wildcard, the PR reworks the code to handle nil
topics during parsing, converting a JSON null into nil []common.Hash topic.
When implementing the new bloombits based filter, I've accidentally broke
nulltopics by removing the special casing ofcommon.Hash{}filter rules (f585f9e#diff-398162818e80393efa0b6f121150278bL205), which acted as the wildcard topic until now.This PR fixes the regression, but instead of using the magic hash
common.Hash{}as thenullwildcard, the PR reworks the code to handleniltopics during parsing, converting a JSONnullinto nil[]common.Hashtopic.