Skip to content
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

feat: tightly pack public logs inside blobs #11752

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

MirandaWood
Copy link
Contributor

@MirandaWood MirandaWood commented Feb 5, 2025

This PR removes trailing zeroes from logs appended to the blobs to save on field space.
e.g. before this PR, a public log would be appended like:

[1, 2, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

but now is appended as:

[1, 2, 3]

with a length prefix.
In ts, we add back the trailing zeroes when constructing tx effects from the blob, so if they are required there should be no issue.
Also, in some logs there are valid zeroes inside the log. The method does not remove these. e.g. a public log like:

[1, 0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

is appended as:

[1, 0, 3]

then reconstructed to the original log once in ts.

@MirandaWood MirandaWood self-assigned this Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Changes to circuit sizes

Generated at commit: b8b57145ce94267ffae553428aa3618e9cd67b7b, compared to commit: 6c9305897c9c333791d333d332cafa352f9bbe58

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_private +72 ❌ +0.05% +267 ❌ +0.02%
rollup_base_public -21,376 ✅ -24.83% -73,721 ✅ -6.30%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_private 137,902 (+72) +0.05% 1,637,626 (+267) +0.02%
rollup_base_public 64,716 (-21,376) -24.83% 1,095,840 (-73,721) -6.30%

@MirandaWood MirandaWood requested a review from LeilaWang February 5, 2025 16:59
Copy link
Collaborator

@LeilaWang LeilaWang left a comment

Choose a reason for hiding this comment

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

As discussed, private logs are always padded to the fixed length, so there won't be trailing 0s.

// As above, but counts from the end of the array.
// Useful for finding trailing zeroes in arrays which may have valid empty values.
// e.g. removing trailing 0s from [1, 0, 2, 0, 0, 0] -> [1, 0, 2]
// TODO: can be removed if logs cannot contain valid 0s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove this TODO (and the one below) as logs will always contain valid 0s!

T: Empty + Eq,
{
/// Safety: this value is constrained in the below loop
let length = unsafe { find_index_hint_from_end(array, |elem: T| is_empty(elem)) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be find_index_hint_from_end(array, |elem: T| !is_empty(elem))
Think it's more intuitive, and matches other libraries' similar functions, to find the index of the element that makes the callback return true.

Comment on lines 63 to 69
let lastZeroIndex = 0;
for (let i = this.toFields().length - 1; i >= 0; i--) {
if (!this.toFields()[i].isZero() && lastZeroIndex == 0) {
lastZeroIndex = i + 1;
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let lastZeroIndex = 0;
for (let i = this.toFields().length - 1; i >= 0; i--) {
if (!this.toFields()[i].isZero() && lastZeroIndex == 0) {
lastZeroIndex = i + 1;
break;
}
}
const fields = this.toFields();
const lastNonZeroIndex = fields.findLast(f => !f.isZero());
return fields.slice(0, lastNonZeroIndex + 1);

@MirandaWood MirandaWood changed the title feat: tightly pack private and public logs inside blobs feat: tightly pack public logs inside blobs Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants