fuzz: call lookup functions before calling Ban

[brunoerg]

Fixes #27924

To not have any discrepancy, it's required to call lookup functions before calling Ban. If we don't do it, the assertion assert(banmap == banmap_read); may fail because BanMapFromJson will call LookupSubNet and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (#27924).

Also, calling lookup functions before banning is what RPC setban does.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, dergoegge
Stale ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[maflcko]

Could rebase on master to remove export FUZZ_TESTS_CONFIG="--exclude banman" # https://github.com/bitcoin/bitcoin/issues/27924?

[brunoerg]

Could rebase on master to remove export FUZZ_TESTS_CONFIG="--exclude banman" # #27924?

Sure.

[brunoerg]

Force-pushed to remove "--exclude banman" in Mac

[brunoerg]

Rebased.

[dergoegge]

This still crashes for me on the following input /r8BAAAC/9LcAF8gMiWADQUA//////////////8AN1wNXMnJyQ3///8Afv///wABAAAAgAAAAADPvg==

[brunoerg]

I'm investigating it.

[brunoerg]

Thanks for reviewing, @dergoegge. I noted that we also need to call LookupHost when banning with CNetAddr. Force-pushed doing it and updated the PR description and title.

Also, this way we're closer to the reality (following what RPC does to set it).

[jonatack]

ACK f4bd8f2

Happy to re-ack if you take the suggestion.

[vasild]

I guess there are two possible approaches:

1. Shorter and less invasive/risky - change the fuzz test to not ban invalid subnets. Calling LookupSubNet() should not be necessary and I guess IsValid() should suffice? Is there a case where a subnet has IsValid() == true but LookupSubNet() fails on it? Also, IPv6 subnets that start with fc while NET_CJDNS is reachable are "invalid".

2. Change BanMan::Ban() to not add such invalid networks to its internal map and change it to return bool, false meaning "I did not ban that" and true meaning "A ban was added".

Maybe do 1. for 26.0 and 2. for 27.0?

[maflcko]

Looks like this can't be merged/backported as-is either way, due to the silent conflict caused by the rafactor (7be62df)?

[brunoerg]

Looks like this can't be merged/backported as-is either way, due to the silent conflict caused by the rafactor (7be62df)?

Gonna check it

[vasild]

One more consideration wrt to changing Ban() to refuse invalid stuff (2. in above #27935 (comment)) - it will not match later anyway due to:

bitcoin/src/netaddress.cpp

Lines 1005 to 1008 in 4458ae8

	bool CSubNet::Match(const CNetAddr &addr) const
	{
	if (!valid || !addr.IsValid() || network.m_net != addr.m_net)
	return false;

[brunoerg]

Rebased and changed the approach in fuzz to call Ban only with valid net address/subnet.

[maflcko]

Are you still working on this?

[brunoerg]

Are you still working on this?

Yes, will push an update soon

[brunoerg]

Force-pushed:

• Added a check to control whether we called Ban with an invalid subnet/netaddr. If so, we don't compare the banmaps.
• I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call Ban with an invalid subnet/netaddr.

[maflcko]

I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call Ban with an invalid subnet/netaddr.

Would be good to explain this a bit more, to make sure this is not a bug.

[brunoerg]

Would be good to explain this a bit more, to make sure this is not a bug.

I think fuzz can generate subnets with a zone identifier, like: "676:c962:7962:b787:b392:fed8:7058:c500%2038004089/121", which is a valid one. However, the lookup call might change the zone identifier. In my machine (macOS), "676:c962:7962:b787:b392:fed8:7058:c500%2038004089/121" becomes "676:c962:7962:b787:b392:fed8:7058:c500%31097/121" after lookup and it makes the assertion fails.

[maflcko]

Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.

[brunoerg]

Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.

Yes, force-pushed addressing it. Also, I think this way our seeds remain great.

[maflcko]

lgtm ACK fca0a89

I did not check the performance difference against 7e64430

[dergoegge]

ACK fca0a89

[vasild]

ACK fca0a89