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.

Sign up for GitHub

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

Jump to bottom

[Wallet] Making CWalletTx store TransactionRef. #2044

Merged
merged 3 commits into from
Jan 29, 2021
Merged

[Wallet] Making CWalletTx store TransactionRef. #2044

merged 3 commits into from
Jan 29, 2021

Conversation

furszy
Copy link

@furszy furszy commented Dec 9, 2020

Instead of inheriting from CTransaction, now CWalletTx will store it as member. Another step forward over the transaction complete immutability goal.

An adaptation of c3f5673 with further needed changes, essentially:

it makes it actually fully immutable and never modifies any of its elements (apart from wit) after construction.

To do so, we heavily rely on C++11. CTransaction gets a (CMutableTransaction&&) constructor that efficiently converts a CMutableTransaction into a CTransaction without copying. In addition, CTransaction gets a deserializing constructors.

One downside is that CWalletTx cannot easily inherent from CTransaction anymore, as CWalletTx needs a way to modify the CTransaction data inside. By turning the superclass into a CTransactionRef field, it can take advantage (not implemented yet) of sharing CTransactions with the mempool.

@furszy furszy self-assigned this Dec 9, 2020
@furszy furszy force-pushed the 2020_making_cwllettx_store_txref branch from 8a64ace to d5f4a06 Compare December 14, 2020 19:12
@Fuzzbawls Fuzzbawls added this to the Future milestone Dec 16, 2020
@Fuzzbawls Fuzzbawls changed the title [Wallet] Making CWalletTx store TransactionRef. [WIP][Wallet] Making CWalletTx store TransactionRef. Dec 16, 2020
@Fuzzbawls Fuzzbawls marked this pull request as draft December 16, 2020 22:32
@furszy furszy changed the title [WIP][Wallet] Making CWalletTx store TransactionRef. [Wallet] Making CWalletTx store TransactionRef. Jan 18, 2021
@furszy furszy force-pushed the 2020_making_cwllettx_store_txref branch from d5f4a06 to 3adfd60 Compare January 18, 2021 16:20
@furszy furszy marked this pull request as ready for review January 18, 2021 16:21
@furszy
Copy link
Author

furszy commented Jan 18, 2021

Rebased on master + completed the work.
Removed the draft tag, ready for review.

@furszy furszy force-pushed the 2020_making_cwllettx_store_txref branch from 3adfd60 to 501edb9 Compare January 21, 2021 23:56
@furszy furszy modified the milestones: Future, 5.1.0 Jan 22, 2021
@furszy furszy added the Wallet label Jan 22, 2021
random-zebra
random-zebra previously approved these changes Jan 22, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Minor concern over the newly introduced IsNormalTx function.
Otherwise looking good, utACK 501edb9

src/primitives/transaction.h Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2020_making_cwllettx_store_txref branch from 501edb9 to 7ecef96 Compare January 26, 2021 22:21
@furszy furszy requested review from random-zebra and Fuzzbawls and removed request for Fuzzbawls January 26, 2021 22:24
random-zebra
random-zebra reviewed Jan 27, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 7ecef96

src/primitives/transaction.h Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2020_making_cwllettx_store_txref branch from 7ecef96 to 652981a Compare January 27, 2021 13:45
random-zebra
random-zebra approved these changes Jan 27, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 652981a

Fuzzbawls
Fuzzbawls approved these changes Jan 29, 2021
View reviewed changes
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.

utACK 652981a

@furszy furszy merged commit b7871cb into PIVX-Project:master Jan 29, 2021
@random-zebra random-zebra mentioned this pull request Feb 1, 2021
Merged
random-zebra added a commit that referenced this pull request Feb 2, 2021
@random-zebra
Merge #2178: [BUG][GUI] Fix CWalletTx* casts to CTransaction*
5e00a6a 
6948abd [BUG][GUI] Fix CWalletTx* casts to CTransaction* (random-zebra)

Pull request description:

  Fix random GUI segfault during send.
  Since #2044 , we cannot cast `CWalletTx*` directly to `CTransaction*` (as `CMerkleTx` no longer inherits from `CTransaction`).
  We must use the `CTransactionRef` stored within it.

ACKs for top commit:
  furszy:
    great catch 👌 , ACK 6948abd
  Fuzzbawls:
    utACK 6948abd

Tree-SHA512: a9395d59bd64553a6e45592a66b6662a393098c560121e793121ada411643a77d349750a335c10ffc66ddafd54950fb1c6263583aa6218849a39aec1707ab6d9
@random-zebra random-zebra mentioned this pull request Feb 11, 2021
Merged
@furszy furszy deleted the 2020_making_cwllettx_store_txref branch November 29, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

@random-zebra random-zebra random-zebra approved these changes

Assignees

@furszy furszy

Labels
Wallet
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@furszy @Fuzzbawls @random-zebra