doc: replace remaining “520” magic nums with MAX_SCRIPT_ELEMENT_SIZE #30024

1. 
   jonatack commented at 5:35 pm on May 2, 2024: contributor

   Noticed these while reviewing BIPs yesterday.

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

2. 
   DrahtBot commented at 5:35 pm on May 2, 2024: contributor

   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.

3. DrahtBot added the label Docs on May 2, 2024
4. 
   instagibbs commented at 5:53 pm on May 2, 2024: member

   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.
5. Replace remaining "520" magic numbers with MAX_SCRIPT_ELEMENT_SIZE ffc674595c
6. jonatack force-pushed on May 2, 2024
7. 
   jonatack commented at 7:18 pm on May 2, 2024: contributor

   maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else?

   Thanks @instagibbs, done.

8. 
   instagibbs commented at 7:21 pm on May 2, 2024: member

   ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
9. MarnixCroes approved
10. 
    MarnixCroes commented at 3:33 am on May 3, 2024: contributor

    lgtm ffc674595cb19b2fdc5705b355bdd3e7f724b860
11. 
    GregTonoski commented at 10:00 am on May 3, 2024: none

    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

12. 
    glozow commented at 10:27 am on May 3, 2024: member

    ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860, agree it’s clearer for these comments to refer to the greppable name of the limit rather than the number
13. 
    GregTonoski commented at 1:07 pm on May 3, 2024: none

    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)?
14. 
    instagibbs commented at 1:43 pm on May 3, 2024: member

    @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

15. 
    GregTonoski commented at 1:51 pm on May 3, 2024: none

    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?

16. 
    instagibbs commented at 1:54 pm on May 3, 2024: member

    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.

17. 
    sipa commented at 1:58 pm on May 3, 2024: member

    ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860

    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.

18. 
    jonatack commented at 2:53 pm on May 3, 2024: contributor

    Thanks for the interesting links! @GregTonoski it turns out that this patch is just a continuation of commit 192cc910ec7cade1d0dce7f3b111e7fc7720e607 doing the same thing, per review #2188 (review) 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.

19. 
    achow101 commented at 4:36 pm on May 3, 2024: member

    ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
20. achow101 merged this on May 3, 2024
21. achow101 closed this on May 3, 2024

    ---

22. jonatack deleted the branch on May 3, 2024

Contributors
jonatack DrahtBot instagibbs MarnixCroes GregTonoski glozow sipa achow101

Labels
Docs