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.

Sign up for GitHub

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

Jump to bottom

(fix): HTTP error handling in VC API #4606

Closed
wants to merge 29 commits into from
Closed

(fix): HTTP error handling in VC API #4606

wants to merge 29 commits into from

Conversation

protocolwhisper
Copy link

Issue Addressed

Closes #4597

Proposed Changes

Updated Handler Return Types: Handlers now return Response directly
Refactored blocking_signed_json_task function

Additional Info

No

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@protocolwhisper protocolwhisper mentioned this pull request Aug 10, 2023
Closed
@michaelsproul michaelsproul added val-client Relates to the validator client binary ready-for-review The code is ready for review labels Aug 14, 2023
@protocolwhisper
Copy link
Author

@michaelsproul, since this PR got merged, shall I update to use the function convert_rejection<T> as a crate in this scope?

@michaelsproul
Copy link
Member

@michaelsproul, since #4595 got merged, shall I update to use the function convert_rejection as a crate in this scope?

I think it's OK to duplicate the definition for now. Or we could move convert_rejection into warp_utils.

@michaelsproul michaelsproul 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 Aug 17, 2023
@jimmygchen
Copy link
Member

@protocolwhisper looks like there are a few CI failures, would you mind taking a look?

@protocolwhisper
Copy link
Author

@michaelsproul @jimmygchen Hello! I'm currently facing an issue. I've made changes to the function as follows:
However, after making these changes, I'm encountering numerous errors, and I'm not sure where to start in order to resolve them. I'd truly appreciate some guidance on this matter.

pub async fn blocking_signed_json_task<S, F, T>(signer: S, func: F) -> Response<Body>
where
    S: Fn(&[u8]) -> String,
    F: FnOnce() -> Result<T, Rejection> + Send + 'static,
    T: Reply + Send + 'static,
{
    // Get the Result from the blocking task
    let result = warp_utils::task::blocking_task(func).await;

    // Convert the result using your convert_rejection function
    let mut response = convert_rejection(result).await;

    // Process the response for the signature
    let body: &Vec<u8> = response.body(); // NOTE: Ensure `body` method exists and returns a Vec<u8>
    let signature = signer(body);
    let header_value = HeaderValue::from_str(&signature).expect("hash can be encoded as header");
    response.headers_mut().append("Signature", header_value);

    response
}

jimmygchen
jimmygchen reviewed Aug 21, 2023
View reviewed changes
validator_client/src/http_api/mod.rs Outdated
///
/// This function should *always* be used to convert rejections into responses. This prevents warp
/// from trying to backtrack in strange ways. See: https://github.com/sigp/lighthouse/issues/3404
pub async fn convert_with_header<T: Reply>(res: Result<T, warp::Rejection>) -> resp {
Copy link
Member

@jimmygchen jimmygchen Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this function isn't used anywhere and can be removed? (There are some compilation errors as well)

jimmygchen
jimmygchen reviewed Aug 21, 2023
View reviewed changes
validator_client/src/http_api/mod.rs Outdated
response
})
let result = warp_utils::task::blocking_task(func).await;
let conversion = convert_rejection(result).await; // It handles the rejection
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following the logic here, convert_rejection isn't available in the scope, perhaps you're trying to call the convert_with_header you defined above?

Copy link
Author

Choose a reason for hiding this comment

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

Right, I’ve already modified it but haven’t uploaded the last commit. I’m using ‘convert_with_header’ instead of ‘convert_rejection

Copy link
Author

Choose a reason for hiding this comment

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

I've made significant progress. I'll check tomorrow to see if only the .then is causing the failure, and then I'll modify the handlers that call blocking_signed_task :)

@jimmygchen
Copy link
Member

@protocolwhisper I think it's best to go through the compiler errors - the explanations are usually pretty clear, there are a few easy ones to fix, e.g. method not found, variable not found, I'd suggest to fix those first if you're not sure where to start. I've pointed out 2 of these in the above comments, let us know if there's any specific error that you're struggling to resolve.

@protocolwhisper
Copy link
Author

@michaelsproul @jimmygchen I've updated the latest PR with the necessary changes and it now passes the tests for the module. Let me know your feedback :) .

@protocolwhisper
Copy link
Author

protocolwhisper commented Aug 28, 2023 •
edited
Loading

Hello 👋🏽, can I get your comments on this PR? I’d like to make changes since the function’s signature has been modified. @michaelsproul @jimmygchen

@jimmygchen
Copy link
Member

Hi @protocolwhisper, thanks for the update!

It looks like @michaelsproul has triggered a CI run, and there are some CI failures possibly related to the changes, would you mind taking a look please?

@protocolwhisper
Copy link
Author

protocolwhisper commented Aug 28, 2023 •
edited
Loading

Hi @protocolwhisper, thanks for the update!

It looks like @michaelsproul has triggered a CI run, and there are some CI failures possibly related to the changes, would you mind taking a look please?

Gotcha, I corrected the lint but since we changed the signature from pub async fn blocking_signed_json_task<S, F, T>( signer: S, func: F, ) -> Result<impl warp::Reply, warp::Rejection> to pub async fn blocking_signed_json_task<S, F, T>(signer: S, func: F) -> resp is this why the test are failing? So I should focus on those changes or the tests? I'm a little lost. When trying to reproduce I'm getting this :
image

@jimmygchen
Copy link
Member

Feel free to ignore the test failure, it seems to be related to an issue we discovered earlier today, which is now being addressed in #4868. I'll re-run the fail jobs now.

jimmygchen
jimmygchen reviewed Dec 7, 2023
View reviewed changes
validator_client/src/http_api/mod.rs Outdated
response
})
}
Copy link
Member

Choose a reason for hiding this comment

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

This is bit isn't really doing what the function is advertised to do; I think it probably falls under the responsibility of the blocking_signed_json_task function instead as it describes here:

lighthouse/validator_client/src/http_api/mod.rs

Lines 1277 to 1280 in f77a5a0

/// Executes `func` in blocking tokio task (i.e., where long-running tasks are permitted).
/// JSON-encodes the return value of `func`, using the `signer` function to produce a signature of
/// those bytes.
pub async fn blocking_signed_json_task<S, F, T>(signer: S, func: F) -> Response<Vec<u8>>

Is it possible to move this block there to keep the convert_rejection only do what it's supposed to do?

Convert a warp Rejection into a Response.

If it's too much of an issue, we could consider renaming this function to something like convert_into_json_response?

@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 Dec 7, 2023
jimmygchen
jimmygchen requested changes Dec 7, 2023
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @protocolwhisper
Thanks for the updates, I've added a few more comments. Let me know if you have any questions.

@protocolwhisper
Copy link
Author

Hi @jimmygchen
I just updated both functions, changed the output and updated the error pattern as the one from beacon node

@jimmygchen 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 Feb 6, 2024
@protocolwhisper
Copy link
Author

Heyy , can I get any comments about this?

@michaelsproul
Copy link
Member

Sorry @protocolwhisper but we've decided to delete the response signing altogether, so this change has been rolled into the PR that implements that: #5529

@protocolwhisper
Copy link
Author

Got it, I'll close this issue then

@jimmygchen
Copy link
Member

@protocolwhisper Sorry this one didn't work out, thanks for your help though! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul Awaiting requested review from michaelsproul

@jimmygchen jimmygchen Awaiting requested review from jimmygchen

Assignees
No one assigned
Labels
HTTP-API ready-for-review The code is ready for review val-client Relates to the validator client binary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@protocolwhisper @CLAassistant @michaelsproul @jimmygchen @jmcph4 @protocolito @dapplion