doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE

[jonatack]

Noticed these while reviewing BIPs yesterday.

It would be clearer and more future-proof to refer to their constant name.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, glozow, sipa, achow101
Concept NACK	GregTonoski

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28307 (rpc, wallet: fix incorrect segwit redeem script size limit by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[instagibbs]

maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else? concept ACK otherwise. I can confirm they're all referring to the same limit at least.

[jonatack]

maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else?

Thanks @instagibbs, done.

[instagibbs]

ACK ffc6745

[MarnixCroes]

lgtm ffc6745

[GregTonoski]

NACK unless clearly explained (reference to specification like BIP or relevant disucssion would be highly appreciated). Why not fixing the policy standard rules so that "Nodes must NEVER send a data item > 520 bytes"?

See also: https://bitcoin.stackexchange.com/questions/38937/what-was-the-original-rationale-for-limiting-the-maximum-push-size

[glozow]

ACK ffc6745, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number

[GregTonoski]

What is the meaning and the purpose of the both the "520" and the "MAX_SCRIPT_ELEMENT_SIZE"? In particular, is the "520" and "MAX_SCRIPT_ELEMENT_SIZE" meant to describe limit of size of any element in order to disregard transactions that would have overused resources (similarily to MAX_SCRIPT_SIZE albeit on a level deeper)?

[instagibbs]

@GregTonoski MAX_SCRIPT_ELEMENT_SIZE is a constant that describes a consensus rule that has been around since satoshi-era where nothing can be pushed to the stack that is larger than 520 bytes. There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.

Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.

reasoning motivation(which doesn't effect this PR): https://bitcoin.stackexchange.com/questions/38937/what-was-the-original-rationale-for-limiting-the-maximum-push-size

[GregTonoski]

There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.

Are they "knock-on effects" or are the effects intentional?

Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.

Is the suggested name the best option? Why not another constant with another name, perhaps?

Why weren't the constants combined in Satoshi era and later?

Is the use case of OP_PUSHDATA4 the only one affected by the constant?

[instagibbs]

Are they "knock-on effects" or are the effects intentional?

Doesn't matter for this PR, frankly. We need to be descriptive about the consensus bits in the code. I linked some historical background for your edification.

Is the suggested name the best option? Why not another constant with another name, perhaps?

Probaby, because it's the constant that has been used for over a decade to describe a consensus-critical constant.

[sipa]

ACK ffc6745

Is the suggested name the best option? Why not another constant with another name, perhaps?

The constant with this exact name and value already exists, it was introduced 11 years ago. There are just a few places in comments where the raw value 520 is used instead so far. This PR fixes that.

[jonatack]

Thanks for the interesting links!

@GregTonoski it turns out that this patch is just a continuation of commit 192cc91 doing the same thing, per review #2188 (comment) requesting this, both back in 2013.

The idea is to clarify that these hardcoded numbers all refer to the same thing and to reduce potential head-scratching by future readers of this code.

[achow101]

ACK ffc6745