fedimint / ui

https://ui-umber-ten.vercel.app
MIT License
27 stars 41 forks source link

Follower guardians cannot submit config gen params #397

Closed okjodom closed 6 months ago

okjodom commented 6 months ago

After connecting to the lead guardian, the RPC call by followers to submit their own config gen params fails with the error below image

Replicated on master branch @https://github.com/fedimint/ui/commit/b9bd211e3b69a89e85a18ebdc98bcacde785296e

okjodom commented 6 months ago

I've been investigating this for two hours now. I noted this bug does not repro when we run setup UI on local FM with env DEVIMINT_BIN=$(realpath ../fedimint/target/debug) yarn nix-guardian

okjodom commented 6 months ago

When followers request config gen params from leader, they get an error, with the leader server reporting the following error

2024-03-27T17:26:25.730039Z  WARN net::api: api request error path="consensus_config_gen_params" error=ApiError { code: 400, message: "Config params were not set on this guardian"
 }
okjodom commented 6 months ago

I suspect that after https://github.com/fedimint/fedimint/pull/4520 , rpc by peers trying to get consensus_config_gen_params from lead peer server fails with the above error.

Do you know what could be going wrong, @dpc

Kodylow commented 6 months ago

Working on this rn, also noticed that setup leader adds the peer as pending even though it fails on the follower with missing field 'consensus'

image

image
Kodylow commented 6 months ago

The "consensus" value here in ConfigGenModuleParams is required to parse the submitted followers config. but the follower config only submits "local"

image

image
Kodylow commented 6 months ago

Introduced by this change by @dpc , going to try mirroring the local values into consensus but not sure what the difference is between them functionally for the setup. We have the followers submitting just local and the leader submits both local and consensus currently which is causing the error

image

Kodylow commented 6 months ago

Checked with eric and made a fix in that PR, we were just filtering out the 'consensus' key and values for the followers which is unnecessary now