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

fix: flaky peg_outs_are_only_allowed_once_per_epoch #1612

Merged
merged 1 commit into from Feb 7, 2023

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Feb 7, 2023

I actually didn't know why it's failing and had to do some debugging
and decided to leave these in.

user.client.reissue_pending_notes(rng()).await.unwrap();
fed.run_consensus_epochs(2).await; // reissue the notes from the tx that failed
fed.await_consensus_epochs(2).await.unwrap(); // reissue the notes from the tx that failed
user.client.fetch_all_notes().await.unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how it worked before without this. After we call reissue_pending_notes, we need to fetch the new issuance, no?

@jkitman

Copy link
Contributor

Choose a reason for hiding this comment

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

The fetch_all_notes is in the assert removed below.

@@ -633,14 +636,19 @@ impl<T: AsRef<ClientConfig> + Clone> Client<T> {

/// Should be called after any transaction that might have failed in order to get any note
/// inputs back.
#[instrument(skip_all, level = "debug")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -49,6 +49,11 @@ impl<T> TieredMulti<T> {
self.0.keys()
}

/// Returns the summary of number of items in each tier
pub fn summary(&self) -> Tiered<usize> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to turn it into a separate struct that displays counts per tier and total together.

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Looks good to me

@dpc dpc merged commit 2b53592 into fedimint:master Feb 7, 2023
@dpc dpc deleted the parallel-tests-fix branch February 7, 2023 22:39
@dpc
Copy link
Contributor Author

dpc commented Feb 7, 2023

Merging, but please @jkitman help me!

I pushed us into this deep water of parallel test execution, and turns out I don't always have enough context to fix issues that arise with it. :D

dbtx.remove_entry(&key).await.expect("DB Error");
}
dbtx.commit_tx().await.expect("DB Error");

self.reissue(all_notes, rng).await
debug!(target: LOG_WALLET, notes_to_reissue = ?notes_to_reissue.summary(), total = %notes_to_reissue.total_amount());
trace!(target: LOG_WALLET, ?notes_to_reissue, "foo");
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I think these are useful, and helped me to debug this.

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.

None yet

3 participants