feat(standards): add expiration to network account config notes - #3715
feat(standards): add expiration to network account config notes#3715onurinanc wants to merge 12 commits into
Conversation
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good! Left a few suggestions, with the important one being avoiding finding and loading the attachment twice.
| #! | ||
| #! The attachment is expected to have the following layout: | ||
| #! [account_id_suffix, account_id_prefix, exec_hint_tag, 0] | ||
| #! [account_id_suffix, account_id_prefix, exec_hint_tag, expiry_block] |
There was a problem hiding this comment.
nit(consistency): in other places we use expiration_block_num rather than expiry. Consider calling it that. Applies to procedure names, too.
| # bind consumption to the targeted account: reject if the consuming account is not the note's | ||
| # target account | ||
| exec.network_account_target::active_account_matches_target_account | ||
| assert.err=ERR_BLOCKLIST_CONFIG_TARGET_ACCOUNT_MISMATCH | ||
| # => [pad(16)] | ||
|
|
||
| # reject a note held past the expiry block of its target attachment | ||
| exec.network_account_target::assert_not_expired | ||
| # => [pad(16)] | ||
|
|
There was a problem hiding this comment.
While this pattern is really easy to read, we find and unhash the same attachment twice, which adds quite a few unnecessary cycles and hashes.
I think we should either:
- load the attachment explicitly, then invoke the two procedures on a copy.
- refactor
network_account_target::active_account_matches_target_accountinto a singlenetwork_account_target::assert_target_id_and_expirationthat does both checks.
I think the second option would be best, unless we need to check the target match individually from expiration, but I doubt this is or should be the case.
There was a problem hiding this comment.
Nice one! I would go with your second option for two reasons:
- It seems like the target matches are individually checked in agglayer notes
- moving the assertion into the shared procedure would collapse all note specific error messages into a single one
ERR_*_TARGET_ACCOUNT_MISMATCH
| dup dup.2 u32lt assert.err=ERR_NETWORK_ACCOUNT_TARGET_EXPIRED | ||
| # => [reference_block, expiry_block] | ||
|
|
||
| u32wrapping_sub |
There was a problem hiding this comment.
Since we checked above, sub is sufficient. We don't need wrapping behavior here.
| #! Otherwise the procedure applies two bounds, and both are required for the expiry to hold: | ||
| #! - it requires the transaction's reference block to lie strictly before the expiry block, and | ||
| #! - it caps the transaction expiration block delta at the number of blocks left until the expiry | ||
| #! block, or [`DEFAULT_EXPIRATION_BLOCK_DELTA`] if that is smaller. |
| # cap the inclusion window at whichever is smaller: the default delta, or the blocks left | ||
| # until the expiration block | ||
| push.DEFAULT_EXPIRATION_BLOCK_DELTA u32min | ||
| # => [expiration_block_delta] |
There was a problem hiding this comment.
I'm not sure this is desirable.
I can imagine some config notes' expiration could be longer than DEFAULT_EXPIRATION_BLOCK_DELTA, in which case a user-supplied expiration would just be ignored.
I think that if the caller has taken enough caution to set the expiration != NETWORK_ACCOUNT_TARGET_NO_EXPIRATION, then they will have provided a meaningful value that they care about enforcing, rather than being min-capped at the default
There was a problem hiding this comment.
I think the documentation makes this a bit confusing as the user-supplied expiration is not
ignored here. It is enforced by the assert just above:
dup dup.2 u32lt assert.err=ERR_NETWORK_ACCOUNT_TARGET_EXPIRED
The u32min only bounds how long after proving the transaction may still be included into the chain. Also, dropping the u32min would break long expirations as the kernel's update_expiration_block_delta only accepts a delta of at most 65535 blocks and aborts the transaction otherwise, so passing expiration_block_num - reference_block would make any note expiring more than 65535 blocks out permanently unconsumable.
So I think fixing the documentation is sufficient here. Or, am I missing something?
There was a problem hiding this comment.
I might be missing something, but I think the numbers don't add up. Walking through some concrete example:
DEFAULT_EXPIRATION_BLOCK_DELTA = 20- assume the tx is proven against a
reference_block = 100
Case 1: tx is expired, can't even be proven, expiration_block_num = 90
throws ERR_NETWORK_ACCOUNT_TARGET_EXPIRED ✅
Case 2: tx can be proven, valid for a few more blocks, expiration_block_num = 105
blocks_until_expiration = 5
expiration_block_delta = min(5, 20) = 5
Case 2a
- the current block is 102, tx succeeds as expected ✅
Case 2b
- the current block is 122, tx fails as expected because it's too stale ✅
Case 3: tx is valid for more than the default, expiration_block_num = 125
blocks_until_expiration = 25
expiration_block_delta = min(25, 20) = 20
update_expiration_block_delta(20) -> admits at most 20 block past
Case 3a:
- current block is 102. Tx is fresh enough ✅
Case 3b:
- current block is 122. Tx is "fresh enough", i.e. within the caller-supplied
expiration_block_numof 25, but because we updatedupdate_expiration_block_deltawith a value of 20, the transaction fails - unexpected ❌
I might have made some mistakes here, I still get confused sometimes about the expiration, so please feel free to point any gaps in my understanding.
3 participants
Summary
NetworkAccountTargetattachment.assert_not_expired, which caps the transaction expiration block delta so a stale reference block cannot pass the check.assert_not_expiredfrom the config note scripts and add an optionalexpiryto their builders.PauseConfignote held past its expiry block.Part of #3560.