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

Transfer_from Duplicate request errors when transfering from different accounts #112

Open
ByronBecker opened this issue Mar 8, 2024 · 7 comments

Comments

@ByronBecker
Copy link

In local testing, I'm currently batching multiple transfer_from calls from a canister to the cycles ledger.

In this simple case, I set up 16 cycle ledger developer "user" accounts, and then make calls in parallel to transfer the exact same amount of cycles from each user to an account owned by a canister (the receiver account is the same for all 16 calls).

Even though the "sender" of cycles in each case is different, I'm receiving duplication errors.

When I stringify the resulting errors (in Motoko) I see one successful transfer, and the rest recognized as duplicates.

[{"ok":"720"},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}},{"err":{"Duplicate":{"duplicate_of":"720"}}}]

Following these deduplication instructions and adding unique memo content for each request solves this problem (no deduplication errors), but it feels like if the sender and receiver are different, the payload is structurally different and this deduplication error shouldn't occur (it shouldn't require me adding a unique memo).

@ByronBecker
Copy link
Author

If it helps, in Motoko, this how one of the requests is made to the cycles ledger canister.

    await* cyclesLedgerActor.icrc2_transfer_from({
      to = {
        owner = args.cycleOpsPrincipal;
        subaccount = args.subaccount };
        fee = ?100000000;
        spender_subaccount = null;
        from = args.account;
        memo = args.memo;
        created_at_time = ?Nat64.fromNat(Int.abs(Time.now()));
        amount = args.amount;
      }
    );

@THLO
Copy link
Collaborator

THLO commented Mar 8, 2024

Thanks for reporting! We'll look into it.
Just to be sure I understand, requests are made from different "users", that is, each request has a different spender, correct?
Do all requests spend funds from the same account? If that's the case, then the requests are identical except for the spender field.

@ByronBecker
Copy link
Author

ByronBecker commented Mar 9, 2024

Just to be sure I understand, requests are made from different "users", that is, each request has a different spender, correct?

Each request is coming from the same account (initiated by the canister principal that wants to transfer the cycles from each sender to it). Each request has a different from account.

Do all requests spend funds from the same account? If that's the case, then the requests are identical except for the spender field.

All requests use transfer_from to transfer funds from different (unique) accounts to a the same receiving account (i.e. many unique -> one).

We are not setting/using the spender_subaccount field (it is null in all of our requests). We are just specifying the from account. Every request has a unique from account value, but they all have the same to account value.

@ByronBecker
Copy link
Author

@THLO 👋

Just wanted to follow up on this.

@MarioDfinity
Copy link
Contributor

MarioDfinity commented Mar 12, 2024

Hey @ByronBecker , I've tested the behavior and I could not reproduce. Let me tell you what I've done so that you can tell me if I've done something wrong:

  1. I setup the cycles-ledger
  2. I deposited 10T cycles to two accounts, account1 and account2
  3. I created a new canister with principal canister_id. The idea is to use the account owned by canister_id with subaccount 32 zeros as spender and the account owned by canister_id with subaccount 32 unos as receiver of the cycles
  4. I approved the spender to spend 1T tokens from account1
  5. I approved the spender to spend 1T tokens from account2
  6. I transfer_from as the spender 1 cycle from account1 to receiver
  7. I transfer_from as the spender 1 cycle from account2 to receiver

Both transfers go through successfully, no deduplication happens. The arguments of the two icrc2_transfer_from are the same except for the from which in the first case if account1 while in the second case is accoun2.

The code of the test is:

#[test]
fn test_icrc2_transfer_from_deduplicate_sender() {
    let env = TestEnv::setup();

    let account1 = account(1, None);
    let account2 = account(2, None);

    let canister_id = env.state_machine.create_canister(None);
    let spender = Account::from(canister_id);
    let account_to = Account {
        owner: canister_id,
        subaccount: Some([1;32]),
    };

    let _deposit_res = env.deposit(account1, 10_000_000_000_000, None);
    let _deposit_res = env.deposit(account2, 10_000_000_000_000, None);

    let args = ApproveArgs {
        from_subaccount: account1.subaccount,
        spender,
        amount: Nat::from(1_000_000_000_000u64),
        expected_allowance: None,
        expires_at: None,
        fee: None,
        memo: None,
        created_at_time: None,
    };
    let _approve_index = env.icrc2_approve_or_trap(account1.owner, args.clone());
    let args = ApproveArgs {
        from_subaccount: account2.subaccount,
        ..args
    };
    let _approve_index = env.icrc2_approve_or_trap(account2.owner, args);

    let args = TransferFromArgs {
        spender_subaccount: spender.subaccount,
        from: account1,
        to: account_to,
        amount: Nat::from(1u64),
        fee: None,
        memo: None,
        created_at_time: Some(env.nanos_since_epoch_u64()),
    };
    let _block_index = env.icrc2_transfer_from_or_trap(spender.owner, args.clone());
    let args = TransferFromArgs {
        from: account2,
        ..args
    };
    let _block_index = env.icrc2_transfer_from_or_trap(spender.owner, args);
}

@ByronBecker
Copy link
Author

ByronBecker commented Mar 12, 2024

@MarioDfinity

A few differences that could potentially trigger this.

  1. Our test is run with a few more accounts than this test (16 accounts)
  2. Our test is run from local dfx (0.16.1) running with default artificial delay
  3. Are these requests happening in parallel (i.e. sent out in parallel without waiting for a response)?
    let _block_index = env.icrc2_transfer_from_or_trap(spender.owner, args.clone());
    let args = TransferFromArgs {
        from: account2,
        ..args
    };
    let _block_index = env.icrc2_transfer_from_or_trap(spender.owner, args);

Here is the batching pattern we're using to make parallel async requests, as well as how we're using the memo as a de-duplication field to get around the error. Hopefully the code makes evident how we are batching transfer_from requests in parallel, and then collecting each future's result.

Is this similar to the behavior covered by your tests?


  // In batch, transfers cycles from the customer account to a cycles ledger account
  // Returns the block index of all transfers
  public func transferCyclesFromCustomerBatch(
    args : {
      caller : Principal;
      // this is a function parameter that is essentially a wrapper around the call to the cycle ledger's icrc2_transfer_from endpoint
      cycleLedgerTransferFrom : WrappedCycleLedger.TransferFrom;
      cycleOpsPrincipal : Principal;
      targets : [(Principal, Nat)];
      subaccount : ?Blob;
      maxBatch : Nat;
      store : TypesV4.CustomerConfigurationStore;
    }
  ) : async* [Result.Result<Nat, CycleLedger.TransferFromError>] {
    // used for deduplication
    // see https://internetcomputer.org/docs/current/references/icrc1-standard#transaction-deduplication-
    var deduplicationMemo = 0;

    let resultBuffer = Buffer.Buffer<Result.Result<Nat, CycleLedger.TransferFromError>>(1);

    let futureBuffer = Buffer.Buffer<(async* Result.Result<Nat, CycleLedger.TransferFromError>)>(1);

    for ((customerId, cycleTransferAmount) in args.targets.vals()) {
      futureBuffer.add(args.cycleLedgerTransferFrom({
        caller = args.caller;
        cycleOpsPrincipal = args.cycleOpsPrincipal;
        customerId;
        amount = cycleTransferAmount;
        subaccount = args.subaccount;
        /* Turn the deduplication Nat into a utf8 encoded blob for the memo */
        memo = ?Text.encodeUtf8(Nat.toText(deduplicationMemo));
      }));

      deduplicationMemo += 1;
      if (futureBuffer.size() > args.maxBatch) {
        for (thisFuture in futureBuffer.vals()) {
          resultBuffer.add(await* thisFuture);
        };
        futureBuffer.clear();
      };
    };

    for (thisFuture in futureBuffer.vals()) {
      resultBuffer.add(await* thisFuture);
    };

    return Buffer.toArray(resultBuffer);
  };

You can imagine that each cycleLedgerTransferFrom() call ends up making this call to the cycle ledger

  await* cyclesLedgerActor.icrc2_transfer_from({
      to = {
        owner = args.cycleOpsPrincipal;
        subaccount = args.subaccount };
        fee = ?100000000;
        spender_subaccount = null;
        from = args.account;
        memo = args.memo;
        // since all requests are made in parallel (during the same round of consensus), this time is the same for all requests
        created_at_time = ?Nat64.fromNat(Int.abs(Time.now()));
        amount = args.amount;
      }
    );

@MarioDfinity
Copy link
Contributor

@ByronBecker can you fetch the blocks created by the Ledger and print them here? Use the endpoint icrc3_get_blocks. I'm curious to see their content.

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

No branches or pull requests

3 participants