Skip to content

Refactor payload_attestation_service and add payload attestation test to validator client#9357

Merged
mergify[bot] merged 25 commits into
sigp:unstablefrom
chong-he:gloas-vc-test
Jun 10, 2026
Merged

Refactor payload_attestation_service and add payload attestation test to validator client#9357
mergify[bot] merged 25 commits into
sigp:unstablefrom
chong-he:gloas-vc-test

Conversation

@chong-he

@chong-he chong-he commented May 26, 2026

Copy link
Copy Markdown
Member

Additional Info

Heavily AI-assisted using Claude Code.

Thanks @jimmygchen for the help along the way.

jimmygchen
jimmygchen reviewed May 27, 2026
View reviewed changes
Comment thread validator_client/validator_services/src/payload_attestation_service.rs Outdated
jimmygchen
jimmygchen reviewed May 27, 2026
View reviewed changes
Comment thread validator_client/validator_services/src/payload_attestation_service.rs
jimmygchen
jimmygchen reviewed May 27, 2026
View reviewed changes
Comment thread testing/validator_test_rig/src/mock_validator_store.rs Outdated
jimmygchen
jimmygchen requested changes May 27, 2026
View reviewed changes

@jimmygchen jimmygchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@chong-he nice work overall! just a few small comments, let me know your thoughts.

@jimmygchen jimmygchen added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label May 27, 2026
jimmygchen
jimmygchen reviewed May 27, 2026
View reviewed changes
Comment thread testing/validator_test_rig/src/mock_beacon_node.rs Outdated
);
let body = request.body().expect("Failed to get request body");

let message = PayloadAttestationMessage::from_ssz_bytes(body)

@jimmygchen jimmygchen May 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The request payload is a list of messages, so this won't work when multiple messages are submitted. May be worth adding a test for this scenario too

@chong-he chong-he May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should be able to handle multiple messages for the real endpoint right? In that case, I should modify the mock_beacon_node to handle that, and then add a test for multiple messages. Sounds good?

@chong-he

chong-he commented May 31, 2026

Copy link
Copy Markdown
Member Author

This PR refactors the code to decouple the functions start_update_service and produce_and_publish (from @jimmygchen), so that we can test them separately (as testing them together proved tricky). I have carefully review the refactor part of the code to ensure that the logic and flow remains the same.

It is now ready for review.

@chong-he chong-he marked this pull request as ready for review May 31, 2026 22:51
@chong-he chong-he 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 May 31, 2026
@jimmygchen jimmygchen self-requested a review June 2, 2026 06:57
jimmygchen
jimmygchen reviewed Jun 5, 2026
View reviewed changes
Comment thread validator_client/validator_services/src/payload_attestation_service.rs Outdated
return;
};

self.produce_and_publish(attestation_slot).await;

@jimmygchen jimmygchen Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we want to keep the self.executor.spawn we had before when calling self.produce_and_publish, so a slow response from BN doesn't block the next update task.

jimmygchen
jimmygchen reviewed Jun 5, 2026
View reviewed changes
Comment thread validator_client/validator_services/Cargo.toml Outdated
slot_clock = { workspace = true }
task_executor = { workspace = true }
tokio = { workspace = true }
tokio = { workspace = true, features = ["rt-multi-thread", "sync", "signal", "macros", "test-util"] }

@jimmygchen jimmygchen Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test features can go into the dev-dependencies section, so we can keep these out of production builds.

jimmygchen
jimmygchen reviewed Jun 5, 2026
View reviewed changes
Comment thread testing/validator_test_rig/src/mock_beacon_node.rs Outdated
);
let body = request.body().expect("Failed to get request body");

let message = <PayloadAttestationMessage>::ssz_fixed_len();

@jimmygchen jimmygchen Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: rename variable to chunk_size

jimmygchen
jimmygchen requested changes Jun 5, 2026
View reviewed changes

@jimmygchen jimmygchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, i think we can merge after fixing the self.executor.spawn issue commented above

@jimmygchen jimmygchen 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 Jun 5, 2026
@chong-he chong-he changed the title Add payload attestation test to validator client Refactor payload_attestation_service and add payload attestation test to validator client Jun 8, 2026
@chong-he

chong-he commented Jun 8, 2026

Copy link
Copy Markdown
Member Author

Nice, i think we can merge after fixing the self.executor.spawn issue commented above

Thanks for the review. I address the comments in 04fba50.

Re on the self.executor.spawn, I thought that is a simplification to have 1 less thread, but now that you say it, I understand why it was written so, thanks!

@chong-he chong-he 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 Jun 8, 2026
jimmygchen
jimmygchen approved these changes Jun 10, 2026
View reviewed changes

@jimmygchen jimmygchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @chong-he ! looks great

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 10, 2026
@mergify mergify Bot added the queued label Jun 10, 2026
@mergify

mergify Bot commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Merge Queue Status

This pull request spent 29 minutes 57 seconds in the queue, including 27 minutes 55 seconds running CI.

Required conditions to merge

mergify Bot added a commit that referenced this pull request Jun 10, 2026
@mergify
Merge of #9357
f31c659
@mergify mergify Bot mentioned this pull request Jun 10, 2026
Closed
8 tasks
@mergify mergify Bot merged commit 844c6dd into sigp:unstable Jun 10, 2026
38 checks passed
@mergify mergify Bot removed the queued label Jun 10, 2026
@chong-he chong-he mentioned this pull request Jun 11, 2026
Open
@chong-he chong-he deleted the gloas-vc-test branch June 17, 2026 10:43
@chong-he chong-he mentioned this pull request Jun 18, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees

No one assigned

Labels

gloas ready-for-merge This PR is ready to merge. test improvement Improve tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chong-he @jimmygchen