Restore compatibility with old CSubNet serialization

[sipa]

#19628 changed CSubNet for IPv4 netmasks, using the first 4 bytes of netmask rather than the last 4 to store the actual mask. Unfortunately, CSubNet objects are serialized on disk in banlist.dat, breaking compatibility with existing banlists (and bringing them into an inconsistent state where entries reported in listbanned cannot be removed).

Fix this by reverting to the old format (just for serialization). Also add a sanity check to the deserializer so that nonsensical banlist.dat entries are ignored (which would otherwise be possible if someone added IPv4 entries after #19628 but without this PR).

Reported by Greg Maxwell.

[kallewoof]

Code review ACK

Slightly prefer if this was handled with versioning instead (i.e. bump banlist version, if there is one, have a grace period for a few versions where both are supported).

I think it's okay to eventually drop backwards compatibility for banlists, but maybe I don't take them seriously enough.

[sipa]

@kallewoof I agree that that would be the way to go if there was actually a feature change. There isn't. The serialization format was just gratuitously broken, and this reverts it.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: simpler setban and new ban manipulation commands #19825 (rpc: simplify setban and consolidate BanMan functions by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 20140, 6bb49ef)	Reference (master, 3caee16)
Lines	+0.0037 %	90.7139 %
Functions	-0.0054 %	86.2441 %
Branches	+0.0097 %	52.0898 %

^{Updated at: 2020-10-15T09:31:57.171185.}

[practicalswift]

Concept ACK

[vasild]

ACK 886be97

Maybe we don't need to ignore the entries that have the IPv4 mask in the first 4 bytes (second commit) because this code was not released?

I am planning to change CSubNet::netmask from uint8_t [16] to just uint8_t (CIDR netmask number) and this is tightly related. I guess I will keep the (un)serialize to use this old format of 16 bytes with IPv4 netmask at the end because introducing a version of banlist.dat does not look justified (it will save 15 bytes per entry on disk).

[sipa]

@vasild Agree that there is no strong requirement to deal with files created/modified using unreleased code. But as it's perhaps a generally useful change (it guarantees that regardless of the file's contents, you'll never end up in a situation where there are entries in listbanned that cannot be remvoed using setban), perhaps it's useful to keep it in?

[vasild]

Yes!

[laanwj]

Thanks for the quick fix!
Code review ACK 886be97

Maybe we don't need to ignore the entries that have the IPv4 mask in the first 4 bytes (second commit) because this code was not released?

I agree about this train of thought. I'm fine with keeping that sanity check if it also serves another purpose, but also understand the argument to not clutter the code with special-purpose workarounds for corruption issues that were in the master branch only for a short time. At the least it would be good to add a comment with a timeframe for removing the work-around.

[gmaxwell]

I would suggest that it assert on any wonky-mask, except then you'd create a crash for anyone that had bans from current git master. You really don't want those in memory, esp if later the ban lookup is changed to a patricia trie they'll turn into memory corruption bugs (unless the author of that code remembers this possibility and inserts the sanitization there).

Perhaps an alternative is to have the sanitization and the plan for removing it is to stop storing masks and instead store prefix lengths, so that the insane values are just not representable.

[sipa]

@gmaxwell This deletes the wonky-mask ones immediately after loading, FWIW. The deserializer constructs them as entries with valid=false, which are then immediately pruned.

[gmaxwell]

sorry, I was intending to respond to @laanwj who seemed to not like the deleting that much!

[sipa]

Oh, I missed that.

@laanwj I'm thinking that this sanity check should probably stay in the code. It isn't specific to the incompatbility here - any other damage to your banlist.dat file could also mean you end up with unremovable entries, so it seems like a generic improvement to protect against that.

[vasild]

stop storing masks and instead store prefix lengths, so that the insane values are just not representable

Just to note that even if we store prefix lengths (CIDR), that would be stored in 1 byte and it is still possible to have insane values, e.g. 150.

[laanwj]

It's good to ward against corrupted data files, especially relatively unimportant ones like the ban list, but at some point, disk is a trusted interface. There are tons of ways disk corruption can crash the daemon. And sometimes it should, so that users are aware of what is happening, making things easier to diagnose (weird behavior like undeletable bans is worse, but also impossible to avoid in general).

That said I'm okay with this fix as it is, and don't think this particular change is worth bikeshedding too much. It needs to be fixed as soon as possible to avoid doing more damage.