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

Public processor updates the merkle tree DB for failing transactions #1869

Open
sirasistant opened this issue Aug 29, 2023 · 0 comments
Open
Labels
T-bug Type: Bug. Something is broken.

Comments

@sirasistant
Copy link
Collaborator

sirasistant commented Aug 29, 2023

During execution of public functions, the simulator updates the uncommited cache of the merkle tree DB. This allows submitted functions that fail to temporarily corrupt the merkle trees state, by introducing writes that will be seen by other Txs in the block, but that will fail to verify in the public kernel.
A way of reproducing this is:
First, modify the transfer function of the public token contract to write before fail:

    // Transfers `amount` of tokens from `msg_sender` to `recipient`.
    open fn transfer(
        inputs: PublicContextInputs,
        amount: Field,
        recipient: Field,
    ) -> pub abi::PublicCircuitPublicInputs {
        let mut context = PublicContext::new(inputs, abi::hash_args([amount, recipient]));

        let storage = Storage::init();
        let sender = context.msg_sender();

        let sender_balance = storage.balances.at(sender);
        let recipient_balance = storage.balances.at(recipient);

        let current_sender_balance: Field = sender_balance.read();
        let current_recipient_balance = recipient_balance.read();

        // TODO: Should support larger numbers.
        let mut return_value = 0;

        let new_recipient_balance = current_recipient_balance + amount;
        recipient_balance.write(new_recipient_balance);
        return_value = new_recipient_balance;

        assert(current_sender_balance as u126 > amount as u126); <= note that the assertion is done after the write

        sender_balance.write(current_sender_balance - amount);
        // TODO: Compiler complains if we don't assign the result of the write to anything
        let _hash = emit_unencrypted_log("Coins transferred");

        context.return_values.push(return_value);

        context.finish()
    }

Then add this e2e test:

  it.only('should not allow to transfer more than your balance', async () => {
    const mintAmount = 42n;

    await deployContract();

    await contract.methods.mint(mintAmount, recipient).send({ origin: recipient }).isMined();

    const failingTx = contract.methods
      .transfer(mintAmount + 1n, anotherRecipient)
      .send({ origin: recipient, skipPublicSimulation: true });

    await sleep(100);
    const shouldAlsoFailTx = contract.methods
      .transfer(1n, recipient)
      .send({ origin: anotherRecipient, skipPublicSimulation: true });

    await Promise.all([failingTx.isMined(), shouldAlsoFailTx.isMined()]);

    expect(await contract.methods.publicBalanceOf(recipient.toField()).view({ from: recipient })).toBe(mintAmount);
    expect(await contract.methods.publicBalanceOf(anotherRecipient.toField()).view({ from: anotherRecipient })).toBe(
      0n,
    );
  }, 60_000);

The test will pass, but the sequencer errors processing shouldAlsoFailTx at the kernel run step, because the simulation passed when it shouldn't:

  aztec:sequencer Error: Membership check failed: base_rollup_circuit: validate_public_data_update_requests index 1
  aztec:sequencer Refer to https://docs.aztec.network/aztec/protocol/errors for more information.
  aztec:sequencer     at Function.fromBuffer (/mnt/user-data/alvaro/aztec-packages/yarn-project/circuits.js/src/structs/circuit_error.ts:30:12)
  aztec:sequencer     at handleCircuitOutput (/mnt/user-data/alvaro/aztec-packages/yarn-project/circuits.js/src/utils/call_wasm.ts:104:30)
  aztec:sequencer     at callWasm (/mnt/user-data/alvaro/aztec-packages/yarn-project/circuits.js/src/utils/call_wasm.ts:47:20)
  aztec:sequencer     at RollupWasmWrapper.simulateBaseRollup (/mnt/user-data/alvaro/aztec-packages/yarn-project/circuits.js/src/rollup/rollup_wasm_wrapper.ts:24:12)
  aztec:sequencer     at WasmRollupCircuitSimulator.baseRollupCircuit (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/simulator/rollup.ts:37:51)
  aztec:sequencer     at SoloBlockBuilder.baseRollupCircuit (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/block_builder/solo_block_builder.ts:243:47)
  aztec:sequencer     at SoloBlockBuilder.runCircuits (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/block_builder/solo_block_builder.ts:216:30)
  aztec:sequencer     at SoloBlockBuilder.buildL2Block (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/block_builder/solo_block_builder.ts:105:37)
  aztec:sequencer     at Sequencer.buildBlock (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/sequencer/sequencer.ts:296:21)
  aztec:sequencer     at Sequencer.work (/mnt/user-data/alvaro/aztec-packages/yarn-project/sequencer-client/src/sequencer/sequencer.ts:171:21)
  aztec:sequencer     at poll (/mnt/user-data/alvaro/aztec-packages/yarn-project/foundation/src/running-promise/index.ts:22:9) +914ms
  aztec:sequencer Rolling back world state DB +30ms
  aztec:sequencer Processing 1 txs... +1s

This is not very problematic now that we drop failing transactions (dropping the problematic transaction ensures that the next time we try to build a block it'll succeed to pass the kernel verification), but the moment that we stop dropping those and that we try to include them in the block to collect the fees, the kernel verification will always fail.

@github-project-automation github-project-automation bot moved this to Todo in A3 Aug 29, 2023
@sirasistant sirasistant changed the title Public processor updates the merkle tree DB during execution Public processor updates the merkle tree DB for failing transactions Aug 29, 2023
@iAmMichaelConnor iAmMichaelConnor added the T-bug Type: Bug. Something is broken. label Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: Bug. Something is broken.
Projects
Status: Todo
Development

No branches or pull requests

2 participants