Do not shadow member variables in serialization

paveljanik commented at 7:54 pm on August 5, 2016: contributor

This solves task 2 from #8105. serialize.h and nVersion, nType shadowing.

in src/net.h: in b8701be4c5 outdated

283@@ -284,7 +284,7 @@ class CBanEntry
284     ADD_SERIALIZE_METHODS;
285 
286     template <typename Stream, typename Operation>
287-    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
288+    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int _nVersion) {

theuni commented at 9:15 pm on August 5, 2016:

I don’t see what this was supposed to be doing before, but this look like a self-assign now.

paveljanik commented at 9:44 pm on August 5, 2016:

I do not understand the original code… :(

https://github.com/bitcoin/bitcoin/blob/master/src/net.h#L286-L293

The original code is assigning to the function argument already.

paveljanik commented at 7:53 am on August 6, 2016:

@jonasschnelli Can you please explain the original code? It is part of your changes in #6310 (https://github.com/bitcoin/bitcoin/pull/6310/commits/409bccfbf52b531b2a9d60ac2308f56223931a2e). Thank you!

paveljanik commented at 8:14 am on August 6, 2016:

The same code is in https://github.com/bitcoin/bitcoin/blob/master/src/qt/walletmodel.h#L68-L78 and https://github.com/bitcoin/bitcoin/blob/master/src/qt/recentrequeststablemodel.h#L29-L34.

jonasschnelli added the label Refactoring on Aug 8, 2016

sipa commented at 1:40 pm on September 1, 2016: member

What is the status here?

paveljanik commented at 2:03 pm on September 1, 2016: contributor

The change in src/net.h is unclear (at least to me ;-), even in the current tree. What is the purpose of the line https://github.com/bitcoin/bitcoin/blob/master/src/net.h#L291? Assigning to a shadowed variable which is not used at all down below this assignment is strange… It has no meaning in READ, it has no meaning in WRITE.

IIRC we had a short talk about this on IRC with @laanwj a few days ago.

Edit: I PRed its removal in #8658 as a test…

laanwj commented at 2:17 pm on September 1, 2016: member

I think the important requirement here, as for any variable shadowing change, is that the executables stay the same after renaming.

paveljanik force-pushed on Sep 23, 2016

paveljanik commented at 8:09 am on September 23, 2016: contributor

Rebased. This is waiting for #8658 though, no need to review now.

paveljanik force-pushed on Sep 23, 2016

Do not shadow member variables in serialization 09a0658689

paveljanik force-pushed on Sep 30, 2016

paveljanik commented at 6:11 am on September 30, 2016: contributor

Rebased.

MarcoFalke commented at 5:20 pm on September 30, 2016: member

Binaries are not the same

laanwj commented at 5:50 pm on September 30, 2016: member

Binaries are not the same

Indeed. Either with -O2 or without, there are binary differences. The following functions are affected:

 0void CBanEntry::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
 1void CBanEntry::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
 2void CBlockHeader::SerializationOp<CAutoFile, CSerActionSerialize>(CAutoFile&, CSerActionSerialize, int, int)
 3void CBlockHeader::SerializationOp<CAutoFile, CSerActionUnserialize>(CAutoFile&, CSerActionUnserialize, int, int)
 4void CBlockHeader::SerializationOp<CBufferedFile, CSerActionUnserialize>(CBufferedFile&, CSerActionUnserialize, int, int)
 5void CBlockHeader::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
 6void CBlockHeader::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
 7void CBlockHeader::SerializationOp<CHashWriter, CSerActionSerialize>(CHashWriter&, CSerActionSerialize, int, int)
 8void CBlockHeader::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize, int, int)
 9void CHDChain::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
10void CHDChain::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
11void CKeyMetadata::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
12void CKeyMetadata::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)

theuni commented at 4:23 pm on October 20, 2016: member

It appears as though the changes come from the fact that the passed variable is now unused. Presumably the compiler notices that and doesn’t bother copying. @paveljanik Rather than passing _nVersion in the unused case, could you just leave the variable unnamed instead? That will also cut down on the “unused parameter” warnings, should we ever decide to enable those.

paveljanik commented at 4:31 pm on October 20, 2016: contributor

@theuni unfortunately nVersion is used in READWRITE(…). Right now, I do not know how to match the requirement to have the same binary…

sipa commented at 11:40 pm on October 29, 2016: member

I don’t think it’s feasible to get the same binary with this kind of changes. Given that, I have a more invasive change to the serialization code that removes the nType and nVersion being passed around entirely instead; see #9039.

paveljanik commented at 6:25 pm on October 30, 2016: contributor

@sipa ’s approach is much better in general and also solves this issue, so closing this one.

paveljanik closed this on Oct 30, 2016

furszy referenced this in commit 8cd9c592a9 on May 29, 2020

DrahtBot locked this on Sep 8, 2021

Do not shadow member variables in serialization #8468