Fix masking of irrelevant bits in address groups. #6556

pull morcos wants to merge 2 commits into bitcoin:master from morcos:patch_netBits changing 2 files +18 −1

morcos commented at 7:27 PM on August 14, 2015: member

If I'm reading this right, I believe the intent is to keep the high order nBits and mask away the others with 1's. But before this fix it was keeping the high order (8 - nBits). I don't think there was any effect, because the only time this was used was in the case of he.net /36 IPv6 groups so nBits was 4.
Fix masking of irrelevant bits in address groups. bba3db1a40
sipa commented at 8:31 PM on August 15, 2015: member

Nice catch. Changing the | into an & seems simpler, though.
dcousens commented at 12:47 AM on August 17, 2015: contributor

Agreed, the & would be much clearer here and is more typical for a masking operation of this type.
morcos commented at 2:55 PM on August 17, 2015: member

I thought about & but I wasn't confident it was ok to change the previous behavior. I also couldn't see how to make it as short? vchRet.push_back(GetByte(15 - nStartByte) & (((1 << nBits) - 1) << (8 - nBits))); or vchRet.push_back(GetByte(15 - nStartByte) & ~((1 << (8-nBits)) - 1));
sipa commented at 2:58 PM on August 17, 2015: member

Good point, we shouldn't change the existing behaviour.

ACK
jonasschnelli commented at 3:37 PM on August 17, 2015: contributor

utACK. Would be nice if there would be test coverage in netbase_tests.cpp for this.
laanwj commented at 3:09 PM on August 19, 2015: member

utACK. There was a similar error in the netmask parsing. Agree @jonasschnelli that unit tests for this function would be nice.
morcos commented at 5:27 PM on August 19, 2015: member

Ok writing unit test. I have a question about IsLocal(), since those addresses are also !IsRoutable() they will be lumped in the NET_UNROUTABLE group instead of having their own group. The check for IsLocal() on line 932 is meaningless. Is that expected? Changing that would obviously change existing behavior.
add unit test for CNetAddr::GetGroup. 1123cdbf4d
morcos force-pushed on Aug 19, 2015
laanwj commented at 2:43 PM on August 20, 2015: member

Yes, that check seems to be redundant. But let's leave removing it for another pull.

Thanks for adding tests.
laanwj merged this on Aug 20, 2015
laanwj closed this on Aug 20, 2015
laanwj referenced this in commit 70ec975ea6 on Aug 20, 2015
laanwj added the label Bug on Aug 20, 2015
furszy referenced this in commit cadf9e6daa on May 21, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels