match out-of-snark and in-snark account-timing updates #5937
Conversation
| ~txn_amount ~txn_global_slot) | ||
| in | ||
| (* Like receiver account in payments, coinbase fee transfer account or other fee transfer account don't have the timing field updated *) | ||
| Account_timing.if_ is_coinbase_or_fee_transfer |
There was a problem hiding this comment.
This if_ call is rather expensive. Can we rework this to change the non-snark logic instead?
There was a problem hiding this comment.
If we update both accounts in coinbase and fee transfers (otherwise it seems inconsistent) then there will still be an if_ call when we update the receiver timing (if user command then don't update the timing). An alternative would be to just update all the accounts' timing field for every type of transaction (stake delegation, receiver account in payments, coinbase, and fee transfer)
There was a problem hiding this comment.
An alternative would be to just update all the accounts' timing field for every type of transaction (stake delegation, receiver account in payments, coinbase, and fee transfer)
This is definitely my preferred solution. This should already be the case for all user commands, it should just be coinbases and fee transfers that need updating in transaction_logic.ml and sparse_ledger.ml.
There was a problem hiding this comment.
For user commands, we currently don't update the receiver account's timing (receiver in payments and delegate in stake delegations). i'll change that as well
There was a problem hiding this comment.
@mrmr1993 Would checking the timing for receiver account be cheaper than the two if_ calls though?
There was a problem hiding this comment.
Do we need the receiver account timing to be updated? I don't think we ever reduce the balance for the receiver account, so I would have thought the timing isn't relevant, unless I'm missing some nuance.
There was a problem hiding this comment.
Well, we don't reduce the balance for coinbase and fee transfer accounts either. But the way we encode those as fee_payer and receiver, we either update both or none otherwise it is inconsistent. Like, one of the fee transfers in a fee transfer transaction has the timing updated but not the other
There was a problem hiding this comment.
In those cases, I had leant on the side of doing what's cheapest in the snark. It seems correct to me to either update the timing or not when we're not decreasing the balance of an account. Since we always re-check the timing whenever it's relevant, it should make no practical difference.
I propose changing the non-snark logic to match the existing snark behaviour, and that we live with the small inconsistency around the receiver account not getting it's timing re-normalised.
There was a problem hiding this comment.
Alright, makes sense in the interest of not increasing the proving time whenever possible
|
Preview: Built with commit eb0b7e9 |
deepthiskumar
added
the
ci-build-me
deepthiskumar
added
the
ready-to-merge
Outside snark, the timing field is updated only for accounts whose balance is deducted (when we check if the account still maintains a min balance). In transaction snark this wasn't true for coinbase and fee transfer, where the coinbase/fee receiver account's timing field was getting set to
Untimedwhen the tokens were unlocked.Changed the out-of-snark behaviour to update the timing of fee transfer/coinbase accounts that is encoded as
fee_payerorsourceaccount in a transaction to avoid checks in the snarkChecklist: