Skip to content

Conversation

@paveljanik
Copy link
Contributor

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

src/net.h Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@paveljanik paveljanik Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonasschnelli Can you please explain the original code? It is part of your changes in #6310 (409bccf). Thank you!

Copy link
Contributor Author

@paveljanik paveljanik Aug 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sipa
Copy link
Member

sipa commented Sep 1, 2016

What is the status here?

@paveljanik
Copy link
Contributor Author

paveljanik commented Sep 1, 2016

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
Copy link
Member

laanwj commented Sep 1, 2016

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

@paveljanik
Copy link
Contributor Author

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

@paveljanik paveljanik force-pushed the 20160805_Wshadow_serialization branch 3 times, most recently from ec93b88 to 386e895 Compare September 23, 2016 14:43
@paveljanik paveljanik force-pushed the 20160805_Wshadow_serialization branch from 386e895 to 09a0658 Compare September 30, 2016 06:09
@paveljanik
Copy link
Contributor Author

Rebased.

@maflcko
Copy link
Member

maflcko commented Sep 30, 2016

Binaries are not the same

@laanwj
Copy link
Member

laanwj commented Sep 30, 2016

Binaries are not the same

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

void CBanEntry::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CBanEntry::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CAutoFile, CSerActionSerialize>(CAutoFile&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CAutoFile, CSerActionUnserialize>(CAutoFile&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CBufferedFile, CSerActionUnserialize>(CBufferedFile&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CHashWriter, CSerActionSerialize>(CHashWriter&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize, 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)

@theuni
Copy link
Member

theuni commented Oct 20, 2016

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
Copy link
Contributor Author

paveljanik commented Oct 20, 2016

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

@sipa
Copy link
Member

sipa commented Oct 29, 2016

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
Copy link
Contributor Author

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

@paveljanik paveljanik closed this Oct 30, 2016
furszy added a commit to PIVX-Project/PIVX that referenced this pull request May 29, 2020
249cc9d Avoid -Wshadow errors (random-zebra)
8e1ec9e Use fixed preallocation instead of costly GetSerializeSize (random-zebra)
9b801d0 Add optimized CSizeComputer serializers (random-zebra)
0035a54 Make CSerAction's ForRead() constexpr (random-zebra)
9730a3f Get rid of nType and nVersion (random-zebra)
25ce2bb Make GetSerializeSize a wrapper on top of CSizeComputer (random-zebra)
1b479db Make nType and nVersion private and sometimes const (random-zebra)
35f1755 Make streams' read and write return void (random-zebra)
a395914 Remove unused ReadVersion and WriteVersion (random-zebra)
52e614c [WIP] Remove unused statement in serialization (random-zebra)
82a2021 Add COMPACTSIZE wrapper similar to VARINT for serialization (random-zebra)
13ad779 add bip32 pubkey serialization (random-zebra)
9e9b7b5 [QA] Update json files with sig hash type in ASM for bitcoin-util-test (random-zebra)
3383983 Resolve issue bitcoin#3166 (random-zebra)

Pull request description:

  -Based on top of
  - [x] #1629

  Backports the following serialization improvements from upstream and adds the required changes for the 2nd layer network and the legacy zerocoin code.

  - bitcoin#5264
    > show scriptSig signature hash types. fixes bitcoin#3166
    >
    > The fix basically appends the scriptSig signature hash types, within parentheses, onto the end of the signature(s) in the various "asm" json outputs. That's just the first formatting idea that came to my mind.
    >
    > Added some tests for this too.

  - bitcoin#6215
    > CExtPubKey should be serializable like CPubKey.
    > This would allow storing extended private and public key to support BIP32/HD wallets.

  - bitcoin#8068 (only commit 5249dac)
     This adds COMPACTSIZE wrapper similar to VARINT for serialization

  - bitcoin#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 bitcoin#8468 for previous discussion.

  - bitcoin#9039
    > The commits in this pull request implement a sequence of changes:
    >
    > - Simplifications:
    >   - **Remove unused ReadVersion and WriteVersion** CDataStream and CAutoFile had a ReadVersion and WriteVersion method that was never used. Remove them.
    >   - **Make nType and nVersion private and sometimes const** Make the various stream implementations' nType and nVersion private and const (except in CDataStream where we really need a setter).
    >   - **Make streams' read and write return void** The stream implementations have two layers (the upper one with operator<< and operator>>, and a lower one with read and write). The lower layer's return values are never used (nor should they, as they should only be used from the higher layer), so make them void.
    >   - **Make GetSerializeSize a wrapper on top of CSizeComputer** Given that in default GetSerializeSize implementations we're already using CSizeComputer(), get rid of the specialized GetSerializeSize methods everywhere, and just use CSizeComputer. This removes a lot of code which isn't actually used anywhere. In a few places, this removes an actually more efficient size computing algorithm, which we'll bring back in the "Add optimized CSizeComputer serializers" commit later.
    >   - **Get rid of nType and nVersion** The big change: remove the nType and nVersion as parameters to all serialization methods and functions. There is only one place where it's read and has an impact (in CAddress), and even there it does not impact any of the member objects' serializations. Instead, the few places that need nType or nVersion read it directly from the stream, through GetType and GetVersion calls which are added to all streams.
    >   - **Avoid -Wshadow errors** As suggested by @paveljanik, remove the few remaining cases of variable shadowing in the serialization code.
    > - Optimizations:
    >   - **Make CSerAction's ForRead() constexpr** The CSerAction's ForRead() method does not depend on any runtime data, so guarantee that requests to it can be optimized out by making it constexpr (suggested by @theuni in bitcoin#8580).
    >   - **Add optimized CSizeComputer serializers** To get the advantages of faster GetSerializeSize implementations back, reintroduce them in the few places where they actually make a difference, in the form of a specialized Serialize implementation. This actually gets us in a better state than before, as these even get used when they're nested inside the serialization of another object.
    >   - **Use fixed preallocation instead of costly GetSerializeSize** dbwrapper uses GetSerializeSize to compute the size of the buffer to preallocate. For some cases (specifically: CCoins) this requires a costly compression call. Avoid this by just using fixed size preallocations instead.
    >
    > This will make it easier to address @TheBlueMatt's comments in bitcoin#8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams.

ACKs for top commit:
  furszy:
    Long and nice PR 👌 , code review ACK 249cc9d .
  Fuzzbawls:
    ACK 249cc9d
  furszy:
    tested ACK 249cc9d and merging.

Tree-SHA512: 56b07634b1e18871e7c9a99d412282c83b85f77f1672ec56330a1131fc7c234cd1ba3a053bdd210cc29f1e636ee374477ff614fa9a930329a7f8f912c5006232
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants