Restrict PTBs that use Random

[benr-ml]

Description

PTBs can only have TransferObjects or MergeCoins following a command that uses Random.

Test Plan

New transactional tests

---

If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

• protocol change
• user-visible impact
• breaking change for a client SDKs
• breaking change for FNs (FN binary must upgrade)
• breaking change for validators or node operators (must upgrade binaries)
• breaking change for on-chain data layout
• necessitate either a data wipe or data migration

Release notes

Programmable transaction blocks (PTBs) can only have TransferObjects or MergeCoins following a command that uses Random.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am
multisig-toolkit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am
sui-kiosk	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 6:33am

[tnowacki]

I suppose we could do these checks at signing, but it would be the first such check (though maybe we check for mut clock usage at signing? so its kind of similar)

If we keep this check at signing (which I'm fine with I suppose), thoughts on adding an invariant violation (or something similar) to the execution layer?

[benr-ml]

I suppose we could do these checks at signing, but it would be the first such check (though maybe we check for mut clock usage at signing? so its kind of similar)

If we keep this check at signing (which I'm fine with I suppose), thoughts on adding an invariant violation (or something similar) to the execution layer?

Not sure I follow - Why do you consider this as "checks at signing"? validity_check is called by handle_transaction_impl. Surprisingly we also call the same function, again, indirectly from handle_transaction_impl and here. The last function also checks mut clock.

Anyhow, I can move it to crates/sui-transaction-checks/src/lib.rs if that's the right place.

[tnowacki]

Not sure I follow - Why do you consider this as "checks at signing"? validity_check is called by handle_transaction_impl. Surprisingly we also call the same function, again, indirectly from handle_transaction_impl and here. The last function also checks mut clock.

Anyhow, I can move it to crates/sui-transaction-checks/src/lib.rs if that's the right place.

Handle transaction is called during signing (and then again during certificate execution), so we will not sign a transaction that uses randomness incorrectly. Which is kind of neat since we can do it quickly.

My point was I was initially expecting these rules to be "type system"-like rules, all of which for PTBs are done dynamically during execution, see https://github.com/MystenLabs/sui/blob/main/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs

[tnowacki]

Adding some other folks who might also have opinions about where these checks go

[mystenmark]

Adding some other folks who might also have opinions about where these checks go

Pretty sure we check for mut clock usage at signing as well. Given that this is easy to check at signing I see no reason not to, but if it makes more sense to keep all the PTB checks in one place and do them at execution only, I'm okay with that as well.

[tnowacki]

Adding some other folks who might also have opinions about where these checks go

Pretty sure we check for mut clock usage at signing as well. Given that this is easy to check at signing I see no reason not to, but if it makes more sense to keep all the PTB checks in one place and do them at execution only, I'm okay with that as well.

IIRC for clock, we go a step further and make sure you cannot publish code that takes an &mut Clock.

@benr-ml, I think since we do that for clock, it makes sense for this to also be a static rule.
Though we might want to think about adding invariant violation checks for both Random and Clock if misused.