Refactor CreateNewBlock to be a method of the BlockAssembler class #7598
Conversation
morcos
force-pushed
the
BlockAssembler
branch
2 times, most recently
from
edd6b83 to
bdc1fd5
Compare
|
Needs rebase. |
|
rebased |
src/miner.cpp
Outdated
| { | ||
| // Create new block | ||
| std::unique_ptr<CBlockTemplate> pblocktemplate(new CBlockTemplate()); | ||
| // Largest block you're willing to create: |
|
utACK 4dc94d1 +/- whitespace. |
|
Fix two bugs identified by @sdaftuar |
|
ACK Verified that the old code and new code produce identical blocks (calling CreateNewBlock every 100 transactions, over two weeks of data from February). |
| } | ||
|
|
||
| void BlockAssembler::resetBlock() | ||
| { |
There was a problem hiding this comment.
This doesn't seem to reset at least pblock. What's the point of having a reset independent from the constructor?
|
|
||
| // Fill in header | ||
| pblock->hashPrevBlock = pindexPrev->GetBlockHash(); | ||
| UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍
had misread the constraints to have blocks as more time dependent.
|
utAck -- apologies for line noise on the time thing, wasn't highly related to this. |
…est coverage 0d55bcf blockassembler: do not lock cs_main and mempool cs for the entire block, only where are needed. (furszy) 70e6430 miner_tests fix locking issues, cs_main and mempool.cs cannot (and don't need) to be locked for the entire test. Only in the places that the locks are really needed. (furszy) c4e3754 [test] functional miner_test, reworked and updated it to make it fully work with our blockchain and consensus rules. (furszy) 1cd3943 [Miner] New blockassembler introduction and connection with the sources. (furszy) Pull request description: Have rewritten the miner code sources, dividing the miner thread from the block assembling process, encapsulating the block creation state inside a new `BlockAssembler` class (adapting bitcoin#7598). Cleaned several mempool redundant checks inside the assembly process (adaptation of dashpay#6898): > The mempool is explicitly assumed to be responsible for maintaining consistency of transactions with respect to not spending non-existent outputs, not double spending, script validity and coinbase maturity. Only finality of transactions is checked before assembling a block. Another point added here is the rework of the previously non-functional `miner_tests` unit test. Which.. have essentially made it work, adding coverage for the miner process, updating it to a fairly recent point down upstream's path + adapting it to our blockchain and consensus rules. The result is a large speed up in the block assembly process, a much better and cleaner code architecture and enable an easier add of new algorithms for block filling in the future. ------------ A good work path on top of this would be to add unit test coverage for PoS, cold staking and Sapling block creation (among others). At the moment, most of the block creation tests are functional ones which are slow and require to setup entire nodes etc. ACKs for top commit: random-zebra: utACK 0d55bcf after rebase Fuzzbawls: ACK 0d55bcf Tree-SHA512: ab0ff3832bceeb28233871a64ee9593cee695843396b95f9471485bf1d9e684614db783342252bcfb49cb3df40c444697d24e3dab1f8a5226b2478bc191822d2
8 participants
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.