Skip to content

core: Use same RNG algorithm as avmplus #21938

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

core: Use same RNG algorithm as avmplus #21938

wants to merge 3 commits into from

Conversation

@zjk261
Copy link

@zjk261 zjk261 commented Oct 16, 2025 •
edited
Loading

I didn't really understand how rng function is called inside avmplus, but this does fix RandomnessTest.swf from #20244, with some multiply opcodes replaced with multiply_i
RandomnessTest.zip

@zjk261 zjk261 marked this pull request as draft October 16, 2025 05:25
torokati44
torokati44 reviewed Oct 16, 2025
View reviewed changes
@zjk261 zjk261 marked this pull request as ready for review October 16, 2025 07:24
@danielhjacobs danielhjacobs added waiting-on-review Waiting on review from a Ruffle team member A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Oct 16, 2025
@kjarosh kjarosh added the squash-on-merge Marks a PR to be squashed before merging. label Oct 16, 2025
@torokati44
Copy link
Member

with some multiply opcodes replaced with multiply_i

Could you please elaborate on that?

I feel like #21907 could be related...

@zjk261
Copy link
Author

zjk261 commented Oct 17, 2025

#20244 (comment)
the original swf still does not produce 2137 with this
so I replaced some multiply opcode with multiply_i with ffdec according to this comment
I know very little about this, sorry

@Randomno
Copy link

Randomno commented Oct 17, 2025 •
edited
Loading

The test requires integer multiplication, and Flash's AVM2 optimizer does this integer multiplication even with the regular multiply opcode, while Ruffle doesn't.

https://docs.google.com/presentation/d/1nUp89sZnQAtvsXFkzxyKdsdzYWsZ2OzV2BhlzTeJVsc/edit?slide=id.g2f09243411c_0_10#slide=id.g2f09243411c_0_10

@torokati44
Copy link
Member

Also, I think the small_rng feature of rand can be dropped now. Unfortunately, not the entire crate, as flash.crypto.generateRandomBytes uses os_rng and std.

Lord-McSweeney
Lord-McSweeney reviewed Oct 17, 2025
View reviewed changes
Lord-McSweeney
Lord-McSweeney approved these changes Oct 17, 2025
View reviewed changes
Copy link
Collaborator

@Lord-McSweeney Lord-McSweeney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the SWF in the issue as a test?

@Lord-McSweeney Lord-McSweeney added waiting-on-author Waiting on the PR author to make the requested changes and removed waiting-on-review Waiting on review from a Ruffle team member labels Oct 17, 2025
@zjk261
Copy link
Author

zjk261 commented Oct 18, 2025

@Randomno can I use modified SWF file as test?

@Randomno
Copy link

Koong (the author of it) says yes

@zjk261 zjk261 force-pushed the avmrng branch 3 times, most recently from 4cb20bf to a14655d Compare October 20, 2025 01:49
@zjk261 zjk261 requested a review from Lord-McSweeney October 21, 2025 02:09
@zjk261 zjk261 force-pushed the avmrng branch 2 times, most recently from c9d1786 to d1f1c98 Compare October 21, 2025 04:58
Lord-McSweeney
Lord-McSweeney reviewed Oct 21, 2025
View reviewed changes
tests/tests/swfs/avm2/rng/output.txt Outdated
@@ -0,0 +1,3 @@
743483584
5402
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these results consistent for you in Flash Player? For me, the first two numbers outputted are random with each run.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not consistent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case can you modify the SWF to only output the last number? For this test I think we'd want the output to be reproducible in Flash Player.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this? Removed most of the extra stuff, just traces true if the two numbers match.

However, it sometimes fails in Flash (as did the original version of the test). I think there's either an error in the algorithm, or Flash is executing stuff weirdly. Maybe should test with a pause between each step.

I tend to get 80% success rate in Flash Player 32.

test.zip

Copy link

@koonggames koonggames Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the consistency issue, it sometimes did not work because a double was not converted to a uint and when the polynomial used in the rng got too large values didn't match up.

rngtestruffle.zip

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, in 1000 attempts I got 7 failures. Target, probe, doublecheck:

7043, 1970344600, 266490418
653, 310941477, 1794949550
6349, 753898449, 1488657303
8564, 749768811, 1196820749
2950, 240060092, 1191158361
1542, 1440380734, 774071793
7838, 1007439541, 429495897

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe triplecheck is needed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell the reversing itself is flawless now, when it breaks it seems to be during the rng calling part. No clue why, I can look into it tomorrow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main.zip
I think this should always work? I tested 1000 times and did not fail

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lord-McSweeney I think it should be consistent now?

Lord-McSweeney
Lord-McSweeney reviewed Oct 21, 2025
View reviewed changes
@zjk261 zjk261 requested a review from Lord-McSweeney October 22, 2025 14:52
@Lord-McSweeney Lord-McSweeney removed the waiting-on-author Waiting on the PR author to make the requested changes label Oct 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@torokati44 torokati44 torokati44 left review comments

@Lord-McSweeney Lord-McSweeney Lord-McSweeney approved these changes

+2 more reviewers

@Randomno Randomno Randomno left review comments

@koonggames koonggames koonggames left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-core Area: Core player, where no other category fits squash-on-merge Marks a PR to be squashed before merging. T-fix Type: Bug fix (in something that's supposed to work already)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@zjk261 @torokati44 @Randomno @koonggames @Lord-McSweeney @kjarosh @danielhjacobs