token-cli: Fix offline signing by making simulated compute unit limit explicit

[joncinque]

Problem

#6501 broke offline signing -- by default, the token client sets the compute unit limit based on simulation. If it's using offline signing, it'll silently swallow the error and pop the compute unit limit instruction.

So when we offline-sign a transaction, it won't have the compute unit limit instruction, and when we broadcast it, we'll simulate and add the compute unit limit instruction, causing a pre-signer error.

This was the classic overly-clever-but-brittle solution. I discovered it while applying the token-cli changes to the Solana CLI.

Solution

Be more explicit:

• don't simulate by default
• allow users to specify --with-compute-unit-limit SIMULATED if they want that
• error if someone sets --with-compute-unit-price without --with-compute-unit-limit. This one might be contentious, and I could be talked down to making it a warning. But since we've only just introduced the flag, I think it's better to do the right thing and force people to be explicit
• improve the offline signing test by actually sending the transaction so we catch this in the future

Let me know how you like the ComputeUnitLimit, because I'll likely do the same exact thing for the Solana CLI. Except there, --with-compute-unit-price has been around for some time, so I would show a warning if someone specifies price without a limit.

[CriesofCarrots]

Sorry for the slow review here! I'm a little confused. If it's expected that a user would use SIMULATED with offline signing, it seems like the --sign-only command would need to return the results of simulation so the values could be made specific in the subsequent submission command. Otherwise, there's a risk the signature is invalid if those values change between signing and submission (as you note in the help text). This is a bit gross to me, although we could let the user take that risk (but places more support burden on us).

If it's not expected that a user would use SIMULATED with offline signing, I think we need to prevent it. And then I don't think the keyword really offers a lot of value, and we should default to simulation in the online case, and require offline signing to be explicit from the get-go. (This is an existing approach/pattern in solana-cli, incidentally, eg split-stake: https://github.com/anza-xyz/agave/blob/180a186c7da1b8d6f5e93ec2cbc5bf2ea17db2cb/cli/src/stake.rs#L551-L560)

[joncinque]

If it's not expected that a user would use SIMULATED with offline signing, I think we need to prevent it

That's the way I was thinking about it. We could eventually create a way for someone to simulate a transaction just to get the compute units and pass it in explicitly for offline signing, but that would be a separate PR.

And then I don't think the keyword really offers a lot of value, and we should default to simulation in the online case, and require offline signing to be explicit from the get-go

Just so I'm sure I understand: require compute_unit_limit if both sign_only and compute_unit_price are set, and then any instruction can also use compute_unit_limit. That would fix the issue.

The problem I'm running into is when to use the simulated compute units. Currently, the code always simulates, but that's an issue when one of the signers is a pre-signer. So we're left with only simulating for compute units if compute_unit_price is set and compute_unit_limit isn't, which will work in all cases.

However, this means it isn't possible to specify, "please simulate my transaction and use the compute unit limit from the result", you're required to also set a compute unit price. Someone could get around this by specifying a compute unit price of 0, but that seems confusing. So that's why I added the SIMULATED keyword.

Do you think that's a worthwhile use case? I think it is, since the scheduler should have an easier time packing transactions with (low) compute unit limits. But maybe I'm wrong.

[CriesofCarrots]

Just so I'm sure I understand: require compute_unit_limit if both sign_only and compute_unit_price are set, and then any instruction can also use compute_unit_limit. That would fix the issue.

Yep, this is what I meant. I can't quite tell from your comment, what do you think of that approach?

[joncinque]

I can't quite tell from your comment, what do you think of that approach?

Sorry, I'm having a hard time articulating this. Your idea fixes the issue, but there's a knock-on effect: it requires someone to specify a compute unit price in order to use the simulated compute unit limit.

I wanted to allow people to specify that they want to use simulated compute units without specifying a compute unit price, which is why I added the SIMULATED variant. Do you think that's worth it?

[CriesofCarrots]

it requires someone to specify a compute unit price in order to use the simulated compute unit limit.

I must be being stupid, because I don't follow why this is required. Let me re-read the code, more closely, and then reply again.

[joncinque]

I added some more testing and integrated the changes your requested. Unfortunately, clap v2 doesn't expose a way to say "require this arg if these two other args are set", required_if only checks if an arg is set to a particular value, and requires_all is not conditional

[CriesofCarrots]

Looks great! Thanks for the go-around on this.
One nit.
Also, is the CI failure expected? It doesn't look related, but did happen twice.

[joncinque]

Also, is the CI failure expected? It doesn't look related, but did happen twice.

It was a legit failure, the override limit that was set for set-interest-rate was being clobbered

[CriesofCarrots]

Woot!