Skip to content

Add rpc processor trait #5541

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

brimigs
Copy link

@brimigs brimigs commented Mar 27, 2025

Problem

Recent dev feedback has been surrounding the local testing experience, requesting to update our testing environment to mimic the functionality of Anvil, which would include additional methods that all work dynamically while the validator is running, like updating account state, cloning accounts from multiple clusters, manipulating token balances in accounts, slot warping, etc.

To be able to safely add these methods without effecting security of mainnet validators, we need to create an RPC processor trait & have two different processor types.

Summary of Changes

This PR covers creating the new trait and adding the ability to run the test validator with an -- enable-test-features flag that will use the new test validator RPC processor type, while ensuring the mainnet validators will always run with the standard RPC processor type and not have the ability to change processor types.

The TestValidatorJsonRpcRequestProcessor derefs the JsonRpcRequestProcessor to access methods and fields from JsonRpcRequestProcessor & the plan is to then add the additional test validator functionality onto the TestValidatorJsonRpcRequestProcessor.

All public methods on the JsonRpcRequestProcessor type were moved to the RpcRequestProcessorTrait, while private methods remained just on the JsonRpcRequestProcessor.

For more context: #5852

Fixes #

@mergify mergify bot requested a review from a team March 27, 2025 17:19
Copy link

mergify bot commented Mar 27, 2025

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @brimigs.

@brimigs brimigs closed this Mar 27, 2025
@brimigs brimigs deleted the add-rpc-processor-trait branch March 31, 2025 14:57
@brimigs brimigs restored the add-rpc-processor-trait branch March 31, 2025 14:59
@brimigs brimigs reopened this Mar 31, 2025
@joncinque
Copy link

I'll take a look at this as well, but requested additional reviews from other RPC people.

Copy link

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

It looks like you made a wrapper trait that holds on one of two JsonRpcRequestProcessor instances (as base) and then sort of proxies calls into them.

Would it instead be possible to generate the trait from the existing impl of JsonRpcRequestProcessor, then make two implementors (LiveJsonRpcRequestProcessor and TestJsonRpcRequestProcessor) with the exact same interface, but different implementations and private methods? That way you wouldn't have to do the whole ‘cast through Any’ thing to convince Hyper that it's dealing with a usable request processor.

image

})
.await
.expect("Failed to spawn blocking task")
fn genesis_creation_time(&self) -> UnixTimestamp {

Choose a reason for hiding this comment

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

So, consider a method like this one. This impl is identical between TestJsonRpcRequestProcessor and LiveJsonRpcRequestProcessor. Could we instead make it the default implementation on the trait itself?

The annoying thing is that traits can't have properties, so you need to turn every property into a getter.

pub trait JsonRpcRequestProcessorForResult<T>: JsonRpcRequestProcessorClone + Send + Sync {
    // An abstract getter for `blockstore`
    fn blockstore(&self) -> &Arc<Blockstore>;
    // A concrete implementation for `check_blockstore_root()` that uses that getter: `blockstore()`.
    fn check_blockstore_root(
        &self,
        result: &std::result::Result<T, BlockstoreError>,
        slot: Slot,
    ) -> Result<()> {
        if let Err(err) = result {
            debug!(
                "check_blockstore_root, slot: {:?}, max root: {:?}, err: {:?}",
                slot,
                self.blockstore().max_root(),
                err
            );
            if slot >= self.blockstore().max_root() {
                return Err(RpcCustomError::BlockNotAvailable { slot }.into());
            }
            if self.blockstore().is_skipped(slot) {
                return Err(RpcCustomError::SlotSkipped { slot }.into());
            }
        }
        Ok(())
    }
}

// Now the only code you have to duplicate is the getters.
#[async_trait]
impl<T> JsonRpcRequestProcessorForResult<T> for LiveJsonRpcRequestProcessor {
    fn blockstore(&self) -> &Arc<Blockstore> {
        &self.blockstore
    }
}
#[async_trait]
impl<T> JsonRpcRequestProcessorForResult<T> for TestJsonRpcRequestProcessor {
    fn blockstore(&self) -> &Arc<Blockstore> {
        &self.blockstore
    }
}

Note the annoying thing you have to do (ie. create multiple traits for each type parameter.

Does that work and cut down on copypasta?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants