Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Remove confusing MAX_BLOCK_BASE_SIZE. #10618
Conversation
| @@ -50,7 +50,7 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const { | ||
| ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) { | ||
| if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty())) | ||
| return READ_STATUS_INVALID; | ||
| - if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_BASE_SIZE) | ||
| + if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / (WITNESS_SCALE_FACTOR * MIN_TRANSACTION_BASE_SIZE)) |
sipa
Jun 17, 2017
Owner
Probably easier to rewrite this expression using a separate constant MIN_TRANSACTION_WEIGHT
gmaxwell
referenced
this pull request
Jun 17, 2017
Merged
Add a comment explaining the use of MAX_BLOCK_BASE_SIZE. #10608
| @@ -237,7 +237,7 @@ bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const | ||
| return true; | ||
| } | ||
| -static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_BASE_SIZE / ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION); // TODO: merge with similar definition in undo.h. | ||
| +static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_WEIGHT / (WITNESS_SCALE_FACTOR * ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION)); // TODO: merge with similar definition in undo.h. |
sipa
Jun 17, 2017
Owner
Similarly, this is probably easier to write using a MIN_TRANSACTION_OUTPUT_WEIGHT.
fanquake
added the
Refactoring
label
Jun 18, 2017
|
utACK, but needs rebase. Why isn't WITNESS_SCALE_FACTOR in consensus/consensus.h ? Thank you very much for this, sometimes I've explained that the max size is superflous once you replace it with the max weight, and often people don't believe me and some times they even link to the max size constant as a "proof" that I am wrong. |
|
Needs rebase after #10608 |
The weight calculations are currently in primtives/transaction.h, and I don't think that file should depend on consensus.h. Perhaps all weight based calculations can be moved to consensus/*, though. |
Why not? consensus/consensus.h it's just consensus constants. Anyway, not a big deal, there with MIN_TRANSACTION_WEIGHT isn't bad.
Do you mean moving primitives/* to consensus/ ? I proposed that in the past, I'm all for it, but that's definitely out of scope for this PR. |
|
No, just moving the weight calculations to consensus. Primitives is data structures and serialization, and shouldn't depend on consensus validity rules. |
|
@sipa I did as you requested! I think it makes more sense moved out of primitives, care to review? |
|
Fast re-review ACK cf88f5d |
|
utACK cf88f5d |
|
utACK 3babbcb |
|
Would be nice for 15, given the level of apparent confusion that still persists about this. |
|
utACK 3babbcb |
|
utACK 3babbcb |
sipa
merged commit 3babbcb
into
bitcoin:master
Jul 15, 2017
1 check passed
sipa
added a commit
that referenced
this pull request
Jul 15, 2017
|
|
sipa |
This commit was signed with a verified signature.
sipa
Pieter Wuille
|
f90603a
|
gmaxwell commentedJun 17, 2017
Some people keep thinking that MAX_BLOCK_BASE_SIZE is a separate
size limit from the weight limit when it fact it is superfluous,
and used in early tests before the witness data has been
validated or just to compute worst case sizes. The size checks
that use it would not behave any differently consensus wise
if they were eliminated completely.
Its correct value is not independently settable but is a function
of the weight limit and weight formula.
This patch just eliminates it and uses the scale factor as
required to compute the worse case constants. It's an alternative
to another PR that adds a long comment that seemingly was still
not sufficient.