Remove unused statements in serialization #8658

As the line

nVersion = this->nVersion;

seems to have no meaning in READ and also in WRITE serialization op, let's remove it and see what our tests/travis will tell us. See #8468 for previous discussion.

I'd suspect the binaries to be the same if any optimization flags are enabled.

Tests/travis is OK. Can anyone test binaries, please?

Rebased to match net.h-> addrdb.h commit from upstream.

The only place where the implicitly-passed-around nVersion variable is actually used is in CAddress::SerializationOp, and there it is just comparing with a constant directly.

@sipa Exactly. And there is no such line like in the proposed change.

utACK 64d9507ea5724634783cdaa290943292132086a9

After this I think we can actually go further and completely remove the nType and nVersion arguments from all SerializationOp methods, and replace them with calls to s.GetType() and s.GetVersion().

Tests/travis is OK. Can anyone test binaries, please? @paveljanik No need to test them. Apparently I get the same binaries, which means this is indeed dead code. (Instead of deleting the lines, you can replace them with empty lines to get rid of the offsets in the objdump). Can you check this?

bitcoind-matches-ACK 64d9507ea5724634783cdaa290943292132086a9 (qt binaries do not match, though)

Eh, isn't this intended to allow the serialised version to override the parameter?

@luke-jr Yes, that was the intention. But I don't think it's very usable. First of all, it requires that everything can be encoded inside a single version number, something that is increasingly hard in a decentralized environment. It also has never ever actually been used. It also is sort of a layer violation, as you need a single namespace for versioning across all of wallet code, network code, consensus-critical hash computation, database storage, ...

I think there are more flexible mechanisms possible using wrapper objects that introduce new serialization formats.

These are all implementation-specific objects, so decentralisation is less of an issue. But I guess it's fine so long as it's never been used before (but I'm not certain of that).

Concept ACK.

Eh, isn't this intended to allow the serialised version to override the parameter?

It can always be added back for a certain serialization op in the oft case that that needs to be used. No need to have dead code around 'just in case'.

I think this is ready for more ACKs. I volunteer for the next steps pointed out by @sipa above.

bitcoind-matches-ACK 64d9507 (qt binaries do not match, though)

Cannot reproduce this, I detect differences in the following functions between 64d9507 and 6898213:

void CBanEntry::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CBanEntry::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CHDChain::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CHDChain::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CKeyMetadata::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CKeyMetadata::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CMerkleTx::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CMerkleTx::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)

35: mov [-$0x34a,%r8d-]{+$0x34b,%r8d+}

Make sure to replace the deleted lines by empty lines to get rid of the offset?

BTW - this is not -Wshadow PR...

I think that we should compare binaries in some higher -O...

I think it's unlikely that this can result in identical binaries. It requires some cross-function reasoning across multiple modules to see this has no effect. We are changing the actual values of arguments passed down - they're just not used.

Make sure to replace the deleted lines by empty lines to get rid of the offset?

So these are line numbers? Ok, that'd make sense, will try again and w/ disabled LINE.

I think it's unlikely that this can result in identical binaries.

Yes the changes in the constructors are more involved.

I think that we should compare binaries in some higher -O...

Increasing -O generally makes it worse, not better. A lot of optimization settings make the output extremely sensitive to small change in the input, as well as cause a change in one function to move another (little related) function, making per-function comparison useless. This is why I went out of my way to find specific flags that work for this in my build/compare script: https://github.com/laanwj/bitcoin-maintainer-tools/blob/master/build-for-compare.py#L33 . It'd be possible to add specific flags if they're known to be safe.

It is very unlikely to produce the same binary, but to be safe, we should fully understand the difference.

Can template inline play some role here?

As a side note: in the "gcc set" -Wshadow I automatically changed nVersion to _nVersion in class CBlock : public CBlockHeader. It was not used at all in the function, so I thought without testing it is OK. But p2p-segwit.py started reproducibly failing. I used that binary to test -rescan on testnet and it always "failed" - see #8808#pullrequestreview-1610754 for details. Thus I had to revert it. Simple rename! Can you shed some light on it? Maybe it is relevant with this one.

So these are line numbers? Ok, that'd make sense, will try again and w/ disabled LINE.

They had to do with __LINE__ usage in LOCK/TRY_LOCK, which is interesting because without lock debugging the line numbers aren't used at all. So our sync.h macros/wrappers do incur a slight overhead even in that case.

In any case I've updated the above list, they no longer show up after removing line number sensitivity there.

We are changing the actual values of arguments passed down - they're just not used.

Right: READWRITE expands to

#define READWRITE(obj)      (::SerReadWrite(s, (obj), nType, nVersion, ser_action))

Thus silently passes nVersion to ::SerReadWrite. This function may or may not use the argument, but it is used.

This does change my opinion on this change from "harmless" to "hard to verify for correctness".

As a side note: in the "gcc set" -Wshadow I automatically changed nVersion to _nVersion in class CBlock : public CBlockHeader. It was not used at all in the function, so I thought without testing it is OK.

Indeed, if you change the argument name, the name binding will change and functions called will suddenly receive this->nVersion instead of the function argument. this->nVersion obviously gets updated by the READWRITE(this->nVersion). This will change the executable, but should not change the behavior, which is essentially the same as nVersion = this->nVersion. So by renaming you'd keep the behavior that there is right now, instead of changing it as in this PR.

At least that's what I would infer. I don't see why it would lead to a crashing segwit test though... This makes it kind of scary.

in class CBlock : public CBlockHeader.

OHH I get it, maybe. Unlike the serialization functions changed in this PR, CBlockHeader::SerializationOp does not have the nVersion = this->nVersion line. This means that the behavior does change if you rename the argument nVersion to _nVersion. After all,

READWRITE(this->nVersion);
READWRITE(hashPrevBlock);  

expands to

(::SerReadWrite(s, (this->nVersion), nType, nVersion, ser_action))
(::SerReadWrite(s, (hashPrevBlock), nType, nVersion, ser_action))

So the first line will changes this->nVersion, the second line passes nVersion. Which, after your argument renaming, refers to this->nVersion too. SO you effectively added nVersion = this->nVersion, which is a behavior change.

Do you really need that change for shadowing? I'd prefer not to do that, it looks to me that we're taking a huge risk just to avoid some compiler warnings. CBlock is as deep in consensus code as you can get.

@laanwj IMHO, this reveals that READWRITE is hiding intricacies which should instead be in plain sight.

@laanwj IMHO, this reveals that READWRITE is hiding intricacies which should instead be in plain sight.

IMO it'd have been better to just write those macros out., they make the code shorter apparently but much more obfuscated. (but that's an issue for another time)

My suggestion was that after this PR we would proceed to get rid of the nVersion and nType implicit parameters, and just replace them with getters on the stream implementation.

That would make things much easier to reason about.

Guess I've been doing it wrong then.

git checkout bitcoin/master && \
git reset --hard HEAD && \
curl https://raw.githubusercontent.com/laanwj/bitcoin-maintainer-tools/6e4425587736144b067f67ad792d9ee904e74fd7/patches/stripbuildinfo.patch | patch -p 1 && \
make -j 2 && \
objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_old && \
curl https://github.com/bitcoin/bitcoin/commit/64d9507ea5724634783cdaa290943292132086a9.diff | patch -p 1 && \
make -j 2 && \
objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_new && \
diff /tmp/d_old /tmp/d_new | wc

      0       0       0

@MarcoFalke the differences there would be:

• I check the commits themselves, you merge the patch on top of master
• I use a specific set of optimization flags, you use default optimization flags (-O2)

I don't think the first would make executables suddenly match. I'll retry with "-O2" and see if I can get a match.

Good news: using -O2 gives a complete match on bitcoind:

317c917ed86f7c32113598876ac3f48f84ea73281d01f69df1de78f429a019ed  /tmp/compare/bitcoind.64d9507.stripped
317c917ed86f7c32113598876ac3f48f84ea73281d01f69df1de78f429a019ed  /tmp/compare/bitcoind.6898213.stripped

as well as on bitcoin-qt

8675a67f3c2e5d4fc76f0083caeaa50585ba612e9b881ca22deea35de7f62c8b  /tmp/compare/bitcoin-qt.64d9507.stripped
8675a67f3c2e5d4fc76f0083caeaa50585ba612e9b881ca22deea35de7f62c8b  /tmp/compare/bitcoin-qt.6898213.stripped

ACK 64d9507

@laanwj Thanks!

This still has a [WIP] tag on the commit. However I'm going to merge nevertheless, as rebasing to change the commit message would have us all re-check executables again...