fix(faucet): resume after restart #517

polydez · 2024-10-11T14:03:41Z

Resolves #504

In this PR faucet web app uses the public faucet account stored in the genesis. User can keep and use faucet.mac file generated by genesis creation command or create a new public faucet account with new *.mac file. Faucet web app retrieves current faucet account state on start and creates new account on the first request, if it wasn't created before.

I also refactored error handling in the faucet. I think, anyhow is much more convenient for console application, so I use it for all initialization steps. For client, there is special ClientError which actually wraps anyhow::Error for now, because we just fail on any error and we may benefit more from good error backtrace instead of fine-grained enum variants.

bobbinth · 2024-10-15T08:46:56Z

Not a review yet (and I have barely looked at the actual code), but a couple of thoughts:

If the faucet is public (i.e., it is a public account), the only thing we need to save locally is the secret key, right? The thought here is that we can always retrieve the faucet state from the chain - and so, no need to save it to disk (we still want to keep it in memory as it is being done now, I think).

There are two cases when we may need to retrieve the faucet state from chain:

During faucet restart.
If trying to send a transaction to the node fails.

The latter case is meant to protect against situations where the faucet's in-memory state somehow gets out of sync with the node. If this happens, we try to get the most recent state from the chain and hopefully, everything will work fine after that. Failing to send a transaction may happen for other reasons though (e.g., network failures) - so, if we can handle these gracefully, that would be ideal.

If the faucet is private, then we do need to state some number of states locally - but is there a downside to making it public?

Mirko-von-Leipzig · 2024-10-15T09:35:13Z

If the faucet is private, then we do need to state some number of states locally - but is there a downside to making it public?

No downside afaik. One can make an argument that we should provide examples of both public and private implementations but that can wait (if we care to at all).

polydez · 2024-10-15T11:00:04Z

As long as transactions referring incorrect faucet account's state are rejected before putting to the block producer's transaction queue, it seems to be the easiest solution for us.

bobbinth · 2024-10-15T21:23:26Z

As long as transactions referring incorrect faucet account's state are rejected before putting to the block producer's transaction queue, it seems to be the easiest solution for us.

Let's do it this way then as this is how the block producer will work for the next couple of years.

… error

…server

bin/faucet/src/store.rs

Mirko-von-Leipzig · 2024-10-21T10:02:03Z

bin/faucet/src/store.rs

+        let empty_input_notes =
+            InputNotes::new(Vec::new()).map_err(DataStoreError::InvalidTransactionInput)?;


Suggested change

let empty_input_notes =

InputNotes::new(Vec::new()).map_err(DataStoreError::InvalidTransactionInput)?;

let empty_input_notes = InputNotes::new(Vec::new()).expect("Empty notes must succeed");

Though I think maybe InputNotes should simply implement Default or provide a const EMPTY. cc @bobbinth

They already have Default implemented so we should just use that :)

bin/faucet/src/store.rs

bin/faucet/src/main.rs

bin/faucet/src/client.rs

bin/faucet/src/errors.rs

Mirko-von-Leipzig · 2024-10-21T10:57:08Z

bin/faucet/src/client.rs

-fn build_account(config: FaucetConfig) -> Result<(Account, Word, SecretKey), InitError> {
+fn build_account(
+    config: &FaucetConfig,
+    init_seed: [u8; 32],


This is likely unrelated to this PR itself.

How is this seed related to the private key seed? Currently this one is simply always zeros afaict.

Do we have three different seeds at play?

Key-pair generation

Account building init seed

Account seed output from (2)

How do these interact/interplay? Not at all?

Good catch, thank you! This is not the only issue I can see here so far. One of the most important things here is a vulnerability with a secret key generation. Since we initialize random number generator with always the same seed, we get the same secret for each faucet. This means, everybody can generate it by themselves and interact with the faucet. We should fix it before merging of this PR.

SecretKey generation yields the public key which is stored on the account's storage which means it will affect the storage commitment, and thus the account seed (3 in your list) as well.

bin/faucet/src/handlers.rs

Mirko-von-Leipzig · 2024-10-21T11:23:29Z

bin/faucet/src/handlers.rs

-    let block_height = client.prove_and_submit_transaction(executed_tx).await?;
+        .map_err(|err| HandlerError::BadRequest(err.to_string()))?;
+
+    let (created_note, block_height) = loop {


The loop scares me a bit. We don't actually expect the state to desync -- and something like this happening would constitute a bug, or having spawned multiple faucets with the same configuration and secret keys -- which itself is an issue.

I suggest we do something like:

On faucet startup, fetch the account state. If the account is not yet on chain, then submit a transaction to declare it. This means that the non-startup txns should always have an account seed of None (iiuc). In other words, for all get_tokens calls the account is known to be on-chain and public etc since these conditions were ensured at startup. This fulfills the requirement to be restartable by itself already.

We then need to decide what to do if get_tokens fails due to state mismatch. imo we should crash the faucet or just not bother differentiating between it and other errors 😅 This is really an unexpected condition and means something is really off.

However we can decide to at least make this somewhat robust against this condition (though I will argue we should not). If we do want then I would rather do something like:

match client.prove_and_submit_transaction(...).await { Err(err) if err.is_state_mismatch() => { // update account state and resubmit txn }, other => other }

which would only retry once. The current loop achieves the same thing ofc, but it does invite trouble if we have a bug elsewhere in the logic causing an infinite retry loop. I would also suggest factoring out the state update code, and the txn proving & submission code to make this easy to read.

Yes, it makes sense, thank you. I will rewrite my solution to get initial state from the node on startup and fail if the state was changed unexpectedly.

I think I'm OK with the "retry once" strategy in case of state mismatch. Overall, here is my current understanding of what we are doing (correct me if I missed anything):

For now we are dealing with public faucets only (we can add support for private faucets later if needed).

On startup, we assume that our faucet is already on-chain and try to fetch its most recent state. If that fails, we crush the faucet.

During minting, we update the local state and submit the transaction to the node. If the transaction fails, we try to get the latest state from the node and try to submit the transaction again. If anything fails here we crush.

One question is how are the faucets created? For our faucet we can use the faucet created in the genesis block as discussed in #517 (comment). But what about other faucets (this could be an improvement we add later on)?

One question is how are the faucets created?

This actually creates the account if its not there as part of get_tokens afaict. Or at least I think that's what the data store's transaction inputs is doing?

Though I'm finding it difficult to keep track 😓 Not this PRs fault; things are quite muddled in the faucet code.

I think our end goal should be something like:

Move faucet account creation to a separate subcommand. This should create a random keypair and create the account on chain in miden-faucet init (but maybe a better name e.g. miden-faucet create).

miden-faucet start then loads account state on startup (crashes if missing), and just updates state in get_tokens. I don't think there is much point in retrying if the state is different - that would constitute a pretty bad internal failure.

Move faucet account creation to a separate subcommand. This should create a random keypair and create the account on chain in miden-faucet init (but maybe a better name e.g. miden-faucet create).

Agree with this - though we should also be able to "import" a faucet given it's ID and private key.

miden-faucet start then loads account state on startup (crashes if missing), and just updates state in get_tokens. I don't think there is much point in retrying if the state is different - that would constitute a pretty bad internal failure.

I'm fine with not doing it now if it complicates things, but I think retrying once could be useful. For example, what if there are two parallel instances of the faucet running and they make updates independently to the same account. Unlikely to happen, but also not sure if this should be a "catastrophic failure".

I'm fine with not doing it now if it complicates things, but I think retrying once could be useful. For example, what if there are two parallel instances of the faucet running and they make updates independently to the same account. Unlikely to happen, but also not sure if this should be a "catastrophic failure".

My counter argument would be that exactly one of the two would crash which to me is optimal solution 😬 And we would know about it instead of both just chugging along.

I like the solution proposed by @Mirko-von-Leipzig (and developed further by @bobbinth): explicitly create faucet account by special command or import of existing faucet by using account ID and secret. And I don't think that we really need to support two faucet web apps working in parallel, it's better to fail one of them (this significantly simplifies get_tokens method). This solution is close to what I'm implementing now, but not exactly the same, so I'm going to update my solution.

Thank you for help!

Co-authored-by: Mirko <[email protected]>

# Conflicts: # CHANGELOG.md

polydez · 2024-10-22T06:55:04Z

@bobbinth, @Mirko-von-Leipzig
We need to decide, what to do with faucets.

Currently we have 2 faucets:

Created in genesis, but not used, as I can see.
Created in the faucet web app.

This is unusual comparing to other blockchains where we have native asset (like ETH, SOL, etc.) which initialized in the genesis and tokens, provided by token contracts (like ERC20 in Etheretum, SPL Token in Solana and so on). Currently it seem that we don't have "native" asset in Miden, but some number of faucets, each minting their own token. Every user can create their own faucet(s) and asset(s).

This design looks good in terms of creation of different decentralized tokens, but what about native cryptocurrency? I think, we will need native Miden asset at least for implementing fees.

I propose to use precreated (predeployed, in other words) faucet from the genesis in our faucet web app. That means, our faucet will be the only place to mint native Miden cryptocurrency. Users will still be able to create their own faucets for tokens they wish, but they will have to use our faucet in testnet to get native cryptocurrency to pay fees and other tasks.

polydez · 2024-10-22T07:04:41Z

By the way, each asset has link to a faucet id which has minted this asset. But we won't use faucets in mainnet. We will probably have governance contract(s) or some other form of initial native cryptocurrency distribution. Will it be correctly to call them as faucets? Maybe mint id will better address the meaning of that field?

bobbinth · 2024-10-23T00:02:47Z

This is unusual comparing to other blockchains where we have native asset (like ETH, SOL, etc.) which initialized in the genesis and tokens, provided by token contracts (like ERC20 in Etheretum, SPL Token in Solana and so on). Currently it seem that we don't have "native" asset in Miden, but some number of faucets, each minting their own token. Every user can create their own faucet(s) and asset(s).

Technically, there are no "native" assets on Miden - or conversely, all assets are native. That is, there is nothing that currently sets one asset apart from another.

This design looks good in terms of creation of different decentralized tokens, but what about native cryptocurrency? I think, we will need native Miden asset at least for implementing fees.

It is possible to do fees without native assets though it would probably complicate the design somewhat. This is something we'll need to think through over the next couple of months.

I propose to use precreated (predeployed, in other words) faucet from the genesis in our faucet web app. That means, our faucet will be the only place to mint native Miden cryptocurrency. Users will still be able to create their own faucets for tokens they wish, but they will have to use our faucet in testnet to get native cryptocurrency to pay fees and other tasks.

I agree with this. It would be great to have the faucet webapp use the faucet that was generated in the genesis block. One question is how would this affect the deployment process.

By the way, each asset has link to a faucet id which has minted this asset. But we won't use faucets in mainnet. We will probably have governance contract(s) or some other form of initial native cryptocurrency distribution. Will it be correctly to call them as faucets? Maybe mint id will better address the meaning of that field?

I think many of these things are still to be defined. Re terminology, In our context "faucet" means "an account which can issue assets." It's definitely not ideal as "faucet" is used differently in other system - so, if we come up with a better term we can switch to it.

Mirko-von-Leipzig · 2024-10-23T07:37:01Z

I agree with this. It would be great to have the faucet webapp use the faucet that was generated in the genesis block. One question is how would this affect the deployment process.

I think its just the secret key that causes issues correct? This could be handled by either

storing the secret key in github secrets for automatic deployment, or
having some alternate custody scheme.

(1) would be good enough for now, but in the long run we'd probably want something else though I have no idea what that looks like.

….mac`, request faucet state on initialization

…z-faucet-restore # Conflicts: # CHANGELOG.md # bin/faucet/src/client.rs

Mirko-von-Leipzig

Should init and create-faucet-account be one command?

Mirko-von-Leipzig · 2024-10-28T14:13:50Z

crates/utils/src/crypto.rs

+pub fn get_rpo_random_coin<T: RngCore>(rng: &mut T) -> RpoRandomCoin {
+    let auth_seed: [u64; 4] = rng.gen();
+    let rng_seed = RpoDigest::from(auth_seed.map(Felt::new));
+
+    RpoRandomCoin::new(rng_seed.into())


I'm surprised that RpoRandomCoin doesn't implement https://docs.rs/rand/latest/rand/trait.SeedableRng.html.

Feels like this is a very natural use case.

Yes, this would simplify usage a bit. But this also would require a bit more complex conversion (from [u8; 32] to RpoDigest).

Mirko-von-Leipzig · 2024-10-28T14:29:03Z

bin/faucet/src/client.rs

+                ClientError::RequestError(status) if status.code() == tonic::Code::NotFound => {
+                    info!(target: COMPONENT, "Faucet account not found in the node");
+
+                    faucet_account_data.account
+                },


Is the idea that the mint transaction will generate the account in this scenario?

If so, could we not move that into the CreateFaucetAccount command? And then fail on all errors here instead.

It was my first try, but then I realized that it's not a straightforward thing. In order to submit transaction, we need to instantiate client with executor, storage and so on, so it will require at least refactoring of the client and all the stuff it uses for the transaction execution and submission. I'm not sure this is worth such refactoring, but I'm open to discussion.

The second challenging thing for me here is that we would need to create faucet account separately from minting transaction and I'm not sure if I understand how to do it correctly. :) Will it be enough to just omit execution script from transaction args and keep the remaining the same?

It was my first try, but then I realized that it's not a straightforward thing. In order to submit transaction, we need to instantiate client with executor, storage and so on, so it will require at least refactoring of the client and all the stuff it uses for the transaction execution and submission. I'm not sure this is worth such refactoring, but I'm open to discussion.

That's a fair point. I think overall the faucet code could do with a larger refactoring but definitely as a separate issue when we have some extra bandwidth again.

The second challenging thing for me here is that we would need to create faucet account separately from minting transaction and I'm not sure if I understand how to do it correctly. :) Will it be enough to just omit execution script from transaction args and keep the remaining the same?

Good question. I have no idea :D

# Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml # bin/faucet/src/client.rs

bobbinth

Thank you! Looks good! I left a few comments inline. Also, we should update the README file with new instructions on how to operate the faucet.

bin/node/src/commands/genesis/mod.rs

bin/faucet/src/main.rs

bin/faucet/src/client.rs

bobbinth

Looks good! Thank you! I left one more comment for the readme file. Once it is addressed, we can merge.

bobbinth · 2024-10-30T04:14:06Z

bin/faucet/README.md

Should we update step 1 in this file as well? I'm actually not sure if docker images work any more.

Also, is there some disconnect between step 1 and step 2? That is, if we run faucet in the testing mode, we should run node in the testing mode as well (maybe that's what's happening already, but I'd make it more explicit).

Yes both should be in the same mode. I do wish we could make the difficulty configurable and not gated behind a compile time feature. Then we could decouple things by having an rpc endpoint to fetch the current difficulty of the node for example.

@bobbinth,

Should we update step 1 in this file as well? I'm actually not sure if docker images work any more.

I've checked, it works.

Also, is there some disconnect between step 1 and step 2? That is, if we run faucet in the testing mode, we should run node in the testing mode as well (maybe that's what's happening already, but I'd make it more explicit).

If we run node by using Docker, it's created in "testing" mode. All instructions in faucet's readme build node and faucet in "testing" mode. I've improved documentation in order to make it more descriptive about that.

# Conflicts: # CHANGELOG.md

polydez added 8 commits October 18, 2024 20:16

feat: improve node errors

fcb66c7

docs: update CHANGELOG.md

40a42f1

feat: save/restore faucet state and keypair to/from files

8873ba5

feat: make faucet account public, request account state on submission…

9053f12

… error

feat: introduce ClientError

f984e67

fix: update config

8e12b04

fix: save new faucet state to in-memory store after getting from the …

4fc851f

…server

fix: remove macros feature from axum dependency

cebc372

polydez force-pushed the polydez-faucet-restore branch from 2c4b098 to cebc372 Compare October 18, 2024 15:24

polydez changed the base branch from next to polydez-improve-node-errors October 18, 2024 15:34

polydez changed the title ~~Save/restore faucet state and keypair to/from files~~ feat(faucet): resume after restart Oct 18, 2024

polydez changed the title ~~feat(faucet): resume after restart~~ fix(faucet): resume after restart Oct 18, 2024

docs: update CHANGELOG.md

be90b6b

polydez requested review from bobbinth and Mirko-von-Leipzig October 18, 2024 15:46

polydez marked this pull request as ready for review October 18, 2024 15:46

Mirko-von-Leipzig reviewed Oct 21, 2024

View reviewed changes

polydez and others added 3 commits October 21, 2024 18:49

docs: add comment for seed field

b808a80

Co-authored-by: Mirko <[email protected]>

fix: log message

dafc01b

Co-authored-by: Mirko <[email protected]>

refactor: address review comments

50fd73e

Base automatically changed from polydez-improve-node-errors to next October 21, 2024 19:20

Merge branch 'next' into polydez-faucet-restore

011002a

# Conflicts: # CHANGELOG.md

Fumuran and others added 3 commits October 25, 2024 01:55

chore: update changelog

af6a0f3

refactor: remove unnecessary async, add warning ignore to makefile

bf9bdc2

refactor: update genesis generation, read faucet account from `faucet…

349c43b

….mac`, request faucet state on initialization

polydez force-pushed the polydez-faucet-restore branch from e4004b4 to 349c43b Compare October 25, 2024 06:58

polydez and others added 9 commits October 25, 2024 17:47

feat: faucet account creation

bf0e97d

fix: handling of account not found error

c9f7fdd

refactor: use Arc and Mutex

9e5d91d

chore: removed unneeded cast

738eae0

fix: lint

df3a19b

docs: update CHANGELOG.md

915edf2

Merge branch 'refs/heads/andrew-migrate-to-miden-vm-0.11' into polyde…

573f5c3

…z-faucet-restore # Conflicts: # CHANGELOG.md # bin/faucet/src/client.rs

fix: compilation errors

4384a76

docs: add TODO for incorrect and unsafe Send implementation

ea5d59c

polydez changed the base branch from next to andrew-migrate-to-miden-vm-0.11 October 28, 2024 13:40

polydez requested a review from Mirko-von-Leipzig October 28, 2024 13:46

refactor: small refactoring

f111115

Mirko-von-Leipzig approved these changes Oct 28, 2024

View reviewed changes

Base automatically changed from andrew-migrate-to-miden-vm-0.11 to next October 28, 2024 17:59

Merge branch 'next' into polydez-faucet-restore

a6c37d0

# Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml # bin/faucet/src/client.rs

bobbinth reviewed Oct 29, 2024

View reviewed changes

bin/node/src/commands/genesis/mod.rs Show resolved Hide resolved

bin/node/src/commands/genesis/mod.rs Outdated Show resolved Hide resolved

bin/faucet/src/main.rs Outdated Show resolved Hide resolved

bin/faucet/src/client.rs Outdated Show resolved Hide resolved

polydez added 2 commits October 29, 2024 11:01

refactor: address review comments

0a9f068

docs: update README.md for faucet

112fcb6

polydez requested a review from bobbinth October 29, 2024 06:02

bobbinth approved these changes Oct 30, 2024

View reviewed changes

polydez added 2 commits October 30, 2024 16:49

docs: improve instructions and clarifications in faucet's README.md

d0b8b5c

Merge branch 'next' into polydez-faucet-restore

96d406d

# Conflicts: # CHANGELOG.md

polydez merged commit f541610 into next Oct 30, 2024
8 checks passed

polydez deleted the polydez-faucet-restore branch October 30, 2024 18:29

polydez mentioned this pull request Oct 30, 2024

[Bug]: testnet faucet is broken #504

Closed

tomyrd mentioned this pull request Oct 31, 2024

fix: genesis account name 0xPolygonMiden/miden-client#568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(faucet): resume after restart #517

fix(faucet): resume after restart #517

polydez commented Oct 11, 2024 •

edited

Loading

bobbinth commented Oct 15, 2024

Mirko-von-Leipzig commented Oct 15, 2024

polydez commented Oct 15, 2024 •

edited

Loading

bobbinth commented Oct 15, 2024

Mirko-von-Leipzig Oct 21, 2024

Mirko-von-Leipzig Oct 24, 2024

Mirko-von-Leipzig Oct 21, 2024

polydez Oct 21, 2024 •

edited

Loading

igamigo Oct 21, 2024

Mirko-von-Leipzig Oct 21, 2024 •

edited

Loading

polydez Oct 21, 2024

bobbinth Oct 23, 2024

Mirko-von-Leipzig Oct 23, 2024

Mirko-von-Leipzig Oct 23, 2024

bobbinth Oct 23, 2024

Mirko-von-Leipzig Oct 23, 2024

polydez Oct 23, 2024

polydez commented Oct 22, 2024 •

edited

Loading

polydez commented Oct 22, 2024 •

edited

Loading

bobbinth commented Oct 23, 2024

Mirko-von-Leipzig commented Oct 23, 2024

Mirko-von-Leipzig left a comment

Mirko-von-Leipzig Oct 28, 2024

polydez Oct 28, 2024

Mirko-von-Leipzig Oct 28, 2024

polydez Oct 28, 2024

Mirko-von-Leipzig Oct 29, 2024

bobbinth left a comment

bobbinth left a comment

bobbinth Oct 30, 2024

Mirko-von-Leipzig Oct 30, 2024

polydez Oct 30, 2024

		let empty_input_notes =
		InputNotes::new(Vec::new()).map_err(DataStoreError::InvalidTransactionInput)?;

	let empty_input_notes =
	InputNotes::new(Vec::new()).map_err(DataStoreError::InvalidTransactionInput)?;
	let empty_input_notes = InputNotes::new(Vec::new()).expect("Empty notes must succeed");

fix(faucet): resume after restart #517

fix(faucet): resume after restart #517

Conversation

polydez commented Oct 11, 2024 • edited Loading

bobbinth commented Oct 15, 2024

Mirko-von-Leipzig commented Oct 15, 2024

polydez commented Oct 15, 2024 • edited Loading

bobbinth commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mirko-von-Leipzig Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez commented Oct 22, 2024 • edited Loading

polydez commented Oct 22, 2024 • edited Loading

bobbinth commented Oct 23, 2024

Mirko-von-Leipzig commented Oct 23, 2024

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez commented Oct 11, 2024 •

edited

Loading

polydez commented Oct 15, 2024 •

edited

Loading

polydez Oct 21, 2024 •

edited

Loading

Mirko-von-Leipzig Oct 21, 2024 •

edited

Loading

polydez commented Oct 22, 2024 •

edited

Loading

polydez commented Oct 22, 2024 •

edited

Loading