Refactor CreateNewBlock to be a method of the BlockAssembler class

morcos commented at 5:19 pm on February 25, 2016: member

This should have no functional changes. I’ll do more extensive testing to make sure it generates the exact same blocks. This refactor will make it easier to add new algorithms for block filling.

jonasschnelli added the label Mining on Feb 25, 2016

morcos force-pushed on Mar 9, 2016

morcos force-pushed on Mar 16, 2016

morcos force-pushed on Apr 1, 2016

TheBlueMatt commented at 11:14 pm on May 11, 2016: member

Needs rebase.

Refactor CreateNewBlock to be a method of the BlockAssembler class 4dc94d1036

morcos force-pushed on May 18, 2016

morcos commented at 6:15 pm on May 18, 2016: member

rebased

in src/miner.cpp: in 4dc94d1036 outdated

75+BlockAssembler::BlockAssembler(const CChainParams& _chainparams)
76+    : chainparams(_chainparams)
77 {
78-    // Create new block
79-    std::unique_ptr<CBlockTemplate> pblocktemplate(new CBlockTemplate());
80+   // Largest block you're willing to create:

TheBlueMatt commented at 1:09 am on May 23, 2016:

Nit: whitespace.

in src/miner.cpp: in 4dc94d1036 outdated

129 
130-    // Largest block you're willing to create:
131-    unsigned int nBlockMaxSize = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
132-    // Limit to between 1K and MAX_BLOCK_SIZE-1K for sanity:
133-    nBlockMaxSize = std::max((unsigned int)1000, std::min((unsigned int)(MAX_BLOCK_SIZE-1000), nBlockMaxSize));
134+    LOCK2(cs_main, mempool.cs);

TheBlueMatt commented at 1:10 am on May 23, 2016:

The old code deliberately unlocks before calling TestBlockValidity…I’m not actually sure its worth anything, but just noting that there is actually a behavior change here.

morcos commented at 8:03 pm on May 24, 2016:

Are you sure?

TheBlueMatt commented at 11:45 pm on May 24, 2016:

Oh, oops, I thought the deliberate scoping had a purpose, but it looks like it does absolutely nothing.

TheBlueMatt commented at 1:13 am on May 23, 2016: member

utACK 4dc94d1036576fe988a066fdabc047350ccddda9 +/- whitespace.

FIX: Account for txs already added to block in addPriorityTxs a278764748

FIX: correctly measure size of priority block c2dd5a3c39

morcos commented at 4:46 pm on June 1, 2016: member

Fix two bugs identified by @sdaftuar

sdaftuar commented at 6:11 pm on June 1, 2016: member

ACK

Verified that the old code and new code produce identical blocks (calling CreateNewBlock every 100 transactions, over two weeks of data from February).

in src/miner.cpp: in c2dd5a3c39

87+    nBlockMinSize = GetArg("-blockminsize", DEFAULT_BLOCK_MIN_SIZE);
88+    nBlockMinSize = std::min(nBlockMaxSize, nBlockMinSize);
89+}
90+
91+void BlockAssembler::resetBlock()
92+{

luke-jr commented at 6:16 pm on June 1, 2016:

This doesn’t seem to reset at least pblock. What’s the point of having a reset independent from the constructor?

laanwj merged this on Jun 13, 2016

laanwj closed this on Jun 13, 2016

laanwj referenced this in commit e1486eb95c on Jun 13, 2016

in src/miner.cpp: in c2dd5a3c39

166+    pblock->vtx[0] = coinbaseTx;
167+    pblocktemplate->vTxFees[0] = -nFees;
168+
169+    // Fill in header
170+    pblock->hashPrevBlock  = pindexPrev->GetBlockHash();
171+    UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);

JeremyRubin commented at 3:19 pm on June 14, 2016:

I think that this is fine to leave as-is, but I suspect that the time should ideally be set by the client (e.g. if the block takes a long time to be found say 70 minutes than the client would have had to update their timestamp in their block).

This could cause a real problem at the halving if there is a sudden reduction in hashpower and mining time goes up past the two hour threshold and all mining clients using this code need to be manually kicked to get a new timestamp. Not setting it at all would cause miners trying to use it to fail always so they would have to be aware of setting it to get anything at all.

If the code is being refactored than this may be an appropriate time to drop it, or at least mark it for future deprecation as I bet someone is relying on it.

JeremyRubin commented at 3:22 pm on June 14, 2016:

(of course this is not a correctness thing, just a potential network edge case if users improperly program against this – I’m well aware this has been the way this code worked for a long time)

sipa commented at 3:27 pm on June 14, 2016:

There is never a requirement to update the time: the constraints are:

block.nTime >= median(block.prev^(n).nTime, n=1..11) [which never changes if no block is found to work on top of]
block.nTime <= realtime + 120 minutes [which never goes from valid to invalid as time passes]

sipa commented at 3:28 pm on June 14, 2016:

In any case, that’s a mining protocol suggestion (ntime rolling is used in many places anyway), not something that affects the internal mining code.

JeremyRubin commented at 3:33 pm on June 14, 2016:

👍

had misread the constraints to have blocks as more time dependent.

in src/miner.cpp: in c2dd5a3c39

472+        double dPriority = mi->GetPriority(nHeight);
473+        CAmount dummy;
474+        mempool.ApplyDeltas(mi->GetTx().GetHash(), dPriority, dummy);
475+        vecPriority.push_back(TxCoinAgePriority(dPriority, mi));
476+    }
477+    std::make_heap(vecPriority.begin(), vecPriority.end(), pricomparer);

JeremyRubin commented at 3:45 pm on June 14, 2016:

nit: Unless there is a reason I am missing, it would be good to refactor this to use std::priority_queue like addScoreTxs for consistency. (std::priority_queue is also safer to use for preserving heap invariants)

in src/miner.cpp: in c2dd5a3c39

283+{
284+    std::priority_queue<CTxMemPool::txiter, std::vector<CTxMemPool::txiter>, ScoreCompare> clearedTxs;
285+    CTxMemPool::setEntries waitSet;
286+    CTxMemPool::indexed_transaction_set::index<mining_score>::type::iterator mi = mempool.mapTx.get<mining_score>().begin();
287+    CTxMemPool::txiter iter;
288+    while (!blockFinished && (mi != mempool.mapTx.get<mining_score>().end() || !clearedTxs.empty()))

JeremyRubin commented at 4:30 pm on June 14, 2016:

nit: could be !(blockFinished || (mi == mempool.mapTx.get<mining_score>().end() && clearedTxs.empty())) via demorgan’s which is (maybe?) a bit easier to read.

JeremyRubin commented at 4:32 pm on June 14, 2016: contributor

utAck – apologies for line noise on the time thing, wasn’t highly related to this.

codablock referenced this in commit 287af63af3 on Sep 16, 2017

codablock referenced this in commit 5571b6eacf on Sep 19, 2017

codablock referenced this in commit 6a698300ab on Dec 22, 2017

andvgal referenced this in commit 650cac81cf on Jan 6, 2019

Fuzzbawls referenced this in commit a88b6ea83f on Feb 3, 2021

DrahtBot locked this on Sep 8, 2021

Refactor CreateNewBlock to be a method of the BlockAssembler class #7598