Skip to content

feat: implement asset callbacks#2571

Open
PhilippGackstatter wants to merge 42 commits intonextfrom
pgackst-asset-callbacks
Open

feat: implement asset callbacks#2571
PhilippGackstatter wants to merge 42 commits intonextfrom
pgackst-asset-callbacks

Conversation

@PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Mar 9, 2026

Implements the on_before_asset_added_to_account callback in the tx kernel.

The PR is deliberately "minimal" and does not:

  • Implement the on_before_asset_added_to_note callback.
  • Implement support for callbacks in non-fungible assets.

Both of these will be done in a separate PR based on feedback to this PR.

Design Decisions

Allow missing callback slots

Since the callback flag is part of the asset key, the callback flag must be set whenever an asset is created, e.g. in miden::protocol::asset::create_fungible_asset. So, this and similar procedures now take enable_callbacks as an input.

To avoid parameterizing implementations like miden::standards::faucets::distribute over this flag, a helper is added that checks whether the active account defines any callbacks: miden::protocol::active_account::has_callbacks. This procedure is called in distribute and passes the returned flag on to the to-be-minted asset.

Decisions

  • Allow callback storage slots to be missing: has_callbacks returns 1 if the slot exists and contains a non-empty word, 0 otherwise. This is to avoid having to add the callback slots to all faucet accounts (and in the future all accounts).
  • Implement this as a kernel procedure rather than as a library procedure in miden::protocol.

The 2nd decision was made by considering the following (post-mainnet) scenario:

  • Suppose has_callbacks is a library-only implementation.
  • BasicFungibleFaucet uses the flag returned by this procedure to create it assets.
  • A new callback is added in the tx kernel. Since has_callbacks cannot be updated, it is deprecated and a has_callbacks_v2 is added that implements the existence check for the new callback.
  • Account initially uses a callback, then upgrades storage to the new callback and removes the old one.
  • BasicFungibleFaucet code still uses the MAST root of the original has_callbacks procedure and now consequently determines that it does not have callbacks, even though the new one is actually used.
  • Outcome: Callbacks are not enabled for the minted assets even though they should be.

When the has_callbacks proc is implemented in the kernel, due to dynamic invocation, it can be upgraded to take into account newly added callbacks without breaking MAST roots.

In theory I would prefer the library-only implementation, but I'm not sure how to properly address such issues otherwise.

Worth noting: A similar problem exists for miden::standards::auth::signature::verify_signature_by_scheme - adding new schemes in the future will change the MAST root and we can't apply the kernel procedure workaround there.

Multiple Procedures Per Callback

This PR assumes the need to assign multiple procedures to the same callback is unlikely to come up. If it would come up anyway, account components would have to manually merge multiple procedures into a single one and then provide that one as the callback procedure.

The approach here is that account components that implement callbacks need to add the callback storage slot themselves, with the help of AssetCallbacks.

Callback Inputs

on_before_asset_added_to_account does not pass the native account ID to the callback to keep the signature simpler. It is easy to retrieve the ID with native_account::get_id. The main idea is:

  • keep callbacks consistent with each other. If one callback passes the native ID, all others should also do that for consistency.
  • keep the asset signature as small as possible.

Applying this consistency rule from the account to the note callback would mean that the note callback would become a bit messy if it were to provide [note_idx, native_account_id_suffix, native_account_id_prefix, ASSET_KEY, ASSET_VALUE], and so it makes sense to me to not pass the native ID to any of them.

Callback Security

I'm also wondering about the security of callback procedures in an account's public interface. If the callback is a public part of the interface, it can be called by anyone. But if it turns out that callbacks will be able to mint assets, then this could probably be exploited:

  • Construct a fake instance ASSET_VALUE of the faucet's asset and invoke the callback from a tx script.
  • Based on the exact impl, callback may mint some additional asset and return it as part of PROCESSED_ASSET_VALUE.
  • Now PROCESSED_ASSET_VALUE - ASSET_VALUE is a valid asset that can be sent to some other account.

Mitigations

  • Do not allow callbacks to mint anything; keep them read-only in general. For now this is the case and that may be the best option until we have thought about this more thoroughly.
  • Somehow make sure that callbacks are only invoked by the tx kernel.
    • For instance: upon loading a faucet account, build a procedure list of all callbacks it contains. In authenticate_account_origin, if the called procedure is a callback, check whether the kernel is in a "callback context". Not yet sure if this protects against all possible ways this could be called.

FungibleAssetDelta

The FungibleAssetDelta needed an update since the (Rust) delta only included the faucet ID, but the callback flag also needs to be included to differentiate assets with and without callbacks. This means including the faucet ID and the asset metadata (= currently callback flag).
This will become much nicer when we generalize the asset delta, so I did not spend time on optimizing the code in that type.

@PhilippGackstatter PhilippGackstatter marked this pull request as ready for review March 10, 2026 19:35
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! Not a full review yet, but I left some comments inline.

"auth_procedure": 62667,
"after_tx_cycles_obtained": 574
"total": 71195,
"auth_procedure": 69694,
Copy link
Contributor

Choose a reason for hiding this comment

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

Auth procedure cycle count went up quite a bit - but I'm guessing this may be because of some prior changes rather than because of this PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked a few key PRs:

before auth component consolidation: 9cfb4f2a
  "auth_procedure": 62641
before asset refactor: a09027b6
  "auth_procedure": 63170
before VM migration: e8570061
  "auth_procedure": 63169
after VM migration: 0897aae6
  "auth_procedure": 69686

So auth component consolidation added ~500 cycles but the VM migration was the main reason for the large increase in cycles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since most of the auth procedure is Falcon signature verification - I wonder if most of the delta is because Falcon signature verification got a bit less efficient.

@Al-Kindi-0 - could you check how many cycles Falcon signature takes now? (maybe some stack re-orientation work made it less efficient?)

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one small comment inline.

Comment on lines 366 to 369
fn get_size_hint(&self) -> usize {
self.0.len().get_size_hint() + self.0.len() * FungibleAsset::SERIALIZED_SIZE
const ENTRY_SIZE: usize = AssetVaultKey::SERIALIZED_SIZE + core::mem::size_of::<u64>();
self.0.len().get_size_hint() + self.0.len() * ENTRY_SIZE
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to the old code, right? Or is there a reason we can't keep using FungibleAsset::SERIALIZED_SIZE here?

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