chore: memoize address_bytes of verification key
#1444
Conversation
SuperFluffy
left a comment
There was a problem hiding this comment.
High level comment on this memoization:
Is there a more explicit way to memoize the address bytes, for example by calculating address_bytes once and storing them near the call site?
This change is relatively minimal for the user (minus the Copy trait, although we don't seem to make ause of it).
But introducing syncing primitives into a relatively innocent newtype wrapper makes me uneasy.
I think doing that means the change would have an impact on callers, which isn't the intent here.
Well, I guess for my money, the need for hand-rolled equality/ordering/hash traits is the biggest drawback here, rather than the fact the new field is a synchronization primitive. |
## Summary Added a lazily-initialized field `address_bytes` to `VerificationKey`. ## Background Testing showed that `address_bytes` was called multiple times on a given `VerificationKey` (up to 11 times in some cases). Each time the key's bytes were being hashed, so this change ensures that hashing only happens once for a given verification key instance. Note that this was implemented previously in #1111 and was then reverted in #1124. However, when reverted, the manual impls of `PartialEq`, `Eq`, `PartialOrd`, `Ord` and `Hash` were left as-is, as were the unit tests for these. Hence this PR doesn't need to make any changes to these trait impls or tests. ## Changes - Added `address_bytes: OnceLock<[u8; ADDRESS_LEN]>` to `VerificiationKey`. ## Testing No new tests required. ## Related Issues Closes #1351.
* main: feat(sequencer): make mempool balance aware (#1408) chore(sequencer): change test addresses to versions with known private keys (#1487) chore(chart): update geth tag (#1485) feat(sequencer): report deposit events (#1447) feat(proto, core, sequencer)!: add traceability to rollup deposits (#1410) fix(bridge-withdrawer, cli, sequencer-client): migrate from `broadcast_tx_commit` to `broadcast_tx_sync` (#1376) fix(sequencer): add `end_block` to `app_execute_transaction_with_every_action_snapshot` (#1455) release: end of iteration release cuts (#1456) chore(charts): rollupName templates (#1458) chore(sequencer-relayer): Add instrumentation (#1375) feat(proto, core, sequencer)!: permit bech32 compatible addresses (#1425) chore: memoize `address_bytes` of verification key (#1444) chore(ci): include `ibc-bridge-test` in `docker` CI target (#1438) chore(charts): bump celestia versions (#1431)
Summary
Added a lazily-initialized field
address_bytestoVerificationKey.Background
Testing showed that
address_byteswas called multiple times on a givenVerificationKey(up to 11 times in some cases). Each time the key's bytes were being hashed, so this change ensures that hashing only happens once for a given verification key instance.Note that this was implemented previously in #1111 and was then reverted in #1124. However, when reverted, the manual impls of
PartialEq,Eq,PartialOrd,OrdandHashwere left as-is, as were the unit tests for these. Hence this PR doesn't need to make any changes to these trait impls or tests.Changes
address_bytes: OnceLock<[u8; ADDRESS_LEN]>toVerificiationKey.Testing
No new tests required.
Related Issues
Closes #1351.