Use trie in merge process#123
Conversation
jurraca
force-pushed
the
add-trie-merge
branch
from
fa1f450 to
391f59b
Compare
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
It turns out there was a bug in our implementation. When we called contains_row to check whether a network was included in the base, it only checks whether that candidate / incoming network is a subnet. But it could also be a super-net, i.e. a shorter prefix. In that case both would exist in the merged output, but would be logically conflicting; we want to consider the base (RPKI) entry as the valid one, and reject the candidate. 5b0eea3 ensures we never overwrite base/RPKI entries, whether the candidate is a subnet or supernet of an existing entry.
If I understand correctly what you are saying I don't think this is a bug. When we have overlapping coverage in our resulting asmap we use the most precise (longest) prefix for the ASN. If there is an overlapping, shorter entry we only use it for the additional coverage that it gets us aside from the one we got from the longer one. So these supernets never "overwrite" the RPKI coverage, they only add coverage where RPKI didn't apply. But maybe there is a misunderstanding about the issue on my side and an example might help, I didn't dig deeper into the code and resulting files on this yet.
Running a reproduction run against the latest collab run, the new final_result file is 3254 entries longer. A diff of the original final_result and the new one gives 15699 entries (1% of total), 95% (15003) of which were previously unassigned. This sounds about right, because we keep the RPKI entries which are often most specific. I'm not sure though.
The count of entries and file size stay about the same, as does the encoded (filled or unfilled) file size.
This result seems counterintuitive since there are more entries even though you are saying we reject more now. If you insist the new approach is better it would be good to find out why this is the case. I haven't looked into it but the only explanation that would make sense to me is that with the approach we are using in master now, we add these supernets from IRR which are usually shorter and add larger areas of coverage for a smaller number of lines. This leads to many potential entries from Routeviews being rejected which are longer than what we got from IRR but don't offer additional coverage. Since RV essentially is BGP traffic it makes sense that it is a lot more fractured than IRR which is used for filters etc. So after the code change here rejects these supernets from IRR this leaves the door open for all these longer routeviews entries to get into the result file, which results in more entries.
This is just an educated guess, I have not looked at the data yet to verify if this is what happens.
| Merge lists of IP networks into a base file. | ||
| """ | ||
| print("Merging extra prefixes that were not included in the base file.") | ||
| print("Creating network index from base file.") |
There was a problem hiding this comment.
Why add this additional logging? I think we just recently removed similar lines to that extend, so I think keeping the logging as is would be better.
There was a problem hiding this comment.
just changing the text, not a new log message. This is one of the messages we decided to keep.
There was a problem hiding this comment.
This line just looks like it's only changing the text but below on L133 the old log message still exists: 1010890#diff-db4db3f74df857bf1194562bdeaa881b0f91197d4185f45345ca7eee6e2cf50eR133 So there are now two logs where there used to be one.
thanks, you're right, I was overthinking it. Longest prefix match wins. I removed the clause and function that attempted to check for a supernet of the checked network. Now it's just subset checking, but doesn't need the root_net index as the previous merge process did, so it's more general. Watching repro run CI... |
|
a diff against a run with this version still yields about a 15k diff, with mostly unassigned as with the previous run. Even after encoding with |
when looking up a network, return an ASN if there is a network with a shorter prefix or a longer prefix than the candidate network (i.e. any overlap). This enforces the idea that the base index is the source of truth, and a base network should not be superseded by an incoming network.
|
After looking further, I found the bug, documented here. To reproduce, using the latest run bitcoin-core/asmap-data#51 , you can run a reproduction run with this branch then run a diff between the result files. It's about 16k line diff file, with 16076 out of 16855 (95%) in the diff are "previously unassigned". It's a bit misleading because the "unassigned" being reported in the diff are just getting rolled up into the larger network, and are not individually assigned in the trie-based result. The lines with previously assigned AS help show the issue. For example, there's the line: Either way the trie impl is better if it caught this. While it would be nice to be able to "reproduce" any old ASmap, the correctness seems more valuable. added a small merge test to make this behavior explicit and to sanity-check myself; we can remove later if necessary. |
2 participants
Use the trie implementation in the merge process, replacing the concurrent, chunked, map-based process.
It turns out there was a bug in our implementation. When we called
contains_rowto check whether a network was included in the base, it only checks whether that candidate / incoming network is a subnet. But it could also be a super-net, i.e. a shorter prefix. In that case both would exist in the merged output, but would be logically conflicting; we want to consider the base (RPKI) entry as the valid one, and reject the candidate. 5b0eea3 ensures we never overwrite base/RPKI entries, whether the candidate is a subnet or supernet of an existing entry.Running a reproduction run against the latest collab run, the new final_result file is 3254 entries longer. A diff of the original final_result and the new one gives 15699 entries (1% of total), 95% (15003) of which were previously unassigned. This sounds about right, because we keep the RPKI entries which are often most specific. I'm not sure though.
The count of entries and file size stay about the same, as does the encoded (filled or unfilled) file size.
Minimal test changes, probably should add some edge case testing, but the merge tests pass without changes.
Overall, this makes the merge code simpler and removes some dependencies. We can probably remove pandas entirely next with a few more changes. It also speeds up the merge process.
However this will break our reproducibility CI check.