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.

Sign up for GitHub

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

Jump to bottom

add user determined fees to peg-outs #276

Merged
merged 2 commits into from
Jul 20, 2022
Merged

add user determined fees to peg-outs #276

merged 2 commits into from
Jul 20, 2022

Conversation

jkitman
Copy link
Contributor

@jkitman jkitman commented Jul 15, 2022 •
edited

Stops the fed from crashing due to peg-outs (fixes step 1 in #233)

  • Peg-outs now only can happen once per epoch and only one peg-out is allowed per UTXO.
  • Broadcasts are immediate after a peg-out is signed.
  • Fees are exact and determined by the estimated min. sat/byte required by FeeConsensus (target confirmation time = 10 blocks) and the calculated transaction weight (no more profits/losses).
  • Rather than always returning 404 if a tx failed the fed will store and return the TransactionStatus::Rejected, allowing clients react to tx errors.
  • Stores pending coins and will reissue them in reissue_pending_coins if a tx is rejected (fixes Make client architecture crash-safe #35)

@jkitman jkitman requested a review from elsirion July 15, 2022 22:48
NicolaLS
NicolaLS approved these changes Jul 16, 2022
View reviewed changes
Copy link
Contributor

@NicolaLS NicolaLS left a comment

Choose a reason for hiding this comment

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

nice, I think the fetch_peg_out_fees naming is a bit confusing tough

client/client-lib/src/api.rs Outdated Show resolved Hide resolved
client/client-lib/src/api.rs Show resolved Hide resolved
client/client-lib/src/transaction.rs Outdated Show resolved Hide resolved
client/client-lib/src/lib.rs Outdated Show resolved Hide resolved
client/client-lib/src/mint/mod.rs Outdated Show resolved Hide resolved
elsirion
elsirion reviewed Jul 17, 2022
View reviewed changes
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.

Great PR! Just a few questions :)

client/client-lib/src/mint/db.rs Outdated Show resolved Hide resolved
client/client-lib/src/mint/db.rs Show resolved Hide resolved
modules/minimint-wallet/src/lib.rs Show resolved Hide resolved
client/client-lib/src/lib.rs Show resolved Hide resolved
minimint-core/src/outcome.rs Outdated Show resolved Hide resolved
minimint/src/consensus/conflictfilter.rs Outdated Show resolved Hide resolved
minimint-core/src/transaction.rs Show resolved Hide resolved
@jkitman jkitman requested a review from elsirion July 17, 2022 19:06
elsirion
elsirion approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@elsirion elsirion left a comment •
edited

Choose a reason for hiding this comment

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

Only reviewed the diff since my last review, only two nits, otherwise LGTM. Waiting for @justinmoon to take a look.

client/client-lib/src/mint/mod.rs Outdated
);
let txid = final_tx.tx_hash();
let mint_tx_id = self.context.api.submit_transaction(final_tx).await?;
// TODO: make check part of submit_transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove that TODO now 😄

client/client-lib/src/mint/mod.rs

tx.build(self.context.secp, &mut rng)
self.context.db.apply_batch(batch).expect("DB error");
Copy link
Contributor

Choose a reason for hiding this comment

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

From a crash-safety pov we should first save and then submit imo. But that's a bigger issue (#35) and could be a separate PR, just something we need to keep in mind.

Copy link
Contributor Author

@jkitman jkitman Jul 20, 2022 •
edited

Choose a reason for hiding this comment

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

Isn't it the other way around? If the tx fails we shouldn't change anything in the DB?

I thought this PR was fixing #35

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what we want is:

  1. "Stage" tx in DB (this PR introduces this intermediate state)
  2. Submit tx
  3. If an error occurs either on submit or the tx isn't confirmed for some reason we can later rollback the staged tx in the client DB. If the tx succeeds we finalize the staged tx in the client DB.

I assumed this apply_batch invocation was for step 1, but maybe I'm mistaken?

@jkitman jkitman requested a review from justinmoon July 20, 2022 13:29
@justinmoon
Copy link
Contributor

LGTM. I sent kitman a couple messages on discord. In general it would be nicer to have smaller commits (I make huge commits too, working on it!). But looks like a great improvement.

@jkitman jkitman force-pushed the add-user-determined-fees branch 2 times, most recently from 98e732d to a1142e4 Compare July 20, 2022 17:01
@jkitman jkitman merged commit 8660bf5 into fedimint:master Jul 20, 2022
@justinmoon justinmoon mentioned this pull request Sep 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elsirion elsirion elsirion approved these changes

@NicolaLS NicolaLS NicolaLS approved these changes

@justinmoon justinmoon Awaiting requested review from justinmoon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make client architecture crash-safe
4 participants
@jkitman @justinmoon @elsirion @NicolaLS