Skip to content

feat: protecting fee payer against griefing#7689

Closed
benesjan wants to merge 9 commits intomasterfrom
07-31-feat_protecting_fee_payer_against_griefing
Closed

feat: protecting fee payer against griefing#7689
benesjan wants to merge 9 commits intomasterfrom
07-31-feat_protecting_fee_payer_against_griefing

Conversation

@benesjan
Copy link
Contributor

@benesjan benesjan commented Jul 31, 2024 •
edited
Loading

Fixes #7650

The fee payer needs to take a flat fee in the non-revertible part of a tx (in this PR's case in a private setup) to protect itself from a griefing attack by the user. Griefing could occur in case the public part of tx reverted --> then the fee payer would not receive the fee note.

Note: The tests in this PR are not finished because I was unable to do so due to #7717

This was referenced Jul 31, 2024
Merged
Merged
Merged
Merged
Merged
@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Jul 31, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan force-pushed the 07-29-fix_checking_funded_amount_is_enough branch from 62128a4 to 6d17618 Compare July 31, 2024 11:39
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from 239ea3b to 888f71b Compare July 31, 2024 11:39
@benesjan benesjan force-pushed the 07-29-fix_checking_funded_amount_is_enough branch from 6d17618 to 7e4bd80 Compare July 31, 2024 11:50
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from 888f71b to b9d2ce8 Compare July 31, 2024 11:50
@benesjan benesjan force-pushed the 07-29-fix_checking_funded_amount_is_enough branch from 7e4bd80 to df09327 Compare July 31, 2024 13:30
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from b9d2ce8 to d3e771a Compare July 31, 2024 13:30
@benesjan benesjan force-pushed the 07-29-fix_checking_funded_amount_is_enough branch from df09327 to 732842f Compare July 31, 2024 21:35
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from d3e771a to 633db8a Compare July 31, 2024 21:35
@AztecBot
Copy link
Collaborator

AztecBot commented Jul 31, 2024 •
edited
Loading

Benchmark results

Metrics with a significant change:

  • avm_simulation_time_ms (Token:mint_public): 103 (+70%)
  • avm_simulation_time_ms (Token:assert_minter_and_mint): 48.5 (-52%)
  • avm_simulation_time_ms (FPC:prepare_fee): 269 (+16%)
  • avm_simulation_time_ms (Token:_increase_public_balance): 81.8 (+546%)
  • avm_simulation_time_ms (FPC:pay_refund_with_shielded_rebate): 74.7 (-49%)
  • protocol_circuit_proving_time_in_ms (base-parity): 1,992 (+40%)
Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Proof generation

Each column represents the number of threads used in proof generation.

Metric 1 threads 4 threads 16 threads 32 threads 64 threads
proof_construction_time_sha256_ms 5,735 1,541 704 (-1%) 743 (-2%) 764
proof_construction_time_sha256_30_ms 11,767 3,174 (+1%) 1,409 1,437 1,468 (-1%)
proof_construction_time_sha256_100_ms 44,149 (+1%) 12,434 5,707 5,624 5,508
proof_construction_time_poseidon_hash_ms 78.0 34.0 34.0 59.0 88.0 (-1%)
proof_construction_time_poseidon_hash_30_ms 1,520 416 199 224 268 (+1%)
proof_construction_time_poseidon_hash_100_ms 5,631 1,515 677 722 (-3%) 745 (-1%)

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 4 txs 8 txs 16 txs
l1_rollup_calldata_size_in_bytes 708 708 708
l1_rollup_calldata_gas 6,600 6,588 6,588
l1_rollup_execution_gas 611,442 611,581 611,732
l2_block_processing_time_in_ms 756 (-1%) 1,449 (+5%) 2,722
l2_block_building_time_in_ms 10,739 20,801 (+2%) 41,208 (-1%)
l2_block_rollup_simulation_time_in_ms 10,738 20,801 (+2%) 41,207 (-1%)
l2_block_public_tx_process_time_in_ms 8,963 18,819 (+2%) 39,170 (-1%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 8 txs.

Metric 3 blocks 5 blocks
node_history_sync_time_in_ms 7,094 (+4%) 10,169 (+6%)
node_database_size_in_bytes 12,734,544 16,912,464
pxe_database_size_in_bytes 16,254 26,813

Circuits stats

Stats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.

Circuit simulation_time_in_ms witness_generation_time_in_ms input_size_in_bytes output_size_in_bytes proving_time_in_ms proof_size_in_bytes num_public_inputs size_in_gates
private-kernel-init 97.1 415 21,798 53,090 N/A N/A N/A N/A
private-kernel-inner 174 720 80,891 53,266 N/A N/A N/A N/A
private-kernel-tail 426 866 (-2%) 61,159 57,447 N/A N/A N/A N/A
base-parity 6.48 680 (+2%) 160 96.0 ⚠️ 1,992 (+40%) 13,188 19.0 131,072
root-parity 112 (-1%) 127 (-1%) 69,084 96.0 32,481 (+2%) 13,188 19.0 4,194,304
base-rollup 3,350 (+1%) 4,972 182,192 632 44,884 (+2%) 13,988 44.0 4,194,304
root-rollup 157 (+1%) 126 (-2%) 54,461 684 28,807 (+3%) 13,956 43.0 4,194,304
public-kernel-setup 103 (+5%) 2,754 (+1%) 112,985 80,246 18,286 (-1%) 141,444 4,027 2,097,152
public-kernel-app-logic 113 (+1%) 4,092 (-1%) 112,985 80,246 11,136 141,444 4,027 1,048,576
public-kernel-tail 631 27,552 (+6%) 410,726 10,814 68,714 (+2%) 26,372 431 8,388,608
private-kernel-reset-tiny 230 902 (+1%) 76,585 52,961 N/A N/A N/A N/A
private-kernel-tail-to-public 5,504 (+4%) 1,611 (-6%) 888,733 1,697 N/A N/A N/A N/A
public-kernel-teardown 94.2 (+3%) 4,031 112,985 80,246 20,199 141,444 4,027 2,097,152
merge-rollup 61.4 (+2%) N/A 35,678 632 N/A N/A N/A N/A
undefined N/A N/A N/A N/A 155,690 (-2%) N/A N/A N/A

Stats on running time collected for app circuits

Function input_size_in_bytes output_size_in_bytes witness_generation_time_in_ms proof_size_in_bytes proving_time_in_ms
ContractClassRegisterer:register 1,312 11,699 349 (+1%) N/A N/A
ContractInstanceDeployer:deploy 1,376 11,699 27.4 (-1%) N/A N/A
MultiCallEntrypoint:entrypoint 1,888 11,699 606 N/A N/A
GasToken:deploy 1,344 11,699 464 (+4%) N/A N/A
SchnorrAccount:constructor 1,280 11,699 430 N/A N/A
SchnorrAccount:entrypoint 2,272 11,699 821 N/A N/A
Token:privately_mint_private_note 1,248 11,699 510 (+2%) N/A N/A
FPC:fee_entrypoint_public 1,312 11,699 89.2 N/A N/A
Token:transfer 1,280 11,699 1,272 N/A N/A
AuthRegistry:set_authorized (avm) 18,427 N/A N/A 102,048 1,968 (+1%)
FPC:prepare_fee (avm) 22,894 N/A N/A 102,112 2,191 (+1%)
Token:transfer_public (avm) 46,817 N/A N/A 102,112 6,611
AuthRegistry:consume (avm) 26,670 N/A N/A 102,080 2,570 (+1%)
FPC:pay_refund (avm) 26,163 N/A N/A 102,080 3,550 (+2%)
Benchmarking:create_note 1,312 11,699 423 (+1%) N/A N/A
SchnorrAccount:verify_private_authwit 1,248 11,699 45.4 (-1%) N/A N/A
Token:unshield 1,344 11,699 1,094 (+1%) N/A N/A
FPC:fee_entrypoint_private 1,344 11,699 1,396 (+1%) N/A N/A

AVM Simulation

Time to simulate various public functions in the AVM.

Function time_ms bytecode_size_in_bytes
GasToken:_increase_public_balance 96.9 8,139
GasToken:set_portal 15.5 (+24%) 2,362
Token:constructor 92.9 (-24%) 16,400
FPC:constructor 91.8 (-1%) 9,304
GasToken:mint_public 83.7 6,150
Token:mint_public ⚠️ 103 (+70%) 11,720
Token:assert_minter_and_mint ⚠️ 48.5 (-52%) 8,028
AuthRegistry:set_authorized 48.1 (+9%) 4,537
FPC:prepare_fee ⚠️ 269 (+16%) 8,812
Token:transfer_public 37.9 (+27%) 32,641
FPC:pay_refund 78.2 (+7%) 12,114
Benchmarking:increment_balance 1,044 (+2%) 7,450
Token:_increase_public_balance ⚠️ 81.8 (+546%) 8,960
FPC:pay_refund_with_shielded_rebate ⚠️ 74.7 (-49%) 12,663

Public DB Access

Time to access various public DBs.

Function time_ms
get-nullifier-index 0.158 (+1%)

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric 1 leaves 16 leaves 64 leaves 128 leaves 256 leaves 512 leaves 1024 leaves
batch_insert_into_append_only_tree_16_depth_ms 10.5 (+2%) 16.7 (+1%) N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.8 31.7 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.606 (+2%) 0.514 (+1%) N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A 48.1 (+1%) 77.1 (+2%) 130 (-2%) 248 (+4%) 472 (+1%)
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A 95.9 159 287 543 1,055
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A 0.492 (+1%) 0.474 (+2%) 0.447 (-2%) 0.449 (+4%) 0.439
batch_insert_into_indexed_tree_20_depth_ms N/A N/A 59.5 (+1%) 111 (+2%) 182 358 (+5%) 690
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A 109 207 355 691 1,363
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A 0.502 (+1%) 0.499 (+1%) 0.482 0.484 (+4%) 0.473 (-1%)
batch_insert_into_indexed_tree_40_depth_ms N/A N/A 73.2 (+2%) N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A 133 N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A 0.520 (+2%) N/A N/A N/A N/A

Miscellaneous

Transaction sizes based on how many contract classes are registered in the tx.

Metric 0 registered classes 1 registered classes
tx_size_in_bytes 72,017 666,910

Transaction size based on fee payment method

| Metric | |
| - | |

@benesjan benesjan force-pushed the 07-29-fix_checking_funded_amount_is_enough branch from 732842f to ec6fb94 Compare August 1, 2024 02:33
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from 633db8a to 36af398 Compare August 1, 2024 02:34
Base automatically changed from 07-29-fix_checking_funded_amount_is_enough to master August 1, 2024 03:15
@benesjan benesjan force-pushed the 07-31-feat_protecting_fee_payer_against_griefing branch from 36af398 to b5b7991 Compare August 1, 2024 07:13
benesjan
benesjan commented Aug 1, 2024
View reviewed changes
yarn-project/end-to-end/src/e2e_auth_contract.test.ts
contract.withWallet(other).methods.set_authorized(authorized.getAddress()).send().wait(),
).rejects.toThrow('caller is not admin');
it('non-admin cannot set authorized', async () => {
await expect(contract.withWallet(other).methods.set_authorized(authorized.getAddress()).prove()).rejects.toThrow(
Copy link
Contributor Author

@benesjan benesjan Aug 1, 2024

Choose a reason for hiding this comment

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

Randomly stumbled upon this and sneaked it in. Sending and waiting for the tx was unnecessary here.

benesjan
benesjan commented Aug 1, 2024
View reviewed changes
yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts
expect(transactionFee).toBeGreaterThan(0);
// In total 4 notes should be inserted: 1 change note for user, 1 flat fee note for fee payer, 1 refund note for
// user and 1 fee note for fee payer.
expect(debugInfo?.noteHashes.length).toBe(4);
Copy link
Contributor Author

@benesjan benesjan Aug 1, 2024

Choose a reason for hiding this comment

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

Added the num notes check here since I wanted to use num notes in the following test and I needed to sanity-check that I am counting the notes correctly.

benesjan
benesjan commented Aug 1, 2024
View reviewed changes
yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts
// There should be 3 nullifiers emitted: 1 for tx hash, 1 for user randomness (emitted in FPC), 1 for the note user
// paid the funded amount with.
// expect(debugInfo?.nullifiers.length).toBe(3); // This is actually 4. Does the reviewer know why? I can't find
// the last nullifier. If not I'll just nuke this check as it's not that important.
Copy link
Contributor Author

@benesjan benesjan Aug 1, 2024

Choose a reason for hiding this comment

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

I could not find the where the 4th nullifier is coming from. If you the reviewer don't know as well I will just nuke the check as it's not that important.

benesjan
benesjan commented Aug 1, 2024
View reviewed changes
yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts
@@ -136,23 +149,32 @@ describe('e2e_fees/private_refunds', () => {
const aliceRandomness = Fr.random(); // Called user_randomness in contracts
Copy link
Contributor Author

@benesjan benesjan Aug 1, 2024

Choose a reason for hiding this comment

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

This test is not really finished because I can't check balances etc. because of #7717.

@benesjan benesjan marked this pull request as ready for review August 1, 2024 12:17
@benesjan benesjan requested review from just-mitch and nventuro and removed request for just-mitch and nventuro August 1, 2024 12:17
@benesjan benesjan marked this pull request as draft August 1, 2024 14:08
@benesjan
Copy link
Contributor Author

benesjan commented Aug 9, 2024

Closing this as it's not planned to be finished in the near future and the approach implemented here is most likely incorrect.

@benesjan benesjan closed this Aug 9, 2024
@benesjan benesjan deleted the 07-31-feat_protecting_fee_payer_against_griefing branch August 9, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Protect FPC against griefing

2 participants

@benesjan @AztecBot