Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Aug 24, 2016

This is an alternative to #8451.

Instead of making CTransaction mutable-but-with-cached-hash, 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.

@laanwj
Copy link
Member

laanwj commented Aug 25, 2016

Concept ACK, thanks for doing this!

@sipa
Copy link
Member Author

sipa commented Aug 25, 2016

@daira This is probably interesting to you as well.

The reason for this alternative is that @laanwj pointed out that #8451 is a step in the wrong direction if we want a more encapsulated CTransaction (which perhaps no longer uses the same representation, but uses a single-malloced block of data for example).

Copy link
Member

Choose a reason for hiding this comment

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

This ignores witness data. Is that intentional?

@sipa
Copy link
Member Author

sipa commented Aug 27, 2016

@luke-jr: No, but it's what the old code did as well (it automatically converted the CMutableTransactions to CTransaction, and then invoked its operator== which only compares the hashes).

@dcousens
Copy link
Contributor

dcousens commented Aug 29, 2016

This PR implements the latter - which I believe to be well-defined but very ugly. Suggestions or rants welcome.

My understanding was that even this hack is illegal and UB.
Another good explanation of the edge cases and reasons against this.
The triviality of causing UB, even with extreme caution is probably not acceptable, especially for miners who could potentially lose out on their reward.

As for an alternative solution, I don't see one beyond the mass-copy at this point, but I'm having a look around.

The underlying conflict is assignment and const... you usually can't have both without doing something illegal... (in C++)

@sipa
Copy link
Member Author

sipa commented Aug 29, 2016 via email

@paveljanik
Copy link
Contributor

S&M request: please rebase pain ;-)

@sipa
Copy link
Member Author

sipa commented Sep 19, 2016

Rebased.

@gmaxwell
Copy link
Contributor

Would it be possible to simply change the representation of a block so the first transaction is handled separately? Perhaps even building up the hashtree so updating the root doesn't require recomputing everything?

@sipa
Copy link
Member Author

sipa commented Sep 23, 2016 via email

@gmaxwell
Copy link
Contributor

Concept ACK.

@gmaxwell
Copy link
Contributor

Needs rebase.

@sipa
Copy link
Member Author

sipa commented Oct 2, 2016

Rebased.

@sipa
Copy link
Member Author

sipa commented Oct 21, 2016

Rebased.

Copy link
Member

Choose a reason for hiding this comment

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

make CSerActionSerialize::ForRead() and CSerActionUnserialize::ForRead() constexpr, then you can static_assert at build-time instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #9039 (though I think after that, we won't actually need this unimplemented branch anymore).

Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

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

utACK except CreateTransaction bug

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could accept a unique_ptr&& so the API documents the ownership semantics

Copy link
Contributor

Choose a reason for hiding this comment

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

moved here, used below

Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Generally LGTM, just one or two comments and some nits (though I didnt look at tests, mostly).

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lol, can we get a real variable name here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a define based on the other block size limits and min transaction size isntead of making it unexplained magic?

src/main.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you std::move here? That object is not used anywhere hereafter, except to be destructed.

Copy link
Member Author

@sipa sipa Oct 29, 2016

Choose a reason for hiding this comment

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

If CTransaction is entirely immutable, a move constructor would just be identical to a copy constructor, so no need. We could leave a move constructor that only moves the (mutable) witness field, but that would get more complicated with #8589 (which moves the witness data inside CTxIn). I think a better (but follow-up) change would be changing CBlock::vtx to a std::vector<std::shared_ptr<CTransaction>>.

src/main.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like you're making a needless copy here? Why not just
CTransaction tx(vRecv, vRecv.nType, vRecv.nVersion) (or, better yet, make a new constructor which knows how to pull out nType and nVersion).

Copy link
Member Author

Choose a reason for hiding this comment

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

See #9039.

src/miner.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can move here, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: needless copy sucks.

src/serialize.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I really dont like adding methods here that we just assert(false) on...would prefer to just call the constructor directly instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: can you move this down to right above SendMoney, so its clear that its not init'd until then 9and in the next two functions as well)

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well make the line above this an assert as well (and remove this line)

zkbot added a commit to zcash/zcash that referenced this pull request Jul 14, 2017
Remove the incorrect const declarations in CTransaction.

Based on upstream bitcoin/bitcoin#8451, which is a much, much safer and easier-to-review fix than bitcoin/bitcoin#8580. fixes #967

Signed-off-by: Daira Hopwood <[email protected]>
codablock added a commit to codablock/dash that referenced this pull request Jan 17, 2018
Instead of deriving from CTransaction, we now have a CTransactionRef member
in CTxLockCandidate. This is needed for the next backported PR bitcoin#8580,
which will make CTransaction immutable.

Also use CTransactionRef in CDarkSendEntry, CDarksendBroadcastTx and
CPrivateSendServer
codablock pushed a commit to codablock/dash that referenced this pull request Jan 17, 2018
81e3228 Make CTransaction actually immutable (Pieter Wuille)
42fd8de Make DecodeHexTx return a CMutableTransaction (Pieter Wuille)
c3f5673 Make CWalletTx store a CTransactionRef instead of inheriting (Pieter Wuille)
a188353 Switch GetTransaction to returning a CTransactionRef (Pieter Wuille)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
Instead of deriving from CTransaction, we now have a CTransactionRef member
in CTxLockCandidate. This is needed for the next backported PR bitcoin#8580,
which will make CTransaction immutable.

Also use CTransactionRef in CDarkSendEntry, CDarksendBroadcastTx and
CPrivateSendServer
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
81e3228 Make CTransaction actually immutable (Pieter Wuille)
42fd8de Make DecodeHexTx return a CMutableTransaction (Pieter Wuille)
c3f5673 Make CWalletTx store a CTransactionRef instead of inheriting (Pieter Wuille)
a188353 Switch GetTransaction to returning a CTransactionRef (Pieter Wuille)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
81e3228 Make CTransaction actually immutable (Pieter Wuille)
42fd8de Make DecodeHexTx return a CMutableTransaction (Pieter Wuille)
c3f5673 Make CWalletTx store a CTransactionRef instead of inheriting (Pieter Wuille)
a188353 Switch GetTransaction to returning a CTransactionRef (Pieter Wuille)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
Instead of deriving from CTransaction, we now have a CTransactionRef member
in CTxLockCandidate. This is needed for the next backported PR bitcoin#8580,
which will make CTransaction immutable.

Also use CTransactionRef in CDarkSendEntry, CDarksendBroadcastTx and
CPrivateSendServer
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
CTransactionRef changes require header to be updated to suit premine disablement validation
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
Issues with bitcoin#8580, bitcoin#9296, and dash commits 4838d42
0d3a429

This must be fixed at a later date
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
Instead of making CTransaction mutable-but-with-cached-hash, 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 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
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Sep 24, 2020
911e31c Make CBlock::vtx a vector of shared_ptr<CTransaction> (furszy)
9d27b75 Add deserializing constructors to CTransaction and CMutableTransaction (Pieter Wuille)
5f90940 Add serialization for unique_ptr and shared_ptr (Pieter Wuille)

Pull request description:

  Inspired on bitcoin#9125 (with many more adaptations), preparation for bitcoin#8580. Ant work 🐜

  Establishing the bases for the work, we will need to continue migrating `CTransaction` to `std::shared_ptr<const CTransaction>` where is possible. Example bitcoin#8126 (can find more in #1726 list).

  TODO:
  * back port final `CTransactionRef` implementation commit.

  EDIT:
  This is now depending on #1816 .

ACKs for top commit:
  random-zebra:
    ACK 911e31c
  Fuzzbawls:
    ACK 911e31c

Tree-SHA512: 61d937aba7dce4ac0867496e7f81ae8351dcdd60b4e72c4f0ed58152a7e556bf455836c766bc97bbca331227e5deed92fa5ce609fa63bb9cb71600b430dc4405
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Feb 12, 2021
4e1f270 Make CTransaction actually immutable (furszy)
896b5e2 Make DecodeHexTx return a CMutableTransaction (Pieter Wuille)
b1e3a38 Switch GetTransaction to returning a CTransactionRef (furszy)

Pull request description:

  Adaptation of bitcoin#8580, making the `CTransaction` object fully immutable. Fixing possible UB over the const members set that should only ever be assigned at construction time.

  first commit was cherry-picked from #2164, that one should get merged first.

ACKs for top commit:
  random-zebra:
    Really nice. ACK 4e1f270.
  Fuzzbawls:
    ACK 4e1f270

Tree-SHA512: 4ade94d55aa1c3abb92a37bc4d5522fec308917e70656bf5c2f2af925b45e73a4928b799e40795546477295af350de4b15b29e0741825123af418cb566bf8331
@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.