Skip to content
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

[Core] Bitcoin 0.12-0.14 serialization improvements #1633

Merged
merged 14 commits into from
May 29, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented May 20, 2020

-Based on top of

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

  • show scriptSig signature hash types in transaction decodes. fixes #3166 bitcoin/bitcoin#5264

    show scriptSig signature hash types. fixes Show sighash flags bitcoin/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.

  • add bip32 pub key serialization bitcoin/bitcoin#6215

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

  • Compact Blocks bitcoin/bitcoin#8068 (only commit 5249dac)
    This adds COMPACTSIZE wrapper similar to VARINT for serialization

  • Remove unused statements in serialization bitcoin/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 Do not shadow member variables in serialization bitcoin/bitcoin#8468 for previous discussion.

  • Various serialization simplifcations and optimizations bitcoin/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 Make CTransaction actually immutable bitcoin/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 Make CTransaction actually immutable bitcoin/bitcoin#8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams.

These changes decode valid SIGHASH types on signatures in assembly (asm)
representations of scriptSig scripts.
This squashed commit incorporates substantial helpful feedback from
jtimon, laanwj, and sipa.

backports bitcoin/bitcoin@af3208b
backports bitcoin/bitcoin@90604f1

PIVX: serialization not needed, only named constants
CDataStream and CAutoFile had a ReadVersion and WriteVersion method
that was never used. Remove them.

backports bitcoin/bitcoin@50e8a9c
The stream implementations had two cascading layers (the upper one
with operator<< and operator>>, and a lower one with read and write).
The lower layer's functions are never cascaded (nor should they, as
they should only be used from the higher layer), so make them return
void instead.

backports bitcoin/bitcoin@c2c5d42
Make the various stream implementations' nType and nVersion private
and const (except in CDataStream where we really need a setter).

backports bitcoin/bitcoin@fad9b66
Given that in default GetSerializeSize implementations created by
ADD_SERIALIZE_METHODS 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.

For CCompactSize and CVarInt this actually removes a more efficient
size computing algorithm, which is brought back in a later commit.

backports bitcoin/bitcoin@657e05a
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 recursively
invoked serializers.

Instead, the few places that need nType or nVersion are changed to read
it directly from the stream object, through GetType() and GetVersion()
methods which are added to all stream classes.

backports bitcoin/bitcoin@5284721
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 Cory Fields.

backports bitcoin/bitcoin@a2929a2
To get the advantages of faster GetSerializeSize() implementations
back that were removed in "Make GetSerializeSize a wrapper on top of
CSizeComputer", reintroduce them in the few places 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 invoked
indirectly in the serialization of another object.

backports bitcoin/bitcoin@25a211a
Dbwrapper used 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.

backports bitcoin/bitcoin@d59a518
Suggested by Pavel Janik.

backports bitcoin/bitcoin@a603925
@random-zebra random-zebra force-pushed the 202005_backport_serialization branch from 4aa1293 to 249cc9d Compare May 27, 2020 10:10
@random-zebra
Copy link
Author

Rebased after #1629 merge. Ready for review/QA.

Copy link

@ambassador000 ambassador000 left a comment

Choose a reason for hiding this comment

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

Functionality tested, working as intended.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Long and nice PR 👌 , code review ACK 249cc9d .

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 249cc9d

@furszy furszy added this to Ready in perpetual updating PIVX Core to BTC Core via automation May 29, 2020
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

tested ACK 249cc9d and merging.

@furszy furszy merged commit 8cd9c59 into PIVX-Project:master May 29, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done May 29, 2020
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 24, 2020
Document the changes introduced in:

- PIVX-Project#1549: Nuke zPIV from the GUI
- PIVX-Project#1586: Minimum value for stake split threshold
- PIVX-Project#1633: Bitcoin 0.12-0.14 serialization improvements
- PIVX-Project#1645: Implement accurate memory accounting for mempool
- PIVX-Project#1647: MemPool package tracking and limits
- PIVX-Project#1650: Benchmarking Framework
- PIVX-Project#1688: TopBar navigation (sync/peers)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 24, 2020
Document the changes introduced in:

- PIVX-Project#1549: Nuke zPIV from the GUI
- PIVX-Project#1586: Minimum value for stake split threshold
- PIVX-Project#1633: Bitcoin 0.12-0.14 serialization improvements
- PIVX-Project#1645: Implement accurate memory accounting for mempool
- PIVX-Project#1647: MemPool package tracking and limits
- PIVX-Project#1650: Benchmarking Framework
- PIVX-Project#1688: TopBar navigation (sync/peers)
random-zebra added a commit that referenced this pull request Jun 29, 2020
4819ee7 [Doc] Add/Update some release notes for 4.2 (random-zebra)

Pull request description:

  Document the changes introduced in:

  - #1549: Nuke zPIV from the GUI
  - #1586: Minimum value for stake split threshold
  - #1633: Bitcoin 0.12-0.14 serialization improvements
  - #1645: Implement accurate memory accounting for mempool
  - #1647: MemPool package tracking and limits
  - #1650: Benchmarking Framework
  - #1688: TopBar navigation (sync/peers)

ACKs for top commit:
  furszy:
    utACK 4819ee7
  Fuzzbawls:
    ACK 4819ee7

Tree-SHA512: 62ad949ea26a2f877ef0b40ec86616cc8105f81e1fcd380c8162cd93af04a46f1093f878c0668408654f198a0059b240798b83af3bf1d5e6c1c1d8611276a325
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Jun 30, 2020
[Doc] Add/Update some release notes for 4.2

Document the changes introduced in:

- PIVX-Project#1549: Nuke zPIV from the GUI
- PIVX-Project#1586: Minimum value for stake split threshold
- PIVX-Project#1633: Bitcoin 0.12-0.14 serialization improvements
- PIVX-Project#1645: Implement accurate memory accounting for mempool
- PIVX-Project#1647: MemPool package tracking and limits
- PIVX-Project#1650: Benchmarking Framework
- PIVX-Project#1688: TopBar navigation (sync/peers)

Update settingsfaqwidget.ui

Fix Linter
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Jun 30, 2020
[Doc] Add/Update some release notes for 4.2

Document the changes introduced in:

- PIVX-Project#1549: Nuke zPIV from the GUI
- PIVX-Project#1586: Minimum value for stake split threshold
- PIVX-Project#1633: Bitcoin 0.12-0.14 serialization improvements
- PIVX-Project#1645: Implement accurate memory accounting for mempool
- PIVX-Project#1647: MemPool package tracking and limits
- PIVX-Project#1650: Benchmarking Framework
- PIVX-Project#1688: TopBar navigation (sync/peers)

Update settingsfaqwidget.ui

Fix Linter

[RPC] Register calls where they are defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Show sighash flags
4 participants