feat: implement manual Packable for structs with sub-Field members#21576
feat: implement manual Packable for structs with sub-Field members#21576benesjan merged 1 commit intomerge-train/fairiesfrom
Conversation
| let post_value = MockStruct { a: 3, b: 4 }; | ||
|
|
||
| let sdc = ScheduledDelayChange::<0u64>::new(Option::some(1), Option::some(50), 2); | ||
| let sdc = ScheduledDelayChange::<0_u64>::new(Option::some(1), Option::some(50), 2); |
There was a problem hiding this comment.
Unrelated change that was a result of me running nargo fmt on the repo.
|
|
||
| fn unpack(packed: [Field; Self::N]) -> Self { | ||
| let remaining_txs = packed[0] as u32; | ||
| let expiry_block_number = ((packed[0] - remaining_txs as Field) / 2.pow_32(32)) as u32; |
There was a problem hiding this comment.
maybe a comment here would be nice. these kinds of unpacking functions normally rely on shifts and bitmasks, I guess we don't have yet those in Noir?
There was a problem hiding this comment.
I see you use the same approach in the other contracts, maybe worth adding a reusable function?
There was a problem hiding this comment.
these kinds of unpacking functions normally rely on shifts and bitmasks, I guess we don't have yet those in Noir?
We have them in Noir but unlike in CPU instruction sets, bit shifts are super inefficient in circuits while multiplication and division are efficient (circuits have MUL and AND gates and you can perform division by multiplying by modulo inverse).
There was a problem hiding this comment.
We intentionally use multiply/divide by powers of 2 rather than bit shifts — they map more efficiently to both AVM opcodes (for public functions) and proving backend primitives (for private functions). The Packable trait docs already recommend this approach.
There was a problem hiding this comment.
I considered this but a reusable helper doesn't really help here — the as TargetType casts (e.g. as u32, as u64, as u128) are the core of each unpacking since they provide the circuit range constraints, and each site uses different type combinations (u32+u32, u32+u64, u128+u64, u128+bool, bools+u32s). A helper returning raw Fields would require callers to still do all the casting, resulting in more lines not fewer. The Packable trait docs already serve as the guide with a worked example.
benesjan
force-pushed
the
nico/f-260-manual-packable-impls
branch
from
090f818 to
e9d06d3
Compare
benesjan
requested review from
a team,
IlyasRidhuan,
charlielye,
dbanks12,
fcarreiro,
jeanmon and
sirasistant
as code owners
benesjan
changed the base branch from
nico/f-260-warn-on-packable-with-small-types
to
merge-train/fairies
benesjan
removed request for
a team,
IlyasRidhuan,
LeilaWang,
charlielye,
dbanks12,
fcarreiro,
jeanmon,
nventuro and
sirasistant
Replace `#[derive(Packable)]` with efficient manual implementations for 6 structs that had sub-optimal packing (multiple sub-Field members each occupying a full Field). This avoids the new compile-time warning and reduces storage/hashing costs. - Asset: 4 -> 3 Fields (pack u128 + u64 together) - Order: 3 -> 2 Fields (pack u128 + bool together) - Game: 13 -> 9 Fields (pack 3 bools + 2 u32s together) - PlayerEntry: 3 -> 2 Fields (pack u32 + u64 together) - Card: 2 -> 1 Field (pack 2 u32s together) - SubscriptionNote: 2 -> 1 Field (pack 2 u32s together) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
benesjan
force-pushed
the
nico/f-260-manual-packable-impls
branch
from
e9d06d3 to
150e534
Compare
|
✅ Successfully backported to backport-to-v4-next-staging #21654. |
BEGIN_COMMIT_OVERRIDE feat: entrypoint replay protection (#21649) feat: guard BoundedVec oracle returns against dirty trailing storage (#21589) feat: implement manual Packable for structs with sub-Field members (#21576) fix: off-by-1 in getBlockHashMembershipWitness archive snapshot (#21648) END_COMMIT_OVERRIDE
3 participants
Motivation for this PR was this other PR implemented by Nico since that PR resulted in a failing CI that in turn was a result of brittle
bootstrap.shscripts that did not expect any output to be coming from the tests. Since that PR now prints out a warning tostdoutwhen an inefficient Packable implementation is detected this broke that assumption.I came to a conclusion that the best approach here is to just do the efficient implementations. Since
bootstrap.shis only an internal script keeping that brittle seems fine.Summary
#[derive(Packable)]with efficient manual implementations for 6 structs that had sub-optimal packing (multiple sub-Field members each occupying a full Field)nargo test --list-testsoutput parsing treated as a test entry)Test plan
warning:lines innargo test --list-testsoutput🤖 Generated with Claude Code