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

Key management API returns 404 on fee recipient #6050

Closed
yorickdowne opened this issue Aug 9, 2022 · 10 comments
Closed

Key management API returns 404 on fee recipient #6050

yorickdowne opened this issue Aug 9, 2022 · 10 comments
Assignees

Comments

@yorickdowne
Copy link

Description

When calling GET on eth/v1/validator/$__pubkey/feerecipient for a $__pubkey that has no individual recipient set, I get a 404 with message Fee recipient not found

As per https://github.com/ethereum/keymanager-APIs/blob/master/apis/fee_recipient.yaml it should return the value of --validators-proposer-default-fee-recipient instead

Likewise, a POST gets me Validator public key not found when attempting to set fee recipient.

The keys were imported with the key manager API, they are not "read-only" file system keys

@rolfyone
Copy link
Contributor

I updated validators-proposer-default-fee-recipient in my configuration, and was not receiving 404 for keys that I knew about...

Without this option set, I was receiving 404, but that's expected and different to what's reported above. I was able to add/remove recipients, and it got updated with 'accepted', and the file was written...

I was using read-only keys for setting/removing my fee-recipient in testing which is admittedly different to the reported setup, I'll have to setup non read-only keys to see if the behaviour differs, but code wise it looks like it should be consistent in that area for fee recipient.

@yorickdowne are you able to share the config settings you were using so that I can closely replicate what you had setup? Also the teku version in use? Obviously just the parameters you had set is fine, just curious that your behaviour is different to what I'm observing... Also if you were running a VC or BN where you saw the behaviour may influence how this all behaves. I'm testing with a BN currently, but can use a VC and see if that changes things. So if your keys are in VC, and you're querying VC, or if your keys are in VC and you're querying BN (I'd expect latter to fail in the way described).

It's possible another code change has fixed the reported issue, I'll try my testing with 22.7.0 and see if it differs.

@yorickdowne
Copy link
Author

yorickdowne commented Aug 19, 2022

--validators-proposer-default-fee-recipient is set. I've only seen this issue with keys imported via key manager, not with local read-only keys. Startup parameters are at https://github.com/eth-educators/eth-docker/blob/merge-getready/teku-base.yml

Teku version: teku/v22.8.0/linux-x86_64/-eclipseadoptium-openjdk64bitservervm-java-17

yorick@ethlinux:~/eth-docker-devel$ ./ethd keys list
Creating eth-docker-devel_validator-keys_run ... done
Validator public keys
0x9449c954822a6b836fdf1be20a27c3d4ff88d887e3619e998818ffa534b15a1a410d367da0d4ded4984ac747bfb074e4
0xa54bbe88143501ed50f7686967de1cbc158b6f1b121a929adc98fb56fae9a19ee8641b40e4c716f1ba2d631de0271d74
0xb44e54bd9715ef64e6963dd849a406ec54258b72278391ce1a241b2756c4ef2af47f47f9ec9258f6913fbe3ddd9497e1
yorick@ethlinux:~/eth-docker-devel$ ./ethd keys get-recipient 0x9449c954822a6b836fdf1be20a27c3d4ff88d887e3619e998818ffa534b15a1a410d367da0d4ded4984ac747bfb074e4
Creating eth-docker-devel_validator-keys_run ... done
Path not found error. Was that the right pubkey? Error: Fee recipient not found
ERROR: 1

Aha the plot thickens. This happens when starting Teku, importing keys via key manager, then attempting to get their fee recipient. If I then quit and restart Teku, I get this:

yorick@ethlinux:~/eth-docker-devel$ ./ethd keys get-recipient 0x9449c954822a6b836fdf1be20a27c3d4ff88d887e3619e998818ffa534b15a1a410d367da0d4ded4984ac747bfb074e4
Creating eth-docker-devel_validator-keys_run ... done
The fee recipient for the validator with public key 0x9449c954822a6b836fdf1be20a27c3d4ff88d887e3619e998818ffa534b15a1a410d367da0d4ded4984ac747bfb074e4 is:
0xADDRESS

Now I am wondering whether the default recipient applies to keys immediately after import. If not, that'd be a bit worrisome.

@rolfyone
Copy link
Contributor

Thanks for that, that's a great breakdown.
I was wondering if there was some kind of specific scenario -thanks for all the detail, it does sound like something odd is going on while we're adding keys but I'll dig from here and get more specifics.

@yorickdowne
Copy link
Author

The scenario is:

  • Start Teku without keys
  • Import keys with key manager
  • Attempt to read default recipient or set new recipient

This fails until Teku has been restarted, at which point it succeeds.

@rolfyone
Copy link
Contributor

I've got a similar thing happening with a minor caveat, and I think the 'caveat' is the point in time you're checking...

  • start VC with keymanager, have validator-proposer-default-fee-recipeint
  • add a key to VC, it comes back 'accepted'
  • request fee recipient: responds 404
  • wait until epoch transition
  • request fee recipient: responds with default fee recipient.

I'm planning on a tiny tweak that should let this work more cleanly, should be up in a few hours barring unexpected issues, but I figured I'd update here so that the scope of the problem is clearer...
The first epoch transition after adding a key updates data currently and after that the functions currently released will work.
There's a definite period where they'll report 404 because they (gas limit, fee recipient) can't find the key, so they think the key isn't ours. I'm planning on linking this more closely so that a better decision can be made about which keys we own.

rolfyone added a commit to rolfyone/teku that referenced this issue Aug 22, 2022
fixes Consensys#6050

Signed-off-by: Paul Harris <paul.harris@consensys.net>
@rolfyone
Copy link
Contributor

It concerns me that you're seeing this until restart, potentially something else is going on also, it may be related to 'validators-proposer-config-refresh-enabled'.

I'll have to check if I had that set. It may be that it needs to be defaulted to true if using the key manager or similar

rolfyone added a commit that referenced this issue Aug 23, 2022
)

partially addresses #6050

Signed-off-by: Paul Harris <paul.harris@consensys.net>
@rolfyone rolfyone removed their assignment Aug 23, 2022
@rolfyone
Copy link
Contributor

@yorickdowne this should be significantly better now, although I confess I wasn't able to replicate your 'until restart' stated above.
Would you be willing to see what you think of master now that this fix is merged?

@rolfyone rolfyone self-assigned this Aug 23, 2022
@yorickdowne
Copy link
Author

Unlike you I may not have waited an epoch before restarting 😅

I'll test master!

@yorickdowne
Copy link
Author

Yep this is great. Thanks for the changes, this helps the UX!

@rolfyone
Copy link
Contributor

Fantastic! thanks for the retest, I appreciate it

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

No branches or pull requests

3 participants