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

[Miner] Unifying the disperse coinbase tx flow + further clean up. #1816

Merged

Conversation

furszy
Copy link

@furszy furszy commented Aug 24, 2020

Another step forward improving the miner CreateNewBlock function.
This time, have unified the coinbase tx creation flow that was previously dispersed over the function and having some redundant initializations/sets.
Plus, cleaned up some unused and/or unneeded fields and function arguments.

This is a preparation for #1815 work. It will not be possible to modify any of the elements of a transaction once them are appended to a block, thus why the transaction needs (more than ever) to be crafted in one single place and not all over the sources.

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 about missing BIP34 on PoS blocks.
Otherwise looking pretty good.

src/miner.cpp Show resolved Hide resolved
src/miner.cpp Outdated Show resolved Hide resolved
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 063401c

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 063401c and merging...

@random-zebra random-zebra merged commit 0fa40d7 into PIVX-Project:master Sep 6, 2020
furszy added a commit that referenced this pull request Sep 8, 2020
261bd22 Staking process: calculate the stakeable utxo only once. (furszy)

Pull request description:

  Work on top of #1816 and part of #1817 coinstake transaction creation speedup goal. Starting on 01814d5

  Improving the following situation: we are calculating the stakeable utxo twice for the same PoS block creation process (looping over the entire wallet's transactions map twice), one before calling `CreateNewBlock` in `CheckForCoins` and the second one inside the `CreateNewBlock` method when we call `CWallet::CreateCoinStake`.
  This PR fixes it adding an unique available coins vector calculated before the block creation, only once per try, in the `CheckForCoins` function and feeding `CreateNewBlock` with it.

ACKs for top commit:
  random-zebra:
    utACK 261bd22

Tree-SHA512: f553667fd48d0d7eb78e2ce87b438c915dc216b32e0426e0214caa52f16a2627143319233c6dd93e4b5d45dcfbb825787c1b39cd209c8e6516bf2391f0516e00
@random-zebra random-zebra added Block Generation Mining/Staking related issues Refactoring labels Sep 10, 2020
@random-zebra random-zebra added this to the 4.3.0 milestone Sep 10, 2020
furszy added a commit 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 furszy deleted the 2020_miner_coinbaseTx_cleanup branch November 29, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Generation Mining/Staking related issues Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants