New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix masking of irrelevant bits in address groups. #6556

Merged
merged 2 commits into from Aug 20, 2015

Conversation

Projects
None yet
5 participants
@morcos
Member

morcos commented Aug 14, 2015

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 15, 2015

Member

Nice catch. Changing the | into an & seems simpler, though.

Member

sipa commented Aug 15, 2015

Nice catch. Changing the | into an & seems simpler, though.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 17, 2015

Contributor

Agreed, the & would be much clearer here and is more typical for a masking operation of this type.

Contributor

dcousens commented Aug 17, 2015

Agreed, the & would be much clearer here and is more typical for a masking operation of this type.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Aug 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));

Member

morcos commented Aug 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 17, 2015

Member

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

ACK

Member

sipa commented Aug 17, 2015

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

ACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 17, 2015

Member

utACK.
Would be nice if there would be test coverage in netbase_tests.cpp for this.

Member

jonasschnelli commented Aug 17, 2015

utACK.
Would be nice if there would be test coverage in netbase_tests.cpp for this.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 19, 2015

Member

utACK.
There was a similar error in the netmask parsing.
Agree @jonasschnelli that unit tests for this function would be nice.

Member

laanwj commented Aug 19, 2015

utACK.
There was a similar error in the netmask parsing.
Agree @jonasschnelli that unit tests for this function would be nice.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Aug 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.

Member

morcos commented Aug 19, 2015

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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 20, 2015

Member

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

Thanks for adding tests.

Member

laanwj commented Aug 20, 2015

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

Thanks for adding tests.

@laanwj laanwj merged commit 1123cdb into bitcoin:master Aug 20, 2015

laanwj added a commit that referenced this pull request Aug 20, 2015

Merge pull request #6556
1123cdb add unit test for CNetAddr::GetGroup. (Alex Morcos)
bba3db1 Fix masking of irrelevant bits in address groups. (Alex Morcos)

@laanwj laanwj added the Bug label Aug 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment