< sipa>
phantomcircuit: or you mean inside bitcoin core codebase? GetMappedAs in CNetAddr
< bitcoin-git>
[bitcoin] fanquake opened pull request #19197: init: use std::thread for ThreadImport() (master...thread_import_no_boost) https://github.com/bitcoin/bitcoin/pull/19197
< phantomcircuit>
sipa, i mean for banning by AS
< phantomcircuit>
sipa, is there a reason the asmap isn't a global? seems like something that should be a global
< sipa>
phantomcircuit: i think connman or addrman is a reasonable place for.it
< bitcoin-git>
bitcoin/master 501e6ab Calvin Kim: doc: Add documentation for 'checklevel' argument in 'verifychain' RPC call
< bitcoin-git>
bitcoin/master 1b90a7b MarcoFalke: Merge #19005: doc: Add documentation for 'checklevel' argument in 'verifyc...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #19005: doc: Add documentation for 'checklevel' argument in 'verifychain' RPC… (master...add-documentation-for-verifychain) https://github.com/bitcoin/bitcoin/pull/19005
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19198: test: Check that peers with forcerelay permission are not asked to feefilter (master...2006-testForcerelayFeefilter) https://github.com/bitcoin/bitcoin/pull/19198
< phantomcircuit>
sipa, can i get all of the subnets in an asn from the asmap?
< sipa>
not without decoding the entire thing
< phantomcircuit>
it looks like no
< sipa>
but that's still pretty fast
< phantomcircuit>
that makes adding asn based bans much harder but yeah definitely still doable
< sipa>
ah i see
< sipa>
you want to translate banned ASes to subnets
< phantomcircuit>
would it be reasonable to do that when adding asn bans? cause that changes it to like two lines of code from many more
< luke-jr>
maybe the banlist should support ASNs directly?
< sipa>
that would certainly be cleaner
< sipa>
but i see how that would be harder to implement
< sipa>
i think if banlists on disk supported ASes natively, but at runtime they're converted to subnets, maybe it's ok?
< phantomcircuit>
i can definitely support asn's directly instead, it's not something where one approach is obviously better to me though
< sipa>
so that they correctly persist across restarts if asmap gets updated
< phantomcircuit>
adding support naively will break backwards compat with banlist.dat though
< luke-jr>
sipa: nah imo
< luke-jr>
sipa: setban should support adding/removing AS bans, without affecting overlapping subnet bans, IMO
< sipa>
luke-jr: that's reasonable
< luke-jr>
plus we might want to update ASmaps at runtime someday
< sipa>
phantomcircuit: i think it's expected that banlist.dat compatibility breaks when you ban an AS - there is no way for an old version to behave correctly
< sipa>
you wouldn't want to run with a partial set of bans
< phantomcircuit>
sipa, no i mean doing this naively would mean clearing all your old bans
< sipa>
why?
< luke-jr>
hmm
< luke-jr>
sipa: do we have a way to sanely handle an incompatible banlist in current/old versions?
< sipa>
no clue
< luke-jr>
losing the banlist entirely would be strictly worse than just losing AS bans
< sipa>
i have never looked at its serialization
< sipa>
agree
< sipa>
well
< phantomcircuit>
sipa, banlist.dat is basically just a serialized banmap_t which is a map of subnets
< sipa>
i think that if you add an AS ban, you should not lose existing subnet bans
< sipa>
but if you add an AS ban, and then downgrade to a version that does not support AS bans, ideally it just fails to start
< sipa>
phantomcircuit: banlist serialized entries have a version number
< sipa>
so there is no problem
< sipa>
just increment the version number of AS-based bans; but keep supporting the old subnet-only one in the deserializer
< sipa>
unfortunately the version number is not enforced
< sipa>
so old versions with read corrupted data
< luke-jr>
looks like if we change the root type (currently map<CSubNet, CBanEntry>) old versions should refuse to load it
< phantomcircuit>
that means changing a bunch of the logic for serialize/deserialize
< luke-jr>
and refuse to start
< phantomcircuit>
or i could just map asn's to subnet and it's potentially much less code and compatible...
< luke-jr>
sipa: there's a checksum to stop corrupted data
< luke-jr>
phantomcircuit: could make a new type that serialises the same for subnets?
< luke-jr>
and just serialise ::0/0 followed by AS info for anAS ban
< luke-jr>
the checksum should stop it from loading that as ::0/0 in old versions
< luke-jr>
(possibly stored as ::ASN/0 in memory to avoid wasting extra RAM
< sipa>
phantomcircuit: i'll have a look at making the ser/deser code more upgradable
< phantomcircuit>
sipa, the addrdb and bandb share the ser/deser logic to do this they probably just need to be split
< sipa>
phantomcircuit: perhaps
< sipa>
phantomcircuit: std::map is serialized with a size prefix
< sipa>
so we could change the serialization to stary with a giant number
< sipa>
to indicate a new version
< sipa>
so that it will fail to load on old versions
< sipa>
*start
< phantomcircuit>
just start with a dummy db that has a bad checksum
< sipa>
that's also possible
< phantomcircuit>
that seems like it would be more obvious
< phantomcircuit>
also changing banmap_t is going to effect a whole bunch of logic
< sipa>
i know
< sipa>
but that code is due for a cleanup anyway, o think
< sipa>
*i
< luke-jr>
…
< luke-jr>
these all sound much more complex than what I just suggested :P
< sipa>
which is?
< luke-jr>
[17:29:56] <luke-jr> and just serialise ::0/0 followed by AS info for anAS ban
< sipa>
that won't work
< luke-jr>
why not?
< sipa>
it will result in old software reading corrupted datae
< luke-jr>
which will fail the checksum
< sipa>
ah, indeed!
< sipa>
so you'd need a new data type instead of CSubNet to use as key in the map
< sipa>
which can represent either a CSubNet or an ASN
< luke-jr>
yes
< sipa>
and csubnet's remain serialized as-is
< luke-jr>
exactly
< sipa>
and ASN are serialized as some sentinel value that's unlikely to be already present in banmans (::0/0 seems right as luke-jr suggests) + version number + ASN
< luke-jr>
I mean, we could use the CSubNet part for ASN data too, so long as we don't end up with the same length
< sipa>
it needs to be recognizable as an AS entry to the deserializer
< sipa>
CSubNets do not have an explicit length in their serialization afaik
< luke-jr>
ah
< phantomcircuit>
uh whuch checksum would that fail?
< phantomcircuit>
i dont see any reason that would fail the checksum in DeserializeDB
< sipa>
phantomcircuit: because the data deserialized wouldn't match
< sipa>
so it would read a checksum in the wrong place
< phantomcircuit>
i mean we can just append the asn stuff then and the checksum will be wrong that way to
< sipa>
yes, but how would a deserializer notice that there is more stuff to follow?
< sipa>
it would need to distinguish ban entries from the checksum...
< phantomcircuit>
start/data/data/hash and try to read data as both a hash and a map of asns
< sipa>
that's pretty ugly i think, but yes it will work
< sipa>
i think it's a bigger change
< phantomcircuit>
sipa, could equally just have v1/v2 deserializers
< sipa>
phantomcircuit: and try both?
< phantomcircuit>
yeah
< sipa>
well, see it this way: i think you're going to need a data type that represents "subnet or ASN" anyway, to put in the banmap map as key
< sipa>
if you give that data type a serialization that's identical to a subnet's for subsets, and to some sentinal + ASN for asns, you have no other serialization issues
< sipa>
*sentinel
< phantomcircuit>
yeah that's a good point
< phantomcircuit>
why is it `using banman_t = std::map...` and not a typedef ?
< sipa>
using is the newer syntax
< sipa>
it does the same as a typedef, but is more flexible