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

Client is less parallelism-safe than expected #3988

Closed
elsirion opened this issue Dec 22, 2023 · 2 comments · Fixed by #4407
Closed

Client is less parallelism-safe than expected #3988

elsirion opened this issue Dec 22, 2023 · 2 comments · Fixed by #4407
Assignees
Labels
bug Something isn't working client
Milestone

Comments

@elsirion
Copy link
Contributor

Reproducible using this test in modules/fedimint-mint-tests/tests/tests.rs:

#[tokio::test(flavor = "multi_thread")]
async fn sends_ecash_oob_highly_parallel() -> anyhow::Result<()> {
    // Print notes for client1
    let fed = fixtures().new_fed().await;
    let (client1, client2) = fed.two_clients().await;
    let client1_dummy_module = client1.get_first_module::<DummyClientModule>();
    let (op, outpoint) = client1_dummy_module.print_money(sats(1000)).await?;
    client1.await_primary_module_output(op, outpoint).await?;

    let num_par = 2;

    // Spend from client1 to client2 25 times in parallel
    let mut spend_tasks = vec![];
    for num_spend in 0..num_par {
        let task_client1 = client1.clone();
        spend_tasks.push(tokio::spawn(async move {
            info!("Starting spend {num_spend}");
            let client1_mint = task_client1.get_first_module::<MintClientModule>();
            let (op, notes) = client1_mint
                .spend_notes(sats(30), TIMEOUT, ())
                .await
                .unwrap();
            let sub1 = &mut client1_mint
                .subscribe_spend_notes(op)
                .await
                .unwrap()
                .into_stream();
            assert_eq!(sub1.ok().await.unwrap(), SpendOOBState::Created);
            notes
        }));
    }

    let note_bags = futures::stream::iter(spend_tasks)
        .then(|handle| async move { handle.await.unwrap() })
        .collect::<Vec<_>>()
        .await;

    let mut reissue_tasks = vec![];
    for (num_reissue, notes) in note_bags.into_iter().enumerate() {
        let task_client2 = client2.clone();
        reissue_tasks.push(tokio::spawn(async move {
            info!("Starting reissue {num_reissue}");
            let client2_mint = task_client2.get_first_module::<MintClientModule>();
            let op = client2_mint
                .reissue_external_notes(notes, ())
                .await
                .unwrap();
            let sub2 = client2_mint
                .subscribe_reissue_external_notes(op)
                .await
                .unwrap();
            let mut sub2 = sub2.into_stream();
            assert_eq!(sub2.ok().await.unwrap(), ReissueExternalNotesState::Created);
            assert_eq!(sub2.ok().await.unwrap(), ReissueExternalNotesState::Issuing);
            assert_eq!(sub2.ok().await.unwrap(), ReissueExternalNotesState::Done);
        }));
    }

    for task in reissue_tasks {
        task.await.unwrap();
    }

    assert_eq!(client1.get_balance().await, sats(1000 - 30 * num_par));
    assert_eq!(client2.get_balance().await, sats(30 * num_par));
    Ok(())
}

Errors with

thread 'tokio-runtime-worker' panicked at modules/fedimint-mint-tests/tests/tests.rs:107:13:
assertion `left == right` failed
  left: Failed("Transaction not accepted \"The transaction had an invalid input\"")
 right: Issuing

which indicates coin-selection being broken/re-using e-cash notes?!

@elsirion
Copy link
Contributor Author

Turns out this is a problem with the MemDatabase implementation that doesn't detect conflicts between transactions. If this test is run with rocksdb it works: elsirion@3b251c8

It might be useful to have a DB impl with our required isolation level that works with any non-transactional KV-backend. For example webimint currently uses the MemDatabase and just persists it, which is very inefficient. If there was a generic implementation I think I could make it only persist the relevant entries. See elsirion/webimint-rs#31 (comment).

@elsirion
Copy link
Contributor Author

@elsirion validate that the test works after #3989 and close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants