Remove confusing MAX_BLOCK_BASE_SIZE. #10618
Conversation
src/blockencodings.cpp
Outdated
| @@ -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)) | |||
There was a problem hiding this comment.
Probably easier to rewrite this expression using a separate constant MIN_TRANSACTION_WEIGHT
src/coins.cpp
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Similarly, this is probably easier to write using a MIN_TRANSACTION_OUTPUT_WEIGHT.
|
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 cf88f5dc43e999e87961b667579912ec4956f63f |
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.
|
utACK 3babbcb |
|
Would be nice for 15, given the level of apparent confusion that still persists about this. |
|
utACK 3babbcb |
1 similar comment
|
utACK 3babbcb |
3babbcb Remove confusing MAX_BLOCK_BASE_SIZE. (Gregory Maxwell) Tree-SHA512: 361293fc4e1e379cd5a0908ed0866a00e1c7a771bdf02fded158fca21b492a29c7a67fea0d13dc40b2a04204c89823bf1836fe5b63a17c9747751b9c845a3527
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the core implementation years ago due to being confusing and superfluous, as it is implied by the block weight limit (see PRs bitcoin#10618 and bitcoin#10608). Since there is also no point in still keeping it in the functional test framework, we switch to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks.
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the core implementation years ago due to being confusing and superfluous, as it is implied by the block weight limit (see PRs bitcoin#10618 and bitcoin#10608). Since there is also no point in still keeping it in the functional test framework, we switch to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks.
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the core implementation years ago due to being confusing and superfluous, as it is implied by the block weight limit (see PRs bitcoin#10618 and bitcoin#10608). Since there is also no point in still keeping it in the functional test framework, we switch to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks.
607076d test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner) 4af97c7 test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner) a084ebe test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner) Pull request description: This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous). Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively. ACKs for top commit: MarcoFalke: review ACK 607076d 🚴 Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
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.