Alternative to #8388 without the code changes. Since the only difference is adding numbers, there shouldn’t be any noticable performance hit with blockmaxsize anyway.
(If we want to eliminate any difference just in case, another code change possibility would be to simply translate blockmaxsize to blockmaxweight when segwit is not enabled (ie, mainnet): 29000050e575a26b7f7c853cbccdb6bc60ddfdd9.)
@luke-jr The advice given is something that’s inevitably going to happen by adopting segwit, and you can’t expect miners to decide to cut in their own flesh in order to making competition easier.
I am fine with not giving advice either way. I prefer to still mention the performance effect, though.
If miners start producing way-too-large blocks immediately when segwit activates, that would be cutting their own flesh.
There is no evidence of a performance effect, and fixing any trivial difference is so simple here (3 easily-reviewed LOC: 29000050e575a26b7f7c853cbccdb6bc60ddfdd9) that it would be very poor judgement to prefer giving bad advice rather than just fixing it.
NACK these changes
It’s important to explain the changes so that miners can make educated decisions on how to use the code. blockmaxweight is now the consensus enforced limit and I think its pretty relevant to explain to miners that using that is the most direct way to create blocks that maximize fees while being consensus compatible. Although it is true in this version you could achieve the same effect using blockmaxsize, you’ll be doing extra computation for no reason.
In the end, I think its most important to provide explanation about what the functionality and command line option changes are and the prior version does a better job at that than this.
NACK removal of mentioning the potential performance effect from using -blockmaxsize.
In rewriting CreateNewBlock to do ancestor feerate mining, an important consideration was being able to fail fast if a package wouldn’t fit in a block, due to size or sigops. In that situation, we are able to skip the call to CalculateMemPoolAncestors. This makes it pretty cheap to iterate the whole mempool looking for things to add, even when we’re very close to the size (or sigops) limit. (The old addScoreTx’s had a heuristic for bailing out early from mempool looping when it got close to the size limit, I believe because it had to do more expensive calculations to figure out if a transaction could be added to a block.)
However, the -blockmaxsize constraint cannot currently be calculated at the package level – we can only test for that constraint after calculating the ancestors of a transaction to be added, and considering each one in turn. Consequently, if someone were to configure the mining code with a -blockmaxsize that is less than -blockmaxweight/4, then we should expect a very inefficient result in CreateNewBlock, because once we’re very close to the -blockmaxsize constraint but still have room on the weight constraint, we’d be walking everything in the mempool and calculating ancestors for each transaction, and then walking those ancestors, just to find that the package fails as we count each transaction’s bytes. With a default chain limit length of 25, I think as a first guess, in the worst case I’d assume that such a configuration could slow down the transaction selection part of CreateNewBlock by roughly a factor of 25.
So to be clear, it is incorrect to say that the only difference between using -blockmaxsize and -blockmaxweight is “adding numbers”. These options can interact badly, and it’s important to disclose that potential performance degradation if misused. I think the existing release notes language does an adequate job of neutrally explaining the issue. I would not object to language changes that further clarify how these parameters interact, but I strongly object to withholding material information about how the software works.
