added check for fee recipient per validator and added unit tests #8454

AbolareRoheemah · 2025-11-24T10:09:27Z

Issue Addressed

Addresses #5403

Proposed Changes

Added check_fee_recipient() method to validate individual validators
Added check_all_fee_recipients() to validate all validators on startup
Validator client now fails to start if any enabled validator lacks a fee recipient and no global flag is used.
Added Clear error messages to guide users on how to fix the issue
Added unit tests

Additional Info

The images show attempts to run the VC: 1. without fee-recipient set for a validator and no global flag 2. Only global flag used. No individual fee-recipient set

cla-assistant · 2025-11-24T10:09:41Z

All committers have signed the CLA.

cla-assistant · 2025-11-24T10:09:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

eserilev

Thanks for this! The added tests are very much appreciated. I think we can make some slight tweaks here to improve the UX a bit.

eserilev · 2025-12-08T15:33:39Z

common/account_utils/src/validator_definitions.rs

+
+        // If nothing set
+        Err(format!(
+            "Validator {} is missing `suggested_fee_recipient`!\n\n\


Can we update the error message to

Validator {} is missing a `suggested_fee_recipient`. Fix this by adding a `suggested_fee_recipient` in your `validator_definitions.yml` or by supplying a fallback fee recipient via the `--suggested-fee-recipient` flag.

We usually avoid \n. We can use \ to continue the string literal across multiple lines

eserilev · 2025-12-08T15:40:29Z

common/account_utils/src/validator_definitions.rs

        })
    }
+
+    pub fn check_fee_recipient(&self, global_fee_recipient: Option<Address>) -> Result<(), String> {


I think for better UX, we should iterate across all fee recipients, build a list of ones missing fee recipients and then log an error to the user. This way they can see the full list of missing fee recipients all at once.

eserilev · 2025-12-08T15:41:45Z

common/account_utils/src/validator_definitions.rs

+                    "Global --suggested-fee-recipient is being used for {} validator(s). \
+                        Consider setting it in validator_definitions.yml for each one.",


Suggested change

"Global --suggested-fee-recipient is being used for {} validator(s). \

Consider setting it in validator_definitions.yml for each one.",

"The fallback --suggested-fee-recipient is being used for {} validator(s). \

Consider setting the fee recipient for each validator individually via `validator_definitions.yml`.",

eserilev · 2025-12-08T15:41:57Z

common/account_utils/src/validator_definitions.rs

+                .filter(|d| d.enabled && d.suggested_fee_recipient.is_none())
+                .count();
+            if missing > 0 {
+                tracing::warn!(


Suggested change

tracing::warn!(

tracing::info!(

…d modified code comment

AbolareRoheemah · 2025-12-11T16:01:41Z

Thanks for this! The added tests are very much appreciated. I think we can make some slight tweaks here to improve the UX a bit.

Hello @eserilev. Much thanks for the review and the kind words. 🙏

I thought to explain the changes I made with this last push. The check_fee_reciepient function now returns the public key of the validator with missing fee recipient. This is done to make collecting all missing validators easy.
Then check_all_fee_recipients now gathers all validators missing a fee recipient and reports them together in a single error message (comma-separated list).

I also modified existing tests to work with the new check_fee_reciepient return type and added a test (check_all_fee_recipients_reports_all_missing) that verifies multiple missing validators are all reported in a single error.

mergify · 2026-01-06T02:44:07Z

This pull request has merge conflicts. Could you please resolve them @AbolareRoheemah? 🙏

…ke-fee-receipient-mandatory

eserilev

LGTM!

mergify · 2026-01-09T01:38:31Z

Some required checks have failed. Could you please take a look @AbolareRoheemah? 🙏

eserilev · 2026-01-09T02:09:21Z

Hi @AbolareRoheemah can you please run cargo fmt --all to fix the CI issues when you get the chance

chong-he

Looks pretty good to me. I have also tested manually and confirm that it's working.

Some comments below.

chong-he · 2026-01-09T02:19:21Z

common/account_utils/src/validator_definitions.rs

+        };
+
+        // Should return None since global fee recipient is set
+        let global_fee = Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());


This is just a nit for better clarity

Suggested change

let global_fee = Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());

let global_fee_recipient = Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());

chong-he · 2026-01-09T02:24:07Z

common/account_utils/src/validator_definitions.rs

+        let check_result = def.check_fee_recipient(global_fee);
+        assert!(check_result.is_none());


For succinctness, we can combine this with the test fee_recipient_check_fails_when_missing() to avoid repeatedly defining the same fields. We can revise the test name a bit, then add the checks in the same tests

Edit: I believe we can even go "all out" for succinctness, to define a general ValidatorDefinitions, then for each test, update the necessary fields for each test purpose, and add an assert right below, i.e., instead of having multiple tests, we can have just 1 or 2 test functions, move the test name (and revise a bit) and put it as a comment right above and before the assert. This will be cleaner as well

chong-he · 2026-01-09T02:28:59Z

common/account_utils/src/validator_definitions.rs

+
+            return Err(format!(
+                "The following validators are missing a `suggested_fee_recipient`: {}. \
+                 Fix this by adding a `suggested_fee_recipient` in your \


I feel that the validator_definitions.yml reads better here, but feel free to take it or leave it

Suggested change

Fix this by adding a `suggested_fee_recipient` in your \

Fix this by adding a `suggested_fee_recipient` in the \

chong-he · 2026-01-09T02:41:09Z

common/account_utils/src/validator_definitions.rs

+            if count > 0 {
+                info!(
+                    "The fallback --suggested-fee-recipient is being used for {} validator(s). \
+                     Consider setting the fee recipient for each validator individually via `validator_definitions.yml`.",


This is about "reducing the severity" of the log, because if users only use the suggested-fee-recipient flag, this log will appear every time a VC starts, so it could be a bit of a warn thingy.

This is a my two cent, keep to hear from the author and other developers about this. (e.g., maybe even shift to debug log)?

Suggested change

Consider setting the fee recipient for each validator individually via `validator_definitions.yml`.",

You may alternatively set the fee recipient for each validator individually via `validator_definitions.yml`.",

added check for fee recipient per validator and added unit tests

4c0dbc5

eserilev requested changes Dec 8, 2025

View reviewed changes

eserilev added UX-and-logs val-client Relates to the validator client binary backwards-incompat Backwards-incompatible API change waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 8, 2025

error now lists all missing validators, using info instead of warn an…

a5443ad

…d modified code comment

jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 6, 2026

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 6, 2026

Merge branch 'unstable' of https://github.com/sigp/lighthouse into ma…

5fe8cb8

…ke-fee-receipient-mandatory

eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 7, 2026

eserilev approved these changes Jan 7, 2026

View reviewed changes

eserilev requested a review from chong-he January 7, 2026 17:26

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 9, 2026

chong-he reviewed Jan 9, 2026

View reviewed changes

		"Global --suggested-fee-recipient is being used for {} validator(s). \
		Consider setting it in validator_definitions.yml for each one.",

	let global_fee = Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());
	let global_fee_recipient = Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap());

		let check_result = def.check_fee_recipient(global_fee);
		assert!(check_result.is_none());

	Fix this by adding a `suggested_fee_recipient` in your \
	Fix this by adding a `suggested_fee_recipient` in the \

	Consider setting the fee recipient for each validator individually via `validator_definitions.yml`.",
	You may alternatively set the fee recipient for each validator individually via `validator_definitions.yml`.",

added check for fee recipient per validator and added unit tests #8454

Are you sure you want to change the base?

added check for fee recipient per validator and added unit tests #8454

Conversation

AbolareRoheemah commented Nov 24, 2025

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

cla-assistant bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-assistant bot commented Nov 24, 2025

Uh oh!

eserilev left a comment

Choose a reason for hiding this comment

Uh oh!

eserilev Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

AbolareRoheemah commented Dec 11, 2025

Uh oh!

mergify bot commented Jan 6, 2026

Uh oh!

eserilev left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jan 9, 2026

Uh oh!

eserilev commented Jan 9, 2026

Uh oh!

chong-he left a comment

Choose a reason for hiding this comment

Uh oh!

chong-he Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

chong-he Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chong-he Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

chong-he Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cla-assistant bot commented Nov 24, 2025 •

edited

Loading

chong-he Jan 9, 2026 •

edited

Loading