-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EIP-2537: MSM gas repricing #9116
Conversation
✅ All reviewers have approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is effectively 2x scaling for G2MSM and 2.5x scaling for G1MSM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If discount tables are split between G1 and G2 they don't make sense any more. Just put the total cost in the table...
+1. Relevant besu benchmarks for msm: |
Do you mean precompute the gas costs for each number of pairs and put that in a table? I think keeping the discount table is cleaner as we couldn't put all pair counts >128 in a table, and also we want to keep |
EIPS/eip-2537.md
Outdated
Discounts table for G1 MSM as a vector of pairs `[k, discount]`: | ||
|
||
``` | ||
[[1, 1800], [2, 888], [3, 955], [4, 802], [5, 743], [6, 821], [7, 750], [8, 680], [9, 657], [10, 635], [11, 612], [12, 591], [13, 664], [14, 637], [15, 611], [16, 585], [17, 578], [18, 571], [19, 564], [20, 557], [21, 628], [22, 620], [23, 612], [24, 604], [25, 596], [26, 588], [27, 578], [28, 570], [29, 562], [30, 693], [31, 683], [32, 673], [33, 670], [34, 665], [35, 663], [36, 658], [37, 655], [38, 650], [39, 648], [40, 643], [41, 640], [42, 635], [43, 633], [44, 628], [45, 625], [46, 620], [47, 618], [48, 613], [49, 610], [50, 605], [51, 603], [52, 598], [53, 595], [54, 590], [55, 588], [56, 583], [57, 580], [58, 578], [59, 573], [60, 570], [61, 565], [62, 563], [63, 558], [64, 555], [65, 553], [66, 550], [67, 548], [68, 548], [69, 545], [70, 543], [71, 540], [72, 540], [73, 538], [74, 535], [75, 533], [76, 533], [77, 530], [78, 528], [79, 528], [80, 525], [81, 523], [82, 520], [83, 520], [84, 518], [85, 515], [86, 513], [87, 513], [88, 510], [89, 508], [90, 505], [91, 505], [92, 503], [93, 500], [94, 498], [95, 498], [96, 495], [97, 493], [98, 490], [99, 490], [100, 488], [101, 485], [102, 483], [103, 483], [104, 480], [105, 478], [106, 478], [107, 475], [108, 473], [109, 470], [110, 470], [111, 468], [112, 465], [113, 463], [114, 463], [115, 460], [116, 458], [117, 455], [118, 455], [119, 453], [120, 450], [121, 448], [122, 448], [123, 445], [124, 443], [125, 440], [126, 440], [127, 438], [128, 435]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For k=1
we should put 1000 (the same cost as multiplication). Otherwise, we incentivize contracts to put a condition to redirect inputs from MSM to MUL in case they have single point.
In other words, if you have single point to multiply, you should not use any MSM algorithm. Just use regular multiplication in your MSM precompile implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are not monotonic, so you have to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the terminal value k=128
is 520
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are not monotonic, so you have to fix this.
missed that. they should definitely be monotonic, regardless of the performance profile for any particular k,
This I don't agree with. We started with a table created 4 years ago that tries to estimate some fucked up curve of gas costs. And now we try to find a constant N that will somehow fix it, but in the end we get a curve fucked up N times. But I don't care some much any more. If people prefer to operate on permilles instead of absolute values let it be. |
Also, at least for Gnark's implementation of MSM, mul isn't even used as a component. So it's very strange that it should be a factor in the cost model. |
I don't know the original rationale for using MUL, however, one reasoning is that MSM is tied to cost of ADD and there's a relationship between the cost of ADD and cost of a MUL |
My proposed "discounts" for G1MSM [[1, 1000], [2, 949], [3, 848], [4, 797], [5, 764], [6, 750], [7, 738], [8, 728], [9, 719], [10, 712], [11, 705], [12, 698], [13, 692], [14, 687], [15, 682], [16, 677], [17, 673], [18, 669], [19, 665], [20, 661], [21, 658], [22, 654], [23, 651], [24, 648], [25, 645], [26, 642], [27, 640], [28, 637], [29, 635], [30, 632], [31, 630], [32, 627], [33, 625], [34, 623], [35, 621], [36, 619], [37, 617], [38, 615], [39, 613], [40, 611], [41, 609], [42, 608], [43, 606], [44, 604], [45, 603], [46, 601], [47, 599], [48, 598], [49, 596], [50, 595], [51, 593], [52, 592], [53, 591], [54, 589], [55, 588], [56, 586], [57, 585], [58, 584], [59, 582], [60, 581], [61, 580], [62, 579], [63, 577], [64, 576], [65, 575], [66, 574], [67, 573], [68, 572], [69, 570], [70, 569], [71, 568], [72, 567], [73, 566], [74, 565], [75, 564], [76, 563], [77, 562], [78, 561], [79, 560], [80, 559], [81, 558], [82, 557], [83, 556], [84, 555], [85, 554], [86, 553], [87, 552], [88, 551], [89, 550], [90, 549], [91, 548], [92, 547], [93, 547], [94, 546], [95, 545], [96, 544], [97, 543], [98, 542], [99, 541], [100, 540], [101, 540], [102, 539], [103, 538], [104, 537], [105, 536], [106, 536], [107, 535], [108, 534], [109, 533], [110, 532], [111, 532], [112, 531], [113, 530], [114, 529], [115, 528], [116, 528], [117, 527], [118, 526], [119, 525], [120, 525], [121, 524], [122, 523], [123, 522], [124, 522], [125, 521], [126, 520], [127, 520], [128, 519]] I plotted the values: original, proposed here and proposed by me (evmone) for visualization (without value for 1 where I argue it should be 1000). |
My proposed "discounts" for G2MSM, based on benchmarking evmone/BLST (the same way as G1MSM). [[1, 1000], [2, 1000], [3, 923], [4, 884], [5, 855], [6, 832], [7, 812], [8, 796], [9, 782], [10, 770], [11, 759], [12, 749], [13, 740], [14, 732], [15, 724], [16, 717], [17, 711], [18, 704], [19, 699], [20, 693], [21, 688], [22, 683], [23, 679], [24, 674], [25, 670], [26, 666], [27, 663], [28, 659], [29, 655], [30, 652], [31, 649], [32, 646], [33, 643], [34, 640], [35, 637], [36, 634], [37, 632], [38, 629], [39, 627], [40, 624], [41, 622], [42, 620], [43, 618], [44, 615], [45, 613], [46, 611], [47, 609], [48, 607], [49, 606], [50, 604], [51, 602], [52, 600], [53, 598], [54, 597], [55, 595], [56, 593], [57, 592], [58, 590], [59, 589], [60, 587], [61, 586], [62, 584], [63, 583], [64, 582], [65, 580], [66, 579], [67, 578], [68, 576], [69, 575], [70, 574], [71, 573], [72, 571], [73, 570], [74, 569], [75, 568], [76, 567], [77, 566], [78, 565], [79, 563], [80, 562], [81, 561], [82, 560], [83, 559], [84, 558], [85, 557], [86, 556], [87, 555], [88, 554], [89, 553], [90, 552], [91, 552], [92, 551], [93, 550], [94, 549], [95, 548], [96, 547], [97, 546], [98, 545], [99, 545], [100, 544], [101, 543], [102, 542], [103, 541], [104, 541], [105, 540], [106, 539], [107, 538], [108, 537], [109, 537], [110, 536], [111, 535], [112, 535], [113, 534], [114, 533], [115, 532], [116, 532], [117, 531], [118, 530], [119, 530], [120, 529], [121, 528], [122, 528], [123, 527], [124, 526], [125, 526], [126, 525], [127, 524], [128, 524]] The table proposed in this PR suggests there is something seriously broken in the implementation benchamarked or in the benchmarks themselves. Because the values suggests that up to 45 points there is no benefit of using MSM over naive chain of MULs (ignoring ADD cost which is only ~2.5% overhead). This means we can't ship this EIP with such implementation. Classing plot for visualization. |
Ok then, I still think it's easier because as I say you would still need to use a similar formula to calculate for values >128 pairs unless we had a massive table. The new discount table looks good to me. |
Here are my benchmarks on Cosntantine with a Ryzen 7840U (laptop 15~28W, 2023) with the current state of discount table.
table change: https://github.com/mratsim/constantine/compare/eip2537-repricing Before
After
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
* chore(tests): pairing ops gas pricing cf ethereum/EIPs#9098 * chore(tests): MAP, MUL & ADD gas pricing cf ethereum/EIPs#9097 * chore(tests): update G1/add G2 msm discount tables ethereum/EIPs#9116
https://github.com/ethereum/EIPs/pull/9097/files https://github.com/ethereum/EIPs/pull/9098/files https://github.com/ethereum/EIPs/pull/9116/files Signed-off-by: garyschulte <garyschulte@gmail.com> adjust unit test gas costs, fix offset-by-one bug in the discount table Signed-off-by: garyschulte <garyschulte@gmail.com> implement bump in gas cost for bls map functions according to ethereum/EIPs@92c94cf Signed-off-by: garyschulte <garyschulte@gmail.com> using Pawel's suggested discount table from ethereum/EIPs#9116 (comment) Signed-off-by: garyschulte <garyschulte@gmail.com> use bls pairing costs from https://github.com/ethereum/EIPs/pull/9098/files Signed-off-by: garyschulte <garyschulte@gmail.com> remove MUL per ethereum/EIPs#8945 Signed-off-by: garyschulte <garyschulte@gmail.com> fix g1 msm max discount case, add g2 msm max discount case Signed-off-by: garyschulte <garyschulte@gmail.com> remove bls mul ops from benchmark subcommand Signed-off-by: garyschulte <garyschulte@gmail.com> use besu-native 1.1.1 Signed-off-by: garyschulte <garyschulte@gmail.com>
Refer to the following: ethereum/EIPs#9116 ethereum/EIPs#9098 ethereum/EIPs#9097 ethereum/EIPs#8945 Issue board: #12401 --------- Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
Refer to the following: ethereum/EIPs#9116 ethereum/EIPs#9098 ethereum/EIPs#9097 ethereum/EIPs#8945 Issue board: #12401 --------- Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
* eip2537: repricing based on ethereum/EIPs#9097 and ethereum/EIPs#9116 * EVM precompile: repricing pairing based on ethereum/EIPs#9098
…13469) Refer to the following: ethereum/EIPs#9116 ethereum/EIPs#9098 ethereum/EIPs#9097 ethereum/EIPs#8945 Issue board: #12401 Cherry pick #13346 --------- Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
This PR makes granular changes to the discount table for MSM, giving G1 and G2 their own discount tables. The following changes are made:
G1:
Also 2.5x for max discount.
G2:
Also 4x for max discount.
Note that this also includes a change to G2 mul which affects G2 MSM price (see #9097).
This gives the following results for Nethermind on a NUC (aiming for ~80MGas/s to match Ecrecover):