Remove confusing MAX_BLOCK_BASE

gmaxwell commented at 0:24 am on June 17, 2017: contributor

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.

in src/blockencodings.cpp:53 in f1cc730e35 outdated

49@@ -50,7 +50,7 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const {
50 ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) {
51     if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
52         return READ_STATUS_INVALID;
53-    if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_BASE_SIZE)
54+    if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / (WITNESS_SCALE_FACTOR * MIN_TRANSACTION_BASE_SIZE))

sipa commented at 0:25 am on June 17, 2017:

Probably easier to rewrite this expression using a separate constant MIN_TRANSACTION_WEIGHT

in src/coins.cpp:240 in f1cc730e35 outdated

236@@ -237,7 +237,7 @@ bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const
237     return true;
238 }
239 
240-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.
241+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 commented at 0:40 am on June 17, 2017:

Similarly, this is probably easier to write using a MIN_TRANSACTION_OUTPUT_WEIGHT.

fanquake added the label Refactoring on Jun 18, 2017

jtimon commented at 5:35 am on June 25, 2017: contributor

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.

laanwj commented at 8:27 am on June 25, 2017: member

Needs rebase after #10608

sipa commented at 7:28 pm on June 25, 2017: member

Why isn’t WITNESS_SCALE_FACTOR in consensus/consensus.h ?

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.

jtimon commented at 8:55 pm on June 25, 2017: contributor

The weight calculations are currently in primtives/transaction.h, and I don’t think that file should depend on consensus.h.

Why not? consensus/consensus.h it’s just consensus constants. Anyway, not a big deal, there with MIN_TRANSACTION_WEIGHT isn’t bad.

Perhaps all weight based calculations can be moved to consensus/*, though.

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.

sipa commented at 9:02 pm on June 25, 2017: member

No, just moving the weight calculations to consensus. Primitives is data structures and serialization, and shouldn’t depend on consensus validity rules.

gmaxwell commented at 1:29 am on July 13, 2017: contributor

@sipa I did as you requested! I think it makes more sense moved out of primitives, care to review?

jtimon commented at 2:04 am on July 13, 2017: contributor

Fast re-review ACK cf88f5d

sipa commented at 8:30 am on July 13, 2017: member

utACK cf88f5dc43e999e87961b667579912ec4956f63f

Remove confusing MAX_BLOCK_BASE_SIZE.

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 also moves the weight factor out of primitives into consensus,
 which is a more logical place for it.

3babbcb487

TheBlueMatt commented at 7:42 pm on July 14, 2017: member

utACK 3babbcb48786372d4b22171674c4cc5a6220c294

TheBlueMatt commented at 7:43 pm on July 14, 2017: member

Would be nice for 15, given the level of apparent confusion that still persists about this.

sipa commented at 11:55 pm on July 14, 2017: member

utACK 3babbcb48786372d4b22171674c4cc5a6220c294

achow101 commented at 0:19 am on July 15, 2017: member

utACK 3babbcb48786372d4b22171674c4cc5a6220c294

sipa merged this on Jul 15, 2017

sipa closed this on Jul 15, 2017

sipa referenced this in commit f90603ac6d on Jul 15, 2017

theStack referenced this in commit a00f02e2f2 on Jul 1, 2021

theStack referenced this in commit e26ad3b39c on Jul 1, 2021

theStack referenced this in commit 607076d01b on Jul 3, 2021

MarcoFalke referenced this in commit b620b2d58a on Aug 2, 2021

DrahtBot locked this on Sep 8, 2021

Remove confusing MAX_BLOCK_BASE_SIZE. #10618