Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction Policies #514

Merged
merged 18 commits into from
Oct 26, 2023
Merged

Transaction Policies #514

merged 18 commits into from
Oct 26, 2023

Conversation

Voxelot
Copy link
Member

@Voxelot Voxelot commented Aug 24, 2023

Summary

Use a dynamic set of policies for transaction validation requirements such as gaslimit & maturity. This will reduce the size of transactions that always leave these fields unset, and also future proof the transaction format for later requirements such as multi-dimensional pricing.

A new policy that's been added is witnessLimit. Having a witness limit allows the network to charge for witness data without exposing users to malicious basefee manipulation by third-parties / relayers. Without a charge & limit for witness data, blockspace could be easily saturated with spurious spammy witness data.

Another significant change is to separate base costs of the transaction from the transaction gas limit. Gas limit will now only apply to script execution, and everything else is charged as part of the minimum transaction fee.

Since the gas limit can't restrict the total cost of the transaction anymore, another policy called MaxFee was added. This is a more direct way to place an upper bound on transaction costs and is also future compatible with multi-dimensional pricing.

Multi-dimension pricing preparation

In order for resources such as execution and storage to be charged for independently, transactions must be able to specify an anticipated consumption limit for each resource kind. By abstracting these limits into a dynamic set of policies, new resource types can be added in the future more easily.

src/tx-format/transaction.md Outdated Show resolved Hide resolved
src/tx-format/policy.md Outdated Show resolved Hide resolved
@Voxelot Voxelot requested a review from xgreenx September 26, 2023 01:30
@xgreenx
Copy link
Contributor

xgreenx commented Sep 27, 2023

While we move some of the fields to policies, we don't plan to change the API of the fuel-tx and GraphQL to minimize the breaks of the change. Getters and setters remain the same.

The changes will affect the serialization and deserialization of the transactions, and a minimal fee to include WhitnessLimit.

Another hidden change is that GasLimit only limits the script execution(not script and predicates). So min_fee is a gas used by predicates plus total_bytes * gas_per_byte * gas_price. The max_fee is a min_fee + gas_limit * gas_price. Maybe it is important for UI, to represent this kind of information to the user to clarify it.

What should be changed to support policies:

  • fuel-tx:

    • Add a new WhitnessLimit getter that returns None<u64>.
    • Add a new enum for policies.
    • Add iteration over policies.
    • Old API keeps the same. Only the implementation looks into policies.
    • The minimal fee considers a WhitnessLimit if it is set.
  • fuel-core:

    • Add support for a new enum in the schema.
    • Add support for policies on the GraphQL level.
    • Old API keeps the same. Only the implementation looks into policies.
    • Maybe reject transactions on the TxPool level if WhitnessLimit is not set.
    • Maybe reject transactions without WhitnessLimit during dry_run.
  • fuels-rs:

    • Add support for WhitnessLimit. It includes automatically setting this field before signing if the user does not set it(maybe for simplicity, we could use some constant).
    • Support new enums from schema.
    • Maybe provide access to policies to the user.
  • fuels-ts:

    • If Add wasm_bindgen support for fuel-tx fuel-vm#579 is not ready and TS SDK doesn't use implementation of the serialization/deserialization from the fuel-tx, then we need to update se/de for Script and Create transactions.
    • Add support for new enum in the schema.
    • Add support for WhitnessLimit, including automating its setting before signing if it is not set by the user.
    • Add manual access to policies.
    • Keep the old API the same, but work with policies instead of fields.
  • sway: Hmm, it looks like there is nothing to change except to use the latest dependencies.

  • fuel-indexer:

    • Add support for a new field whitness_limit.
  • Frontend:

    • Maybe add support of whitness_limit for the wallet.
    • Suppose we have any use cases where we need to include more than one signature, then WhitnessLimit will break the code. It requires manual setting of the whitness_limit(if we don't use any constant on SDK side).
    • Maybe add support for min_fee and max_fee on the wallet/UI side instead of relying on gas_limit.
  • Documentation:

    • Add a description of how policies work. Highlight that if the WitnessLimit is not specified, the block producer(or any man in the middle) can increase min_fee and drain all funds from inputs.
    • Update how the min_fee and max_fee work.

@Voxelot
Copy link
Member Author

Voxelot commented Sep 27, 2023

I don't think specifying a witness limit should be mandatory at the protocol level, but maybe the SDK's should validate this for users.

@Voxelot
Copy link
Member Author

Voxelot commented Sep 27, 2023

I also realized this PR needs to update the GTF opcode, although I don't think this will impact sway a lot except for some additive changes to the std-lib.

add gtf args for policies
pad gtf args to avoid conflicts
@Voxelot
Copy link
Member Author

Voxelot commented Oct 11, 2023

@xgreenx I've updated the PR to move gasPrice to policies, added requirements to tx-validity specifying that the gasPrice policy is required for now, and also added policies to the GTF args.

@Voxelot Voxelot marked this pull request as ready for review October 11, 2023 21:23
src/tx-format/transaction.md Outdated Show resolved Hide resolved
src/tx-format/policy.md Outdated Show resolved Hide resolved
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
@Voxelot
Copy link
Member Author

Voxelot commented Oct 13, 2023

@xgreenx fixed the comments you left, and pushed a separate commit for a different idea I had about using bitfields to index policies and reduce the amount of data that policies consume.

Copy link
Contributor

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

I like the idea of using bitmask as the length provider and type of policy provider. It saves 8 bytes per policy. It makes implementation a little bit complicated, but it is worth of that.

|--------------------|-----------------------------|----------------------------------------|
| `scriptLength` | `uint16` | Script length, in instructions. |
| `scriptDataLength` | `uint16` | Length of script input data, in bytes. |
| `policyTypes` | `uint32` | Bitfield of used policy types. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to use u64 here since it takes 8 bytes in canonical format.

Suggested change
| `policyTypes` | `uint32` | Bitfield of used policy types. |
| `policyTypes` | `uint64` | Bitfield of used policy types. |

Copy link
Member Author

Choose a reason for hiding this comment

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

True but it could save data outside of the VM for things like DA

src/tx-format/transaction.md Outdated Show resolved Hide resolved
segregate predicateGasUsed from tx.gasLimit
src/tx-format/policy.md Outdated Show resolved Hide resolved
Copy link
Contributor

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

LGTM!

src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
@@ -83,7 +83,11 @@ Transaction is invalid if:
- `scriptDataLength > MAX_SCRIPT_DATA_LENGTH`
- `scriptLength * 4 != len(script)`
- `scriptDataLength != len(scriptData)`
- `gasLimit` is less than the sum of all `predicateGasUsed` for `InputType.Coin` or `InputType.Message` where predicate length is greater than zero.
- No policy of type `PolicyType.GasPrice`
- No policy of type `PolicyType.GasLimit`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can allow not havePolicyType.GasLimit in the case of an empty script.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made gas limit a default field for script transactions. The only reason to keep it as a policy is to save a u64 off of basic transfers that don't use any script logic.

@luizstacio
Copy link
Member

luizstacio commented Oct 23, 2023

Frontend:

The fuels-ts support will block us, so our timeline will be additional to the time they need to implement first. The time expected can be up to 10 days.

  • Fuel Wallet
    • Update transfer to correct use the new policy fields. [small];
    • Update preview transaction to show the correct gas cost. [small];
    • Update view transaction to show the correct gas cost. [small];
    • Create a new Policies summary component to show the policies set to the transaction; [small];
      • Add component on the Transaction view;
      • Add component on the Transaction Preview;
    • Update all API changes that will be updated on the fuels-ts. [medium] (this needs a better understanding of what APIs will be changed and how they will be implemented);
  • Fuel Bridge
    • Update all API changes that will be updated on the fuels-ts. [medium] (this needs a better understanding of what APIs will be changed and how they will be implemented);
  • Fuel Explorer
    • Ensure the gas usage is showing the correct amount [small];
    • Create a new UI for showing policies. [small].

small - up to one day
medium - up to three days
large - up to a week.

@Voxelot
Copy link
Member Author

Voxelot commented Oct 24, 2023

@luizstacio could this time be shortened by using fuel-tx wasm for serialization? cc @digorithm

@luizstacio
Copy link
Member

luizstacio commented Oct 25, 2023

@luizstacio could this time be shortened by using fuel-tx wasm for serialization? cc @digorithm

I think if we change fuel-tx this will be a big push on the fuels-ts side and potentially include multiple break changes. This means more time on ts-sdk and more time on FE.

@Voxelot Voxelot requested a review from xgreenx October 25, 2023 23:07
@Voxelot Voxelot requested a review from xgreenx October 26, 2023 01:19
@Voxelot Voxelot self-assigned this Oct 26, 2023
@Voxelot Voxelot merged commit 4a841c5 into master Oct 26, 2023
4 checks passed
@Voxelot Voxelot deleted the Voxelot/tx-policies branch October 26, 2023 17:16
@xgreenx xgreenx mentioned this pull request Jan 24, 2024
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.

3 participants